perf(callgraph): match the cold-build path's write mechanics on incremental refresh

[iceteaSA]

Four of the five findings from #214. One file, no new dependencies.

Finding 1 from that issue — the full-graph re-read via ProjectIndex::from_db_and_callers — is deliberately NOT here. It's architectural and you may well have a design in mind; mixing it in would make this unmergeable as a unit. This is the mechanical subset only.

What changed

Write transaction no longer spans file I/O and parsing. build_file_extract (read + tree-sitter parse) now runs before the transaction opens, collecting into changed_extracts / caller_extracts; the transaction then does only DB work. Previously the write lock was held across every read and parse in the batch.

Incremental inserts reuse the cold-build prepared statements. ColdBuildInsertStatements is constructed once per refresh transaction instead of the path issuing per-row tx.execute. Statement preparations per refresh drop from F+N+D+H+B+R+E to 7.

dispatch_hints gets an index on file. Two queries filter on that column; the only index was on method_name. Added at both schema sites — initialize_schema (fresh/open-time CREATE IF NOT EXISTS) and create_cold_build_secondary_indexes (cold-build recreation), plus the matching drop list, so a freshly-created DB and a rebuilt one don't diverge.

before:  SCAN dispatch_hints                                        (SELECT and DELETE)
after:   SEARCH dispatch_hints USING INDEX idx_dispatch_hints_file (file=?)

delete_ref_ids reuses two prepared statements instead of re-preparing two per id: 2R → 2 per call.

No public signature changed. aft_callgraph impact: load_file_row has one refresh caller, ref_ids_depending_on three plus a test.

One behaviour change, corrected from the original description

I originally wrote "no behaviour change." That was wrong, and you caught it on review — thanks.

clear_backend_state_for_file is now called on the refresh paths (2 call sites removed from the old inline path, 3 added in the restructured one; net +1). It is required by the mechanics switch: the prepared cold-build inserts write backend_file_state rows with a plain INSERT, so the row must be cleared first. It is also a correctness improvement in its own right — backend rows now refresh with the file instead of going stale.

Everything else is mechanics-only: same rows, same results, same ordering.

Verification

Rebased onto 10d6d20c (picks up the gh_shim XDG fixture fix).

4388 tests run: 4386 passed, 2 failed, 28 skipped

Identical failure names to unmodified base in the same checkout (post_edit_wait_does_not_start_a_server_on_a_cold_root, native_sandbox_pty_denies_outside_write_and_renders_screen — both environmental here). cargo fmt --check and bun run lint clean.

Worth flagging for your CI's sake: this was first gated in a linked worktree, where 19 callgraph_test::* tests fail as environment artifacts — a secondary checkout is a borrow-only artifact root, so the persisted callgraph store is unavailable and those tests fail identically on base. That means the tests covering the code this PR changes do not run in a worktree at all. I re-ran them in the artifact-owner checkout to get a real signal:

callgraph tests, owner checkout, with this change:   253 passed, 0 failed
same tests in a linked worktree:                      19 FAIL (borrow-only, base and head alike)

If your CI runs the suite in a linked worktree anywhere, this class of change is currently ungated there.

Performance evidence

Mechanical rather than benchmarked — there's no callgraph refresh benchmark in-tree and building one is a bigger piece of work than the fix:

	before	after
insert preparations / refresh txn	F+N+D+H+B+R+E	7
delete_ref_ids preparations	2R	2
dispatch_hints file-filtered queries	full scan	index seek
work inside the write txn	read + parse + DB	DB only

Happy to split this into four commits if you'd rather review them separately, or to drop any individual piece.

Greptile Summary

The PR reduces incremental callgraph refresh transaction duration and statement-preparation overhead while adding the missing file index for dispatch hints.

• Parses changed files and dependent callers before opening the write transaction.
• Reuses cold-build prepared statements for incremental inserts.
• Clears and refreshes backend file state on rewritten and deleted files.
• Reuses prepared deletion statements and indexes dispatch_hints.file.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
crates/aft/src/callgraph_store/mod.rs	Moves parsing outside the write transaction, reuses prepared statements, refreshes backend state, and adds the dispatch-hint file index without an eligible follow-up defect being established.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  F[Changed files] --> R[Read freshness and dependencies]
  R --> P[Parse changed files and callers]
  P --> T[Open SQLite transaction]
  T --> D[Delete superseded rows and backend state]
  D --> I[Insert extracts through prepared statements]
  I --> E[Resolve references and dispatch edges]
  E --> V[Bump projection revision]
  V --> C[Commit under lifecycle fence]

Loading

_{Reviews (3): Last reviewed commit: "perf(callgraph): match the cold-build pa..." | Re-trigger Greptile}

Context used:

• Knowledge Base — AFT Rust code intelligence

[cubic-dev-ai]

No issues found across 1 file

_{Re-trigger cubic}

[iceteaSA]

CI is red on two Windows jobs. Both fail on main at this PR's base commit, and neither failing test touches the code this PR changes — but the two have different evidential strength, so I'll separate them rather than wave both off.

28 of 30 checks green, including all three Linux integration shards, all three macOS shards, Windows shard 2/3 and 3/3, Windows deny-warnings, MSRV, and every E2E job.

Job 1 — Cargo libtest + watcher (Windows): proven pre-existing

Byte-identical failure on main @ 1d2da675 (run 32067609839, 2026-08-17) — same test, same assertion, same line:

thread 'gh_shim::tests::xdg_connection_config_precedes_home_config' panicked at crates\aft\src\gh_shim.rs:2078:9:
assertion `left == right` failed
  left: None
 right: Some("C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\...\\xdg-connection.json")

Identical on this PR. XDG config precedence in gh_shim; nothing in this diff is reachable from it.

Job 2 — Cargo integration shard 1/3 (Windows): same job red on main, but a different test

Being precise here because same-job-fails is not the same claim as same-failure:

main @ 1d2da675 (my base):  lsp_manager_test::astro_uses_the_nearest_project_typescript_sdk
this PR:                    configure_test::configure_does_not_warn_for_file_discovered_non_auto_installable_lsp
                            → "timed out after 10s waiting for aft process exit"

That job is red on 3 of the last 4 Tests runs on main, with the failing test rotating. Consistent with a Windows timing flake surfacing through nextest's fail-fast: the base run reached 317/493 before tripping, mine reached 123/493, so which test fails first moves with timing rather than with content.

Both are LSP/process-lifecycle tests. This PR touches only callgraph_store/mod.rs — no LSP path, no process lifecycle, no gh_shim.

What I can prove: job 1 is identical on base. Job 2's job is chronically red on main with rotating tests, and its failure mode is a process-exit timeout rather than a wrong result.
What I can't prove: that job 2's specific test would pass on this sha, since I can't re-run jobs as an outside contributor and the failure doesn't reproduce on Linux.

The callgraph tests — the ones that actually cover this change — pass 253/253, though only in an artifact-owner checkout, per the worktree caveat in the PR description.

Happy to rebase onto a green main once those two Windows jobs are sorted, if you'd rather see a clean run before reviewing.

[alfonso-aft]

Both halves of your CI analysis verified and acted on — this is exactly the evidential split I'd want (byte-identical-on-base vs same-job-different-test, argued separately):

1. Job 1 confirmed as ours, now fixed on main (10d6d20c): the gh_shim XDG fixture wrote Path::display() raw into a JSON string — on Windows that's unescaped backslashes, invalid JSON, parses to None. Landed two days ago with the shim module; your report is the first Windows signal on it.
2. Job 2 confirmed as the chronic shard-1/3 process-exit-timeout flake (rotating test names, 3 of last 4 main runs) — being taken up separately; not your PR's concern either way.

On the change itself: reviewed at full diff. The clear_backend_state_for_file addition on refresh paths is the part your "no behaviour change" line undersells — the prepared cold-build inserts write backend_file_state rows (plain INSERT), so the clear is required by the mechanics switch, and it's a correctness improvement (backend rows now refresh with the file instead of going stale). Worth a sentence in the description if you touch it again.

Please rebase onto current main (≥ 10d6d20c) — that picks up the fixture fix so your run can go green, and clears the merge gate's finding-newer-than-head check (your own CI analysis comment is what's tripping it, ironically). Merging on the rebased run.

[alfonso-aft]

Update on your job-2 analysis — the "rotating flake" story decomposes further, and mostly into our own bugs:

• astro_uses_the_nearest_project_typescript_sdk (your base run's failure, and this PR's attempt-2 failure) was deterministic on Windows, not a flake: the test compared a fixture PathBuf that kept its forward-slash joins against the product's native-separator output — two spellings of one path. Fixed on main (9dae63eb), alongside the earlier XDG fixture fix (10d6d20c). Both landed with features from the last 48h, which is why shard 1/3 went chronically red this week specifically.
• The remaining genuine flake signature (configure_test process-exit timeout) is under a separate census investigation.

Please rebase once more onto ≥ 9dae63eb — with both deterministic Windows failures fixed, your run has a real chance at fully green, and the merge gate takes it from there. Apologies for the churn; your PR keeps colliding with the exact test debt it helped surface.

[iceteaSA]

Rebase is on 10d6d20c, pushed as 967bedfa. 29/30 green — Cargo libtest + watcher (Windows) is now passing, so the gh_shim fixture fix did it.

The one remaining red is shard 1/3 (Windows), and I need to correct what I told you about it: it isn't a flake. I read rotating test names as timing and stopped there. The log on this run:

lsp_manager_test::astro_uses_the_nearest_project_typescript_sdk
  left:  "...\\workspace\\apps/site\\node_modules\\typescript\\lib"
 right:  "...\\workspace\\apps/site\\node_modules/typescript/lib"

Deterministic path-separator mismatch. The test builds its expectation as member.join("node_modules/typescript/lib") — one hop, embedded slashes preserved — while find_project_typescript_sdk walks .join("node_modules").join("typescript").join("lib"). Identical on unix, never equal on Windows.

Two distinct failures live in that job and fail-fast surfaces whichever trips first, which is what produced the rotation I mistook for flakiness:

main @ 1d2da675    astro_uses_the_nearest_project_typescript_sdk    ← deterministic
#246 @ bfaa0d93    configure_does_not_warn_for_file_discovered_…    ← process-exit timeout
#246 @ 967bedfa    astro_uses_the_nearest_project_typescript_sdk    ← deterministic, again

The process-exit timeout is genuinely timing-dependent and still open; the astro one is a real bug that was never going to pass.

Fix sent as #248, off main, one test file, two-line change plus a comment. It also notes the class: that join("a/b/c") shape appears 425 times across 49 integration test files, though only the ones compared against production-built paths can break — I fixed the one that's demonstrably red rather than sweeping blind.

With #248 in, shard 1/3 should be down to just the process-exit timeout. Not asking you to reorder anything — merge #248 first and I'll rebase #246 onto it for a clean run, or take #246 as-is since the remaining red is provably unrelated to it. Your call.

[iceteaSA]

Rebased onto 9dae63eb, pushed as 85f6728a. Gate re-run at that sha: 4393 tests run: 4391 passed, 2 failed — the same two environmental failures as unmodified base in this checkout, no diff-induced change.

Closed #248 as superseded. We landed the same diagnosis about two minutes apart — genuinely concurrent, not duplicated work. Your normalize-at-assertion approach and my build-the-expectation-like-production approach differ only if the product ever emits a mixed spelling; noted it on that PR and left it there rather than making it an argument.

No apology needed on the churn — the collisions were the useful part. Both Windows failures were real bugs sitting in shard 1/3, and neither had produced a signal until something forced a run against them. If the shard is chronically red, nobody reads it, and the next real regression there is invisible too; two fixes in one evening is the shard becoming a working detector again.

One thing worth keeping from it, since it cost us both a round: I called the shard a flake from the shape of the evidence — rotating test names, moving cancel point — without opening the log to see whether the underlying failures were the same. Rotation is a fail-fast artifact, not a diagnosis; it looks identical whether the failures are timing or deterministic. The log was one click away and would have named the path mismatch immediately. Worth a habit on chronically-red jobs: read the actual assertion before classifying, because the cheap read of the pattern is the one that lets a deterministic bug hide behind the word "flake."

That leaves the configure_test process-exit timeout as the only known red in that shard, and you have a census on it.

[alfonso-aft]

Merged. Thanks for the persistence through three rebases — the PR itself was mergeable on day one; everything in between was our own Windows test debt that your CI analysis kept surfacing (and your #248 close was graceful — same diagnosis, two minutes apart).

Ledger for #214: this lands findings 2–5 (transaction scoped to DB work, prepared-statement reuse, dispatch_hints file index, prepared delete statements). Finding 1 — the full-graph from_db_and_callers re-read — stays open there; it's architectural and interacts with a lane-occupancy redesign already in review, so it'll be taken up in that context rather than as a mechanical follow-up.