Skip to content

fix(graph): stop a single write rebuilding the whole vault graph - #346

Closed
Artexis10 wants to merge 1 commit into
mainfrom
fix/graph-rebuild-outside-write-boundary
Closed

fix(graph): stop a single write rebuilding the whole vault graph#346
Artexis10 wants to merge 1 commit into
mainfrom
fix/graph-rebuild-outside-write-boundary

Conversation

@Artexis10

Copy link
Copy Markdown
Owner

A single ordinary write could rebuild the entire vault graph while holding the vault mutation boundary.

The path

commit -> index_sync -> epistemic_graph.upsert_after_write()
       -> EpistemicGraphIndex.refresh_paths()      # takes the mutation boundary
       -> _refresh_paths_locked()
            if not self.available():
                return self._rebuild_all_locked()  # full-vault rebuild, inside the boundary

available() is false whenever the sidecar is missing, unopenable, or schema-invalid — so the first write after the graph sidecar goes away rebuilds the whole vault inline.

Measured, not theorised

Our own CI benchmark recorded it while I was investigating something else:

vault mutation boundary held too long ... hold_ms=39092.51    # 2,000 pages
vault mutation boundary held too long ... hold_ms=172205.67   # 8,000 pages

39 seconds at 2k pages, 172 seconds at 8k. For that whole window every other vault mutation is blocked, and the writing client is free to give up on a write that then succeeds anyway.

I originally dismissed those log lines as a benchmark artefact, and #344 only gave them a name. That was wrong — the hold is real production behaviour on the write path.

The fix

A missing sidecar is drift, not a write's problem to solve. graph_drift() already reports precisely this condition ("graph sidecar missing, schema-mismatched, or relation-registry hash drift"), and reconcile.py:360 already calls rebuild_all() for it. So the write defers and lets the existing healing path do the work.

Availability is screened before acquiring the boundary, so an unbuildable graph does not even contend for it.

_refresh_paths_locked keeps a defensive check — the sidecar can vanish between the screen and the hold — but defers there too rather than rebuilding. The write hook already treats graph maintenance as best-effort; this makes a missing sidecar behave like the other best-effort failures instead of the one case that blocks the vault for minutes.

Verification

  • The tests catch the regression: with the fix stashed, test_a_write_does_not_rebuild_when_the_sidecar_is_missing, test_an_unavailable_graph_does_not_even_take_the_boundary, and test_the_write_hook_stays_bounded_without_a_sidecar all fail; with it, all pass. The other three are invariants — that drift is still reported, that rebuild_all still builds a usable sidecar and clears the drift, and that a healthy graph still indexes incrementally.
  • pytest tests/test_epistemic_graph.py tests/test_graph_write_boundary.py tests/test_reconcile.py tests/test_graph_lane_perf.py — the only failures are 14 in test_reconcile.py, identical with and without this change (14 failed / 18 passed both ways). Pre-existing Windows failures.
  • uvx ruff check — clean.

Full suite deferred to CI — native Windows can't run it.

Still open

rebuild_all itself still holds the boundary for its whole duration on the reconcile path. That is a background job rather than a user write, so a contending writer now fails fast on the bounded 5s wait instead of stalling — but blocking writes for 39–172s during a periodic reconcile is its own availability defect. Fixing it properly means restructuring the rebuild (build into a temp database, swap atomically) and deserves its own proposal rather than being folded in here.

Possible relevance to Yusuke's report

This is the best candidate yet for his 300-second remember that timed out client-side and then appeared on disk: he had just run a fresh exomem setup, and a first write against an unbuilt graph sidecar does a full inline rebuild. I am not claiming it as confirmed — it needs his re-test — but the shape matches, and unlike my earlier guesses this one is a measured behaviour on the actual write path.

upsert_after_write -> refresh_paths takes the vault mutation boundary, and whenever the graph sidecar was missing or schema-invalid it escalated to a full-vault _rebuild_all_locked() inside that boundary. CI measured the hold at 39s over 2,000 pages and 172s over 8,000: every other vault mutation blocked for the duration, and the writing client free to time out on a write that then landed anyway.

A missing sidecar is drift, not a write's problem to solve. graph_drift already reports exactly this condition and reconcile already owns rebuild_all for it, so the write defers instead. Availability is screened before the hold, so an unbuildable graph does not even contend for the boundary.

Three of the six new tests fail against the previous code and pass against this one; the rest pin that the healing path still builds the sidecar and clears the drift.
@Artexis10
Artexis10 marked this pull request as draft July 27, 2026 14:17
@Artexis10

Copy link
Copy Markdown
Owner Author

Converting to draft: the premise is wrong.

test_refresh_missing_sidecar_routes_to_full_rebuild asserts that a write against a missing sidecar indexes the whole vault (indexed_files == 2) and leaves available() is True. The escalation I removed is deliberate, named, and load-bearing — the write path is responsible for producing a usable graph, not just for deferring to reconcile. Three other tests depend on it too: the spawned-mutator boundary contract, forced re-resolution after a relation-registry change, and the schema-v3 sidecar migration.

So 'skip the rebuild' cannot be the fix. The 39s/172s boundary hold is still real, but the correct fix is the one I deferred in this PR's own description: keep the rebuild and stop it holding the global mutation boundary — build into a temp database and swap atomically. That belongs in its own change with a spec.

@Artexis10

Copy link
Copy Markdown
Owner Author

Closing as superseded by #347. This branch removes a load-bearing full-rebuild contract, and its draft note already confirms the premise is wrong. History and branch are preserved.

@Artexis10 Artexis10 closed this Jul 29, 2026
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.

1 participant