Skip to content

fix(graph): re-parse a cached entry that holds a parse error - #317

Open
datrixlab wants to merge 1 commit into
trailhq:mainfrom
datrixlab:fix/no-replay-cached-parse-errors
Open

fix(graph): re-parse a cached entry that holds a parse error#317
datrixlab wants to merge 1 commit into
trailhq:mainfrom
datrixlab:fix/no-replay-cached-parse-errors

Conversation

@datrixlab

Copy link
Copy Markdown

What

An extraction-cache entry that carries an error is no longer replayed. buildGraph re-parses it instead, so a failure that belonged to the run cannot outlive the run.

-    if (cached && hash === cached.hash) {
+    if (cached && hash === cached.hash && !cached.error) {
       entries[rel] = { ...cached, size: f.size, mtimeMs: f.mtimeMs };
       sources.set(rel, source);
       reused++;
-      if (cached.error) {
-        errors.push(cached.error); // this file failed to parse last time too
-        return;
-      }

Fixes #312

Why this option

@Ercaner1988 listed three. This is option 2 — persist the entry, never let it stand in for a result — and the other two both have a hole:

  • Never persisting the entry costs the thing the entry exists for. The comment on the read-error path spells it out: without a record, the freshness probe reports the file as new on every query, so graft check reports drift that a rebuild can't clear. Persisting without the error field is worse still: the file would then replay as successfully parsed with zero nodes, and the error disappears instead of merely being wrong.
  • Classifying transient vs deterministic means string-matching Aborted() and RuntimeError: memory access out of bounds against messages that come out of wasm, which is a list to keep up to date and a silent wrong answer whenever it's out of date.

Re-parsing is cheap by construction: the only entries it re-parses are the ones that produced no nodes. On a healthy repo that's zero files.

The self-healing property matters as much as the fix. A cache poisoned by a run that has since been fixed — a smaller workspace, a --only-dir, more memory — repairs itself on the next ordinary graft build. Today it needs someone to know that --no-reuse exists, which requires first noticing a graph that reports and exit 0.

The docstring already promised this

ExtractEntry.error in src/graph/extract-cache.ts said:

replaying it re-reports the same error and contributes no nodes, exactly as a cold build would

That is the invariant the whole file is built on — its header states it twice ("an incremental build must produce a byte-identical wiring.json to a cold one"). A replayed abort breaks it: the cold build parses kanit.rs and gets 11 nodes, the incremental one replays a failure and gets none. The comment has been updated to say what the code now does.

The same repo already gets this right one line up

A file that can't be read recovers by itself. an unreadable file is still recorded, so it can't look new on every probe chmods a file to 000, builds twice, chmods it back, and asserts the next ordinary build indexes it — no --no-reuse needed. That works because a read error is recorded with hash: "", which can never match, so the entry is retried every build.

A file that can't be parsed gets a real hash, so it is never retried. Same cache, same kind of entry, opposite lifetime — and the parse case is the one that can fail for a reason that has nothing to do with the file.

Test

One test in test/graph-incremental.test.ts, which is where that invariant lives.

It builds cold, then poisons the memo the way a grammar abort does — the entry keeps the file's real content hash, so nothing about the file looks stale — and rebuilds normally. It asserts the three things the report distinguishes:

  • wiring.json is byte-identical to the cold build (the nodes come back)
  • languages still contains the language (the symptom: [python] vs [python, rust])
  • errors is empty (the stale message is not re-reported), and the entry on disk no longer carries one, so the next build is cheap again

Two-way: reverting only src/graph/build.ts fails it on the first assertion.

Not in scope

The report also suggests warning when a language that has files in the index produces zero nodes, and surfacing it in graft check. That's a real gap and it stays open with #312 — but it's a new signal with its own false-positive question (a language whose files are all .d.ts-shaped, say), not part of making the cache honest. Happy to follow up separately if you want it.

A grammar abort is a process-global, run-dependent failure, but the
extraction cache stores it as a per-file entry keyed on content hash and
replays it on every later build. An untouched file then stays broken
forever, and a whole language can drop out of a graph that still exits 0
with a success line.

Let an entry carrying an error fall through to the parse path instead of
being replayed. The entry is still persisted, so the freshness probe does
not report the file as new on every query; it just never stands in for a
result again. Re-parsing costs only files that produced no nodes, and a
run that succeeds repairs the entry.

Fixes trailhq#312
@trailhq-graft

trailhq-graft Bot commented Sep 9, 2026

Copy link
Copy Markdown

🌱 graft blast radius

1 area changed → 6 areas can be affected. 12 dependent symbols, depth 2.
Tests: 1 area updated its tests.
Tag: @anirudhkumar-nanonets — 5 of 7 areas · @shhdwi — 6 of 7 areas · @bhavesh-gupta-investis — Synchronous Execution

flowchart TB
  A0(("Pull Request Review<br/>4 symbols"))
  A1(("Graph Freshness<br/>3 symbols"))
  A2(("Graph Initialization<br/>2 symbols"))
  A3(("Viewer Build<br/>1 symbol"))
  A4(("Tool Invocation<br/>1 symbol"))
  AX(("1 smaller area<br/>1 symbol"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1,A2,A3,A4 reached;
  classDef tail fill:#EEF2F3,stroke:#9AA4A9,stroke-width:1px,color:#3A4247;
  class AX tail;
Loading
Can be affected Symbols Nearest hop Reached from
Pull Request Review 4 src/app/review.ts:L45-L99 reviewPullRequest — calls, depth 1 Graph Construction
Graph Freshness 3 src/graph/refresh.ts:L150-L227 ensureFreshGraph — calls, depth 1 Graph Construction
Graph Initialization 2 src/engine.ts:L91-L101 graph — calls, depth 1 Graph Construction
Viewer Build 1 scripts/build-viewer.mjs:L1-L45 build-viewer.mjs — calls, depth 2 Graph Construction
Tool Invocation 1 src/mcp/tools.ts:L216-L244 callTool — calls, depth 2 Graph Construction
Synchronous Execution 1 src/claude/sync-run.ts:L19-L33 runSync — calls, depth 2 Graph Construction
Who knows this code — 3 people across 7 areas
Area Who knows it
Graph Construction · changed @anirudhkumar-nanonets — 19 commits, last 20d ago · @shhdwi — 5 commits, last 28d ago
Pull Request Review · affected @anirudhkumar-nanonets — 5 commits, last 8d ago
Graph Freshness · affected @anirudhkumar-nanonets — 8 commits, last 1mo ago · @shhdwi — 3 commits, last 28d ago
Graph Initialization · affected @anirudhkumar-nanonets — 38 commits, last 7d ago · @shhdwi — 24 commits, last 27d ago
Viewer Build · affected @shhdwi — 2 commits, last 28d ago
Tool Invocation · affected @shhdwi — 14 commits, last 28d ago · @anirudhkumar-nanonets — 7 commits, last 7d ago
Synchronous Execution · affected @shhdwi — 3 commits, last 2mo ago · @bhavesh-gupta-investis — 1 commit, last 16d ago

Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no @ has no GitHub handle in its commit email — tag them by hand, or add a .mailmap entry. A suggestion from history, not a CODEOWNERS rule.

All 12 dependent symbols, grouped by area

Pull Request Review — 4 symbols in 4 files

  • src/app/review.ts:L45-L99 — reviewPullRequest (calls, depth 1)
    55: await buildGraph(checkout.dir);
  • src/app/review-process.ts:L179-L183 — childReviewer (references, depth 2)
  • src/app/review-worker.ts:L67-L87 — run (calls, depth 2)
  • src/app/server.ts:L31-L37 — AppSeams (references, depth 2)

Graph Freshness — 3 symbols in 2 files

  • src/graph/refresh.ts:L150-L227 — ensureFreshGraph (calls, depth 1)
    216: await buildGraph(dir, { contextDir: opts.contextDir, graphOnly: true, onlyDirs });
  • src/graph/refresh.ts:L235-L261 — ensureFreshChildren (calls, depth 2)
  • src/graph/workspace-cli.ts:L49-L70 — buildChild (calls, depth 2)

Graph Initialization — 2 symbols in 2 files

  • src/engine.ts:L91-L101 — graph (calls, depth 1)
    92: return buildGraph(dir, {
  • src/cli.ts:L163-L173 — refreshBefore (calls, depth 2)

Viewer Build — 1 symbol in 1 file

  • scripts/build-viewer.mjs:L1-L45 — build-viewer.mjs (calls, depth 2)
    3: * assets). Runs as part of `npm run build`; the bundle ships in the package

Tool Invocation — 1 symbol in 1 file

  • src/mcp/tools.ts:L216-L244 — callTool (calls, depth 2)

Synchronous Execution — 1 symbol in 1 file

  • src/claude/sync-run.ts:L19-L33 — runSync (calls, depth 2)
Test signal per changed area — 1 ✓

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • Graph Construction — 1 of 1 reached · 1 test file changed here: test/graph-incremental.test.ts
32 test suites also reference this code

40 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.

  • test/ask-index.test.ts
  • test/ask.test.ts
  • test/container-extract.test.ts
  • test/context-only-dir.test.ts
  • test/context.test.ts
  • test/covers.test.ts
  • test/generic-extract.test.ts
  • test/graph-go.test.ts
  • test/graph-invariants.test.ts
  • test/graph-java.test.ts
  • test/graph-languages.test.ts
  • test/graph-php.test.ts
  • test/graph-posix-paths.test.ts
  • test/graph-python.test.ts
  • test/graph-r-classes.test.ts
  • test/graph-r-phase3.test.ts
  • test/graph-r-phase4.test.ts
  • test/graph-r-phase5.test.ts
  • test/graph-r.test.ts
  • test/graph-references.test.ts
  • …12 more

graft blast · origin/main...HEAD · depth 2 · 3 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

github-actions Bot added a commit that referenced this pull request Sep 9, 2026
Ercaner1988 added a commit to Ercaner1988/Graft that referenced this pull request Sep 12, 2026
- generic-extract.test.ts: the union merge of trailhq#196, trailhq#200 and trailhq#201 spliced
  their appended tests into one another and left the file unterminated.
  Rebuilt as trailhq#196's file plus trailhq#200's and trailhq#201's appended blocks verbatim.
- The trailhq#139 test asserted that a cached parse failure is replayed without a
  re-parse; trailhq#317 (fixes trailhq#312) deliberately re-parses it instead and did not
  update the test. It now asserts the re-parse and that the error still
  comes back.
- The trailhq#319 symlink test (trailhq#328) needed symlink privilege on Windows. Directory
  links are junctions there, as test/graph-root and test/ingest-fs already
  do, and the file-link case is skipped with a diagnostic when it cannot be
  created.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

Transient grammar aborts are cached as per-file parse errors and replayed forever: a repo silently loses a whole language until --no-reuse

1 participant