Skip to content

fix(code): rebase concurrent map updates - #2324

Merged
oceanwaves630 merged 4 commits into
mainfrom
codex/code-map-atomic-updates
Sep 11, 2026
Merged

fix(code): rebase concurrent map updates#2324
oceanwaves630 merged 4 commits into
mainfrom
codex/code-map-atomic-updates

Conversation

@ohdearquant

Copy link
Copy Markdown
Owner

Summary

  • add conditional entity/edge inserts as the missing-row companion to guarded replacement
  • reapply semantic deltas against fresh code-map rows across all entity and edge read-modify-write sites
  • advance revisions strictly and emit FTS/report effects only after one write wins
  • document the same-map concurrency contract with no schema or wire change

Regression proof

Before the fix, forced same-revision interleavings lost one unresolved specifier and one dependency-evidence kind. The same two-runtime tests now retain both writers, advance the winning revision, and record each entity FTS effect once.

Verification

  • cargo test -p khive-pack-code (159 tests passed)
  • cargo test -p khive-storage (137 tests passed)
  • cargo test -p khive-db --lib (839 passed; one unrelated walpin timestamp-boundary flake passed on exact isolated rerun)
  • cargo check --workspace --all-targets
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo fmt --all --manifest-path crates/Cargo.toml -- --check
  • git diff --check

Closes #2231

@ohdearquant

Copy link
Copy Markdown
Owner Author

Reviewed at head 097f506f25b63b9ad6cbf25edf98ea817125d648. The storage half of this is well built — the
conditional inserts are genuinely conditional, the revision rule is enforced on both sides, and the
effect gating holds. Two things need work before it lands, and the more important one is in the tests
rather than the code.

The two race tests cannot fail on the implementation they exist to disprove

race_seam::pause_after_row_read() has exactly two call sites, and both are inside the new helpers:

740:        race_seam::pause_after_row_read().await;   // in mutate_entity, after get_entity_including_deleted
806:        race_seam::pause_after_row_read().await;   // in mutate_edge,   after get_edge_including_deleted

Both are #[cfg(test)] lines within mutate_entity / mutate_edge, which are new in this change. So
if the implementation is reverted to the old read-then-unconditional-upsert path, those call sites
disappear with it, the installed task-local is never consulted, and neither task ever reaches the
barrier. The two writers then interleave — or don't — purely by scheduler luck, and both tests can go
green against the exact lost-update implementation they are advertised as proving is broken.

That makes them change-detectors for the new code rather than regression proofs for the bug. It
matters here specifically because ADR-085 Amendment 8 cites forced entity/edge races as a requirement,
and because a concurrency test that passes by scheduling is one that will also go green on its own the
first time it flakes — the failure is silent in both directions.

The fix is to put the forced interleaving on a seam that exists in both implementations — a pause
injected at the storage read, or an explicit "both snapshots taken before either write" construction —
and then demonstrate the test red against the old path. Until a test has been observed failing for the
right reason, it has not proved anything about the bug.

The L2 sweep decides outside the rebase boundary, so "every RMW" is not true yet

run_l2_sweep reads the module once and keeps using that read across the guarded write:

let existing_module = get_entity_opt(rt, token, precomputed_module_id).await?;   // :3205
let needs_reparse = l2_needs_reparse(/* ...from existing_module... */);          // :3206
let Some(module_id) = upsert_module(..., !needs_reparse, ...)                    // :3232
...
if !needs_reparse {
    let declaration_ids = existing_module.as_ref()...                            // :3252

mutate_entity re-reads on a CAS retry, but it cannot recompute this, because the semantic decision
was made outside the helper. So a writer that loses, rebases and wins still applies its captured
preserve_l2_state, and then refreshes and marks current a declaration list read before the
conflicting write.

Worth being precise about provenance, because it changes what the fix is. This shape is not new — main
carries the same read-once-then-reuse structure at source_ingest.rs:3012-3060. What is new is the
retry loop underneath it: on main the write simply clobbered, so there was no rebase for the decision
to fall out of step with. This change creates the rebase and leaves the decision behind it.

So this is an incomplete fix rather than a regression, and it reads as a defect mainly because of what
the diff also asserts. ADR-085 Amendment 8 promises that every code-map entity/edge RMW uses fresh
rebasing; this path does not. Either move the reparse decision and declaration selection inside the
mutation/retry boundary (or re-derive them from the winning row after upsert_module), or scope the
ADR sentence to the primitives actually covered. The ADR claim and the code should not disagree, and
right now the ADR is the more optimistic of the two.

Natural-key insert loss retries the same absent id

mutate_edge reads get_edge_including_deleted(link_id) for the caller's id only. When
insert_edge_if_absent returns false because a different-id row won on the natural key, the next
iteration reads the same still-absent id and reissues the same insert until the attempt budget is
exhausted, then errors. The winner is never read and the delta is never reapplied.

Reach bounds this: source-ingest callers derive edge_uuid deterministically, so id and natural key
move together and the divergent case is not reachable from them. But upsert_edge accepts an
arbitrary id and the storage contract explicitly covers natural-key conflicts, so the helper's identity
assumption is currently implicit. Either resolve the natural-key winner after a failed absent insert,
or state the deterministic-id precondition in the helper and reject a violation loudly rather than
spending 16 attempts on it.

What holds up

  • The conditional inserts are database-conditional, not check-then-write —
    INSERT ... ON CONFLICT DO NOTHING at entity.rs:107-119 and graph.rs:130-143, so no isolation
    argument is load-bearing.
  • The trait defaults fail closed, and for the stated reason. insert_entity_if_absent and
    insert_edge_if_absent default to Unsupported with the rationale written into the doc comment:
    falling back to upsert "would overwrite the winning row and violate this method's conditional-insert
    contract." That is the fallback most implementations reach for, and declining it is the right call.
  • Revisions advance strictly on both sidesmax(requested, current + 1) with checked overflow
    in Rust, and the CAS predicate independently requires the replacement revision to exceed the stored
    one, so two writers on the same read cannot both win.
  • Effects are gated on the winning outcome. FTS indexing runs only on Some(outcome), and report
    counters increment after a successful mutation, so a losing attempt reaches neither. Note this
    prevents duplicate and loser effects but does not make the row and the FTS write atomic — a later
    winner can still index after an earlier one, and nothing instruments that ordering.

Coverage gaps worth naming

Beyond the revert-red problem: no test covers a retry whose second attempt also conflicts, a
concurrent conditional insert of the same key, the different-id natural-key winner through
mutate_edge, or the exact revision sequence for two successive winners. The added tests assert final
state and updated_at against a baseline, which cannot distinguish "rebased correctly" from "raced and
happened to end up right."

No builds or tests were run for this pass; every claim above was checked against source at the head
named, including the base-ref comparison, and the test-plan checkboxes in the description were not
relied on.

@ohdearquant
ohdearquant marked this pull request as ready for review September 1, 2026 16:39

@ohdearquant ohdearquant left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head 964a72b: REQUEST-CHANGES, 3 blocking findings. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.

One conflict, in `crates/khive-db/src/stores/graph.rs`, and it was an insertion
collision rather than a disagreement: both sides added a new edge-insert helper
immediately after `edge_upsert_statement`, so git could not tell they were
different functions. Both are kept.

- main's `edge_insert_only_guarded_by_endpoints_statement` inserts only while
  both endpoints still exist.
- this branch's `edge_insert_if_absent_statement` leaves an existing edge
  untouched on either conflict so the caller can read the winner.

`cargo check -p khive-db --all-targets` is clean at this tree, which is the
control that matters here: the two functions returning the same `statement`
binding is exactly the shape a careless resolution would have silently merged
into one.
@oceanwaves630
oceanwaves630 merged commit d53f7b0 into main Sep 11, 2026
29 checks passed
@oceanwaves630
oceanwaves630 deleted the codex/code-map-atomic-updates branch September 11, 2026 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

storage(code): full-row read-modify-write in the code map can lose concurrent updates

2 participants