Skip to content

perf(daemon): hash the executable image once per process; announce missed hook deadlines - #1767

Open
bmcnaboe wants to merge 1 commit into
DeusData:mainfrom
bmcnaboe:fix/hook-augment-startup-deadline
Open

bmcnaboe wants to merge 1 commit into
DeusData:mainfrom
bmcnaboe:fix/hook-augment-startup-deadline

Conversation

@bmcnaboe

@bmcnaboe bmcnaboe commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor

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.
  • 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.

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) this branch
cli index_repository (spawns the internal daemon) 6471 ms 5198 ms
daemon start → ready 6497 ms 5329 ms
hook-augment against the warm daemon, 5 runs 1140–1151 ms, context emitted 1129–1160 ms, context emitted

Tests

  • daemon_runtime_process_fingerprint_hashes_own_image_once_per_process: the real path. Two cbm_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.
  • Both stub-driven tests gather their results and disarm the stub before the first assertion, so a failure cannot leave the stub armed for the rest of the suite.
  • cli_hook_augment_deadline_breadcrumb_issue858 extended: 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> --help lines moved to #2443. The sanitized-build timeout backstops that used to ride along here live in #1742.

Refs #1335, #2058

@github-actions

Copy link
Copy Markdown

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. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

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.

@bmcnaboe
bmcnaboe marked this pull request as ready for review August 24, 2026 19:54
@bmcnaboe
bmcnaboe requested a review from DeusData as a code owner August 24, 2026 19:54
@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory editor/integration Editor compatibility and CLI integration priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 1, 2026
@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

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.

@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

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 substance

This is the second cause of the same silent failure. The comment above ha_deadline_ms records #858: the original 300 ms budget self-terminated on cold starts so augmentation "never appeared in real sessions (0/24 observed)". #1880 found that a typo in CBM_HOOK_DEADLINE_MS silently clamps to 50 ms. You have found that the hook spends ~2.3 s SHA-256'ing its own ~295 MB executable before doing anything at all. Three different routes to a hook that emits nothing and exits 0.

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 cli_sha256_platform_path_matches_scalar is the test that stops it.

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

pr-smoke (macos-14) fails at scripts/smoke-test.sh:549:

FAIL: config get auto_watch printed '' (expected the stored value or the default 'true')

I checked, and I cannot honestly call this unrelated:

  • auto_watch is a real key (CBM_CONFIG_AUTO_WATCH, cli.h:425), so the test is not stale.
  • You add zero lines touching config handling, which argues it is not yours.
  • But the assertion runs config get ... 2>/dev/null || true, so an empty result means the binary failed or printed nothing — not that the key is missing. And this PR changes what happens at process start, in runtime_process_image_reference_acquire, for every command including config get, against a fresh CBM_CACHE_DIR.

That is a plausible mechanism, and "adds no config code" does not rule it out.

The run is also from 20 August, and main has moved a great deal since — it was briefly broken by a duplicate-symbol merge yesterday, repaired by #1993, and #1703 landed after that. So the result may simply be stale.

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 config get can print is the shape that would produce exactly this output.

This is also DIRTY, so a rebase is needed regardless.

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.

@DeusData

DeusData commented Sep 3, 2026

Copy link
Copy Markdown
Owner

The pr-smoke (macos-14) red is real, it is reproducible, and it is caused by your speed-up — but not by a mistake in it. I chased it to the bottom locally on an arm64 Mac, so you don't have to guess from a swallowed stderr.

What the smoke sees

Phase 3z3 runs CBM_CACHE_DIR=<fresh dir> codebase-memory-mcp config get auto_watch immediately after Phase 3z2, which drives the MCP stdio server through 24 pipelined calls and lets it exit. On your build that config get prints nothing and exits 1 — twice in a row across your last two runs, while the six other PRs re-validated against the same main pass it. Ubuntu and Windows pass on your branch too.

What stderr actually says

codebase-memory-mcp: CBM could not start because the active account daemon uses a different
cache directory (active cache 91a55c…; requested cache 92e0e2…). Close all CBM sessions and
commands, then retry.

Mechanism (measured)

The MCP stdio session auto-starts --cbm-daemon-internal, and that daemon outlives its last client by a short window — this is pre-existing on main:

binary daemon gone after client exit
main 0.56 s / 0.58 s / 0.57 s
this PR 0.31 s / 0.31 s / 0.30 s

The next one-shot CLI (config get is LOCAL_CLI) computes its own build fingerprint in main_build_identity() before it looks for a daemon. On main that is the scalar SHA-256 of the whole executable — ~1 s here, ~3–4 s on the macos-14 runner — which is longer than the daemon's linger, so the CLI never meets it. Your CommonCrypto path does the same hash in ~0.1 s, so the CLI arrives while the old daemon is still visible, the version-cohort acquisition sees a live daemon bound to a different cache directory, and refuses hard. Control experiment on your binary, same repro:

  • no pause between 3z2 and 3z3 → printed='' rc=1 (2/2)
  • sleep 1 between them → printed='true' rc=0

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 how

The smoke assertion is a real production race that your speed-up exposes deterministically: any command under a different CBM_CACHE_DIR issued within ~0.5 s of an MCP session ending is refused. Removing that startup cost is the whole point of the PR, so it needs to land with the race closed rather than re-masked. Two production-side shapes that fit:

  1. Deterministic hand-off on the daemon side — the stdio client does not return until the internal daemon has released its cohort/endpoint registration (or the daemon's exit is made synchronous with its last client). This is the wait-for-the-asserted-state shape and is what I'd lean to.
  2. Tolerant admission on the CLI side — cohort acquisition treats a daemon whose last client is gone (draining) as absent, or retries a cache-directory conflict against a draining daemon within its existing MAIN_STARTUP_TIMEOUT_MS deadline instead of failing on first sight.

What is not on the table is a sleep in the smoke, or budget-tuning the linger — the race would still be there for real users on fast machines.

Local repro that reproduces it 2/2 (CBM_RUNTIME_DIR just isolates you from your own installed daemon):

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 test-windows-guards red, which is ours. The substance of the PR still reads well to me — and finding this race is a real contribution in its own right; a slow hash was hiding a genuine lifecycle bug.

@DeusData

DeusData commented Sep 4, 2026

Copy link
Copy Markdown
Owner

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 config get under a fresh CBM_CACHE_DIR arrives a few hundred milliseconds after the internal daemon's last stdio client exits. In that window the draining daemon still holds the cohort lifetime lock, so the CLI reads a holder record with a different cache root and cbm_version_cohort_acquire refused immediately instead of honouring its deadline. Main's slow scalar self-hash happened to push the CLI past that window; your CommonCrypto hash removes the accidental margin and the race shows up on every run. Filed as #2046 with the full trace.

The fix lives in #2047 (the cohort acquire now retries a mismatched holder until the caller's deadline, the same handoff wait host.c already does for the claim marker). With that applied on top of your head, the previously failing sequence passes 3/3 locally. So please don't work around it in this branch — once #2047 lands I will update this PR against main and re-run the matrix, and this branch stays exactly the hash change it is.

Thanks for the patience on this one, and for a change that flushed out a real lifecycle bug on its way in.

DeusData added a commit to Jumaga2015/codebase-memory-mcp that referenced this pull request Sep 4, 2026
…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>
@DeusData

DeusData commented Sep 9, 2026

Copy link
Copy Markdown
Owner

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 pr-smoke (macos-14) lane (the rest of the matrix — full unit suites on macOS/Linux/Windows, tsan/msan, CodeQL, lint, the security gates — is green), so it looks isolated to the macOS smoke/packaging step; could you take a look? Happy to help triage if it's not obvious. (2) The sanitized-build test-timeout backstops in tests/test_daemon_runtime.c are fine in principle (upper-bound liveness, not the assertion), but they're a bit orthogonal to the caching change — your call whether to keep them here or split them out.

No rush — flip it out of draft whenever you're ready and we'll take another pass. Really appreciate the careful work on this.

@bmcnaboe
bmcnaboe force-pushed the fix/hook-augment-startup-deadline branch from 8a74d81 to 036562d Compare September 14, 2026 14:51
@bmcnaboe bmcnaboe changed the title fix(hook-augment): cache the build fingerprint so the startup deadline is met fix(hook-augment): cache the image fingerprint per process and announce a missed deadline on stderr Sep 14, 2026
@bmcnaboe

Copy link
Copy Markdown
Contributor Author

Thanks — all three addressed, plus one measurement you should see before the next pass.

CommonCrypto is gone. src/foundation/sha256.{c,h} are byte-identical to main again; what remains is the per-process fingerprint cache, the stderr breadcrumb, and the --help line. Rebased onto main (339b3f4) as one signed-off commit, so the conflict is cleared.

pr-smoke (macos-14) was the #2046 race you traced on Sept 3: the CommonCrypto hash let config get reach the draining internal daemon inside its linger window. #2047 (merged Sept 4) retries a mismatched cohort holder until the caller's deadline, and this rebase picks that up; with the scalar hash back, the branch's one-shot CLI start-up timing is main's again anyway. scripts/smoke-local.sh passes locally on arm64 (3z2 → 3z3 included).

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, src/ of this repo indexed, warm permanent daemon):

main (339b3f4) this branch
cli index_repository (spawns the internal daemon) 6471 ms 5198 ms
daemon start → ready 6497 ms 5329 ms
hook-augment, 5 runs 1140–1151 ms 1129–1160 ms

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 hook-augment hashes exactly once on main and on this branch — the peer check on the same inode is metadata-only, so there is no second hash for the cache to absorb — and its latency is unchanged: one scalar hash per invocation (1.15 s here, ~2.3 s on the machine that first hit the deadline). So "the cache alone solves the deadline" doesn't hold for the cold hook; whether it fits the 2 s budget is now purely the per-machine hash cost. I've retitled the PR and dropped Fixes #1335 to match what it delivers; the stderr breadcrumb is what makes the miss visible where it still happens.

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.

@bmcnaboe

Copy link
Copy Markdown
Contributor Author

CI note: the one red lane, test / test-windows-guards, is section_cold_storm failing at endpoint creation before any code this PR touches (same failure on main on Sept 12, tracked in #2057); a rerun should clear it if you can trigger one.

@DeusData

Copy link
Copy Markdown
Owner

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 DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Retitle and fix the comment. Something like perf(daemon): hash the executable image once per process; announce missed hook deadlines. The comment at runtime.c:757 still says the hash is paid on every peer rendezvous and causes the hook deadline miss.
  2. 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.
  3. 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.
  4. Fill the cache only after the post-hash stat check passes. On Windows, key it on FILE_BASIC_INFO.ChangeTime rather than the creation time, which SetFileTime can reset.
  5. Move the new daemon --help lines to a separate PR. They're outside this change, and a warm daemon doesn't remove the hook's own hash.
  6. 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.

daidaiJ pushed a commit to daidaiJ/codebase-memory-mcp that referenced this pull request Sep 29, 2026
…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>
@bmcnaboe
bmcnaboe force-pushed the fix/hook-augment-startup-deadline branch from 036562d to 9545ea4 Compare September 30, 2026 15:30
@bmcnaboe bmcnaboe changed the title fix(hook-augment): cache the image fingerprint per process and announce a missed deadline on stderr perf(daemon): hash the executable image once per process; announce missed hook deadlines Sep 30, 2026
@bmcnaboe

Copy link
Copy Markdown
Contributor Author

Thanks for the careful pass. All six are addressed in 9545ea4, rebased onto current main:

  1. Retitled to perf(daemon): hash the executable image once per process; announce missed hook deadlines. The cache comment now says what the cache does: it removes the daemon's second start-up hash, it is process-local, and a fresh hook process still hashes its own image once.
  2. Real path tested. daemon_runtime_process_fingerprint_hashes_own_image_once_per_process calls cbm_daemon_runtime_process_build_fingerprint(getpid()) twice with the hash stub armed and asserts exactly one hash, so reverting an acquire-site hunk now fails the suite.
  3. Stub disarmed before asserting. Both stub-driven tests gather their results into locals, reset the cache (which also disarms the stub), and only then assert. The synthetic-identity seam also refuses to run unless the stub is armed, so it can never reach a real hash.
  4. Fill only after the post-hash check. The hash path is now resolve-then-commit: runtime_build_fingerprint_resolve looks up or hashes without storing, and each acquire site commits in its if (ok) block, after the post-hash stat check (and on Linux the re-open verify), under the identity observed after the hash. On Windows the key uses FILE_BASIC_INFO.ChangeTime via GetFileInformationByHandleEx instead of the creation time, and the commit re-reads it, so a change time that moved under the hash is never cached. daemon_runtime_fingerprint_cache_skips_image_changed_under_hash covers the guard.
  5. --help lines moved to docs(cli): list daemon <start|stop|status> in --help #2443, with the hook-latency claim dropped.
  6. __NetBSD__ added to the cache guard.

scripts/test.sh --suites "cli daemon_runtime" (ASan+UBSan), clang-format, diff-scoped clang-tidy and the security audit pass locally on arm64. Sequencing behind #2335 is fine by me.

@DeusData

DeusData commented Oct 1, 2026

Copy link
Copy Markdown
Owner

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.

@DeusData

Copy link
Copy Markdown
Owner

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 src/cli/hook_augment.c and src/daemon/runtime.h. Before you rebase, three review points:

  1. Please split the PR. The stderr notice is independent of the cache and clearly safe. It leaves stdout and the exit code untouched for every client dialect, and the installed shims discard stderr anyway. As its own PR it can land quickly.
  2. Please narrow the cache. The measured saving comes from the daemon re-hashing its own image at start. The cache, though, sits in every fingerprinting acquire, including peer verification, which the description doesn't mention. Please limit it to the process's own image (process_id equal to the current process), or follow fix(daemon): hash a verified peer image copy once, not on every HELLO (#1955) #2335 and keep the verified image open.
  3. Windows key. The comment near runtime.c:762 says ChangeTime cannot be reset. That is true for SetFileTime, but SetFileInformationByHandle(FileBasicInfo) can set it, and can suspend its updates, with only FILE_WRITE_ATTRIBUTES. For the process's own running image the OS write lock covers this; for any other file it does not. That is one more reason for point 2.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working editor/integration Editor compatibility and CLI integration priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. security Security vulnerabilities, hardening stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hook-augment: needs a warm daemon to ever emit context; install leaves it timing out on every Grep/Glob (follow-up to #858)

2 participants