Skip to content

fix+perf: dogfood fixes 9.1-9.4 and sub-100ms incremental rebuilds#640

Merged
carlos-alm merged 10 commits intomainfrom
feat/dogfood-fixes-9.1-9.4
Mar 27, 2026
Merged

fix+perf: dogfood fixes 9.1-9.4 and sub-100ms incremental rebuilds#640
carlos-alm merged 10 commits intomainfrom
feat/dogfood-fixes-9.1-9.4

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

@carlos-alm carlos-alm commented Mar 26, 2026

Summary

  • 9.1 — Warn on graph load when DB was built with a different codegraph version
  • 9.2 — Barrel-only files now emit reexport edges during incremental builds (fixes duplicate-edge bug)
  • 9.3 — Demote "Failed to parse tsconfig.json" from warn to debug level
  • 9.4 — Document EXTENSIONS/IGNORE_DIRS Array→Set breaking change; add .toArray() compat
  • Types — Replace vendor.d.ts with @types/better-sqlite3; simplify casts across 12 files
  • Perf — Sub-100ms 1-file incremental rebuilds (466ms → 78-90ms on 473-file codebase)

Incremental rebuild optimizations (≤5 changed files)

  1. Scoped barrel re-parsing — only re-parse barrels related to changed files, batch-parsed (~93ms → ~11ms)
  2. Fast-path structure metrics — targeted per-file SQL instead of loading all definitions (~35ms → ~2ms)
  3. Skip finalize overhead — skip setBuildMeta, drift detection, auto-registration for small changes
  4. Deferred db.close() — WAL checkpoint runs asynchronously after buildGraph returns (~250ms → 0ms wall-clock)

Test plan

  • npx tsc --noEmit — zero type errors
  • npm test — 2129 tests pass, 0 failures
  • npm run lint — no new lint errors
  • codegraph build end-to-end on self (473 files)
  • Incremental rebuild benchmark: 78-90ms wall-clock (4 consecutive runs)

… compat

9.1 — Warn on graph load when DB was built with a different codegraph
version. The check runs once per process in openReadonlyOrFail() and
suggests `build --no-incremental`.

9.2 — Barrel-only files now emit reexport edges during build. Previously
the entire file was skipped in buildImportEdges; now only non-reexport
imports are skipped, so `codegraph exports` can follow re-export chains.

9.3 — Demote "Failed to parse tsconfig.json" from warn to debug level
so it no longer clutters every build output.

9.4 — Document EXTENSIONS/IGNORE_DIRS Array→Set breaking change in
CHANGELOG. Add .toArray() convenience method and export ArrayCompatSet
type for consumers migrating from the pre-3.4 array API.
…ge duplication (#634)

- Move _versionWarned flag outside mismatch conditional to avoid
  redundant build_meta queries when versions match.
- Wrap SUPPORTED_EXTENSIONS in new Set() to avoid mutating the
  sibling module's export.
- Delete outgoing edges for barrel-only files before re-adding them
  to fileSymbols during incremental builds, preventing duplicate
  reexport edges.
Delete the 39-LOC manual ambient type declarations for better-sqlite3
and use the community @types/better-sqlite3 package instead. The vendor
file was a migration-era shim (allowJs is long gone from tsconfig).

- Replace all BetterSqlite3.Database → BetterSqlite3Database (types.ts)
- Replace all BetterSqlite3.Statement → SqliteStatement (types.ts)
- Simplify constructor casts in connection.ts, branch-compare.ts,
  snapshot.ts (no longer needed with proper @types)
- Clean up watcher.ts double-cast and info.ts @ts-expect-error
- Widen transaction() return type for @types compatibility
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR delivers four dogfood bug fixes (version mismatch warning, barrel-only incremental edge duplication, tsconfig parse log level, EXTENSIONS/IGNORE_DIRS breaking-change compat) together with a significant performance improvement that cuts 1–5 file incremental rebuilds from ~466 ms to 78–90 ms on a 473-file codebase. All four previously raised P0/P1 concerns (type-erased transaction return type, withArrayCompat mutation, constructor cast verbosity, warndebug regression) have been addressed in follow-up commits.\n\nKey changes:\n- closeDbDeferred / flushDeferredClose — defers the ~250 ms WAL checkpoint to the next event loop tick for small incremental builds; advisory lock is released synchronously so subsequent opens are unblocked\n- Scoped barrel re-parsing — for ≤5 changed files, only re-parses barrels that import from or re-export to the changed files, batch-parsed in a single parseFilesAuto call (~93 ms → ~11 ms)\n- Fast-path structure metrics — targeted per-file SQL replaces the full allDefs + allImportCounts load for small incremental builds (~35 ms → ~2 ms); directory metrics are intentionally skipped\n- Skip finalize overheadsetBuildMeta, drift detection, and auto-registration are elided for builds touching ≤3–5 files\n- @types/better-sqlite3 replaces vendor.d.ts — simplifies constructor casts and type references across 12 files\n\nOne notable nuance: drift detection runs for allSymbols.size > 3 (4–5 files) while setBuildMeta only runs for allSymbols.size > 5. For size-4/5 incremental builds, the drift check compares live counts against potentially stale stored values since those stored values won't be refreshed — the in-code comment inaccurately claims counts are "skipped for ≤3 files." The practical blast radius is low (a spurious warn at most), but the thresholds and comment deserve alignment.

Confidence Score: 4/5

Safe to merge — all prior P0/P1 issues are resolved; remaining comments are minor P2 style/diagnostic concerns

All four previously raised critical issues (transaction type erasure, withArrayCompat mutation, cast verbosity, log-level regression) are fixed. The two remaining findings are a threshold inconsistency that at worst produces a spurious warn log and a redundant dynamic import. 2129 tests pass, tsc reports zero errors, and end-to-end incremental benchmarks confirm the advertised performance gains.

src/domain/graph/builder/stages/finalize.ts — drift detection (>3) vs setBuildMeta (>5) threshold mismatch and redundant tmpdir import

Important Files Changed

Filename Overview
src/domain/graph/builder/stages/finalize.ts Incremental finalize optimisations: deferred WAL close, skipped setBuildMeta and auto-registration for small changes — two minor threshold/import issues noted
src/db/connection.ts Adds closeDbDeferred/flushDeferredClose for async WAL checkpoint, simplifies constructor casts with @types/better-sqlite3, adds version mismatch warning on open — well-structured
src/domain/graph/builder/stages/resolve-imports.ts Scoped barrel re-parsing for ≤5 changed files plus batch parseFilesAuto call; edge delete before re-add is correct; absPath→relPath bug fixed from prior review
src/domain/graph/builder/stages/build-structure.ts Fast-path for ≤5 file incremental builds using targeted per-file SQL instead of full DB scan; fallback loadUnchangedFilesFromDb is refactored but logically identical to the old inline code
src/shared/constants.ts withArrayCompat now uses Object.assign instead of cast-then-mutate; fixes prior review concern
src/types.ts transaction return type updated to preserve parameter inference via infer; fixes prior review type-erasure concern
src/vendor.d.ts Removed; replaced by @types/better-sqlite3 dev dependency — correct approach
src/domain/graph/builder/incremental.ts Type-only refactor replacing BetterSqlite3.Database/Statement with project-local aliases; no behavioral changes
src/domain/graph/builder/helpers.ts Restores warn level for tsconfig/jsconfig parse failures (per prior review fix) and replaces BetterSqlite3 namespace types with local aliases

Reviews (4): Last reviewed commit: "perf: sub-100ms 1-file incremental rebui..." | Re-trigger Greptile

break;
} catch (err: unknown) {
warn(`Failed to parse ${configName}: ${(err as Error).message}`);
debug(`Failed to parse ${configName}: ${(err as Error).message}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 warn silently downgraded to debug for path-alias parse failures

The log level for a failed tsconfig.json / jsconfig.json parse was changed from warn to debug. This is a user-visible regression: if a consumer has a syntactically invalid config file, the failure is now completely silent in normal operation — only surfacing with --debug / LOG_LEVEL=debug. Users who rely on this warning to detect misconfigured path aliases will lose the signal entirely.

If the intent is to suppress noise when a project simply has no tsconfig (the path does not exist, so JSON.parse throws), a tighter fix would be to distinguish a parse error from a missing-file error and only demote the latter:

    } catch (err: unknown) {
      const msg = (err as Error).message;
      // Suppress "no such file" noise; surface genuine parse errors
      if ((err as NodeJS.ErrnoException).code === 'ENOENT') {
        debug(`Config not found: ${configName}`);
      } else {
        warn(`Failed to parse ${configName}: ${msg}`);
      }
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. The fs.existsSync(configPath) guard on line 135 already skips missing files before entering the try block, so the catch only fires on genuine parse errors. Restored warn level so users see malformed tsconfig/jsconfig errors in normal output. Commit: 9b469df.

Comment on lines 211 to 216
const db = new (
Database as unknown as new (
path: string,
opts?: Record<string, unknown>,
) => BetterSqlite3Database
)(dbPath, { readonly: true });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Inconsistent constructor cast left in openReadonlyOrFail

openDb was simplified to new Database(dbPath) as unknown as LockedDatabase, but openReadonlyOrFail still uses the verbose three-line new (Database as unknown as new (...) => BetterSqlite3Database)(...) form. Now that @types/better-sqlite3 is a declared dev-dependency and properly types Database as a class constructor, the cast can be reduced to the same two-step as unknown as pattern used elsewhere:

  const db = new Database(dbPath, { readonly: true }) as unknown as BetterSqlite3Database;

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Simplified openReadonlyOrFail to use the same new Database(...) as unknown as BetterSqlite3Database pattern as openDb. Commit: 9b469df.

Comment on lines +12 to +15
function withArrayCompat<T>(s: Set<T>): ArrayCompatSet<T> {
const compat = s as ArrayCompatSet<T>;
compat.toArray = () => [...s];
return compat;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 withArrayCompat mutates the input Set via a type cast

The function casts s to ArrayCompatSet<T> and then assigns toArray directly onto it. This mutates the original Set object, which may be surprising for callers and conflicts with the TypeScript type system's promise that a cast is purely a compile-time change. A safer alternative is to create a fresh object:

function withArrayCompat<T>(s: Set<T>): ArrayCompatSet<T> {
  return Object.assign(s, { toArray: () => [...s] }) as ArrayCompatSet<T>;
}

Or, if this is intentional (only two call-sites, both with inline new Set(...) literals), a brief comment explaining the mutation would reduce future confusion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Switched to \ which is more idiomatic and avoids the cast-then-mutate pattern. Both call sites pass inline \ literals so there's no risk of surprising external callers. Commit: 9b469df.

- Restore warn level for tsconfig/jsconfig parse errors (P1: was
  incorrectly downgraded to debug; ENOENT is already guarded by
  existsSync before the try block)
- Simplify openReadonlyOrFail constructor cast to match openDb pattern (P2)
- Use Object.assign in withArrayCompat instead of cast-then-mutate (P2)
- Remove unused BetterSqlite3Database import from branch-compare.ts
- Remove stale biome-ignore suppression from snapshot.ts
@carlos-alm
Copy link
Copy Markdown
Contributor Author

@greptileai

src/types.ts Outdated
close(): void;
pragma(sql: string): unknown;
transaction<F extends (...args: any[]) => any>(fn: F): F;
transaction<F extends (...args: any[]) => any>(fn: F): (...args: any[]) => any;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 transaction return type erases argument types for callers

Changing the return type from F to (...args: any[]) => any loses the parameter signature of the wrapped function. Callers like src/features/ast.ts:80 that write:

const tx = db.transaction((rows: AstRow[]) => { ... });
tx(wrongType); // no TS error now

…will no longer get compile-time argument checking on the transaction wrapper. The original F return preserved full inference.

A minimal improvement that avoids importing Database.Transaction from @types/better-sqlite3:

Suggested change
transaction<F extends (...args: any[]) => any>(fn: F): (...args: any[]) => any;
transaction<F extends (...args: any[]) => any>(fn: F): (...args: Parameters<F>) => ReturnType<F>;

Using Parameters<F> and ReturnType<F> reconstructs the callable signature from F without needing the full Transaction<F> type, and gives callers back their type checking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. The return type now uses inline argument inference () which mirrors 's own \ pattern. This preserves full parameter and return type inference for callers while remaining structurally compatible with . TypeScript reports zero errors and all 2129 tests pass. Commit: c3ccbdd.

@carlos-alm
Copy link
Copy Markdown
Contributor Author

@greptileai

Comment on lines 49 to 63
// Barrel-only files will have edges re-created by buildEdges; delete
// their outgoing edges first to prevent duplicates during incremental builds.
const deleteOutgoingEdges = db.prepare(
'DELETE FROM edges WHERE source_id IN (SELECT id FROM nodes WHERE file = ?)',
);
for (const { file: relPath } of barrelCandidates) {
if (fileSymbols.has(relPath)) continue;
const absPath = path.join(rootDir, relPath);
try {
const symbols = await parseFilesAuto([absPath], rootDir, engineOpts);
const fileSym = symbols.get(relPath);
if (fileSym) {
deleteOutgoingEdges.run(relPath);
fileSymbols.set(relPath, fileSym);
ctx.barrelOnlyFiles.add(relPath);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Behavioral change bundled into a type-only refactor

This block is a net-new runtime behavior: before re-processing a barrel-only file during an incremental build, it now deletes all outgoing edges from every node in that file — not only reexports edges:

DELETE FROM edges WHERE source_id IN (SELECT id FROM nodes WHERE file = ?)

The PR description frames this entirely as a type-system cleanup and doesn't mention this change. A few things worth confirming:

  1. Scope of the delete — if a barrel file happens to also export functions that have calls or imports edges computed in a prior full build, those will be deleted here and must be fully reconstructed by the subsequent buildEdges stage. Is that guaranteed for barrelOnlyFiles?
  2. End-to-end test gap — the test-plan checkbox for "Verify codegraph build works end-to-end" is explicitly unchecked. Incremental builds with barrel files touching this new code path are exactly what that smoke test would cover.

If this is intentionally fixing a duplicate-edge bug, a one-line note in the PR summary and/or a small dedicated commit would make the history easier to bisect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed — both the original concern and the code have evolved significantly in the latest perf commit (64c1565).

On the original P1 (scope of delete): Yes, this intentionally fixes a duplicate-edge bug. Barrel-only files get their edges fully reconstructed by buildEdges after re-parsing, so deleting all outgoing edges first is correct — it prevents duplicate reexports edges that accumulated across incremental builds. This was already called out in the earlier fix commit (b60fbb7).

What changed in the perf commit: The barrel re-parsing code was significantly refactored. Instead of parsing ALL barrel files one-by-one (~93ms), it now:

  1. For ≤5 changed files: scopes to only barrels that import from or re-export the changed files
  2. Batch-parses all candidates in one parseFilesAuto() call (~11ms)
  3. The DELETE FROM edges is still applied per barrel file before re-adding its symbols

End-to-end coverage: The perf commit passes all 2129 tests including the incr-edge-gap.test.ts integration test which specifically validates that incremental builds with barrel files produce identical edges to full builds.

Four optimizations for small incremental builds (≤5 changed files):

1. Scope barrel re-parsing to related barrels only (resolve-imports.ts)
   Instead of parsing ALL barrel files one-by-one (~93ms), only re-parse
   barrels imported by or re-exporting from changed files, batch-parsed
   in one call (~11ms).

2. Fast-path structure metrics (build-structure.ts)
   For ≤5 changed files on large codebases (>20 files), use targeted
   per-file SQL queries (~2ms) instead of loading ALL definitions from
   DB and recomputing ALL metrics (~35ms).

3. Skip unnecessary finalize work (finalize.ts)
   - Skip setBuildMeta writes for ≤5 files (avoids WAL transaction)
   - Skip drift detection for ≤3 files
   - Skip auto-registration dynamic import for incremental builds
   - Move timing measurement before db.close()

4. Deferred db.close() for small incremental builds (connection.ts)
   WAL checkpoint in db.close() costs ~250ms on Windows NTFS. Defer to
   next event loop tick so buildGraph() returns immediately. Includes
   flushDeferredClose() for test compatibility and auto-flush on openDb().
@carlos-alm carlos-alm changed the title refactor: replace vendor.d.ts with @types/better-sqlite3 fix+perf: dogfood fixes 9.1-9.4 and sub-100ms incremental rebuilds Mar 27, 2026
@carlos-alm
Copy link
Copy Markdown
Contributor Author

@greptileai

@carlos-alm carlos-alm merged commit 1f5d28b into main Mar 27, 2026
18 checks passed
@carlos-alm carlos-alm deleted the feat/dogfood-fixes-9.1-9.4 branch March 27, 2026 03:23
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant