Repository navigation
fix(store): ADR sidecar follow-ups from #2315 review - #2864
Merged
Merged
Conversation
Three small items promised in the #2315 review; behaviour is otherwise unchanged. 1. A failed ADR restore on artifact import is no longer silent (src/pipeline/artifact.c). The import dropped both a failed reopen of the installed DB and the status of cbm_store_adr_restore_from_graph_if_absent, so a team ADR that never reached the sidecar left no trace. The restore moves into artifact_import_restore_adr, which logs level=warn msg=artifact.import err=adr_restore detail=<store error> path=<db> on either failure. The import still returns 0: the graph DB is already installed when the restore runs, and the restore stays best-effort as documented. adr_restore_if_absent now sets the store error for its begin, copy and commit failures, so the logged detail names the failing step. 2. The doc comment of adr_copy_legacy_once sat above adr_copy_rows, stacked on top of the comment that documents adr_copy_rows, which left adr_copy_legacy_once undocumented. It now sits above adr_copy_legacy_once (src/store/store.c). 3. adr_restore_if_absent no longer inserts the migrated marker. That INSERT OR IGNORE could never change state. Its only caller, cbm_store_adr_restore_from_graph_if_absent, takes the sidecar handle from adr_conn_write, which hands out a handle (cached or fresh) only after adr_copy_legacy_once returned OK, and that function returns OK only when the marker row is already committed (fast path, or the re-check under the write lock) or it just inserted and committed it. No code deletes an adr_meta row, so the insert hit the existing primary key every time, and its result was discarded anyway. Tests: artifact_import_logs_failed_adr_restore puts a directory at <db>.adr.db (the hook-free trick from the #2315 pipeline test), then asserts the import still succeeds, the graph DB is installed, and the warn line carries the store reason. It is red without the artifact.c change. artifact_export_roundtrip_keeps_adr now also asserts that the fresh destination sidecar is marked migrated after the import, which pins the invariant item 3 relies on. Refs #2315 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #2315: the three small points we said we would handle ourselves
in the 2026-09-28 review. No behaviour change beyond the new log line.
After the imported graph DB is installed, cbm_artifact_import restores the
artifact ADR into the .adr.db sidecar. A failed reopen and a failed
cbm_store_adr_restore_from_graph_if_absent were both discarded, so a team
ADR that never arrived left no trace. The restore now lives in
artifact_import_restore_adr and logs
level=warn msg=artifact.import err=adr_restore detail= path=.
The import still returns 0: the graph DB is already installed and the
restore stays best-effort. adr_restore_if_absent now sets the store error
for its begin, copy and commit failures, so the detail names the step.
adr_copy_legacy_once sat above adr_copy_rows, stacked on top of the comment
that documents adr_copy_rows. It now sits above adr_copy_legacy_once.
INSERT OR IGNORE INTO adr_meta (migrated), but its handle always comes from
adr_conn_write, which hands out a sidecar handle only after
adr_copy_legacy_once has committed that marker, and nothing deletes it.
The insert always hit the existing key (its result was discarded too),
so it is removed.
Test plan
(the hook-free trick from fix(pipeline): preserve concurrent manage_adr write across reindex swap #2315) makes the sidecar open fail. Asserts the
import still returns 0, the graph DB is installed, and the warn line
carries the store reason. Red without the artifact.c change, green with it.
destination sidecar is marked migrated after an import, the invariant
item 3 relies on.
mcp 399, incremental 164, httpd 95, cli 373, daemon_application 91 - all
green (only the existing Windows-only platform skips).
clean, memory-core ratchet unchanged (no file grew), NOLINT gate clean.
Refs #2315. Follow-up to #2315 by @0xb007ab1e - thank you again for the
sidecar design and for carrying it through every review round.