fix(hook-augment): cache the image fingerprint per process and announce a missed deadline on stderr - #1767
fix(hook-augment): cache the image fingerprint per process and announce a missed deadline on stderr#1767bmcnaboe wants to merge 1 commit into
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
|
Thank you for measuring the fingerprint cost, covering cache rollover, and making missed hook deadlines observable. Runtime identity caching and platform SHA paths touch startup and trust boundaries, so we need more time to review them carefully. The contribution queue is quite full, but we will return with grounded feedback as soon as possible. |
|
Reviewed. The diagnosis is excellent and it closes the other half of a symptom this repo has chased before. One CI failure I am not willing to call environmental, and I would rather say why than wave it through. The substanceThis is the second cause of the same silent failure. The comment above Keying the cache on (device, inode, size, mtime, ctime) is the right identity. A rebuilt binary rolls every one of those, so the cache cannot serve a stale fingerprint for a different image — which is the failure mode that would make this worse than no cache. Asserting the CommonCrypto path bit-identical to the scalar one is what makes the hardware path safe to take. A faster hash that disagrees on one input would be a very unpleasant bug to find later, and But the most valuable line in this PR is the third bullet. Recording a missed deadline on both the timeouts log and stderr, "never a silent 0-byte exit 0", is what turns this class of problem from invisible into reportable. Even with the cache, some future machine will be slow enough; the difference is whether anyone can tell. The failure I am not attributing
I checked, and I cannot honestly call this unrelated:
That is a plausible mechanism, and "adds no config code" does not rule it out. The run is also from 20 August, and Please rebase and let it re-run. If it clears, it was the base. If it persists, the place to look is fingerprint acquisition on a cold cache directory — a startup path failing before This is also Three well-argued fixes to the hook-augment path from you now. Thank you — this one has the most user-visible payoff of the three. |
fde7929 to
0657651
Compare
|
The What the smoke seesPhase 3z3 runs What stderr actually saysMechanism (measured)The MCP stdio session auto-starts
The next one-shot CLI (
Linux/Windows keep the scalar hash, so the mask stays in place there — which is exactly why only the macOS leg goes red. Why this is yours to close, and howThe smoke assertion is a real production race that your speed-up exposes deterministically: any command under a different
What is not on the table is a Local repro that reproduces it 2/2 ( export CBM_RUNTIME_DIR=$(mktemp -d)
python3 - "$BIN" <<'PY' # the smoke's own Phase 3z2 snippet
import json, subprocess, sys
BIN=sys.argv[1]; rpc=lambda i,m,p: json.dumps({"jsonrpc":"2.0","id":i,"method":m,"params":p})
lines=[rpc(1,"initialize",{"protocolVersion":"2025-06-18","capabilities":{},"clientInfo":{"name":"smoke-pipe","version":"0"}}),
json.dumps({"jsonrpc":"2.0","method":"notifications/initialized"})]
lines+=[rpc(100+i,"tools/call",{"name":"list_projects","arguments":{}}) for i in range(24)]
subprocess.run([BIN],input="\n".join(lines)+"\n",capture_output=True,text=True,timeout=300)
PY
CBM_CACHE_DIR=$(mktemp -d) "$BIN" config get auto_watch; echo "rc=$?"Everything else on your branch is green apart from the standing |
|
Closing the loop on the macOS pr-smoke red: it is not your change, but your change is what exposed it — and that turned out to be useful. The failing The fix lives in #2047 (the cohort acquire now retries a mismatched holder until the caller's deadline, the same handoff wait Thanks for the patience on this one, and for a change that flushed out a real lifecycle bug on its way in. |
…line A participant admitted to the version cohort holds the cohort lifetime lock SH until its lease is released at exit. An internal daemon that has just lost its last stdio client keeps that lock through its teardown, so a local CLI (`config get`, `index_status`, ...) arriving in that few-hundred-ms window met a holder whose cache root differed and was refused immediately with "active account daemon uses a different cache directory" — a pure lifecycle race, previously masked by the slow scalar self-hash and exposed as soon as the hash got fast (DeusData#1767 pr-smoke on macOS). cbm_version_cohort_acquire now retries a CONFLICT until the caller's finite deadline_ms, holding no guard between attempts, exactly as host.c already waits out the same handoff for the daemon claim marker. Every production caller passes a finite deadline; UINT64_MAX keeps failing fast so a genuine conflict against a long-lived peer is never waited on indefinitely. Tests (deterministic, no timing assertions on transient windows): one proves the retry runs until the deadline and a UINT64_MAX caller still fails immediately; one proves a waiter that met the mismatched holder is admitted the moment the holder releases. Both fail with the retry removed. Fixes DeusData#2046 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for this — the fingerprint cache is exactly the right idea for the hook-budget problem (#1335/#2058), and the design is genuinely careful: keying on (dev, inode, size, mtime, ctime) is a nice touch, since ctime moves on any in-place write and can't be rolled back with utimes, so a same-size/reset-mtime tamper still misses; and every cache hit is still bracketed by the acquire path's stat + process-maps checks, so a hit stays bound to the verified image. That's the security-relevant part and it holds up. One change we'd like before this lands: please drop the Apple CommonCrypto SHA-256 path (src/foundation/sha256.c/.h) and keep just the cache. The cache alone solves the deadline — once the fingerprint is memoized, the ~2.3 s hash runs at most once per process, so CommonCrypto only speeds a cache miss. For a lean C codebase, a permanent macOS-only, platform-divergent crypto path is a maintenance cost we'd rather not take on for that marginal miss-path win. Keeping a single scalar SHA-256 everywhere keeps the behaviour identical across platforms. Two smaller notes: (1) CI is red only on the No rush — flip it out of draft whenever you're ready and we'll take another pass. Really appreciate the careful work on this. |
…ce a missed deadline on stderr The build-identity fingerprint is a SHA-256 over the entire ~295 MB executable: ~1.1 s per hash on an M5 Pro, ~2.3 s on the machine where this was first measured. A daemon pays it twice at start (the supervisor's startup capture and the runtime service's active-image check), and a peer whose image is not inode-identical to the daemon's own is re-hashed in full on every rendezvous. - Cache the fingerprint per process at runtime_process_image_reference_acquire, keyed by (device, inode, size, mtime, ctime): the tuple the acquire path already verifies as stable across the hash, so a rebuilt or replaced binary rolls the key. Every hit is still bracketed by the same before/after stat and process-maps checks, so a cached digest stays bound to the verified image. Measured with a 296 MB image: daemon cold start 6.5 s -> 5.3 s. A cold hook-augment still hashes exactly once (1.15 s here) and is unchanged by the cache. - A missed hook deadline is announced on stderr as well as the timeouts log, so a fired deadline is never a silent 0-byte exit 0. The breadcrumb is formatted before the log path is resolved, so stderr gets it even when the log cannot be opened. CBM_HOOK_DEADLINE_MS is unchanged. - `daemon <start|stop|status>` is listed in --help. SHA-256 itself is unchanged: one portable scalar implementation on every platform. Refs DeusData#1335, DeusData#2058 Signed-off-by: Brian McNaboe <bmcnaboe@gmail.com>
8a74d81 to
036562d
Compare
|
Thanks — all three addressed, plus one measurement you should see before the next pass. CommonCrypto is gone.
Backstops are out of this PR; they already live in #1742, unchanged. The measurement. With the hardware path gone I timed both binaries in a sandboxed profile (M5 Pro, 296 MB image,
The cache saves the daemon's second start-up hash (the supervisor capture plus the runtime service's active-image check) and the full re-hash of any peer that is not inode-identical. But a cold If you want the cold hook's hash gone without a platform crypto path, the shape I'd propose as a follow-up is persisting the same-keyed digest in the owner-only account cache root (already the storage authority for admission), so a cold process skips the hash when the tuple matches and a rebuilt binary still rolls it. That is a trust-boundary call, so I'd rather have your steer before writing it. Out of draft and ready for another pass. |
|
CI note: the one red lane, |
The build-identity fingerprint is a SHA-256 over the entire ~295 MB executable (~1.1 s per hash on an M5 Pro, ~2.3 s on the machine where this was first measured). A daemon pays it twice at start (the supervisor's startup capture and the runtime service's active-image check), and a peer whose image is not inode-identical to the daemon's own is re-hashed in full on every rendezvous.
runtime_process_image_reference_acquire, keyed by (device, inode, size, mtime, ctime): the tuple the acquire path already verifies as stable across the hash, so a rebuilt or replaced binary rolls the key. Every hit is still bracketed by the same before/after stat and process-maps checks, so a cached digest stays bound to the verified image.CBM_HOOK_DEADLINE_MSis unchanged.daemon <start|stop|status>is listed in--help.SHA-256 itself is unchanged: one portable scalar implementation on every platform (the earlier CommonCrypto path was dropped at review).
Measured (M5 Pro, 296 MB image, sandboxed profile,
src/of this repo indexed)main(339b3f4)cli index_repository(spawns the internal daemon)daemon start→ readyhook-augmentagainst the warm daemon, 5 runsThe cache removes the daemon's second start-up hash. A cold
hook-augmenthashes exactly once on both binaries, so it is unchanged by the cache; whether it fits the 2 s budget is a function of the per-machine scalar hash cost. That is why this no longer claims to close #1335 on its own.Tests
daemon_runtime_fingerprint_cache_hit_miss_key_roll: miss → hit → key roll on mtime and on inode, via a test seam that stubs the image hash and counts calls.cli_hook_augment_deadline_breadcrumb_issue858extended: the breadcrumb is asserted on stderr as well as in the log.scripts/test.sh --suites "cli daemon_runtime"(ASan+UBSan) andscripts/smoke-local.shpass locally on arm64.The sanitized-build timeout backstops that used to ride along here live in #1742.
Refs #1335, #2058