fix: harden daemon release paths and verification#1252
Merged
Conversation
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
DeusData
added
bug
DeusData
added
editor/integration
… checks The native Windows soak could never start its server, so that gate had never actually measured anything. The root cause was the harness, not the daemon: an MSYS filesystem FIFO feeding a native Windows process delivers empty stdin (reproduced 5/5), while a Bash anonymous coprocess carries the full JSON-RPC initialize exchange to the same protected payload, and the failing daemon log showed an orderly client disconnect rather than a daemon-side eviction. The soak now uses a named coprocess on native Windows only, duplicating its endpoints to stable fd3/fd4 and closing the originals so closing fd3 still delivers EOF; POSIX keeps the FIFO path. The coproc syntax sits inside eval because macOS system Bash 3.2 must still parse this file even though only MSYS2 Bash 5 executes that branch. With transport repaired the next exact failure surfaced: native Windows Python cannot open an MSYS /c/... diagnostics path. Both consumers now pipe the file through stdin, the pattern already established elsewhere in the repo, and the soak recovery contract forbids reintroducing a direct native-Python open. Also on the release path: - The smoke fixture server no longer fsyncs before atomically publishing its port. This is ephemeral readiness signalling, not crash-durable state, and the macOS Intel runner failed inside that durability sync. When the contract does fail it now reports the observable state -- waited, exit status, port file, staged temp files, interpreter, startup log -- because the previous verdict named nothing on the one runner we cannot reproduce locally. - The POSIX publication test hook no longer compiles into Windows builds, fixing an exact -Werror unused-variable failure. The setter keeps a parameter-consuming Windows stub because it is public API. - The Wine leg assembles the real release layout (payload plus canonical launcher) and version-checks both, running the launcher through cmd so it has a Windows-visible parent. The unsupported Wine soak is removed: Wine stays a fast compile/package/version check, and native Windows is authoritative for daemon, locking, ACL and process-lifetime semantics. - run.sh soak-windows routes to the native Windows VM, which validates the payload, builds the protected per-user temp root, stamps ACLs, and refuses success without a completion summary. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…lookup The macOS Intel leg failed the fixture contract on every run while every other platform passed. Removing the readiness fsync did not change it; the hardened diagnostics named the real cause on the first remote run: waited=30.0s, exit_status=alive, port_file=absent, staged_temp_files=none, empty startup log -- the process was healthy but had never reached publish_port. http.server.HTTPServer.server_bind() resolves socket.getfqdn(host). On a host whose resolver does not answer for the bind address that call blocks for the resolver timeout, so the constructor never returns and no port is ever published. The fixture now binds through a subclass that keeps the threading server but skips the FQDN resolution, which only feeds CGI-style variables this fixture never serves. Proven both directions locally by forcing socket.getfqdn to hang: the subclass publishes its port immediately, the stock server never does. That forcing hook is kept as a permanent contract guard, so the reverse-DNS dependency cannot return without turning the gate red on every platform rather than on one runner nobody can reproduce. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Growth investigations were guessing, because the diagnostics snapshot only carried process totals: RSS and committed bytes say memory grew, never where. The map adds allocator-live bytes and a size-class histogram to each snapshot, so a distinctive class identifies the allocation without per-callsite instrumentation. The design constraint is honesty about coverage. Walking the allocator reaches the aggregate heap plus this thread's heap and its abandoned pages, which is not the whole process, and in a build where malloc does not route through the allocator at all it reaches nothing. A per-subsystem tally would report zeros there and read as a clean bill of health. So every snapshot carries three independent totals and an explicit residual -- os_committed, allocator-live, residual = the difference -- and the triple localises growth instead of implying it: live grows leak on a walked heap; buckets say which size residual grows, flat another thread, retention, or non-allocator memory both flat, RSS grows mappings or stacks, not the heap That guard paid for itself immediately. Pointed at the open Windows growth in #581 it reported live=0 with zero blocks and the residual carrying all of it -- not 'the heap is innocent' but 'this walk saw nothing', which is the correct reading for a process demonstrably holding tens of megabytes. Together with the growth surviving a build with the allocator override compiled out, that says the Windows allocations under investigation never reach the allocator, so an allocator-based map cannot localise them and the next instrument has to sit lower down. Covered by a test that allocates a known volume in a known size class and requires the map to attribute it there, so the map can never regress into silently reporting zeros. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The allocator walk could say how much memory was live and in what size class, but not who was holding it -- and when growth does not route through the allocator at all, as on Windows, it could say nothing useful. Phase marks close that gap using the one metric that does see such growth: OS committed bytes. Each mark attributes the committed delta since the previous mark to the previous label, so over many passes the label whose total climbs is where memory stays. The design is deliberately coarse and says so. Committed bytes are process-wide, so a busy concurrent thread lands noise in whichever label is open; signal comes from accumulation over many passes, never one delta. Marks must bracket a path with no unlabelled gap, or growth hides in the gap -- so the request path labels its tail 'idle' explicitly, which is also what makes request-path retention distinguishable from background growth. Off unless CBM_MEM_PHASES=1. Pointed at #581 it localised the growth in two steps. At request granularity dispatch_tool held +102 MB over 27 requests while scope begin/end and idle were flat, which independently killed the theory that the growth was time-based. Splitting the handler then put +88.4 MB of it in resolve_store against +14.1 MB for the query body, with the matching release returning only -35.9 MB. So each request resolves a store, commits ~3.4 MB, and gets ~1.4 MB back: a ~2 MB net ratchet per request that never returns. That accounts for the observed growth, for its scaling with corpus size, for why every tool leaks identically, and for why only Windows shows it. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Adds mem_malloc_owned to every diagnostics snapshot: a real malloc/free pair probed with mi_is_in_heap_region rather than an inference from build flags, because the Windows static-CRT override is defined at compile time yet can fail to take effect at link time -- which is exactly what happens here. This single bit distinguishes 'nothing is live' from 'the allocator does not own this build', which are opposite conclusions drawn from the same zero live_bytes. On Windows it reads false, which means every allocator tuning in cbm_mem_init -- immediate purge, purge-decommits, page reclaim, arena policy -- is inert for ordinary allocations there, while POSIX gets all of it. That is the parity gap behind #581, and reading this bit first would have replaced a long elimination hunt with one measurement. Also reverts the interim _heapmin trim explored while localising #581. It cut the ten-minute Windows soak from 1168 MB to 232 MB but never reached the 13-to-15 MB flatness POSIX achieves, and it cost roughly four times the query latency because it walks the whole heap on the request path. Trimming a heap the allocator should have owned treats the symptom; the override itself is the fix. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
A static MinGW link has no DllMain and registers no TLS callback, so nothing ran at thread exit and every thread left its mimalloc thread-heap behind for the life of the process. After 300 MCP requests the allocator reported 607 thread-heaps, peak equal to total equal to current -- none ever released -- holding 170.5 MiB against a live set of roughly 300 KiB, growing linearly and without bound (#581). POSIX is unaffected because a pthread TSD destructor already calls this, which is precisely why POSIX stayed flat under the identical workload. The heaps were invisible to mi_heap_visit, which only walks the main and calling heaps, so the growth appeared to belong to no allocator at all and survived every allocator option that was tried against it. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The probe assumed the walk would attribute over half of a 12 MB allocation. mimalloc v3 exposes only the main heap, abandoned pages and the calling thread's theap -- there is no API to enumerate every theap -- so on Windows the probe's blocks are unreachable through all three and the walk saw ~190 KB. Asserts the triple documented in mem.h instead: what the walk cannot see, the residual must carry. Either attribution covers the probe or the committed total grew by at least as much, so memory can never go missing from the map unnoticed. Bucket attribution is checked where the walk did reach the probe. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Three CI failures, one cause each:
- Vendored integrity flagged a MISMATCH on the Windows mimalloc primitives.
The decommit counters added there were diagnostic and have served their
purpose -- 16k calls, zero failures, which retired the theory that purge was
silently failing. Restored to pristine, and the census fields they fed are
gone with them.
- GCC rejected an anonymous enum declared inside a struct ('declaration does
not declare anything'); clang accepted it, so it only broke the Ubuntu and
Windows smoke builds. Hoisted to file scope.
- clang-format across the files touched this session.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The shim exists only so the allocation profiler has an observation point on Linux. Applying its --wrap flags to the test and TSan link redirected malloc into mimalloc underneath ASan's own interception, putting two allocators on the same pointers. Production keeps the shim; sanitized builds do not. Windows is unchanged -- there the wrap flags are what make mimalloc own allocations. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Walking mi_heap_main() reads the process-wide aggregate heap while other threads allocate into it, which is a data race by construction; TSan reported it on macOS as init.c:452 in mi_heap_main. A diagnostic must not introduce a race into the code it measures. Now walks only what this thread may safely read: its own theap, plus abandoned pages, which have no owning thread left to race with. The lost coverage is carried by the residual, which is what it is for -- an unmeasured map must never read as an empty one. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Calling mi_heap_main() is itself the race: it reads the allocator's main-heap pointer while an exiting thread rewrites it via _mi_thread_done -> _mi_theap_default_set. TSan paired those two directly on macOS, with the read coming from the diagnostics thread. Removing the aggregate walk was not enough because the abandoned-page walk still passed mi_heap_main() as its argument. The map now touches only this thread's own theap. Everything it can no longer see is carried by the residual, which is what mem.h promises. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Reading the allocator's per-thread bookkeeping races a thread exiting concurrently: even mi_theap_get_default() reads state that _mi_thread_done rewrites via _mi_theap_default_set, and TSan paired those two directly with the read coming from the periodic diagnostics thread. Splits the collector. cbm_mem_map_collect_os() fills OS totals only and is what the periodic snapshot uses; the walking variant stays for tests and explicitly requested diagnostics, where no other thread is exiting. What the OS-only form cannot see is carried by the residual, as mem.h promises. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Reverts 3cd1d33. It was built on the theory that per-connection thread-heap churn drove #581, and the measurements later falsified that: growth scaled with requests rather than threads, and the real cause was thread heaps never being released at all, fixed separately by the TLS detach callback. It also broke a Windows regression guard -- test_daemon_stability section_cold_storm, 'a fixed Windows bug is broken again' -- which is what concurrent clients racing to spawn a daemon do to reworked reaping and shutdown ordering. A concurrency rewrite that fixes nothing it claimed and reopens a closed bug does not belong in a release-blocking PR. The allocator thread-heap fix is untouched. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
These were added under the theory that abandoned thread-heap pages needed collecting by hand. The TLS detach callback now releases each thread's heap properly, so an explicit mi_collect at every job and connection exit buys nothing and adds allocator teardown work on paths that were crashing a parallel Windows ARM64 shard (daemon_application, test-par exit 2 with the suite summary otherwise clean). Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Measurement retired the theory it was built on: the slab installs correctly now (config_failed=0) and the leak was unchanged, because the cause was thread heaps never being released rather than page-cache fragmentation. It also runs at the top of main() for every process, including the launcher, and the managed reinstall guard went red. A change that fixes nothing it claimed does not get to stay on a release-blocking PR while a Windows guard is failing. cbm_store_configure_pagecache remains available for a future attempt wired ahead of sqlite3_initialize deliberately. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…check The immediate-reinstall guard reused the portable pair from work/portable-install, whose adjacent launcher the earlier managed installs consume. Managed install then refuses correctly -- 'requires a verified adjacent codebase-memory-mcp.exe launcher' -- so the guard was failing on the product behaving as designed rather than on the contract it protects. Reproduced by running the install command directly (exit 1, that exact error) and confirmed identical on origin/main, so this was never a regression from this branch. Stages its own pair, leaving the assertions untouched. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The adjacent launcher was present but refused as unverified: copied files inherit Administrators ownership on runner images, and the launcher's exact-owner validators require the current user, which is what stamp_fixture_owner_current exists to simulate. Without it the guard failed on the product's ownership check rather than on the immediate-reinstall contract. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Neither the fresh portable pair nor the ownership stamp fixed the immediate managed reinstall, so both were guesses and do not belong on a release-blocking PR. The guard's failure is pre-existing on main -- byte-identical output there, and unchanged with the #581 fix disabled -- so this branch carries no regression and no speculative test edits either. What the investigation did establish: the reinstall's install command fails with 'managed install requires a verified adjacent codebase-memory-mcp.exe launcher' when run directly, and emits no diagnostic on the stream the guard captures, which is why the guard reports only a version banner. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The guard reported only a version banner, so a silent nonzero exit was indistinguishable from a rejected precondition. The install command works in isolation and across a plain uninstall-then-reinstall, so the exit code is the remaining unknown -- a crash code would say so directly. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Three distinct refusals -- an insecure source directory, a failed snapshot, and a missing source file -- collapsed into one boolean, and the frees on the way out clobber GetLastError, so the caller reported 'staging transaction open failed (status -3, os 0)': an I/O error with no cause and no refusal note. The Windows launcher guard's immediate-reinstall step fails through exactly this path, and with no cause reported each hypothesis cost a full ten-minute guard run. Captures the OS error before the frees and names the predicate that refused. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…saction activation_transaction_prepare refused an insecure target directory and returned IO with no note, while destroy() on the way out clobbered GetLastError -- so the caller printed 'staging transaction open failed (status -3, os 0)': an I/O error with neither a cause nor an OS code. That is the path the Windows launcher guard's immediate reinstall takes, and the missing note is why each hypothesis about it cost a ten-minute guard run. A target directory failing the exact-owner rule is a different bug from an unreadable source, and now says so. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Fourteen paths returned CBM_ACTIVATION_TRANSACTION_IO without setting a refusal note, so the caller could only ever print 'status -3, os 0' -- an I/O error with no cause. Identifying which one fires cost a ten-minute Windows guard run per hypothesis, and two targeted guesses missed. Labels every site so a single run names the path. Temporary labels; once the guard's path is known they become descriptive. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The copy loop's five distinct failures -- short read/write, an empty source, a failed fsync, a failed close, and a failed directory sync -- shared one branch and one unnamed IO status. Exhaustive labelling identified this as the path the Windows launcher guard's immediate reinstall takes; this names which of the five actually fires. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The copy loop's failure was reported as 'os 0' because the cleanup and close calls after the loop clobber the thread's last error. Captures it at the failure point, and separates a failed read from a failed write -- copying a payload that another process still holds is a different problem from a full disk. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Reverts the #581 fix mechanism, not the diagnosis. main's dry run passes both windows-11-arm shards and test-windows-guards; this branch fails all three. The only change here that affects process LOAD is the .CRT$XLB detach callback, and the arm64 failures are load-time crashes (STATUS_ILLEGAL_INSTRUCTION, pass=0, secs=0, five suites in the same second) -- while the guards exercise many launcher and payload spawns. The same binary loads fine on a UTM arm64 VM, so the placement is tolerated by some loaders and not others, which is not something to ship on a supported platform. #581's root cause stands and is documented: a static MinGW link has no DllMain and registers no TLS callback, so mi_thread_done never runs and every thread leaks its heap (607 heaps after 300 requests, 170 MiB against ~300 KiB live). The delivery mechanism needs to be one the loader accepts everywhere -- an FLS callback registered at runtime is the obvious candidate, with no linker section placement involved. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The hosted windows-11-arm shards crash at process load (STATUS_ILLEGAL_INSTRUCTION, pass=0, secs=0) while main passes them, and removing the TLS detach callback did not help -- so the remaining candidate is this layer, which touches every allocation on Windows through the interposer. It declares a _Thread_local guard read on every malloc, and thread-local storage in a static MinGW aarch64 link is exactly the fragile case; nothing in it needs to ship. The profiler did its job: it produced the attribution that identified #581's mechanism, and that analysis is recorded. Sources stay in the tree, unbuilt, so a future investigation can re-enable them deliberately. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The refusal notes changed the error text the Windows launcher guards match against, and test-windows-guards went red on this PR after they landed. They were investigation instrumentation and they succeeded -- they turned 'staging transaction open failed (status -3, os 0)' into ERROR_DISK_FULL and named the exact site -- but a diagnostic that shifts a matched error string does not belong in a release-blocking change. Worth redoing deliberately on its own, with the guards' expectations updated in the same commit. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
The coordination daemon is now a long-lived shared component used by concurrent
MCP sessions. Final release testing exposed operational risks around stale agent
configuration, Windows process cleanup, partial release smoke coverage, and
local-CI environment drift.
This PR hardens those paths without adding new daemon features.
What changed
Safe agent-config repair
exact match with the previous managed executable is repairable.
validated ancestor handles, rejects reparse points and unsafe namespaces, and
denies write/delete sharing during classification.
foreign-shaped, inaccessible, or otherwise ambiguous commands.
Deterministic parallel test scheduling
xargs -> bash -c -> timeout -> native runnertopology that couldhang indefinitely under MSYS2.
bounded timeouts, parses completion summaries, and writes one durable result
per suite.
trees through
taskkill /T /F.process-tree cleanup cannot be proven.
timeout-after-summary, missing summaries, stubborn descendants, post-exit
accounting, and the timeout-boundary leader-exit race.
Full release-fixture smoke coverage
publishes it atomically.
launcher/payload pairs, and Linux portable aliases.
phases in local and PR smoke runs.
overrides.
inside each smoke root.
install.ps1directly with native paths and fail on executionerrors.
Windows safety and local-CI hardening
loopback fixture URL; malformed gating fails closed.
HKCU\Environment\Pathwithout restoring oroverwriting concurrent user changes.
with strict DACLs; inability to establish that root fails the gate.
metacharacters are not reinterpreted.
selection, daemon failure diagnostics, and hostile Git
config/template/hook isolation.
CI workflow impact
infrastructure.
preserves concurrent race detection while avoiding a high-core,
sanitizer-only allocator convoy; native, ASan, smoke, and soak remain
uncapped.
PID 1; this prevents killed test grandchildren from persisting as zombies and
producing false process-containment failures.
uses the maintained native/MSYS smoke harness.
explicitly provides
curl,zip, andunzip.the standard library.
and install/update/uninstall behavior rather than skipping those phases.
Security and fail-closed behavior
ambiguous.
paths and resists reparse/component-swap races.
disposable roots.
gate is present but invalid.
is proven.
stop the relevant gate.
Known Windows cleanup limitation
The scheduler does not yet use a Windows Job Object.
taskkill /Tcan provedescendant cleanup only while the suite leader PID still exists. If the leader
exits exactly at the timeout boundary before
taskkillruns, the schedulerreturns an infrastructure failure instead of recording an ordinary timeout or
silently continuing. The deterministic contract covers this race; its outer
test controller cleans the intentionally created descendant.
Local verification
rendering, and diff checks passed; full exact lint is still running locally
after this early PR push.
security allow-list and sanitizer checks passed. The final pipeline change
was rebuilt and rerun separately: 225 passed, 0 failed.
121 suites.
scheduler/test-runner processes.
removed, live user PATH remained byte-for-byte unchanged, and no process
remained.
hashes matched.
3 skipped per run, with no race/fatal/stall signature.
PR push.
[PENDING: runs before merge]
The branch was pushed once the changed-path discovery gates were green so
GitHub CI could run in parallel with the remaining redundant local matrix.
Merge remains blocked on every mandatory local leg and the complete GitHub
matrix passing on the exact final SHA.
Explicitly parked follow-ups
share the main repository graph and store only their delta. This is a
downstream feature, not part of daemon-v1 stabilization.
CAS update path in a separate change.
--noproxyobservationseparately; it is not introduced or broadened here.
sanitizer exposed an 18-way busy-spin convoy, but native high-core runs have
not reproduced a product liveness failure; redesign the lock only with native
contention and throughput evidence.
same-account, loopback, run-ID, and HKCU-sentinel gates limit impact to the
caller's own install result, but a future test-infrastructure change could
exercise and compare-and-swap-restore the dedicated test account's live PATH
instead of retaining a production test branch.
basenames and preflight drive-letter mappings without
GetDriveTypeW.Current handling fails ambiguous/device paths closed; this is defense in
depth against user-configured mapped/device path probes.