Repository navigation
perf(sync): an unchanged file with a new mtime is read once, not on every sync - #3198
bob-codedaptive wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f768651db5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // sync reads and hashes this file again. The stat was taken before the | ||
| // read, so a write landing after it leaves a newer mtime on disk and | ||
| // the next sync still reads the file. | ||
| this.queries.upsertFile({ ...tracked, size: stat.size, modifiedAt: stat.mtimeMs }); |
There was a problem hiding this comment.
After a checkout or copy changes many mtimes, these upserts commit once per file while CodeGraph.sync() has wal_autocheckpoint=0. Unlike the later indexFile loop, this loop never invokes the supplied backpressure hook. If a concurrent reader pins WAL frames, timer-driven PASSIVE checkpoints cannot advance, so these writes can exceed the valve's hard/file caps before the final foldNow() can stop them, risking disk exhaustion. A native SQLite probe using the actual table and upsert SQL generated approximately 32 MiB of WAL from 2,000 refreshes despite repeated PASSIVE checkpoints. Await backpressure after each refresh or a bounded batch so timestamp-only syncs retain the disk-growth protection used by content-changing syncs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. On the PR commit, 2,000 stat-only refreshes with a reader pinning the WAL wrote about 32 MiB before any cap was checked, and the valve's backpressure was never called. The refresh loop now awaits backpressure after each refresh, the same way the loop that re-indexes changed files does. With a 4 MiB valve and a pinned reader the sync now stops at the hard cap instead of writing the whole 32 MiB. checks the WAL valve after every stat-only refresh pins it: three refreshed files give exactly three calls, and it fails without the change.
…very sync Sync skips a tracked file without reading it when its size and mtime match the index. When only the mtime moved (a fresh checkout, a copied index, a touch), sync read and hashed the file, found the same content, and kept the old size and mtime, so every later sync read and hashed that file again. Sync now stores the file's current size and mtime when the hash matches, so the next sync skips it at the stat check. The stat is the one taken before the read: a write that lands after it leaves a newer mtime on disk, and the next sync still reads the file. Validation: the new test touches an indexed file and counts the opens of it across two syncs. On main the second sync opens it again; with this change it opens it zero times.
f768651 to
cb940a1
Compare
Symptom
After something gives a project's files new timestamps without changing their content (switching branches and back before a sync, a
.codegraph/copied in from another checkout of the same code, atouch), every latercodegraph sync, and every sync the file watcher or the MCP server runs, reads and hashes all of those files again. Each sync correctly finds nothing changed (filesModifiedis 0), so nothing looks wrong. The cost shows up only as syncs that stay slow and read the disk for files that have not changed. With an index copied into a new checkout, that is every file in the project, on every sync, until it is indexed again.Cause
ExtractionOrchestrator.sync()skips a tracked file without reading it when its size and mtime both match thefilesrow. When they don't match, it reads the file, hashes it, and re-indexes it only if the hash differs. When the hash matches, it did nothing, so the row kept the old mtime and the next sync failed the stat check, read the file and hashed it again.Change
When the hash matches, sync now writes the file's current size and mtime to its row (
upsertFilewith the tracked record and the new stat). The next sync then skips the file at the stat check.Each of these writes commits on its own, and a sync runs with WAL auto-checkpointing deferred, so after each one sync gives the WAL valve the same between-commits check (
backpressure) it gives after each re-indexed file. A checkout that moves thousands of mtimes therefore cannot grow the WAL past the valve's caps while a reader pins frames: 2,000 refreshes with a pinned reader wrote about 32 MiB of WAL before any cap was checked, and with the check the valve stops the writer at its hard cap instead.The stat stored is the one taken before the read. If a write lands between the stat and the read, the file on disk has a newer mtime than the one stored, so the next sync still reads it. A content change that keeps both size and mtime exactly remains the blind spot the pre-filter already documents, with
index --forceas the way out.Files whose row records a grammar-load failure take a different branch and are not affected: they are still re-indexed.
How it was measured
__tests__/sync-mtime-refresh.test.tsindexes a one-file project, moves the file's mtime forward without changing its bytes, and countsfs.openSynccalls on that file (sync reads throughreadBoundedSourceSync) across two syncs:filesModifiedis 0;main, the file is opened again (the test fails withexpected 1 to be +0); with this change it is opened 0 times.A second test touches three tracked files, spies on the valve's
backpressureand expects exactly three calls from the sync that refreshes them, and none from the next sync. No timing benchmark was run; the tests count reads and calls, not seconds.Validation
npx tsc --noEmit -p .: clean.npx vitest run __tests__/sync-mtime-refresh.test.ts: 2 passed. Withsrc/extraction/index.tsreverted tomain: the open-count test fails at the second sync, and the valve test fails withexpected "backpressure" to be called 3 times, but got 0 times.🤖 Generated with Claude Code