Skip to content

fix(hook-augment): cache the build fingerprint so the startup deadline is met - #1767

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

fix(hook-augment): cache the build fingerprint so the startup deadline is met#1767
bmcnaboe wants to merge 2 commits into
DeusData:mainfrom
bmcnaboe:fix/hook-augment-startup-deadline

Conversation

@bmcnaboe

Copy link
Copy Markdown

Every process start SHA-256s its own ~295 MB executable to build the build-identity fingerprint (~2.3 s, software path), pushing the PreToolUse hook past its default deadline on large graphs so it emits 0 bytes and exits 0.

  • Cache the fingerprint keyed by (device, inode, size, mtime, ctime) at runtime_process_image_reference_acquire, covering the self-fingerprint and per-peer rendezvous; a rebuilt binary rolls the key.
  • Back the streaming SHA-256 with CommonCrypto on Apple (hardware path), dropping even a cache miss to ~0.25 s; asserted bit-identical to the scalar path.
  • A missed deadline is recorded on both the timeouts log and stderr (never a silent 0-byte exit 0); CBM_HOOK_DEADLINE_MS still honored; daemon <start|stop|status> documented in --help.

Tests: daemon_runtime_fingerprint_cache_hit_miss_key_roll, cli_sha256_platform_path_matches_scalar, and the extended deadline-breadcrumb test. Opened as a draft.

Fixes #1335

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

…e is met

Every process start SHA-256s its own ~295 MB executable to build the
build-identity fingerprint (~2.3 s, software path), which pushes the
PreToolUse hook past its default deadline on large graphs so it emits
0 bytes and exits 0.

- Cache the fingerprint keyed by (device, inode, size, mtime, ctime) at
  runtime_process_image_reference_acquire, covering both the self-
  fingerprint and the per-peer rendezvous; a rebuilt binary rolls the key.
- Back the streaming SHA-256 with CommonCrypto on Apple (hardware path),
  dropping even a cache miss to ~0.25 s; asserted bit-identical to scalar.
- A missed deadline is now recorded on BOTH the timeouts log and stderr
  (never a silent 0-byte exit 0); CBM_HOOK_DEADLINE_MS still honored; and
  `daemon <start|stop|status>` is documented in --help.

Fixes DeusData#1335

Signed-off-by: Brian McNaboe <bmcnaboe@gmail.com>
@bmcnaboe
bmcnaboe force-pushed the fix/hook-augment-startup-deadline branch from fde7929 to 0657651 Compare September 3, 2026 12:46
@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>
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. 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