fix(pipeline): preserve ADRs during incremental reindex

[M-Marbouh]

What does this PR do?

Preserves manage_adr content when index_repository takes the incremental reindex path.

This is a follow-up to #516. The full reindex path already captures ADR content from project_summaries.summary before replacing the SQLite DB and restores it afterward. The incremental path also replaces the DB in dump_and_persist(), but it only repersisted graph data, file hashes, coverage rows, and FTS state. Any ADR stored through manage_adr was therefore silently dropped on the next incremental rebuild.

The failure shape is:

1. Index a project.
2. Store ADR content with manage_adr mode update.
3. Read it back with mode get; content is present.
4. Change an existing source file so the next index_repository routes through incremental reindex.
5. Read the ADR again; before this fix it could return successfully with empty content because project_summaries was recreated without the previous summary.

This PR mirrors the full-reindex preservation behavior in src/pipeline/pipeline_incremental.c:

• before unlinking/rebuilding the DB, read any existing ADR content for the project;
• after cbm_gbuf_dump_to_sqlite() recreates the DB and it is reopened, write that ADR content back with cbm_store_adr_store();
• log a restore failure instead of silently losing the ADR.

It also adds pipeline_adr_survives_incremental_reindex, which indexes a tiny repo, stores an ADR, changes one existing file to force the incremental route, and asserts that the ADR content is still present afterward.

This PR is intentionally scoped to ADR loss during incremental reindex. It does not attempt to address any separate stale-WAL/listed-but-unqueryable project lookup behavior.

Verification

• git fetch origin main and rebase onto current origin/main (6b57db2) succeeded without conflicts.
• make -j2 -f Makefile.cbm cbm succeeded.
• Direct MCP repro against the rebuilt binary passed:
  • manage_adr update stored content;
  • immediate manage_adr get returned the content;
  • a second index_repository after changing one file reported "adr_present":true;
  • post-incremental manage_adr get returned the original content.
• make -j2 -f Makefile.cbm build/c/test-runner succeeded.
• ASAN_OPTIONS=detect_leaks=0 build/c/test-runner pipeline reached and passed the new pipeline_adr_survives_incremental_reindex test, including the pipeline.route path=incremental branch.
• The selected pipeline suite still exits non-zero with an unrelated existing failure: pipeline_backpressure_futile_nap_disengages at tests/test_pipeline.c:6801 (220 passed, 1 failed).

Checklist

• Every commit is signed off (git commit -s) — required, CI rejects
  unsigned commits (DCO, see CONTRIBUTING.md)
• Tests pass locally (make -f Makefile.cbm test)
• Lint passes (make -f Makefile.cbm lint-ci)
• New behavior is covered by a test (reproduce-first for bug fixes)

[DeusData]

Thanks for the ADR persistence fix. I have triaged this into 0.9.1-rc as a normal-priority UX/behavior bug fix. Review focus will be ADR state across incremental DB replacement and making sure we do not restore stale summary data by accident.

[M-Marbouh]

Thanks.
That review focus makes sense. The intended invariant here is the same as the full-reindex path from #516: capture the ADR for the exact project key from the existing DB immediately before the DB replacement, then restore it to the same project key immediately after cbm_gbuf_dump_to_sqlite() recreates the DB.
I kept the PR scoped to preserving project_summaries.summary across the incremental replacement path only. If you want an additional guard/test around stale summary restoration, I’m happy to add it.

[DeusData]

Reviewed and verified — this is correct and ready; it just needs a rebase onto current main (which moved since you opened it). I did the rebase locally to confirm the resolution and that the fix still works, so here's exactly what's needed:

Verified: both pipeline_adr_survives_full_reindex and pipeline_adr_survives_incremental_reindex pass on the rebased branch, and the new incremental test is a genuine guard — it fails RED without your ADR-restore change (the ADR is dropped, cbm_store_adr_store never runs), GREEN with it. lint clean. Security/scope clean (the two fopens are test-fixture writes).

The only conflict is in dump_and_persist() in pipeline_incremental.c: since you branched, persist_hashes was changed to return bool hash_records_complete. Your ADR restore just needs to sit before it. The resolved block:

        /* #992: restore the captured ADR before persisting hashes, mirroring the
         * full-reindex path (#516) — the DB replacement above dropped it. */
        if (saved_adr && cbm_store_adr_store(hash_store, project, saved_adr) != CBM_STORE_OK) {
            cbm_log_error("incremental.err", "msg", "adr_restore", "project", project);
        }

        bool hash_records_complete = persist_hashes(hash_store, project, files, file_count,
                                                    mode_skipped, mode_skipped_count);

(Your saved_adr capture before the dump and the free(saved_adr) after are unaffected — only this restore site conflicts.)

Rebase onto current main with that resolution and push; once it's green I'll merge immediately. Thanks — preventing ADR loss on the incremental path is a genuine data-safety fix, and mirroring #516's invariant is exactly right.

[M-Marbouh]

Rebased onto current main and pushed 207e9a5.

Resolved the dump_and_persist() conflict by restoring the saved ADR immediately before the new bool hash_records_complete = persist_hashes(...) call, as suggested.

Local verification:

• git diff --check passed
• bash scripts/check-no-test-skips.sh passed
• make -f Makefile.cbm lint-no-suppress passed
• ASAN_OPTIONS=detect_leaks=0 make -f Makefile.cbm test-focused TEST_SUITES=pipeline passed: 222 tests, including pipeline_adr_survives_full_reindex and pipeline_adr_survives_incremental_reindex

I could not run lint-format locally because this environment currently has no clang-format on PATH, but CI lint is now running on the pushed commit.

Thank you.

[DeusData]

Reviewed, and I want to lead with this: your premise is correct and this is a real user data-loss bug. I verified it from the code rather than taking the description's word for it, because a data-loss claim deserves that.

dump_and_persist (src/pipeline/pipeline_incremental.c:640) unlinks the database and its sidecars, rebuilds via cbm_gbuf_dump_to_sqlite, and then re-persists only file hashes, coverage rows and FTS. project_summaries appears nowhere in that file — git grep returns zero hits — and the gbuf dump never touches that table either. So on any incremental rebuild (that is, any run where a file actually changed or was deleted; only the no-op fast path leaves the DB alone) a DB-stored ADR is silently dropped.

The full-reindex path already protects them via the #516 work at pipeline.c:1193/1298, and a mode-change forced rebuild routes through that same branch. The incremental path is the genuine gap, and it is exactly the one you fixed. Thank you for scoping it that tightly — a +80/−0 diff that fixes user data loss is a good trade.

Two more things I checked that you got right:

• Restore ordering. Writing the ADR back before the trailing cbm_artifact_export means auto-updated artifacts carry it, since the export is a VACUUM INTO whole-DB snapshot. Getting that order wrong would have produced artifacts silently missing the ADR.
• The test is genuinely binding. Without the fix, the rebuilt DB has no project_summaries row, cbm_store_adr_get returns CBM_STORE_NOT_FOUND, and your ASSERT_EQ(rc, CBM_STORE_OK) goes red. That is precisely the kind of reproduction a data-loss claim needs, and it is not vacuous.

What I need before it can merge: a rebase, plus one thing to watch while you do it.

dump_and_persist on current main has been refactored since you branched — it is now static int with early returns at roughly :650 (clear-staging failure), :657 (dump failure) and :663 (open failure), and its signature changed. That is mechanical drift rather than a competing fix; nothing on main has addressed the ADR gap.

The hazard: a straightforward rebase that keeps capture-at-the-top and free(saved_adr) only at the tail would leak on every one of those early-return paths. Either free on all exits, or hang it off a struct that gets freed centrally — the full-reindex path does the latter around pipeline.c:252-254 if you want a pattern to copy.

One optional hardening: nothing in the test asserts which route was actually taken, so a future change to the routing thresholds could quietly turn it vacuous. Asserting the pipeline.route path=incremental line would close that. The sibling #516 test has the same looseness, so this is a suggestion rather than a requirement.

Two last notes, both pre-existing and shared with the #516 path rather than introduced by you: the restore resets created_at/updated_at because the row is re-inserted, and a strdup OOM would silently drop the ADR. Not blockers, just worth knowing.

Rebase it and I will pick this straight back up — the fix itself is right and I would like it on main.

[M-Marbouh]

Thanks for the thorough review. The exact failure analysis and the warning about the new early returns made the rebase straightforward.

I’ve rebased #992 onto current main. saved_adr now has a single cleanup path, so the clear-staging, dump, and reopen failures all release it. I also took the optional hardening: the regression now proves it observed pipeline.route path=incremental before checking the ADR.

I verified the signal on the rebased tree:

• Fixed: pipeline suite passes 239/239.
• Negative control with only the ADR restoration removed: the incremental-route assertion still passed, then ADR lookup returned CBM_STORE_NOT_FOUND (238 passed, 1 failed).
• git diff --check is clean.

No unrelated changes were bundled. Thanks again for verifying the original data-loss path so carefully.

[DeusData]

A quick note so you are not left looking at two red checks wondering what you broke: neither is yours, and I have re-run them.

Both failed inside the same minute, and both are network:

• lint — the job could not install clang-format-20: Failed to fetch http://apt.llvm.org/...clang-format-20_...deb — Cannot initiate the connection to apt.llvm.org:80 (Network is unreachable). It never reached your code.
• changes — Get "https://api.github.com/repos/.../pulls/992/files": dial tcp 140.82.112.5:443: i/o timeout. That job only asks GitHub which files the PR touches, so it also never reached your code. ci-ok is just the aggregate of the two.

A re-run is the right fix here rather than a rebase: a re-run re-tests the same recorded merge commit, which is exactly what you want when the failure was a transient network blip rather than something stale in the base.

Separately, thank you for the negative control. Removing only the ADR restoration and showing that the incremental-route assertion still passed before the ADR lookup returned CBM_STORE_NOT_FOUND is the part that matters most on this fix — it proves the test is failing for the right reason, not because the run quietly took the full-rebuild path where ADRs survive anyway. That distinction is easy to get wrong and easy to skip, and it is what makes the 238/1 result meaningful.

The content review is in progress and I will come back with it shortly.

[DeusData]

Reviewed the rebased version in full. The two things I warned about last round are both genuinely fixed, and I verified them by enumeration rather than by reading the description.

The single cleanup path holds. I walked every exit between the capture and the free:

• clear_staging_failed → goto cleanup (rc stays CBM_STORE_ERR)
• dump_rc != 0 → rc = dump_rc; goto cleanup
• open_staging_after_dump → goto cleanup
• normal completion → falls through to cleanup:

There is no return anywhere between saved_adr = strdup(...) and cleanup:, free(saved_adr) appears exactly once, and nothing else frees it. No leak on any path, no double-free.

And the test binds for the right reason, which on this fix matters more than that it binds at all. Capturing pipeline.route ... incremental through a log sink and asserting g_incremental_route_seen means it cannot pass by quietly taking the full-rebuild path — where ADRs survive anyway and the test would be measuring nothing. Your negative control (238/1 with the route assertion still passing before the ADR lookup returned CBM_STORE_NOT_FOUND) is the evidence that closes it. I also noticed you restore the sink before the assertions rather than after, so a failing run cannot leave a global sink installed for the rest of the suite. That is the kind of detail that usually gets found the hard way.

One thing I would like changed before merge, and it is three characters.

The ADR restore logs on failure but does not affect rc:

if (saved_adr && cbm_store_adr_store(hash_store, project, saved_adr) != CBM_STORE_OK) {
    cbm_log_error("incremental.err", "msg", "adr_restore", "project", project);
}

The coverage write forty lines below does the opposite:

if (cbm_store_coverage_replace_ex(...) != CBM_STORE_OK) {
    cbm_log_error("incremental.err", "msg", "persist_coverage", "project", project);
    rc = CBM_STORE_ERR;
}

So if the restore fails, the old database has already been unlinked, the ADR is not in the new one, and the reindex reports success. That is the same silent-data-loss shape this PR exists to remove, just in a much rarer branch — and it is inconsistent with the convention the surrounding code already follows. Adding rc = CBM_STORE_ERR; to that branch makes the failure observable.

I recognise there is a judgement call underneath it: failing the whole reindex because an ADR could not be restored costs the user their index refresh too. My view is that a loud failure is the right trade for a fix whose entire purpose is that user-authored content must not disappear quietly — but if you disagree, say so and I will take it to the maintainer rather than treat it as settled.

On the two red checks: neither is yours, and I re-ran them — lint could not reach apt.llvm.org to install clang-format-20, and changes timed out calling the GitHub API. Details in my earlier comment.

Worth stating plainly, since it has been open a while: this closes a real user-data-loss path. Incremental reindex was discarding user-authored ADRs, and a routine watcher-driven refresh was enough to trigger it.

[M-Marbouh]

Thanks for catching this. I agree that an ADR restore failure needs to fail the incremental index instead of continuing successfully.

I pushed the follow-up in 0cdd6fbd. One detail turned out to matter when I checked the caller contract: CBM_STORE_ERR is -1, and this path interprets -1 as “fall back to a full rebuild.” That would recover through a different route instead of returning a terminal failure.

The change now records the restore failure, completes the remaining staging maintenance, and returns the existing CBM_PIPELINE_ABORT_PRESERVE_DB signal. This causes the staging database to be discarded while leaving the committed database untouched.

I also forced this failure path locally. The probe confirmed that the route was incremental, the replacement dump ran, the operation returned failure, and the original ADR remained byte-identical. After removing the injection, the normal sanitized pipeline suite passed at 239/239.

Thanks for pointing out the silent-success gap.