fix(pipeline): preserve concurrent manage_adr write across reindex swap

[0xb007ab1e]

What

Fixes ADR loss across a reindex by moving ADRs into a per-project sidecar DB (<graph_db>.adr.db) that index publication never touches — the structural fix (Alternative 1) from the original discussion, not the lock. Closes #516.

Why the sidecar (not the lock)

When the ADR doesn't live in the file a reindex regenerates, there is no capture, re-read, or re-apply to keep in step, and no lock to coordinate across processes — the race is removed, not narrowed. No sleeps, polling, or timeout deciding an outcome.

Design

• store.c — ADR ops route to the sidecar. Writes use a cached RW handle (sqlite3_open_v2(RW|CREATE) + busy_timeout=10000 + WAL, matching the graph store). Reads never write: use the cached RW handle if open, else open READONLY when the file exists, else read the legacy graph row without creating anything. A :memory: store keeps ADRs in the graph DB (never file-swapped). One-time legacy migration guarded by an adr_meta marker (cbm_store_adr_migrate_once); sidecar path via cbm_path_for_file_api.
• pipeline.c — migrate a legacy ADR into the sidecar before the format-change / forced-full routes unlink the old generation (targeting the final, not staging, path); preserve-and-abort on failure. The old early-capture/re-apply machinery + its test hook are removed.
• artifact.c — the exported graph.db.zst carries the ADR: inject the sidecar's ADR into the snapshot before compression; restore it into the sidecar on import.
• mcp.c / cli.c / README — delete_project removes the sidecar (documented in the tool + README); *.adr.db excluded from index listing/counting.

Tests

File-based: ADR written mid-reindex survives; a legacy row migrates before a forced-full/format reindex with no prior read; delete_project (via the MCP handler) removes the sidecar and a same-name re-index has no ADR; artifact export→import round trip keeps the ADR.

Verification (local)

-Werror clean; lint-memory-core passes (store.c 358, pipeline.c 131, pipeline_incremental.c 134); pipeline 289; store_arch mcp mcp_mutation_guard index_format artifact 475 (+4 skipped). Only clang-format-19 available locally (CI pins 20); git clang-format shows the changed lines clean.

Compatibility

Existing on-disk indexes keep their ADR with no reindex (migrated on first write or before the next rebuild. No config or API breakage; cbm_store_adr_* signatures unchanged (plus one new cbm_store_adr_migrate_once).

[github-actions]

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.

[DeusData]

Thank you for #2314 and #2315. Silently losing an ADR is exactly the kind of bug nobody notices until the decision record is gone, and your root-cause write-up (a shared project_summaries row, early capture and late swap, in-process guards only) is precise. We appreciate the honest risk section too.

We'd like to go with the direction you named as Alternative 1: a separate per-project ADR store that a reindex never replaces. Here is why we prefer it over the lock:

• It removes the race instead of narrowing it. When the ADR doesn't live in the file a reindex regenerates, there is no capture, re-read or re-apply to keep in step, and no lock to coordinate across processes.
• The lock as written conflicts with two of our standing rules. We don't put sleeps or polling waits into the code, and we don't let a timeout decide an outcome. The 10 ms retry loop, with its 30 s fail-open, does both: a slow peer silently brings the data loss back.
• The re-apply happens after the swap. For a moment the published DB shows the stale ADR, and if the re-apply write fails, the rescued ADR is lost with only a log line. An ADR deleted during the reindex would also come back from the early capture.

What we'd look for in the reworked PR:

1. ADRs stored in their own file beside the project DB (your <project>.adr.db naming is fine), read and written by manage_adr, and never touched by index publication.
2. A one-time migration: an existing ADR in project_summaries moves to the new store the first time it is read or written, and the old row is no longer used as the source of truth. Existing indexes must keep their ADRs without a reindex.
3. delete_project removes the ADR store with the project, or keeps it deliberately and says so. Please pick one and document it.
4. Tests: an ADR written between the start of a reindex and its publish survives (your hook-based test adapts directly); an ADR created before upgrading is migrated and still readable after a reindex; delete behaves as documented.
5. The early-capture and re-apply code in the pipeline can then go away, which makes publication simpler.

If you'd rather not take on the larger change, tell us and we will pick it up ourselves, crediting your analysis and test. Either way, thank you. This was a careful find.

[0xb007ab1e]

Reworked to Alternative 1 as you asked — thanks for the precise review; agreed on removing the race rather than narrowing it, and on the no-sleep/no-timeout rules.

• ADRs now live in a per-project <graph_db>.adr.db sidecar, read/written by cbm_store_adr_* (lazily-opened, cached connection), and never touched by index publication. No lock, no sleeps/polling, no timeout deciding an outcome. A :memory: store keeps ADRs in the graph DB (never file-swapped, so no race there).
• One-time migration: the first ADR read/write copies any legacy project_summaries row from the graph DB into the sidecar (INSERT OR IGNORE), so an index built before this change keeps its ADR with no reindex.
• delete_project now removes the sidecar (<db>.adr.db + its -wal/-shm/-journal) with the project — documented in the handler.
• The early-capture/re-apply is gone: capture_existing_adr, the saved_adr field, the generation.adr_content plumbing, the publish-time re-apply, and the test-only fail-capture hook. The reindex-delete path only unlinks the graph DB + -wal/-shm/-journal (cbm_remove_db_sidecars); .adr.db is a different file and is left intact — publication is simpler, as you noted.
• adr_update_sections' BEGIN IMMEDIATE read-modify-write now runs on the ADR connection, so its write lock actually covers the get/store.

Tests (file-based): an ADR seeded in the legacy project_summaries row is migrated on first read and survives a full reindex; an ADR write creates the sidecar, leaves the graph DB's project_summaries empty, and unlinking the sidecar removes the ADR. The existing #516 survive-reindex and incremental-preserve-ADR tests now validate the sidecar unchanged; the obsolete capture-failure test (and its hook) are removed.

Built clean under -Werror; pipeline (289) and store_arch mcp mcp_mutation_guard index_format (409) suites green locally. One caveat: I only have clang-format-19 locally (CI pins 20) — git clang-format shows my changed lines clean, but if CI's 20 reformats anything, point me at it and I'll match it.

[DeusData]

Thank you, @0xb007ab1e, for such a fast and thorough rework. You took the structural fix from your own "Alternatives" section, and the result is much simpler: publication no longer writes ADRs at all, the capture and re-apply machinery and its fail-capture hook are gone, and BEGIN IMMEDIATE now runs on the connection it is meant to protect. That is exactly the direction we hoped for.

A few things before we can merge, each with the reason:

1. Migrate before a rebuild can destroy the legacy row. Migration currently runs lazily, on the first ADR read or write. But the format-change route (pipeline.c around 1749-1758) and the forced-full route (around 1770-1775) unlink the old generation, and a full rebuild publishes a fresh project_summaries. On an upgrade whose first action is a full reindex (a format bump, a forced full, or the watcher's startup reindex), the ADR is therefore lost before anything copies it. Could you migrate the legacy row into the sidecar at the start of routing, whenever an existing generation is present and before any unlink or publish, and preserve and abort if that fails? A done-marker in the sidecar (for example an adr_meta row) would make the migration truly one-time. Today it re-runs on every new connection and could bring back a deleted ADR from a delta-cloned legacy row.
2. Lock behaviour of the sidecar. Please open it the way the graph store is opened, with PRAGMA busy_timeout = 10000 and journal_mode = WAL (the bare sqlite3_open in store.c around 9694). Without them, a second process's write fails immediately with SQLITE_BUSY, and a read during a commit reports "no ADR". That is precisely the cross-process case this PR exists for.
3. Reads must not write. Query-only stores also carry db_path, so manage_adr get, index_repository's adr_present and GET /api/adr currently create <db>.adr.db. On a read-only cache, a legacy ADR that used to be readable now errors out. For reads, please open the sidecar SQLITE_OPEN_READONLY if it exists, and otherwise read the legacy row from the graph DB without migrating.
4. Path helper. Please build the sidecar path through cbm_path_for_file_api with a 4096-byte buffer, like store_open_internal. On Windows that adds the long-path \\?\ prefix; without it, a long cache path opens the graph but not the ADR.
5. Memory-core gate. store.c goes from 358 to 359 raw allocator sites against a baseline of 358, so make -f Makefile.cbm lint-memory-core will fail. adr_db_path is written but never read, so dropping the field and its free fixes it. Lowering the baselines for pipeline.c (to 129) and pipeline_incremental.c (to 134) in the same change is welcome.
6. The ADR should travel with the exported artifact. The team-shared .codebase-memory/graph.db.zst used to carry the ADR in project_summaries, so a teammate's first index started with the team's decisions. With the sidecar that silently stops. Please carry it across: write the sidecar's ADR into the exported DB, and restore it into the sidecar on import when no local ADR exists.
7. Tests. Please add:
   • (a) an ADR written from a pipeline test hook between the start of the reindex and publication survives, on both the full and the delta route;
   • (b) the upgrade order: seed a legacy row, run a forced-full or format-change reindex with no ADR read first, then read the ADR;
   • (c) delete_project through the MCP handler removes <db>.adr.db and its -wal/-shm/-journal, and a re-indexed project of the same name starts with no ADR;
   • (d) the artifact round trip keeps the ADR.
8. Docs and description. Please mention in the delete_project tool description and in the README that deleting a project also deletes its ADR, and update the PR description to the sidecar design. It still describes the lock and compat_fs.c.

A smaller note, not a blocker: the sidecar name ends in .db, so the CLI's cbm_list_indexes / count_db_indexes count it as an index. A one-line exclusion there would help.

Thanks again. The hard part is done, and this is close.

[0xb007ab1e]

Round 2 pushed — thank you for the precise, itemized review. All eight plus the minor:

1. Migrate before a rebuild. New cbm_store_adr_migrate_once runs at the start of both delete routes in pipeline.c (format-change and forced-full), before the cbm_unlink, and preserve-and-aborts (ABORT_PRESERVE_DB) on failure. It is one-time via an adr_meta migrated marker in the sidecar, so it never re-runs or resurrects a deleted row. Good catch on the delta-clone case. One extra subtlety I hit: routing's db_path is a staging path, so migration now targets the final path (staging suffix stripped) — otherwise the ADR landed in a discarded stage's sidecar; there's a test for it.
2. Lock behaviour. Sidecar opens via sqlite3_open_v2(RW|CREATE) + PRAGMA busy_timeout=10000 + journal_mode=WAL, matching the graph store (SQLite's own lock wait, no hand-rolled poll).
3. Reads must not write. cbm_store_adr_get reads the cached RW handle if one is open, else opens the sidecar SQLITE_OPEN_READONLY when the file exists, else reads the legacy graph row without creating anything. A query-only cache is never mutated.
4. Path helper. Sidecar path is built through cbm_path_for_file_api with a 4096-byte buffer.
5. Memory-core gate. Dropped the write-only adr_db_path field → store.c back to 358; lowered pipeline_incremental.c 144→134 and pipeline.c 134→131 (not 129 — migrate-before-delete adds two free(db_path) abort-branch sites; still down). lint-memory-core passes.
6. ADR travels with the export. cbm_artifact_export fetches the ADR from the sidecar (read-only) and injects it into the snapshot's project_summaries before compression; cbm_artifact_import restores it into the destination sidecar via migrate_once (INSERT OR IGNORE — never clobbers a local ADR).
7. Tests. pipeline_adr_written_during_reindex_survives (hook mid-reindex), pipeline_adr_legacy_migrates_before_forced_reindex (upgrade order, no prior read), tool_delete_project_removes_adr_sidecar (via the MCP handler; same-name re-index has no ADR), artifact_export_roundtrip_keeps_adr.
8. Docs. delete_project tool description + README note that deleting a project deletes its ADR. Minor: cbm_list_indexes/count_db_indexes now exclude *.adr.db.

Verified locally: -Werror clean, lint-memory-core passes, pipeline 289, store_arch mcp mcp_mutation_guard index_format artifact 475 (+4 skipped). Still only clang-format-19 here (CI pins 20) — git clang-format shows my changed lines clean; flag anything CI's 20 reformats and I'll match it.

[DeusData]

Thank you for round 2, @0xb007ab1e. This is a careful, thorough rework:

• Moving ADRs into the per-project sidecar removes the race rather than narrowing it, and deleting the capture and re-apply machinery made publication noticeably simpler.
• Reusing stage_root_length for the staging-to-final derivation was the right call. It recognises only the exact minted .stage.XXXXXX basename, so a real path containing .stage. is safe.
• You brought the memory-core baselines down in pipeline.c and pipeline_incremental.c while keeping store.c at 358. Appreciated.

Items 2, 3, 4, 5, 7c, 7d and 8 are done. A few small things before we merge:

1. The migrate-before-delete step fails open (pipeline.c around 1728-1731). cbm_store_open_path_query returns NULL for open failures as well as for a missing file, and on this route the final generation is known to exist. So if (!mig) return true; can let a rebuild publish over a legacy ADR it never moved. Suggested: if (!mig) return !cbm_file_exists(final_path);.
2. A deleted ADR can come back through the artifact. After migration, the legacy project_summaries row stays in the graph DB, and incremental and closure-repair clones keep carrying it. VACUUM INTO copies it into the snapshot. When the sidecar has no ADR, nothing is injected, the stale row ships, and a teammate's import restores it. Suggested: in snapshot_inject_adr, always DELETE FROM project_summaries first, then insert only when an ADR exists.
3. Make the one-time migration atomic (store.c around 9673-9768). Please wrap the marker check, the legacy copy and the marker insert in one BEGIN IMMEDIATE … COMMIT. Otherwise two processes migrating at once can re-copy a row the other just deleted. It would also help if cbm_store_adr_get fell back to the legacy graph row when the sidecar exists but has no migrated marker, so a half-created sidecar never hides an ADR.
4. Tests.
   • Please add the delta / closure-repair variant of the ADR-written-mid-reindex test.
   • Pin the route in both new pipeline tests with cbm_pipeline_incremental_test_last_route() (FORCED_FULL, and CLOSURE_REPAIR for the delta one), so they stay bound to their routes if routing changes.
   • The preserve-and-abort path on migration failure lost its test when the old hook went away. Putting a directory at <db>.adr.db should make it fail deterministically with no hook: assert CBM_PIPELINE_ABORT_PRESERVE_DB and that the old generation is intact.

Nits, take them or leave them:

• Give the read-only sidecar handle the same 10 s busy timeout.
• adr_sidecar_path should compare (size_t)n >= raw_sz rather than against an (int) cast.
• In delete_project, remove the sidecar only once the graph DB unlink has succeeded, and size adr_path like path plus the suffix.
• is_project_db_file (mcp.c) and the cross-repo cache scan could skip *.adr.db the way the CLI listing now does.
• In the PR body, "migrated on first access" now means "on first write or before the next rebuild".

Thanks again. This is close, and the design is exactly where we wanted it.

[0xb007ab1e]

Round 3 pushed — thank you again for the depth here.

Correctness

1. Fail-open fixed. adr_migrate_before_delete now returns !cbm_file_exists(final_path) when the query open returns NULL, so a real open failure preserves-and-aborts instead of publishing over an un-migrated ADR.
2. No resurrection via the artifact. snapshot_inject_adr always DELETE FROM project_summaries first and inserts only when an ADR is present, so a no-ADR export ships an empty table and a lingering legacy row can't ride along.
3. Atomic migration + half-created fallback. adr_copy_legacy_once wraps the marker check, copy, and marker insert in one BEGIN IMMEDIATE … COMMIT (re-checking the marker under the lock; ROLLBACK→ERR on failure). cbm_store_adr_get now, when the sidecar exists but is unmarked, falls back to the legacy graph row so a half-created sidecar never hides an ADR.

Tests

• Both survival tests are route-pinned (FORCED_FULL and CLOSURE_REPAIR) via cbm_pipeline_incremental_test_last_route().
• pipeline_adr_migrate_failure_preserves_generation: a directory at <db>.adr.db fails migration deterministically (no hook) → asserts CBM_PIPELINE_ABORT_PRESERVE_DB and an unchanged node count.
• Extra edge regressions I added while hardening: an export with a deleted ADR does not resurrect the legacy row on import; an import does not clobber a local ADR (INSERT OR IGNORE); and a half-created sidecar falls back to the legacy row.

Nits — all applied: the read-only handle gets the 10 s busy timeout; adr_sidecar_path compares (size_t)n >= raw_sz; delete_project removes the sidecar only after the graph unlink succeeds and sizes the buffer as the graph path plus the suffix; is_project_db_file skips *.adr.db. PR description updated to "migrated on first write or before the next rebuild."

Verified locally: -Werror clean, lint-memory-core passes, and the ADR/sidecar suites (store_arch, pipeline, mcp, artifact) pass with zero failures under ASan/UBSan. (The monolithic make test also surfaces pre-existing LSAN leaks in the unrelated test_extraction/test_parallel spill suites — untouched by this PR and absent from every ADR/sidecar path.)

If any other maintainers or contributors want to weigh in on the sidecar approach, the migration ordering, or edges I might have missed, I'd welcome the input — happy to adjust.