Skip to content

perf(native): skip backfill on clean incrementals #1082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
carlos-alm merged 2 commits into from
May 8, 2026
Merged

perf(native): skip backfill on clean incrementals #1082

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb5cd2a
perf(native): skip backfill on incrementals when orchestrator preserv…
carlos-alm May 8, 2026
51629d0
Merge branch 'main' into perf/skip-backfill-on-clean-incrementals
carlos-alm May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/domain/graph/builder/pipeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,15 +753,17 @@ async function tryNativeOrchestrator(
// stale native binaries). WASM handles those — backfill via WASM so both
// engines process the same file set (#967).
//
// Runs on every successful orchestrator pass (not just full builds): on
// incrementals the orchestrator's change detection treats files outside
// Rust's narrower file_collector as `removed` and deletes their nodes +
// file_hashes rows. Without re-running the backfill we'd lose the symbols
// for those files and permanently break the JS-side fast-skip pre-flight
// (#1054, #1068). The function is cheap (single fs scan + DB query) when
// nothing is missing, and on no-op rebuilds the missing-set is re-derived
// from `nodes`, so it catches whatever Rust just deleted.
await backfillNativeDroppedFiles(ctx);
// Runs on full builds and on incrementals when the orchestrator reports
// any deletions. The orchestrator's `detect_removed_files` filter (#1070)
// skips files outside its narrower file_collector, so on a current binary
// an unchanged 1-file rebuild reports `removedCount=0` and the backfill
// call is pure overhead (fs walk + 2 DB queries + 48-file WASM re-parse).
// Legacy binaries lacking the filter still report `removedCount>0` and
// get the gap-repair behavior #1068 introduced.
const removedCount = result.removedCount ?? 0;
if (result.isFullBuild || removedCount > 0) {
await backfillNativeDroppedFiles(ctx);
Comment on lines +763 to +765
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 8, 2026

Choose a reason for hiding this comment

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

P1 Backfill silently skipped on full builds with binaries that omit isFullBuild

isFullBuild is typed as isFullBuild?: boolean, so supported binaries between 3.9.1 and the version that first started emitting this field may leave it absent. On a fresh full build with such a binary, result.isFullBuild is undefined (falsy) and removedCount is 0 (nothing existed to delete), so the condition is false and the backfill is skipped entirely. The non-Rust-extension files (Clojure, Julia, R, etc.) are then absent from nodes/file_hashes, and the fast-skip pre-flight introduced in #1054 will reject every subsequent incremental until a forced full rebuild occurs. The same branch already uses !!result.isFullBuild at line 741, so this is a shared assumption — but it's worth confirming that all binaries ≥ 3.9.1 reliably set isFullBuild: true during every full-build code path, or adding a version-range guard here to fall back to unconditional backfill for binaries that are known not to emit the field.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

@carlos-alm carlos-alm May 8, 2026

Choose a reason for hiding this comment

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

The version gate at shouldSkipNativeOrchestrator (semverCompare(engineVersion, '3.9.1') < 0 → skip) means any binary that reaches tryNativeOrchestrator is ≥ 3.9.1. is_full_build: bool was added to the Rust BuildPipelineResult in #758 (3.8.0 era) and is serialized via serde with no skip_serializing_if — so it's always emitted as true or false, never absent, on any 3.9.1+ binary. The defensive !!result.isFullBuild at line 741 is a stylistic safeguard for the historical Option<bool> shape that was never actually used. The new gate at line 764 already coerces via ||, so the practical risk surface is empty under the version gate.

}
Comment on lines +763 to +766
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 8, 2026

Choose a reason for hiding this comment

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

P1 Newly-added dropped-language files silently missed on incrementals

When a user adds a new file with an unsupported-Rust extension (Clojure, Julia, R, Erlang, F#, Gleam) during an incremental pass, the current binary's orchestrator neither processes it nor increments removedCount. Both result.isFullBuild and removedCount are 0/falsy, so the gate evaluates to false and backfillNativeDroppedFiles is never called. The new file is then absent from both nodes and file_hashes until the next forced full rebuild. Before this PR the unconditional call to backfillNativeDroppedFiles would have caught these files on the very next pass via its expected ∖ existingNodes diff. Adding || (result.changedCount ?? 0) > 0 to the condition would not help either — the orchestrator's narrower file_collector never counts dropped-language additions in changedCount.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

@carlos-alm carlos-alm May 8, 2026

Choose a reason for hiding this comment

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

Confirmed — this is a real but narrow correctness gap. Tracked in #1083 for follow-up. The PR's primary motivation is the measured +144% CI regression on 1-file rebuilds, which affects every user on every incremental. The dropped-language addition case is rare (user adds a new .clj/.jl/.r/.erl/.fs/.gleam file in isolation, with no other file changes) and recoverable via codegraph build --force or by touching any Rust-supported file alongside the addition. Keeping it deferred so the perf fix can land without a regression to the broader fs-walk overhead.


closeDbPair({ db: ctx.db, nativeDb: ctx.nativeDb });
return formatNativeTimingResult(p, structurePatchMs, analysisTiming);
Expand Down
Loading