h-dv/code-index

Watch

Fork 0

bug: the client never compares the daemon's recorded version — a stale daemon serves degraded answers silently #83

New issue

Closed

opened 2026-08-26 14:22:53 +02:00 by buildagent · 1 comment

buildagent commented

2026-08-26 14:22:53 +02:00

Member

Found while dogfooding v0.22.3. It cost an hour and produced a confident wrong diagnosis of an already-fixed bug (#82), which is the real argument for fixing it: a silent build mismatch does not merely degrade answers, it sends the reader after the wrong cause.

What happened

A daemon was started for an isolated harness as:

code-index-daemon --root /path/to/repo --db /tmp/.../scratchpad/index.db

It wrote /path/to/repo/.code-index/daemon.toml and became the daemon for that root:

pid = 398794
version = "0.22.2"
root = "/home/master/code/rust/cosi-mcp"

Hours later the binaries were rebuilt and installed at 0.22.3. The MCP client reconnected, re-attached to that registration, and talked to a 0.22.2 daemon serving a different database — for the rest of the session, with no signal of any kind.

Consequence: FileOverlap.content_hash (added in c807368) was absent, so the client correctly fell back to the mtime proxy and reported files stale. The degraded path behaved exactly as designed — every three-state contract held. There was simply nothing anywhere saying why the answers were degraded, so a fixed bug looked live.

Two separable defects

1. The version is recorded, parsed, and discarded.

Lockfile (crates/daemon/src/lifecycle.rs:67-71) carries version: String, and Lockfile::read parses it on every attach. Nothing compares it to the client's own version. schema_guard polls schema_version only (schema_guard.rs:89-97) and the schema was identical here, so it never fired — correctly, because the schema had not changed. The code had.

This is the I063 shape exactly: a fact that is computed, stored, read, and then thrown away, leaving the consumer to infer a cause it cannot see.

2. Daemon identity is root, not (root, db).

Lockfile.root exists precisely to "reject a payload that describes a different project than the one being queried" (issue #7, lifecycle.rs:72-80). It did not apply: the rogue daemon had the same root and a different --db. So a daemon serving database B can capture the registration for root A, and every client for A is silently routed to B.

The pair (--root, --db) is what actually determines what a daemon answers, and only half of it participates in identity.

Why this is not just operator error

It was operator error in this instance — I passed the real root to a scratchpad daemon. But:

The CLI accepts --root X --db Y as a first-class combination; nothing warns that it hijacks X's registration.
The failure is silent and durable: it survives rebuild, reinstall and reconnect, because re-attach prefers an existing registration (I027).
Every tool answered plausibly throughout. Counts were sane, disclosures were internally consistent, three-state contracts held. Only a direct ps + daemon.toml inspection revealed it.

Proposed fix

A. Compare the version on attach. The client knows its own; the lockfile carries the daemon's. On mismatch, either refuse and respawn (the I027 machinery already spawns when re-attach fails), or attach and disclose it in the payload — daemon_version_skew: { client, daemon } — so an agent seeing degraded three-state fields can tell whether it is looking at a genuine gap or an old daemon. Silence is the one option that should be off the table.

Note the honest subtlety: an older daemon is not automatically wrong, and forcing a respawn on every patch bump would be disruptive for long-lived sessions. Disclosure is the minimum; refusal is a policy choice worth arguing separately.

B. Make identity (root, db). Record the canonical db path in the lockfile beside root, and have Lockfile::read reject a payload whose db differs from the one the caller asked for — the same defence-in-depth root already provides, applied to the other half of the pair. That alone would have made this impossible.

Test shape

A: a lockfile whose version differs from the client's → the reply carries the skew disclosure (or the attach is refused), and — anti-vacuity — a matching version produces neither.
B: a daemon registered for (root, dbA); a client asking for (root, dbB) must not attach to it. Mutation: drop the db from the comparison → the test goes red.
Both on the daemon leg; neither is observable in snapshot mode.

Same family as the skew work in #75/#78 — the lifecycle analysis there noted there is no channel on the wire for rule-set identity, and the health RPC carries only (root, schema_version) (rpc_index.rs:665-673). This is that gap, reachable today, without any of the template machinery.

Found while dogfooding v0.22.3. It cost an hour and produced a confident **wrong diagnosis** of an already-fixed bug (#82), which is the real argument for fixing it: a silent build mismatch does not merely degrade answers, it sends the reader after the wrong cause. ## What happened A daemon was started for an isolated harness as: ``` code-index-daemon --root /path/to/repo --db /tmp/.../scratchpad/index.db ``` It wrote `/path/to/repo/.code-index/daemon.toml` and became **the** daemon for that root: ```toml pid = 398794 version = "0.22.2" root = "/home/master/code/rust/cosi-mcp" ``` Hours later the binaries were rebuilt and installed at 0.22.3. The MCP client reconnected, re-attached to that registration, and talked to a **0.22.2 daemon serving a different database** — for the rest of the session, with no signal of any kind. Consequence: `FileOverlap.content_hash` (added in `c807368`) was absent, so the client correctly fell back to the mtime proxy and reported files stale. **The degraded path behaved exactly as designed** — every three-state contract held. There was simply nothing anywhere saying *why* the answers were degraded, so a fixed bug looked live. ## Two separable defects **1. The version is recorded, parsed, and discarded.** `Lockfile` (`crates/daemon/src/lifecycle.rs:67-71`) carries `version: String`, and `Lockfile::read` parses it on every attach. Nothing compares it to the client's own version. `schema_guard` polls `schema_version` only (`schema_guard.rs:89-97`) and the schema was **identical here**, so it never fired — correctly, because the schema had not changed. The *code* had. This is the I063 shape exactly: a fact that is computed, stored, read, and then thrown away, leaving the consumer to infer a cause it cannot see. **2. Daemon identity is `root`, not `(root, db)`.** `Lockfile.root` exists precisely to "reject a payload that describes a different project than the one being queried" (issue #7, `lifecycle.rs:72-80`). It did not apply: the rogue daemon had the **same** root and a different `--db`. So a daemon serving database B can capture the registration for root A, and every client for A is silently routed to B. The pair `(--root, --db)` is what actually determines what a daemon answers, and only half of it participates in identity. ## Why this is not just operator error It was operator error in this instance — I passed the real root to a scratchpad daemon. But: - The CLI accepts `--root X --db Y` as a first-class combination; nothing warns that it hijacks X's registration. - The failure is **silent and durable**: it survives rebuild, reinstall and reconnect, because re-attach prefers an existing registration (I027). - Every tool answered plausibly throughout. Counts were sane, disclosures were internally consistent, three-state contracts held. Only a direct `ps` + `daemon.toml` inspection revealed it. ## Proposed fix **A. Compare the version on attach.** The client knows its own; the lockfile carries the daemon's. On mismatch, either refuse and respawn (the I027 machinery already spawns when re-attach fails), or attach and **disclose it in the payload** — `daemon_version_skew: { client, daemon }` — so an agent seeing degraded three-state fields can tell whether it is looking at a genuine gap or an old daemon. Silence is the one option that should be off the table. Note the honest subtlety: an older daemon is not automatically wrong, and forcing a respawn on every patch bump would be disruptive for long-lived sessions. Disclosure is the minimum; refusal is a policy choice worth arguing separately. **B. Make identity `(root, db)`.** Record the canonical db path in the lockfile beside `root`, and have `Lockfile::read` reject a payload whose db differs from the one the caller asked for — the same defence-in-depth `root` already provides, applied to the other half of the pair. That alone would have made this impossible. ## Test shape - **A:** a lockfile whose `version` differs from the client's → the reply carries the skew disclosure (or the attach is refused), and — anti-vacuity — a matching version produces neither. - **B:** a daemon registered for `(root, dbA)`; a client asking for `(root, dbB)` must not attach to it. Mutation: drop the db from the comparison → the test goes red. - Both on the daemon leg; neither is observable in snapshot mode. ## Related Same family as the skew work in #75/#78 — the lifecycle analysis there noted there is *no channel on the wire* for rule-set identity, and the health RPC carries only `(root, schema_version)` (`rpc_index.rs:665-673`). This is that gap, reachable today, without any of the template machinery.

buildagent added the

labels

2026-08-26 14:22:57 +02:00

buildagent referenced this issue

2026-09-05 12:53:34 +02:00

The first upgrade can SIGKILL the daemon mid-migration, repeatedly, and a bricked project has no stated exit #144

dhoyer referenced this issue from a commit

2026-09-06 00:09:43 +02:00

feat: six silent-loss surfaces render the fact the process already held

buildagent commented

2026-09-06 09:42:33 +02:00

Author

Member

Triage 2026-09-06: CLOSING. Both halves shipped — the skew reaches the payload, and attach refuses on a different database.

Verified against master; landed in d0279c0.

A — the version comparison, and it is read at call time

pub enum DaemonBuild with four states (NoDaemonLeg, Matched, Skewed{client,daemon,pid}, Unknown{why}) — crates/daemon/src/access.rs:36; fn daemon_build() on IndexAccess at :73, defaulting to NoDaemonLeg.

It is read at call time, not latched at attach — crates/daemon/src/rpc_index.rs:837-885, with the reasoning written into the doc: an I027 respawn must not be able to leave a stale verdict behind. That is the right call on this repo, where the daemon idle-exits at 30 minutes and respawns mid-session.

It reaches the payload rather than a log: render_daemon_build (call site crates/mcp-server/src/server.rs:4470-4500) emits skew / client_version / daemon_version / daemon_pid / semantics, emits nothing on Matched and NoDaemonLeg, and availability: unavailable + why on Unknown. The previous behaviour was tracing::warn! — a channel no agent reads.

B — identity becomes `(root, db)`

pub db: Option<String> — crates/daemon/src/lifecycle.rs:110, optional with skip_if_none, so absent ⇒ DbIdentity::Unrecorded, which is not Same. db_identity at :466.

And it is enforced, not merely reported: crates/mcp-server/src/main.rs:863 — if lock.db_identity(db_path) == DbIdentity::Different { bail!(… pid …) }. The error names the recorded db, the asked-for db, and the pid.

Runs (exit 0)

cargo test -p code-index-mcp --bin code-index-mcp a_build_skew_reaches_the_payload_and_a_match_does_not → 1 passed
cargo test -p code-index-daemon --test payload_state_e2e                                               → 5 passed

incl. db_identity_has_a_did_not_report_state and the_four_payload_states_are_distinct (RUN mutation recorded at payload_state_e2e.rs:241).

Residual, and it is worth reading before assuming this is fully graded

This issue's test shape asked for both halves on the daemon leg. Neither is graded there:

db_identity is exercised only by unit-level calls on Lockfile::db_identity. Nothing starts a daemon on (root, dbA) and attaches a client asking dbB, so the main.rs:863 refusal path itself is untested.
daemon_build appears in no test file at all — the skew disclosure is graded by a render_daemon_build unit test, not over the wire.

Both mechanisms are real and present; the attach-time halves are ungraded. On this repo that is a familiar shape — an e2e that never crosses the wire is how five RPC arms once stayed deletable-green — so it is worth a follow-up test rather than being forgotten. I am closing on the mechanism shipping and being read at the right time, and recording the gap here rather than in a new issue, since it is a test to add and not a defect to fix.

There is a live illustration of exactly why this matters, from today: the installed binary is 0.26.1 (4555887), older than the tree it indexes, and a probe against it returns pre-fix answers. That is the class this issue exists to make visible.

🤖 Triage lane, 2026-09-06, master 45cf6e4

## Triage 2026-09-06: CLOSING. Both halves shipped — the skew reaches the payload, and attach **refuses** on a different database. Verified against master; landed in `d0279c0`. ### A — the version comparison, and it is read at call time `pub enum DaemonBuild` with four states (`NoDaemonLeg`, `Matched`, `Skewed{client,daemon,pid}`, `Unknown{why}`) — `crates/daemon/src/access.rs:36`; `fn daemon_build()` on `IndexAccess` at `:73`, defaulting to `NoDaemonLeg`. It is read **at call time**, not latched at attach — `crates/daemon/src/rpc_index.rs:837-885`, with the reasoning written into the doc: an I027 respawn must not be able to leave a stale verdict behind. That is the right call on this repo, where the daemon idle-exits at 30 minutes and respawns mid-session. It reaches the payload rather than a log: `render_daemon_build` (call site `crates/mcp-server/src/server.rs:4470-4500`) emits `skew` / `client_version` / `daemon_version` / `daemon_pid` / `semantics`, emits **nothing** on `Matched` and `NoDaemonLeg`, and `availability: unavailable` + `why` on `Unknown`. The previous behaviour was `tracing::warn!` — a channel no agent reads. ### B — identity becomes `(root, db)` `pub db: Option<String>` — `crates/daemon/src/lifecycle.rs:110`, optional with `skip_if_none`, so **absent ⇒ `DbIdentity::Unrecorded`, which is not `Same`**. `db_identity` at `:466`. And it is enforced, not merely reported: `crates/mcp-server/src/main.rs:863` — `if lock.db_identity(db_path) == DbIdentity::Different { bail!(… pid …) }`. The error names the recorded db, the asked-for db, and the pid. ### Runs (exit 0) ``` cargo test -p code-index-mcp --bin code-index-mcp a_build_skew_reaches_the_payload_and_a_match_does_not → 1 passed cargo test -p code-index-daemon --test payload_state_e2e → 5 passed ``` incl. `db_identity_has_a_did_not_report_state` and `the_four_payload_states_are_distinct` (RUN mutation recorded at `payload_state_e2e.rs:241`). ### Residual, and it is worth reading before assuming this is fully graded This issue's test shape asked for both halves **on the daemon leg**. Neither is graded there: - `db_identity` is exercised only by unit-level calls on `Lockfile::db_identity`. **Nothing starts a daemon on `(root, dbA)` and attaches a client asking `dbB`**, so the `main.rs:863` refusal path itself is untested. - `daemon_build` appears in **no test file at all** — the skew disclosure is graded by a `render_daemon_build` unit test, not over the wire. Both mechanisms are real and present; the **attach-time** halves are ungraded. On this repo that is a familiar shape — an e2e that never crosses the wire is how five RPC arms once stayed deletable-green — so it is worth a follow-up test rather than being forgotten. I am closing on the mechanism shipping and being read at the right time, and recording the gap here rather than in a new issue, since it is a test to add and not a defect to fix. There is a live illustration of exactly why this matters, from today: the installed binary is `0.26.1 (4555887)`, older than the tree it indexes, and a probe against it returns pre-fix answers. That is the class this issue exists to make visible. 🤖 Triage lane, 2026-09-06, master `45cf6e4`

buildagent closed this issue

2026-09-06 09:42:35 +02:00

buildagent referenced this issue

2026-09-06 09:52:21 +02:00

No reply field says the answering binary is older than the tree it indexed, and that is why #118 read as unfixed from inside the session that fixed it #181

buildagent referenced this issue

2026-09-06 15:48:43 +02:00

No reply field says the answering binary is older than the tree it indexed, and that is why #118 read as unfixed from inside the session that fixed it #181

dhoyer referenced this issue from a commit

2026-09-06 17:27:25 +02:00

feat: the answer says which binary produced it and which tree it had read