resolver: retain import-adjacency projections within a resolve pass - #531
Conversation
zzet
left a comment
There was a problem hiding this comment.
@pbednarcik The one thing worth fixing before merge
The entire invalidation half of the PR is unpinned. Stripping all 14 production noteImportEdgeReindexes/noteImportTargetReindexes calls via go test -overlay leaves ./internal/resolver and ./internal/indexer green — reproduced independently by two verifiers. Every new test calls the note funcs directly from the test body, and graph.Graph implements no ProjectImportAdjacency, so all other tests take the never-retained fallback path.
I confirmed the companion hole from the coverage profile directly:
resolve_all_pending.go:209.54,215.3 2 0 <- gen-drift clear(p.importAdjacency): NEVER EXECUTED
resolve_all_pending.go:215.8,218.53 1 1 <- per-file dirty path: covered
The fix is cheap — one test driving prepare() → a production write site → prepare(). The reviewers built exactly that probe; it fails without the notes (STALE: reachable dirs = map[repo:{}], missing dep2).
Relatedly, reachability_projection_test.go:339's comment claims the generation path but the test sets Edge.FilePath, so it exercises the dirty-file path and importEdgeGen stays 0. The comment conceals the hole.
Non-blocking
- resolver.go:4828 — the identity-preserving skip never fires from the page-commit path its comment cites (resolveEdge returns changed only when To moved; that batch always sets OldKind). The only shape satisfying it, terminalClears at 1354, runs past the last prepare(). Dead but harmless.
- razor_using.go:154 — the added note is inert: its batch is all EdgeReferences, so importsRow is always false. The real adjacency mutation is the RemoveEdge at 159–161, and the PR has no removal helper. Latent only, but resolver.go:4786 miscategorizes this file under "precise invalidation."
- resolver.go:4564 — reuses reachabilityStableFileCap, whose "a few megabytes" rationale was written for deduped dir sets, not raw node-ID slices. Measured ~5 MB real, ~38 MB at cap. Comment-only.
- reach_fallback is a boolean logged as a count; reach_retained is occupancy, not per-page retention. Since the PR argues these counters are the operational proof, they're worth correcting.
|
Thanks for the all the reviews. I will look at the found issues and potential improvements once I am back from vacation next week. |
|
@pbednarcik enjoy your vacation! |
Problem
Cold-run telemetry on my production C# repo: the resolve phase spent 142.1 s of 341.9 s building per-page reachability, and 99.9% of that was
ProjectImportAdjacencystore reads. Caller files recur across resolve pages (~9× on average — 75,843 page-file appearances), but the existing pass-scoped retention only keeps a file whose imports are all resolved. Mid-pass, unresolved imports are the norm — that's what the pass is resolving — so 87.5% of recomputes were files the cache had already refused once. Worst page: 4.33 s.Change
First commit is instrumentation only: the existing
resolver: prepare page indexeslog line gains per-pagereach_*fields (files, cached, missing, unstable, adjacency hits, dirty count, generation, fallback, retained size, and the project/place/match timing split). Everything after is driven by what those counters showed.The retention itself:
ProjectImportAdjacencyresults per caller file for the life of the pass — stable or not. Only the store read is retained; the per-page derivation still reruns, so reachable dirs pick up targets resolved later in the same pass.complete=falseprojections are never retained (canonicality signal, not a cacheable answer); known-empty projections are (else importless files re-project forever). Same overflow contract as the stable retention: past the entry cap, clear wholesale rather than evict piecemeal.noteImportEdgeWrite.One pinned test renegotiated, called out explicitly:
TestReachabilityProjectionDoesNotCacheUnresolvedImportspinned "unstable files are never retained". Its replacementTestReachabilityProjectionUnresolvedImportsStayFreshpins what actually matters — freshness, not absence: a mid-pass import write still invalidates, and the next page sees the newly resolved target.Numbers
Idle machine, from-scratch index, same instrumentation on both sides of the A/B:
The reachability sub-metric is the honest primary — it comes from the same per-page instrumentation on both sides. Phase totals are secondary: resolve varies by ~±25 s day-to-day on this machine for identical code. Retention served 34,225 page-file appearances across 5,988 unique files; the legacy fallback ran on 3 of 96 pages (genuinely malformed batches — the conservative path doing its job). Outcome counters were digit-identical across five consecutive cold runs.
Relationship to #529
On Windows, these numbers require #529: the projection fast-path was disabled there by the path canonicality guard, so this retention had nothing to retain (fallback results are never cached — by design). The diffs are independent and touch different packages; on separator-stable platforms the retention stands alone.
How it was validated
The diagnostics commits exist because the first deployment served zero hits, and each iteration was eliminated by a counter rather than a theory:
adj_cached=0→ generation counter showed no wholesale clears (gen=0all run) → dirty-set counter showed per-file invalidation was small (~75/page vs ~47k recomputes) → the fallback counter showedcomplete=falseon 93 of 93 pages → the Windows path guard (#529). Outcomes stayed digit-identical through every inert run, so the retention path was provably harmless while it was being debugged.Those
reach_*fields aren't scaffolding to strip afterwards — they're the operational proof the retention serves. If a future write site misses its audit verdict or provenance changes shape, it shows up asadj_cached=0in the daemon log instead of needing a bisect.Tests
Retention across pages, cap-overflow wholesale clear, per-file invalidation surviving unrelated import writes, identity-preserving rewrites not invalidating, provenance-less writes falling back to the generation, the freshness pin, and fallback-never-retained are each pinned individually. Full graph+resolver failure set vs clean main is identical on my Windows box (pre-existing platform bucket only, zero new).