fix: prevent SQLite B-tree corruption with WAL tuning and batched agent logs - #30
Merged
gsxdsm merged 17 commits intoMay 5, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDatabase initialization adds a pre-write integrity check with WAL checkpoint recovery on corruption and exposes the DB path; SQLite WAL auto-checkpoint tuning reduced to 100. Store now buffers agent-log writes with timed/batch flush, ensuring read-after-write consistency and flushing on task deletes/close. Tests and CI/workflow updates exercise these behaviors. ChangesAgent Log Buffering (store.ts + tests)
Database startup, integrity, and WAL tuning (db.ts + tests)
Repo toolchain / release metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Contributor
Greptile Summary
Confidence Score: 5/5Safe to merge — all previously flagged P1s are resolved and no new P1/P0 issues found. All prior P1 findings (timer re-arm after close, integrity check ordering, flush-before-delete race, missing try/finally, size-triggered flush without error handling, appendAgentLogBatch ordering inversion) are verifiably addressed in this diff. Only two P2 style-consistency nits remain. Test coverage for both subsystems is solid (14 new tests). No files require special attention — Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant TaskStore
participant Buffer as agentLogBuffer
participant Timer as flushTimer
participant DB as SQLite (WAL)
Caller->>TaskStore: appendAgentLog(taskId, text, type)
TaskStore->>Buffer: push entry
TaskStore->>Caller: emit("agent:log") immediately
alt buffer.length >= 50 (BUFFER_SIZE)
TaskStore->>TaskStore: flushAgentLogBuffer()
TaskStore->>DB: transaction { SELECT live taskIds, INSERT batch, bumpLastModified }
DB-->>TaskStore: commit
TaskStore->>Buffer: splice(0, flushCount)
else buffer.length < 50 and no timer
TaskStore->>Timer: setTimeout(2000ms).unref()
end
Note over Timer,DB: 2 s later (or on read/delete/close)
Timer->>TaskStore: flushAgentLogBuffer()
TaskStore->>Timer: clearTimeout
TaskStore->>DB: transaction { filter live tasks, INSERT valid, bumpLastModified }
alt transaction succeeds
DB-->>TaskStore: commit
TaskStore->>Buffer: splice(0, flushCount)
else transaction fails
DB-->>TaskStore: error
TaskStore->>Buffer: splice(0, flushCount)
TaskStore->>Buffer: unshift(validEntries) — requeue
TaskStore->>Timer: setTimeout(2000ms).unref() — retry
end
Caller->>TaskStore: getAgentLogs / getAgentLogCount / deleteTask / close
TaskStore->>TaskStore: flushAgentLogBuffer() before operation
Reviews (17): Last reviewed commit: "fix: resolve Greptile P1 close() timer l..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/db.ts (1)
848-908:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the integrity check before any writes, and abort if recovery does not return
ok.Lines 849-869 already create/alter schema and seed rows before this check runs, so a damaged database can still be mutated during
init(). Then Lines 889-906 only log on an unrecovered failure and leave the connection usable, which means the process can keep writing to a known-corrupt DB.Suggested direction
init(): void { + if (!this._inMemory) { + this.assertHealthyOrRecover(); + } + this.db.exec(SCHEMA_SQL); @@ - if (!this._inMemory) { - try { - const result = this.db.prepare("PRAGMA integrity_check").get() as - | { integrity_check?: string } - | undefined; - if (result?.integrity_check !== "ok") { - console.warn( - `[fusion] Database integrity check failed at startup. ` + - `Attempting WAL checkpoint recovery...`, - ); - try { - this.db.exec("PRAGMA wal_checkpoint(TRUNCATE)"); - const recheck = this.db.prepare("PRAGMA integrity_check").get() as - | { integrity_check?: string } - | undefined; - if (recheck?.integrity_check === "ok") { - console.warn(`[fusion] Database recovered via WAL checkpoint.`); - } else { - console.error( - `[fusion] Database is corrupted and could not be auto-recovered. ` + - `Run: sqlite3 ${this.dbPath} ".recover" | sqlite3 ${this.dbPath}.recovered`, - ); - } - } catch { - console.error( - `[fusion] Database corruption detected and checkpoint recovery failed. ` + - `Manual recovery required.`, - ); - } - } - } catch { - // Integrity check itself failed — serious corruption - console.error(`[fusion] Could not run integrity check on ${this.dbPath}`); - } - } } + + private assertHealthyOrRecover(): void { + const result = this.db.prepare("PRAGMA integrity_check").get() as + | { integrity_check?: string } + | undefined; + if (result?.integrity_check === "ok") return; + + console.warn("[fusion] Database integrity check failed at startup. Attempting WAL checkpoint recovery..."); + this.db.exec("PRAGMA wal_checkpoint(TRUNCATE)"); + + const recheck = this.db.prepare("PRAGMA integrity_check").get() as + | { integrity_check?: string } + | undefined; + if (recheck?.integrity_check !== "ok") { + throw new Error(`Database integrity check failed for ${this.dbPath}`); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/db.ts` around lines 848 - 908, Move the integrity check to run at the very start of init() before any db.exec or schema/seed writes (i.e., before SCHEMA_SQL, INSERT OR IGNORE into __meta and config, and before calling migrate() / ensureRoutinesSchemaCompatibility()), and if the first integrity_check != "ok" then attempt the PRAGMA wal_checkpoint(TRUNCATE) recovery and immediately re-run integrity_check; if recovery does not produce "ok" abort initialization by throwing an error or closing the connection so no further writes occur (use the same logic currently in the try/catch that inspects integrity_check and wal_checkpoint, but relocate and convert the non-recovered path into a hard failure referencing this.dbPath and this.db operations like this.db.prepare("PRAGMA integrity_check").get()).
🧹 Nitpick comments (1)
packages/core/src/store.ts (1)
5269-5281: 💤 Low value
getAgentLogsByTimeRangemay return stale data — buffer is not flushed.Unlike
getAgentLogs()andgetAgentLogCount()which flush before reading, this method reads directly from the database. If buffered entries fall within the requested time range, they won't be returned.If this method is expected to have read-after-write consistency like its siblings, add a flush call at the start:
♻️ Optional fix for consistency
async getAgentLogsByTimeRange( taskId: string, startIso: string, endIso: string | null, ): Promise<AgentLogEntry[]> { + this.flushAgentLogBuffer(); const end = endIso ?? new Date().toISOString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/store.ts` around lines 5269 - 5281, getAgentLogsByTimeRange reads directly from the DB and can miss buffered entries; add the same flush call used by getAgentLogs() and getAgentLogCount() at the start of getAgentLogsByTimeRange so pending buffer entries are written before the SELECT. Locate getAgentLogsByTimeRange and insert a call to the existing flush method (the one invoked by getAgentLogs()/getAgentLogCount()) as the first statement, then proceed with computing end and querying the DB.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/__tests__/db.test.ts`:
- Around line 212-219: The test creates an extra temp directory (freshDir via
makeTmpDir) that isn't removed by the suite-level afterEach; after closing
freshDb you must delete the freshDir so it doesn't leak on repeated runs. Update
the test that creates freshDir/freshFusionDir and Database(freshFusionDir)
(symbols: makeTmpDir, freshDir, freshFusionDir, Database, freshDb) to remove the
freshDir at the end (e.g., call the existing cleanup helper or
fs.rmSync/fs.rmdir equivalent with recursive/force) or register freshDir with
the suite-level temp cleanup so the directory is removed after the test. Ensure
removal happens after freshDb.close().
In `@packages/core/src/__tests__/store.test.ts`:
- Around line 4716-4730: The test title claims a "single flush" but only asserts
per-task counts; add an explicit assertion that the store's flush method was
invoked exactly once to prove batching. Locate the test that uses
createTestTask(), store.createTask(), store.appendAgentLog(), and
store.getAgentLogCount(); before the loop spyOn the store flush/commit function
(e.g., jest.spyOn(store, "flushBufferedLogs") or the actual internal method that
performs the batched write) or inject a mock, run the appends, then assert the
spy was called once (and keep the existing per-task count assertions), or
alternatively change the test title to stop claiming a single flush if you
cannot reliably spy the flush method.
- Around line 4629-4631: The test currently uses getAgentLogCount(task.id) which
implicitly triggers an auto-flush and makes the test a false positive; instead,
assert the buffered entries directly (e.g., inspect store.agentLogBuffer or
store._agentLogBuffer for task.id and expect length 50), verify the DB count
remains unchanged (call a non-flushing DB count helper or query the underlying
DB adapter directly), then explicitly call the flush method (e.g.,
store.flushAgentLogBuffer(task.id) or store.flushAllAgentLogs()) and finally
call getAgentLogCount(task.id) to assert the DB now has 50 entries; replace the
single getAgentLogCount assertion with these steps and avoid relying on
getAgentLogCount to observe the buffer state.
---
Outside diff comments:
In `@packages/core/src/db.ts`:
- Around line 848-908: Move the integrity check to run at the very start of
init() before any db.exec or schema/seed writes (i.e., before SCHEMA_SQL, INSERT
OR IGNORE into __meta and config, and before calling migrate() /
ensureRoutinesSchemaCompatibility()), and if the first integrity_check != "ok"
then attempt the PRAGMA wal_checkpoint(TRUNCATE) recovery and immediately re-run
integrity_check; if recovery does not produce "ok" abort initialization by
throwing an error or closing the connection so no further writes occur (use the
same logic currently in the try/catch that inspects integrity_check and
wal_checkpoint, but relocate and convert the non-recovered path into a hard
failure referencing this.dbPath and this.db operations like
this.db.prepare("PRAGMA integrity_check").get()).
---
Nitpick comments:
In `@packages/core/src/store.ts`:
- Around line 5269-5281: getAgentLogsByTimeRange reads directly from the DB and
can miss buffered entries; add the same flush call used by getAgentLogs() and
getAgentLogCount() at the start of getAgentLogsByTimeRange so pending buffer
entries are written before the SELECT. Locate getAgentLogsByTimeRange and insert
a call to the existing flush method (the one invoked by
getAgentLogs()/getAgentLogCount()) as the first statement, then proceed with
computing end and querying the DB.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8197c8be-c030-4105-b513-f27d1786ee4f
📒 Files selected for processing (4)
packages/core/src/__tests__/db.test.tspackages/core/src/__tests__/store.test.tspackages/core/src/db.tspackages/core/src/store.ts
…nt logs The agentLogEntries table was doing individual auto-committed INSERTs, creating extreme WAL pressure that caused recurring B-tree corruption. This adds three WAL PRAGMAs (aggressive autocheckpoint, size limit, synchronous=NORMAL), a write-behind buffer for agent log entries, and a startup integrity check with auto-recovery attempt. Refs: Runfusion#24 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ck ordering - Move integrity check before migrations/seeds to avoid writing to a corrupted database (makes corruption worse) - Fix buffer swap-before-commit hazard: entries are now only removed from the buffer after the transaction succeeds - Add try-catch to timer-triggered flush to prevent uncaught exceptions from crashing the process - Log warning in close() catch instead of silently swallowing errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
timothyjlaurent
force-pushed
the
fix/sqlite-corruption-wal-pragmas
branch
from
May 3, 2026 15:59
7a53c4d to
78ff1be
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/store.ts`:
- Around line 5449-5450: getAgentLogsByTimeRange (and the other time-range read
helper around the later block) currently reads only persisted rows and can miss
recent buffered appends; before performing the time-range query, call
this.flushAgentLogBuffer() to ensure buffered entries are visible. Locate the
methods getAgentLogsByTimeRange and the corresponding time-range read block (the
later block around the second occurrence noted) and insert
this.flushAgentLogBuffer() just prior to executing the DB/time-range read so the
read-after-write consistency matches getAgentLogs()/getAgentLogCount().
- Around line 3755-3756: appendAgentLog() can enqueue logs for tasks that have
been deleted concurrently with deleteTask(), causing FK violations during
flushAgentLogBuffer() and leaving stale rows in memory; to fix, make
appendAgentLog() validate the target task is still live (e.g., call the existing
task-liveness check or look up task status) and reject or drop appends for
non-live tasks, and additionally harden flushAgentLogBuffer() to filter
out/stage-remove any buffered entries whose task no longer exists before
starting the DB transaction so a late log can’t abort and leave stale entries in
memory; update code paths referencing appendAgentLog(), deleteTask(), and
flushAgentLogBuffer() accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f14126c-0ab5-41a0-8542-131f077dd73d
📒 Files selected for processing (4)
packages/core/src/__tests__/db.test.tspackages/core/src/__tests__/store.test.tspackages/core/src/db.tspackages/core/src/store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/tests/store.test.ts
Ensures operators can identify which database has integrity issues when multiple DB instances are running. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
|
Thanks for creating this. Will merge in once comments addressed |
…in logs - Flush buffer before appendAgentLogBatch to prevent rowid ordering inversion when callers interleave buffered and direct writes (P1) - Wrap size-triggered flush in try-catch for consistency with timer path - Move bumpLastModified inside transactions for atomicity - Expose db.path getter and include DB path in all flush error logs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Thanks for the thorough review. Addressed all flagged issues in P1 — Size-triggered flush missing try-catch: Fixed. The
DB identification in logs: Added a public Re: Re: "entries not spliced on transaction failure": The current pattern uses All 811 tests pass. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/core/src/store.ts (2)
3755-3756:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftA deleted task can still poison the shared agent-log buffer.
deleteTask()flushes too early to make the buffer safe:appendAgentLog()is still free to enqueue for the same task after that pre-delete flush, andflushAgentLogBuffer()writes the whole batch in one transaction. One FK violation aborts every row and the bad entry stays in memory, so later reads/close calls that flush first can keep failing too.Also applies to: 4692-4700, 4743-4757
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/store.ts` around lines 3755 - 3756, deleteTask() currently calls flushAgentLogBuffer() too early, allowing appendAgentLog() to race and leave FK-violating rows in the shared in-memory buffer; change deleteTask() to first remove any buffered entries for that task from the agent-log buffer (i.e., filter out entries with the task's id) and mark the task as deleted (or add its id to a deletedTask set) before performing the DB delete, and update appendAgentLog() to consult that marker (reject/apply backpressure) so no new entries for a deleted task are enqueued; alternatively, perform the flush+delete in a single DB transaction, but at minimum ensure flushAgentLogBuffer() is not called until buffered entries for the target task have been purged or new appends are prevented.
5520-5532:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFlush buffered rows before time-range reads too.
getAgentLogsByTimeRange()still reads only persisted rows, so an immediate append followed by a time-range query can miss entries thatgetAgentLogs()andgetAgentLogCount()now include. Add the same pre-read flush here for consistent read-after-write behavior.Proposed fix
async getAgentLogsByTimeRange( taskId: string, startIso: string, endIso: string | null, ): Promise<AgentLogEntry[]> { + this.flushAgentLogBuffer(); const end = endIso ?? new Date().toISOString(); const selectClause = this.getAgentLogSelectClause(); const rows = this.db.prepare(` SELECT ${selectClause} FROM agentLogEntries🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/store.ts` around lines 5520 - 5532, getAgentLogsByTimeRange currently reads only persisted rows; add the same pre-read flush used by getAgentLogs() and getAgentLogCount() so buffered appends are included. Insert a call to the flush method (e.g. this.flushBufferedAgentLogRows()) at the start of getAgentLogsByTimeRange (before computing end/select and running the query) to flush in-memory/buffered agent log rows to the DB, then proceed with the existing select and mapping logic.
🧹 Nitpick comments (1)
packages/core/src/db.ts (1)
701-702: 💤 Low valueConsider removing duplicate API surface.
The new
pathgetter duplicates the existinggetPath()method at line 2518. Both returnthis.dbPath. Having two ways to access the same value can cause confusion. Consider either:
- Removing
getPath()and using only the getter (breaking change for existing callers)- Keeping only
getPath()for consistency withCentralDatabase.getPath()(see relevant code snippet 3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/db.ts` around lines 701 - 702, The new getter path duplicates the existing getPath() API (both return this.dbPath); remove the redundant path getter to avoid a duplicate surface, and update any internal callers of the path property to call getPath() instead (or add a small forwarding implementation that calls getPath() if you need a transitional change). Ensure consistency with CentralDatabase.getPath() by keeping getPath() as the single source-of-truth for the database path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/db.ts`:
- Around line 944-949: The catch block that handles checkpoint recovery is
currently empty and discards the thrown error; update the catch to capture the
error (e.g., catch (err)) and include the error message/stack in the
console.error call so the log for `[fusion:db] Database corruption detected for
${this.dbPath} and checkpoint recovery failed.` also prints the underlying error
details (use err.message and/or err.stack) to aid diagnosis when checkpoint
recovery fails.
- Around line 923-927: The integrity check is currently called after performing
writes (db.exec(SCHEMA_SQL) and the INSERT OR IGNORE INTO __meta statements)
which contradicts the comment; either move the integrityCheck() invocation to
run immediately after opening the DB connection and before db.exec(SCHEMA_SQL)
and any INSERT OR IGNORE INTO __meta operations, or update the comment to
reflect that integrityCheck() runs after schema/seed writes. Locate the
integrityCheck() call and the db.exec(SCHEMA_SQL) / INSERT OR IGNORE INTO __meta
statements and reorder so integrityCheck() runs first, or change the comment
text to accurately describe the current ordering.
---
Duplicate comments:
In `@packages/core/src/store.ts`:
- Around line 3755-3756: deleteTask() currently calls flushAgentLogBuffer() too
early, allowing appendAgentLog() to race and leave FK-violating rows in the
shared in-memory buffer; change deleteTask() to first remove any buffered
entries for that task from the agent-log buffer (i.e., filter out entries with
the task's id) and mark the task as deleted (or add its id to a deletedTask set)
before performing the DB delete, and update appendAgentLog() to consult that
marker (reject/apply backpressure) so no new entries for a deleted task are
enqueued; alternatively, perform the flush+delete in a single DB transaction,
but at minimum ensure flushAgentLogBuffer() is not called until buffered entries
for the target task have been purged or new appends are prevented.
- Around line 5520-5532: getAgentLogsByTimeRange currently reads only persisted
rows; add the same pre-read flush used by getAgentLogs() and getAgentLogCount()
so buffered appends are included. Insert a call to the flush method (e.g.
this.flushBufferedAgentLogRows()) at the start of getAgentLogsByTimeRange
(before computing end/select and running the query) to flush in-memory/buffered
agent log rows to the DB, then proceed with the existing select and mapping
logic.
---
Nitpick comments:
In `@packages/core/src/db.ts`:
- Around line 701-702: The new getter path duplicates the existing getPath() API
(both return this.dbPath); remove the redundant path getter to avoid a duplicate
surface, and update any internal callers of the path property to call getPath()
instead (or add a small forwarding implementation that calls getPath() if you
need a transitional change). Ensure consistency with CentralDatabase.getPath()
by keeping getPath() as the single source-of-truth for the database path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24350fe9-a4fc-4559-94a9-d1ffc4cad321
📒 Files selected for processing (2)
packages/core/src/db.tspackages/core/src/store.ts
- Move integrity check before SCHEMA_SQL writes (was after, contradicting comment) - Log error message in checkpoint recovery catch block - Make flushAgentLogBuffer private - Move deleteTask flush inside withTaskLock to prevent race - Use try/finally in flush so splice always runs on failure - Filter stale task entries during flush to prevent buffer poisoning - Add flush before getAgentLogsByTimeRange for read consistency - Clean up leaked temp dir in fresh DB test - Fix false-positive buffer-capacity test (query DB directly) - Rename misleading "single flush" test title Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed all remaining review feedback in db.ts:
store.ts:
Tests:
All 811 tests pass. Ready for re-review. |
Contributor
|
Review rate limit: 10/10 reviews remaining, available now. |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/core/src/__tests__/store.test.ts (1)
4976-4991:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid
getAgentLogCount()for the size-triggered flush assertion.This has the same false-positive risk as the earlier count-based buffering test:
getAgentLogCount()auto-flushes, so the test can still pass even if the buffer-full flush path is broken. QueryagentLogEntriesdirectly after the loop to verify the batch write actually landed.Suggested fix
- const countA = await store.getAgentLogCount(taskA.id); - const countB = await store.getAgentLogCount(taskB.id); - expect(countA).toBe(25); - expect(countB).toBe(25); + const rows = (store as any).db + .prepare(` + SELECT taskId, COUNT(*) AS count + FROM agentLogEntries + WHERE taskId IN (?, ?) + GROUP BY taskId + `) + .all(taskA.id, taskB.id) as Array<{ taskId: string; count: number }>; + expect(rows).toEqual( + expect.arrayContaining([ + { taskId: taskA.id, count: 25 }, + { taskId: taskB.id, count: 25 }, + ]), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/__tests__/store.test.ts` around lines 4976 - 4991, The test uses store.getAgentLogCount() which itself triggers an auto-flush, causing a false positive; instead, after the interleaved append loop, query the persisted entries directly via store.agentLogEntries (or whatever read method returns stored rows for a given taskId) for taskA.id and taskB.id and assert those lengths equal 25; keep the same setup that calls store.appendAgentLog in the loop (and createTestTask/store.createTask), but replace the getAgentLogCount assertions with direct queries to store.agentLogEntries(taskId) and compare the returned arrays' lengths to 25 to confirm the buffer-triggered batch write actually occurred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/__tests__/store.test.ts`:
- Around line 4916-4927: The test currently only checks the final DB count and
can pass if deleteTask() drops buffered entries instead of flushing; modify the
test to prove flush happens first by either (a) spying/mocking the
store.flushAgentLogBuffer method and asserting it was called before
store.deleteTask, or (b) asserting the buffered row exists in the SQLite table
prior to calling store.deleteTask (e.g., after appendAgentLog use (store as
any).db.prepare("SELECT COUNT(*)... FROM agentLogEntries WHERE taskId =
?").get(task.id) and expect count > 0), then call deleteTask() and assert count
becomes 0, referencing the functions appendAgentLog, flushAgentLogBuffer, and
deleteTask to locate the code to change.
In `@packages/core/src/db.ts`:
- Around line 913-939: The startup integrity check currently continues
initialization after WAL checkpoint recovery fails; modify the block that calls
this.integrityCheck(), this.db.exec("PRAGMA wal_checkpoint(TRUNCATE)"), and the
subsequent recheck so that if recheck.ok is false you throw an Error (or
otherwise abort initialization) instead of proceeding, and likewise rethrow or
throw after catching an exception from db.exec; ensure you set
this.corruptionDetected before throwing and include this.dbPath and the caught
error message in the thrown Error to surface context to callers.
In `@packages/core/src/store.ts`:
- Around line 4770-4774: The finally block unconditionally calls
this.agentLogBuffer.splice(0, flushCount) which drops buffered entries even when
the DB flush transaction failed; change the logic so that splice is only
executed after a confirmed successful commit (or when the flush operation
indicates success). In practice, add a success flag around the flush/transaction
code in the method containing the try/catch/finally, set success = true after
the transaction completes/commits, and move or guard the
this.agentLogBuffer.splice(0, flushCount) call to run only when success is true
(keep rollback/error handling separate so failed flushes leave the buffer intact
for retry). Ensure flushCount is still computed correctly and used only when
success is true.
---
Duplicate comments:
In `@packages/core/src/__tests__/store.test.ts`:
- Around line 4976-4991: The test uses store.getAgentLogCount() which itself
triggers an auto-flush, causing a false positive; instead, after the interleaved
append loop, query the persisted entries directly via store.agentLogEntries (or
whatever read method returns stored rows for a given taskId) for taskA.id and
taskB.id and assert those lengths equal 25; keep the same setup that calls
store.appendAgentLog in the loop (and createTestTask/store.createTask), but
replace the getAgentLogCount assertions with direct queries to
store.agentLogEntries(taskId) and compare the returned arrays' lengths to 25 to
confirm the buffer-triggered batch write actually occurred.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 41873b5a-fc95-4e79-af5d-5302dd49513a
📒 Files selected for processing (4)
packages/core/src/__tests__/db.test.tspackages/core/src/__tests__/store.test.tspackages/core/src/db.tspackages/core/src/store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/tests/db.test.ts
- Throw from init() when integrity check fails and recovery doesn't help, preventing writes to a known-corrupt database - Only drain buffer on successful flush; requeue valid entries on transient failures (busy/IO) so they aren't silently lost - Add spy on flushAgentLogBuffer in deleteTask test to prove flush-before-delete Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Re: "Stop initialization when integrity remains failed after recovery" (db.ts) Fixed in |
Contributor
Author
|
Re: "Avoid dropping valid buffered logs when a flush transaction fails" (store.ts) Fixed in
|
Contributor
Author
|
Re: "Prove the flush happens before delete" (store.test.ts) Fixed in |
Collaborator
|
Thanks for addressing the comments on this will merge in asap |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/store.ts`:
- Around line 4747-4770: The current flushAgentLogBuffer() captures liveTaskIds
before starting the transaction, creating a TOCTOU where a concurrent delete can
cause an FK rollback; make the delete-skip atomic by performing the existence
check inside the DB transaction: inside the this.db.transaction block use a
prepared statement that inserts only when the task exists (e.g. an INSERT ...
SELECT ... FROM tasks WHERE id = ? pattern or INSERT INTO agentLogEntries(...)
SELECT ?, ?, ?, ?, ?, ? WHERE EXISTS (SELECT 1 FROM tasks WHERE id = ?)), loop
over the batch and execute that statement for each entry, and remove the
pre-filter/liveTaskIds logic; alternatively, implement a single retry on FK
constraint failure inside the transaction (catch FK error from the insert loop,
re-filter by querying tasks inside the same transaction and retry the inserts
once) to close the TOCTOU window (refer to agentLogEntries, flushAgentLogBuffer,
this.db.transaction and this.db.prepare).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ee3cc77c-eccb-4bc0-83db-431c0d0ed553
📒 Files selected for processing (3)
packages/core/src/__tests__/store.test.tspackages/core/src/db.tspackages/core/src/store.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/db.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/tests/store.test.ts
Addresses CodeRabbit TOCTOU concern: querying live task IDs inside the transaction ensures the check and inserts are atomic under SQLite locking, preventing FK violations from concurrent deletes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
CI Failure Analysis — Pre-existing, Not Related to This PRBoth failing shards contain failures that are unrelated to our changes ( Shard 1/3 — Shard 3/3 — 161 The last CI run on Our changes: All 3,354 core package tests pass locally. The PR only modifies |
Merges origin/main to bring in the droid runtime plugin (needed by plugin-loader test) and all recent changes. Adds a "Build plugins" step to pr-checks.yml so plugin dist/ directories are compiled before tests run — fixes ERR_MODULE_NOT_FOUND for hermes and droid plugins. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/db.ts`:
- Around line 950-989: The Store getters are caching this._db / this._archiveDb
before awaiting Database.init(), which can leave a partially initialized (and
potentially corrupt) Database instance in the cache if init() throws; change the
getter logic to await db.init() first using a local variable (e.g., const db =
new Database(...); await db.init();) and only assign this._db = db after init()
succeeds, and mirror the exact same pattern for the archive
getter/this._archiveDb so the instance is only published to the Store once
initialization completes successfully.
In `@packages/core/src/store.ts`:
- Around line 4756-4832: The agent log retry backlog needs a hard cap to avoid
unbounded memory growth: add a constant (e.g. AGENT_LOG_BACKLOG_CAP) and enforce
it in appendAgentLog() before pushing new entries and in flushAgentLogBuffer()
when requeuing validEntries; if accepting the new entries would exceed the cap,
drop entries deterministically (either oldest or newest), increment/log a clear
warning including this.db.path and the drop count, and ensure the buffer length
is trimmed to the cap after any unshift() requeueing so transient failures
cannot grow agentLogBuffer unbounded; update any related comments and tests for
appendAgentLog, flushAgentLogBuffer, and the
AGENT_LOG_BUFFER_SIZE/AGENT_LOG_FLUSH_MS behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ffa46d0-c515-4cdc-adaa-2086b6214e62
📒 Files selected for processing (6)
.changeset/auto-provision-default-agents.md.github/workflows/pr-checks.ymlpackages/core/src/__tests__/db.test.tspackages/core/src/__tests__/store.test.tspackages/core/src/db.tspackages/core/src/store.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/auto-provision-default-agents.md
- packages/core/src/tests/db.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/tests/store.test.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…failure Two fixes from CodeRabbit review: - Add MAX_AGENT_LOG_BACKLOG cap (5000 entries) to prevent unbounded memory growth during prolonged SQLite outages. Oldest entries are dropped when cap is exceeded. - Only assign this._db/this._archiveDb after init() succeeds. On failure, close the DB handle before throwing so callers never get a partially initialized instance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/store.ts (1)
4881-4903:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake batch inserts tolerate deleted tasks too.
appendAgentLogBatch()still inserts every row blindly. If one entry targets a task deleted concurrently, the FK violation rolls back the whole transaction and drops unrelated valid rows in the same batch.flushAgentLogBuffer()already hardened this path; the batch API should apply the same live-task filtering inside the transaction and only emit events for rows that were actually persisted.💡 Proposed fix
- this.db.transaction(() => { - for (const entry of normalizedEntries) { + const persistedEntries = this.db.transaction(() => { + const liveTaskIds = new Set( + (this.db.prepare("SELECT id FROM tasks").all() as Array<{ id: string }>).map((r) => r.id), + ); + const validEntries = normalizedEntries.filter((entry) => liveTaskIds.has(entry.taskId)); + const dropped = normalizedEntries.length - validEntries.length; + if (dropped > 0) { + console.warn( + `[fusion] Dropped ${dropped} batched agent log entries for deleted tasks (${this.db.path})`, + ); + } + for (const entry of validEntries) { stmt.run( entry.taskId, timestamp, @@ entry.detail ?? null, entry.agent ?? null, ); } this.db.bumpLastModified(); + return validEntries; }); - for (const entry of normalizedEntries) { + for (const entry of persistedEntries) { this.emit("agent:log", { timestamp, taskId: entry.taskId,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/store.ts` around lines 4881 - 4903, appendAgentLogBatch() currently inserts all normalizedEntries inside this.db.transaction() causing the whole batch to roll back on FK violations for concurrently deleted tasks; instead, inside the same transaction (where stmt is prepared and this.db.bumpLastModified() is called) look up or check live task IDs and filter normalizedEntries to only those whose taskId still exists before calling stmt.run for each, collect which rows were actually inserted and emit events only for those persisted rows (mirroring the live-task filtering behavior in flushAgentLogBuffer()); ensure you do the existence check using the same DB handle/transaction so race conditions are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/store.ts`:
- Around line 4853-4860: The flush code currently requeues validEntries on
transient failure but leaves agentLogFlushTimer cleared so buffered logs may
never be retried; update the finally block in flushAgentLogBuffer to re-arm the
flush timer when you unshift validEntries back into agentLogBuffer (use the
existing scheduling helper or set agentLogFlushTimer to a new timeout to call
flushAgentLogBuffer after the normal delay) so retries are scheduled
automatically after a transient SQLite/IO failure.
---
Outside diff comments:
In `@packages/core/src/store.ts`:
- Around line 4881-4903: appendAgentLogBatch() currently inserts all
normalizedEntries inside this.db.transaction() causing the whole batch to roll
back on FK violations for concurrently deleted tasks; instead, inside the same
transaction (where stmt is prepared and this.db.bumpLastModified() is called)
look up or check live task IDs and filter normalizedEntries to only those whose
taskId still exists before calling stmt.run for each, collect which rows were
actually inserted and emit events only for those persisted rows (mirroring the
live-task filtering behavior in flushAgentLogBuffer()); ensure you do the
existence check using the same DB handle/transaction so race conditions are
avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f84bcc8e-5c11-4727-a02e-31750f3ab185
📒 Files selected for processing (1)
packages/core/src/store.ts
When flushAgentLogBuffer() requeues valid entries on transient failure, the timer was cleared but never re-armed, leaving entries in memory indefinitely. Now schedules a retry after AGENT_LOG_FLUSH_MS. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add `pnpm build` step to test-shards job so plugin dist/ and core packages are compiled before tests run (fixes plugin-loader test) - Add cli-alias/index.js to ESLint node scripts pattern so Node globals (process, AbortController, fetch) are recognized Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
|
Looks good. Thanks !! |
- task.test.ts: save/delete GITHUB_REPOSITORY env before runTaskPrCreate fallback test so CI env doesn't bypass the getCurrentRepo code path - app.test.tsx: increase waitForFrameContains timeout for ASCII QR render from 3s to 6s to accommodate slower CI environments - remote-auth.test.ts: switch createRemoteSettings default from tailscale to cloudflare with a static ingressUrl so resolveRemoteBaseUrl doesn't 409 when no tunnel is running Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Combined both branches: TaskStore import from main + rmSync import from our branch (needed for fresh DB test cleanup). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rd test - store.ts close(): clear retry timer and buffer before nulling _db to prevent re-opening a new connection after close (Greptile P1) - merger-staging-allowlist.test.ts: use `git init -b main` so the default branch is always named 'main' regardless of git config - SettingsModal.test.tsx: fix expected text for compact DroidCli card — shows "✓ Active" badge, not "✓ Connected — 1.2.3" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gsxdsm
approved these changes
May 5, 2026
Collaborator
|
Thank you for the hard work on this!! |
gsxdsm
added a commit
that referenced
this pull request
May 18, 2026
…-pragmas fix: prevent SQLite B-tree corruption with WAL tuning and batched agent logs
This was referenced May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes recurring SQLite B-tree corruption in
fusion.dbcaused by extreme WAL (Write-Ahead Logging) pressure from unbatched writes to theagentLogEntriestable.Three commits:
0a09c68a— WAL PRAGMA tuning + write-behind agent log buffer + startup integrity check78ff1be1— Review-driven fixes: buffer exception safety, integrity check ordering before migrations1976297b— IncludedbPathin corruption log messages for multi-instance operatorsRoot Cause
The
agentLogEntriestable was the corruption hotspot. Every agent log entry was written as an individual auto-committed INSERT — during normal operation, 29K+ rows accumulated, each triggering its own WAL write and (withsynchronous=FULL) anfsync. Combined with the default 1000-page autocheckpoint interval, the WAL file grew to hundreds of MB. Eventually B-tree pages corrupted, causingSQLITE_CORRUPTerrors on reads.The fix addresses all three contributing factors: WAL growth rate, checkpoint frequency, and write amplification.
Changes by File
packages/core/src/db.ts— WAL PRAGMAs + Integrity CheckWAL tuning (constructor, disk DBs only):
synchronousNORMALfsyncper commit — the biggest write amplification source.wal_autocheckpoint100synchronous=NORMALandjournal_size_limit=4MBbut kept autocheckpoint at 1000; we lower it to 100.journal_size_limit4194304(4 MB)All three PRAGMAs are skipped for in-memory databases (no WAL to tune).
Startup integrity check (
init()method):PRAGMA integrity_checkbeforemigrate()and schema operations. Running migrations against a corrupted database would make corruption worse by writing new pages into a damaged B-tree.PRAGMA wal_checkpoint(TRUNCATE)to force a full WAL checkpoint, then rechecks integrity. If the checkpoint resolves it, logs recovery success. If not, logs a manual recovery command with the DB path.corruptionDetectedflag for upstream's health-check mechanisms.this.dbPathso operators can identify which database is affected when multiple instances run.packages/core/src/store.ts— Write-Behind Agent Log BufferProblem: Each
appendAgentLog()call was a standaloneINSERT INTO agentLogEntries— auto-committed, hitting WAL individually.Solution: A write-behind buffer that batches up to 50 entries (or flushes every 2 seconds), then writes them all in a single transaction.
Key design decisions:
slice()d before the transaction starts. Only after the transaction commits successfully doessplice(0, flushCount)remove the entries. If the transaction fails, the buffer is untouched and will retry on the next flush cycle.this.emit("agentLog", ...)fires immediately so UI and consumers see log entries in real-time.getAgentLogs,getAgentLogCount), beforedeleteTask, and onclose()— ensuring queries always see complete data..unref(): The flush timer calls.unref()so it doesn't prevent the Node process from exiting naturally.taskIdare caught and skipped (stale buffer entries after task deletion).normalizedDetail: Buffer pushes sanitized detail via the upstreamnormalizedDetailmethod rather than raw user input.packages/core/src/__tests__/db.test.ts— 6 new testspackages/core/src/__tests__/store.test.ts— 8 new testsWhy
wal_autocheckpoint=100(not 1000)?The default 1000 pages means the WAL can grow to ~4 MB before checkpointing starts. Under heavy write load (agent log bursts), this window is large enough for corruption to take hold before a checkpoint consolidates pages. At 100 pages (~400 KB), checkpoints happen frequently enough to keep the WAL small, but not so frequently that they create I/O contention. This is the single value upstream has not adopted — their v0.16.0 kept 1000.
Test plan
integrityCheck()andrecoverDatabase())Refs: #24
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
New Behavior
Tests
Chores