fix(journal): stamp header timestamp on watcher appends (#997)#1001
fix(journal): stamp header timestamp on watcher appends (#997)#1001carlos-alm merged 6 commits intomainfrom
Conversation
The watcher appended journal entries without touching the header timestamp, so after any watch session the header remained frozen at the previous build's finalize time while DB file_hashes.mtime advanced. Tier 0 change detection then rejected the journal (timestamp stale) and fell through to the expensive mtime+size + SHA256 rescan — a silent performance cliff on the first build after every watch. Introduces appendJournalEntriesAndStampHeader, an atomic tmp+rename that preserves prior entries and advances the header in one write. Watcher uses it on both the debounced batch path and the SIGINT flush path, so the "entries newer than header" invariant violation described in #997 can no longer occur. Impact: 4 functions changed, 8 affected
|
Claude finished @carlos-alm's task —— View job Pull Request Review CompleteReviewing PR #1001: fix(journal): stamp header timestamp on watcher appends Root Cause AnalysisProblem Being Solved: The watcher appended journal entries but never updated the header timestamp, causing it to remain frozen at the previous build's finalize time. This created a performance cliff where the next build's Tier 0 check would see Why This Approach: The solution introduces Risk Assessment: Low risk. The implementation uses standard atomic write patterns (tmp file + rename), has comprehensive test coverage including regression guards, and the change is narrowly scoped to fix a specific performance issue without affecting correctness. Backlog Compliance
Code Quality AssessmentStrengths:
Implementation Details:
Watcher Integration:
Test Coverage:
No Red Flags:
Final Assessment |
Greptile SummaryThis PR fixes the header-lag bug from #997 by introducing
Confidence Score: 4/5Safe to merge with awareness that the debounce-timer/SIGINT race from the prior review remains open. The core header-lag fix is correct and well-tested; the deletion-detection P1 from the prior review is now addressed. The outstanding P1 (debounce timer not cancelled before closeDb/process.exit) was already flagged in the previous review cycle and is explicitly out of scope for this PR — it keeps the score at 4 rather than 5. src/domain/graph/watcher.ts — specifically the SIGINT handler in Important Files Changed
Sequence DiagramsequenceDiagram
participant W as Watcher (debounce / SIGINT)
participant JN as journal.ts
participant FS as Filesystem
note over W,FS: Debounced batch path
W->>W: processPendingFiles(files)
W->>W: rebuildFile() per file
W->>JN: appendJournalEntriesAndStampHeader(rootDir, entries, Date.now())
JN->>FS: readFileSync(journal) — grab existing body
JN->>FS: writeFileSync(journal.tmp, header + existingBody + newEntries)
JN->>FS: renameSync(journal.tmp → journal) [atomic]
note over W,FS: SIGINT flush path
W->>W: buildFlushEntriesFromPending(rootDir, pending)
W->>FS: existsSync(filePath) per pending path — detect deletions
W->>JN: appendJournalEntriesAndStampHeader(rootDir, entries, Date.now())
JN->>FS: readFileSync(journal)
JN->>FS: writeFileSync(journal.tmp, header + existingBody + newEntries)
JN->>FS: renameSync(journal.tmp → journal) [atomic]
note over W,FS: Next incremental build
W->>JN: readJournal(rootDir)
JN-->>W: {timestamp >= MAX(file_hashes.mtime), changed[], removed[]}
note over W: Tier 0 check passes — fast path preserved
Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/journal-hea..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!fs.existsSync(dir)) { | ||
| fs.mkdirSync(dir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
existsSync + mkdirSync redundant check
fs.mkdirSync(dir, { recursive: true }) is a no-op when the directory already exists, so the existsSync guard is unnecessary. The same pattern appears in appendJournalEntries and writeJournalHeader; removing it here would make the code more consistent with idiomatic Node.js and eliminate a minor TOCTOU window between the check and the create.
| if (!fs.existsSync(dir)) { | |
| fs.mkdirSync(dir, { recursive: true }); | |
| } | |
| fs.mkdirSync(dir, { recursive: true }); |
There was a problem hiding this comment.
Fixed in 2274eda — replaced the existsSync+mkdirSync pair with bare fs.mkdirSync(dir, { recursive: true }) at all three callsites in journal.ts (appendJournalEntries, writeJournalHeader, appendJournalEntriesAndStampHeader). Closes the TOCTOU window and matches idiomatic Node usage.
Codegraph Impact Analysis7 functions changed → 12 callers affected across 5 files
|
ctx.pending is an untyped Set<string> with no event-type metadata. The
SIGINT flush previously mapped every entry without a deleted flag, so a
file removed during a watch session was journaled as 'changed' and the
next incremental build would try to re-parse the missing path instead of
removing it from the graph.
Extract buildFlushEntriesFromPending which probes each path with
existsSync and emits { deleted: !exists }. Mirrors the deletion detection
in rebuildFile (builder/incremental.ts).
Addresses Greptile P1.
Impact: 3 functions changed, 3 affected
fs.mkdirSync(dir, { recursive: true }) is a no-op when the directory
already exists, so the existsSync guard is dead weight and introduces a
minor TOCTOU window between check and create.
Apply to all three callsites in journal.ts (appendJournalEntries,
writeJournalHeader, appendJournalEntriesAndStampHeader).
Addresses Greptile P2.
Impact: 3 functions changed, 8 affected
|
Addressed Greptile's review feedback:
|
1 participant
Fixes #997.
Summary
journal.timestampstayed frozen at the previous build's finalize time while DBfile_hashes.mtimeadvanced. The next build's Tier 0 check then sawjournal.timestamp < MAX(file_hashes.mtime), bailed out, and fell through to the expensive mtime+size and SHA256 rescans — a silent performance cliff on the first build after every watch.appendJournalEntriesAndStampHeaderinsrc/domain/graph/journal.ts. It reads the existing entries, writes a new header + existing entries + new entries to a tmp file, then atomically renames over the journal. Entries are preserved; the header advances in the same write.processPendingFiles) and the SIGINT flush handler. The "entries newer than header" invariant violation described in bug(journal): header timestamp lags entries during watch → Tier 0 invalidation forces expensive rescan #997 can no longer occur.Scoped to the header-lag bug only — the parallel-append locking issue tracked in #996 is out of scope.
Test plan
npx vitest run tests/unit/journal.test.ts— 22 tests, all pass (added 5 new tests covering creation, header advance, empty-entries stamping, atomic rename, and a bug(journal): header timestamp lags entries during watch → Tier 0 invalidation forces expensive rescan #997 regression guard comparingjournal.timestampagainst a simulatedlatestDbMtime).npx vitest run tests/builder/detect-changes.test.ts— Tier 0 path unaffected.npx vitest run tests/integration/watcher-rebuild.test.ts tests/incremental/watcher-incremental.test.ts— watcher tests pass.npx biome check src/domain/graph/journal.ts src/domain/graph/watcher.ts tests/unit/journal.test.ts— clean.