Repository navigation
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. |
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, |
|
Thank you for removing the platform-crypto changes and clarifying the process-local cache limitation. The current proposal should be reviewed on its actual cache/deadline behavior; a per-process cache does not remove the initial fingerprint cost from the next fresh hook process. We are not asking you to add a persistent digest cache here. CI attribution remains maintainer work, not a request for a no-op push. |
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @bmcnaboe, and thanks for the honest measurement. You were right that a per-process cache can't help a fresh hook process, and our 9 September note was wrong to say the cache alone solves the deadline. Here is the review we owed you, on the actual behaviour. We ran it on the merge result with current main: cli + daemon_runtime 378/378 pass, and lint-ci and the memory-core ratchet are green.
The stderr breadcrumb is great. It is safe for hook hosts (exit stays 0, stdout is untouched, and the installed wrappers discard stderr), and it makes a missed deadline visible instead of silent.
The cache is a real win for the daemon. It removes the daemon's second start-up hash (the supervisor capture plus the active-image check). A few asks before it lands:
- Retitle and fix the comment. Something like
perf(daemon): hash the executable image once per process; announce missed hook deadlines. The comment atruntime.c:757still says the hash is paid on every peer rendezvous and causes the hook deadline miss. - Test the real path. Two
cbm_daemon_runtime_process_build_fingerprint(getpid())calls with the hash stub set should cost exactly one hash. Today, reverting the macOS acquire-site hunk leaves the suite green, because the existing test only drives the seam. - Disarm the stub before asserting in the cache test. A failure there currently leaves the stub armed and cascades into about 33 unrelated daemon_runtime failures.
- Fill the cache only after the post-hash stat check passes. On Windows, key it on
FILE_BASIC_INFO.ChangeTimerather than the creation time, whichSetFileTimecan reset. - Move the new
daemon--helplines to a separate PR. They're outside this change, and a warm daemon doesn't remove the hook's own hash. - Tiny: add
__NetBSD__to the cache guard. The FreeBSD/NetBSD acquire branch calls the helper.
Heads-up: #2335 addresses re-hashing of peer copies separately, and we'll sequence the two. Thank you for the care and patience on this one.
…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>
…ssed hook deadlines 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. - Cache the fingerprint per process at runtime_process_image_reference_acquire, keyed by (device, inode, size, mtime, change time): the identity the acquire path already verifies as stable across the hash, so a rebuilt or replaced binary rolls the key. On Windows the change time is FILE_BASIC_INFO.ChangeTime, which the kernel maintains and SetFileTime cannot reset. 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. A freshly hashed digest is committed only after the acquire chain, including the post-hash stat check, has passed, and only under the identity observed after the hash, so an image that changed under the hash is never cached. The cache is process-local: it removes the daemon's second start-up hash (cold start 6.5 s -> 5.3 s with a 296 MB image); a fresh hook process still hashes its own image once. - 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. 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>
036562d to
9545ea4
Compare
|
Thanks for the careful pass. All six are addressed in 9545ea4, rebased onto current
|
|
Thank you, @bmcnaboe, for the September 30 follow-up, the explicit process-local limitation, and for splitting the help change into #2443. We need more time to review the updated identity-cache code and its tests, including the sequencing with #2335. That review is on our side; this is not another request for a speculative rebase or a claim that the current head is security-cleared. Thanks again for the careful follow-through. |
|
Thank you, @bmcnaboe, for your patience here and for the careful identity-cache work. Thanks too for the missed-deadline notice on stderr: it is exactly the visibility #2441 showed we were missing. Two things changed since your last push. #2575 (e93e438) moved the hash onto the CPU's SHA instructions, and main now conflicts with this branch in
The remaining cost, that each hook process still hashes its own image once, is now tracked as a design question in #2683, so this PR doesn't have to solve it. Thanks again. This PR found the right problem, and the long wait for review was on our side. |
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.
runtime_process_image_reference_acquire, keyed by (device, inode, size, mtime, change time): the identity the acquire path already verifies as stable across the hash, so a rebuilt or replaced binary rolls the key. On Windows the change time isFILE_BASIC_INFO.ChangeTime, which the kernel maintains andSetFileTimecannot reset. 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. A freshly hashed digest is committed only after the acquire chain, including the post-hash stat check, has passed, and only under the identity observed after the hash, so an image that changed under the hash is never cached.CBM_HOOK_DEADLINE_MSis unchanged.The cache is process-local. It removes the daemon's second start-up hash; a fresh hook process still hashes its own image once, so this does not close #1335 on its own. Re-hashing of peer copies is handled separately in #2335.
SHA-256 itself is unchanged: one portable scalar implementation on every platform.
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 runsTests
daemon_runtime_process_fingerprint_hashes_own_image_once_per_process: the real path. Twocbm_daemon_runtime_process_build_fingerprint(getpid())calls with the hash stub armed cost exactly one hash, so a cache wired into the seam but not into a platform's acquire site fails here.daemon_runtime_fingerprint_cache_hit_miss_key_roll: miss → hit → key roll on mtime, change time and inode, through the same resolve-then-commit path the acquire sites use.daemon_runtime_fingerprint_cache_skips_image_changed_under_hash: a digest whose identity moved under the hash is not committed, so the next sight of the original identity hashes again.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) passes locally on arm64.The
daemon <start|stop|status>--helplines moved to #2443. The sanitized-build timeout backstops that used to ride along here live in #1742.Refs #1335, #2058