feat(sqlite): harden multiprocess resiliency and safety#15
Conversation
|
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:
📝 WalkthroughWalkthroughAdds cross-process advisory write locks, PID liveness and daemon/status APIs, daemon-aware platform-worker orchestration with enqueue-only ↔ in-process fallback, non-destructive SQLite maintenance commands (checkpoint/integrity) with preflight gating, and broad docs/tests for coordinated multi-process DB access and recovery. Changes
Sequence Diagram(s)sequenceDiagram
actor Worker as Platform Worker
participant Daemon as Daemon Manager
participant PID as PID Liveness
participant Queue as Queue Runtime
participant DB as Database
rect rgba(76, 175, 80, 0.5)
Note over Worker,DB: Startup: decide queue mode based on daemon PID
Worker->>Daemon: getDaemonStatus(dbPath)
Daemon->>PID: getPidLiveness(daemonPidPath, removeStale=true)
PID-->>Daemon: {state, pid, stalePid, stalePidRemoved}
alt daemon running
Daemon-->>Worker: {running: true, pid}
Worker->>Queue: setEnqueueOnly()
else daemon missing/dead
Daemon-->>Worker: {running: false}
Worker->>Queue: setInProcess()
end
Worker->>DB: createDatabase(dbPath, {processRole: "platform-worker-..."})
end
sequenceDiagram
actor ProcessA as Writer Process A
participant Lock as Advisory Lock
participant FS as Filesystem
actor ProcessB as Writer Process B
participant DB as Database
rect rgba(156, 39, 176, 0.5)
Note over ProcessA,ProcessB: Advisory write lock acquire/retry/release for coordinated writes
ProcessA->>Lock: acquireWriteLock(lockPath, {role: "daemon"})
Lock->>FS: open(lockPath, "wx")
FS-->>Lock: fd or EEXIST
alt exclusive obtained
Lock->>FS: write owner metadata
Lock-->>ProcessA: WriteLockHandle(reentrant=false)
ProcessA->>DB: perform write (BEGIN IMMEDIATE ... COMMIT)
ProcessA->>Lock: release()
Lock->>FS: unlink(lockPath)
else already held
Lock->>FS: read owner metadata
loop retry until timeout
Lock->>Lock: sleep(retryInterval)
Lock->>FS: check lock existence
end
Lock-->>ProcessB: AdvisoryLockTimeoutError
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR substantially hardens SQLite multi-process safety by introducing a file-based advisory write lock ( Verified findings:
Key strengths:
Confidence Score: 4/5
Last reviewed commit: d51963b |
Additional Comments (5)
Use
Same problem as in
The spin loop reads the owner metadata but never checks whether the owner process (identified by The observedOwner = readOwnerMetadata(lockPath);
// If the owning PID is gone, the lock is stale — remove it and retry immediately
if (observedOwner && !isProcessAlive(observedOwner.pid)) {
try { unlinkSync(lockPath); } catch { /* TOCTOU-safe */ }
continue;
}
Consider making the intent explicit with a named factory or a dedicated read-only observer type, or at minimum add an inline comment explaining why |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/maintenance.ts (1)
190-209:⚠️ Potential issue | 🟠 MajorEnsure folder-context rebuild always closes the database handle.
The current close happens only on the happy path. Any exception before Line 209 leaks the connection.
Proposed fix
Database.enableExtensionSupport(); const db = createDatabase(config.dbPath, { processRole: "maintenance" }); - initializeSchema(db, { - hasVectorExtension: db.hasVectorExtension, - embeddingDimension: config.embeddingDimension, - }); - const sessions = new SessionRepository(db); - const observations = new ObservationRepository(db); - const result = await rebuildFolderContext( - projectPath, - sessions, - observations, - { - maxDepth: config.folderContextMaxDepth, - mode: config.folderContextMode, - filename: config.folderContextFilename, - }, - dryRun, - ); - db.close(); + try { + initializeSchema(db, { + hasVectorExtension: db.hasVectorExtension, + embeddingDimension: config.embeddingDimension, + }); + const sessions = new SessionRepository(db); + const observations = new ObservationRepository(db); + const result = await rebuildFolderContext( + projectPath, + sessions, + observations, + { + maxDepth: config.folderContextMaxDepth, + mode: config.folderContextMode, + filename: config.folderContextFilename, + }, + dryRun, + ); + console.log( + `${dryRun ? "[dry-run] " : ""}Rebuilt context from ${result.observations} observations, touched ${result.filesTouched} files.`, + ); + } finally { + db.close(); + } - console.log( - `${dryRun ? "[dry-run] " : ""}Rebuilt context from ${result.observations} observations, touched ${result.filesTouched} files.`, - ); return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maintenance.ts` around lines 190 - 209, The database handle created by createDatabase(...) is only closed on the happy path; wrap the call to rebuildFolderContext(...) (and any initialization that follows Database.enableExtensionSupport(), createDatabase(...), initializeSchema(...), SessionRepository, ObservationRepository) in a try/finally so db.close() is always executed in the finally block; ensure you still return/propagate the rebuildFolderContext result or rethrow errors after closing; reference createDatabase, initializeSchema, SessionRepository, ObservationRepository, rebuildFolderContext, and db.close when locating where to add the try/finally.
🧹 Nitpick comments (3)
tests/queue/processor-mode.test.ts (1)
143-151: LGTM!The test correctly validates the onEnqueue callback invocation in enqueue-only mode. The setup follows the established pattern and the assertion properly verifies single invocation per enqueue call.
Optional enhancement: Consider adding a complementary test verifying that
onEnqueueis not invoked when mode is"in-process"to ensure the guard condition works both ways.💡 Optional: Test for in-process mode non-invocation
test("in-process mode does not invoke onEnqueue callback", () => { const processor = buildProcessor(); const onEnqueue = mock(() => {}); sessionRepo.create("sess-1", "/tmp/proj"); processor.setOnEnqueue(onEnqueue); processor.enqueue("sess-1", "Read", "output", "call-1"); expect(onEnqueue).toHaveBeenCalledTimes(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/queue/processor-mode.test.ts` around lines 143 - 151, Add a complementary test to assert onEnqueue is not called when the processor is in "in-process" mode: create a processor via buildProcessor(), register a mock with processor.setOnEnqueue(...), create the session with sessionRepo.create("sess-1", "/tmp/proj"), ensure the processor is in "in-process" mode (either by not calling setMode or explicitly calling processor.setMode("in-process")), call processor.enqueue("sess-1", "Read", "output", "call-1"), and assert the mock was called 0 times to validate the guard in enqueue.src/daemon/pid.ts (1)
94-100: Minor: redundant ternary expression.Since
kindis already typed as"claude" | "cursor", the ternary on line 98 is unnecessary —kindcan be used directly.♻️ Simplify suffix derivation
export function getPlatformWorkerPidPath(dbPath: string, kind: PlatformWorkerProcessKind): string { const lastSlash = dbPath.lastIndexOf("/"); const pidDir = lastSlash >= 0 ? dbPath.substring(0, lastSlash) : "."; - const suffix = kind === "claude" ? "claude" : "cursor"; - return `${pidDir}/platform-worker-${suffix}.pid`; + return `${pidDir}/platform-worker-${kind}.pid`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon/pid.ts` around lines 94 - 100, In getPlatformWorkerPidPath simplify the suffix derivation by removing the redundant ternary and use the already-typed kind value directly (i.e., replace the suffix assignment that checks kind === "claude" ? "claude" : "cursor" with using kind itself); keep the rest of the logic (lastSlash, pidDir, return template) unchanged to preserve behavior.tests/e2e/build.test.ts (1)
64-78: Case-sensitive error matching in test assertion — minor improvement suggested.The
docs:buildscript exists in package.json and VitePress is properly configured, so the primary CI failure concerns can be disregarded. However, the error check on line 77 using.not.toContain("error")is case-sensitive and won't catch variations likeErrororERRORin output. Consider using.toLowerCase()for more robust matching:- expect(`${stdout}\n${stderr}`).not.toContain("error"); + expect(`${stdout}\n${stderr}`.toLowerCase()).not.toContain("error");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/build.test.ts` around lines 64 - 78, The test's case-sensitive assertion uses expect(`${stdout}\n${stderr}`).not.toContain("error") which misses other casings like "Error" or "ERROR"; update the assertion in the "docs build command succeeds after documentation updates" test to perform a case-insensitive check by converting the combined output (the variables stdout and stderr gathered from proc) to lowercase (or use a case-insensitive regex) before asserting .not.toContain("error"), referencing the proc, stdout, stderr and exitCode variables in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/db/database.ts`:
- Around line 397-417: The current code passes the user callback fn into
withRetry, which can re-execute fn on retryable DB errors and duplicate external
side effects; change the flow so withRetry does NOT wrap the execution of fn.
Specifically, keep the outer withAdvisoryWriteLock(this.processRole, ...) but
remove the wrapping of the transaction body in withRetry; instead call
this.db.transaction(fn) and inspect wrapped.immediate (as in the existing const
wrapped = this.db.transaction(fn) & { immediate?: () => T }) and, if present,
call wrapped.immediate(); otherwise start the manual "BEGIN IMMEDIATE" / commit
/ rollback sequence and execute fn exactly once inside that sequence; any
retries should only cover low-level DB operations (e.g., exec/driver calls), not
the invocation of fn itself.
- Around line 215-230: The configure-failure path in throwConfigureFailure
currently leaves the opened DB handle open; update throwConfigureFailure to
first close the opened DB handle (check the instance field that stores the
connection, e.g., this.db or this.handle created when the constructor opens the
connection) before throwing: call and await the connection's close method if it
returns a promise (or call the sync close otherwise), wrap that close in its own
try/catch to avoid masking the original configure error, then proceed to log and
rethrow the startupError as before.
In `@src/maintenance.ts`:
- Around line 90-95: The createDatabase call (and
Database.enableExtensionSupport/resolveConfig sequence) is currently outside the
structured failure-reporting try/catch so any exception from createDatabase
escapes the JSON error contract; move or wrap Database.enableExtensionSupport(),
resolveConfig(projectPath) and createDatabase(config.dbPath, { processRole:
"maintenance" }) inside the same try block that catches errors and emits the
structured JSON failure response (same pattern used for the checkpoint/integrity
flow), and apply the same change to the other maintenance branch using
createDatabase (the block referenced around the later occurrence) so both paths
report failures via the existing JSON error handler instead of throwing uncaught
exceptions.
- Around line 83-86: The reset routine currently removes database, -wal and -shm
files but misses the advisory write-lock file; update the cleanup to also
rmSync(`${config.dbPath}.write.lock`, { force: true }) (use the same rmSync and
config.dbPath references as the existing deletions) and include it in the
console log so the reset removes the advisory lock file and reports it.
In `@tests/db/advisory-lock.test.ts`:
- Around line 36-40: The test embeds a machine-specific absolute path via
advisoryLockModulePath/resolve in tests/db/advisory-lock.test.ts which breaks
CI; change it to compute a portable path (e.g., use
path.resolve(path.join(__dirname, '..', '..', 'src', 'db', 'advisory-lock.ts'))
or require.resolve with a relative module specifier) and inject that into the
spawned script string that imports acquireWriteLock so the child process can
locate the module on any machine/CI runner.
In `@tests/db/database.test.ts`:
- Around line 371-374: Replace the hardcoded absolute path in the
databaseModulePath variable with a repository-relative path so the spawned
script can import database.ts across environments (CI/local). Update the code
that sets databaseModulePath (the variable named databaseModulePath used in the
template string assigned to script where createDatabase is imported) to compute
the path relative to the repo root (e.g., using process.cwd() or __dirname to
build "src/db/database.ts") and keep using JSON.stringify(databaseModulePath) in
the script template so the child process receives a portable module path.
---
Outside diff comments:
In `@src/maintenance.ts`:
- Around line 190-209: The database handle created by createDatabase(...) is
only closed on the happy path; wrap the call to rebuildFolderContext(...) (and
any initialization that follows Database.enableExtensionSupport(),
createDatabase(...), initializeSchema(...), SessionRepository,
ObservationRepository) in a try/finally so db.close() is always executed in the
finally block; ensure you still return/propagate the rebuildFolderContext result
or rethrow errors after closing; reference createDatabase, initializeSchema,
SessionRepository, ObservationRepository, rebuildFolderContext, and db.close
when locating where to add the try/finally.
---
Nitpick comments:
In `@src/daemon/pid.ts`:
- Around line 94-100: In getPlatformWorkerPidPath simplify the suffix derivation
by removing the redundant ternary and use the already-typed kind value directly
(i.e., replace the suffix assignment that checks kind === "claude" ? "claude" :
"cursor" with using kind itself); keep the rest of the logic (lastSlash, pidDir,
return template) unchanged to preserve behavior.
In `@tests/e2e/build.test.ts`:
- Around line 64-78: The test's case-sensitive assertion uses
expect(`${stdout}\n${stderr}`).not.toContain("error") which misses other casings
like "Error" or "ERROR"; update the assertion in the "docs build command
succeeds after documentation updates" test to perform a case-insensitive check
by converting the combined output (the variables stdout and stderr gathered from
proc) to lowercase (or use a case-insensitive regex) before asserting
.not.toContain("error"), referencing the proc, stdout, stderr and exitCode
variables in the test.
In `@tests/queue/processor-mode.test.ts`:
- Around line 143-151: Add a complementary test to assert onEnqueue is not
called when the processor is in "in-process" mode: create a processor via
buildProcessor(), register a mock with processor.setOnEnqueue(...), create the
session with sessionRepo.create("sess-1", "/tmp/proj"), ensure the processor is
in "in-process" mode (either by not calling setMode or explicitly calling
processor.setMode("in-process")), call processor.enqueue("sess-1", "Read",
"output", "call-1"), and assert the mock was called 0 times to validate the
guard in enqueue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2d63c08-b4f1-498c-ac42-802047c1b6e9
📒 Files selected for processing (20)
README.mddocs/platforms.mddocs/troubleshooting.mdsrc/daemon.tssrc/daemon/manager.tssrc/daemon/pid.tssrc/db/advisory-lock.tssrc/db/database.tssrc/index.tssrc/maintenance.tssrc/platform-worker.tssrc/runtime/queue-runtime.tstests/daemon/manager.test.tstests/daemon/pid.test.tstests/db/advisory-lock.test.tstests/db/database.test.tstests/e2e/build.test.tstests/integration/maintenance-cli.test.tstests/integration/platform-worker.test.tstests/queue/processor-mode.test.ts
Additional Comments (2)
The fix is to write the owner JSON through the already-open file descriptor with const fd = openSync(lockPath, "wx");
try {
const ownerJson = JSON.stringify(owner);
const buf = Buffer.from(ownerJson, "utf8");
let written = 0;
while (written < buf.length) {
written += writeSync(fd, buf, written);
}
} catch (writeError) {
releaseLockPath(lockPath, fd);
throw writeError;
}
Between The PID file should be written before the queue is started: const state = initialize(platform, args.projectDir);
writePid(state.workerPidPath); // ← write PID first
state.queueRuntime.start(); // ← then start the queueThis requires refactoring |
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 `@src/db/advisory-lock.ts`:
- Around line 205-209: After opening the lock file with openSync(lockPath, "wx")
and obtaining fd, do not call writeFileSync(lockPath, ...); instead write the
JSON metadata through the acquired file descriptor (fd) to avoid reopening the
path and the race window—use fs.writeFileSync(fd, JSON.stringify(owner), {
encoding: "utf8" }) or equivalent fs.writeSync with a Buffer; keep the existing
catch block that calls releaseLockPath(lockPath, fd) but ensure releaseLockPath
closes the fd, and propagate or log the write error as before.
In `@src/platform-worker.ts`:
- Around line 308-313: The PID file is being removed unconditionally which can
delete another live worker's PID; update cleanup and any other removal points
(e.g., the other removal around removePid usage) to first read and parse the PID
file written by writePid, compare it to the current process PID (process.pid),
and only call removePid if they match; if the file is unreadable or the PID
differs, do not delete and log a warning/error instead to avoid removing another
worker's PID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b78d8da-547b-4875-9f68-fe6020320fe8
📒 Files selected for processing (9)
src/daemon/pid.tssrc/db/advisory-lock.tssrc/db/database.tssrc/maintenance.tssrc/platform-worker.tstests/db/advisory-lock.test.tstests/db/database.test.tstests/e2e/build.test.tstests/queue/processor-mode.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/queue/processor-mode.test.ts
- tests/e2e/build.test.ts
- src/daemon/pid.ts
Additional Comments (4)
In the transaction fallback path, if While Bun's
There is a time-of-check/time-of-use (TOCTOU) window between the
After step 5, the live process A safer approach is to re-read and compare the PID before unlinking (matching the
The same pattern appears in The inline comment acknowledges the constraint ("Bun SQLite is synchronous"), but the implications for the host plugin's main thread responsiveness are significant if lock contention is sustained. Consider documenting the worst-case latency impact in the advisory-lock API docs, and exposing the timeout as a configuration option in contexts like
Consider either:
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/daemon/pid.ts (1)
30-39:⚠️ Potential issue | 🟠 MajorHandle PID read races without throwing.
Line 31 + Line 34 is a TOCTOU pattern; another process can remove/change the file between checks, and
readFileSyncwill throw. In liveness polling, that can crash status flows.🔧 Proposed fix
export function readPid(pidPath: string): number | null { - if (!existsSync(pidPath)) { - return null; - } - const content = readFileSync(pidPath, "utf-8").trim(); - const pid = Number.parseInt(content, 10); - if (Number.isNaN(pid)) { - return null; - } - return pid; + try { + const content = readFileSync(pidPath, "utf-8").trim(); + if (!/^\d+$/.test(content)) { + return null; + } + const pid = Number.parseInt(content, 10); + return pid > 0 ? pid : null; + } catch { + return null; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon/pid.ts` around lines 30 - 39, The readPid function has a TOCTOU race between existsSync(pidPath) and readFileSync(pidPath) causing readFileSync to throw if the file is removed/changed; wrap the file read/parse in a try/catch inside readPid (avoid relying on existsSync), so any FS error (e.g., ENOENT) or parse errors are caught and the function returns null instead of throwing; reference the readPid function and the uses of existsSync/readFileSync when making this change.
♻️ Duplicate comments (1)
src/platform-worker.ts (1)
301-301:⚠️ Potential issue | 🟠 MajorGuard against overwriting a live worker PID file at startup.
Line 301 writes the PID file unconditionally. If a same-kind worker is already alive, this can clobber its ownership metadata and break liveness/cleanup semantics.
🔧 Proposed fix
-import { getPlatformWorkerPidPath, removePidIfMatches, writePid } from "./daemon/pid"; +import { + getPlatformWorkerPidPath, + isProcessAlive, + readPid, + removePidIfMatches, + writePid, +} from "./daemon/pid"; @@ - writePid(state.workerPidPath); + const existingPid = readPid(state.workerPidPath); + if (existingPid !== null && existingPid !== process.pid && isProcessAlive(existingPid)) { + throw new Error( + `[open-mem] Refusing to overwrite live worker PID file (${state.workerPidPath}, pid=${existingPid})`, + ); + } + writePid(state.workerPidPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform-worker.ts` at line 301, The PID file is being written unconditionally via writePid(state.workerPidPath), which can clobber a live worker's PID; before calling writePid, check whether state.workerPidPath exists, read its PID, and verify the process is still alive (e.g., via a helper like isProcessAlive(pid) or sending signal 0) and whether it represents the same kind of worker; if a live same-kind process is found, abort startup or log and exit gracefully, otherwise remove/stale the file and then call writePid(state.workerPidPath) to create the new PID file.
🧹 Nitpick comments (2)
src/db/database.ts (1)
331-336: Reuse the sleep wait buffer inside retry loop.Line 335 allocates a new buffer every retry attempt. Consider reusing a module-level wait array to reduce pressure under lock contention.
♻️ Proposed refactor
+const RETRY_WAIT_ARRAY = new Int32Array(new SharedArrayBuffer(4)); @@ - Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, delayMs); + Atomics.wait(RETRY_WAIT_ARRAY, 0, 0, delayMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/database.ts` around lines 331 - 336, The retry loop currently allocates a new SharedArrayBuffer for each Atomics.wait call (new Int32Array(new SharedArrayBuffer(4))) which increases allocations under contention; introduce a module-level or function-scoped reusable Int32Array (e.g., waitArray) created once (using new SharedArrayBuffer(4)) and replace the per-attempt allocation in the retry loop where Atomics.wait is called (inside the code path that computes delayMs using BASE_DELAY_MS and then calls Atomics.wait) to use that reusable waitArray; keep all other logic (delayMs calculation and subsequent call to getSqliteErrorDetails) unchanged.src/db/advisory-lock.ts (1)
70-73: Avoid per-call buffer allocation insleepSync.Line 72 creates a new
SharedArrayBuffer/Int32Arrayevery retry sleep. Reuse a module-level wait buffer to reduce allocation churn in contention paths.♻️ Proposed refactor
+const SLEEP_WAIT_ARRAY = new Int32Array(new SharedArrayBuffer(4)); + function sleepSync(ms: number): void { if (ms <= 0) return; - Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); + Atomics.wait(SLEEP_WAIT_ARRAY, 0, 0, ms); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/advisory-lock.ts` around lines 70 - 73, The sleepSync function currently allocates a new SharedArrayBuffer and Int32Array on every call which causes allocation churn; instead create a module-level SharedArrayBuffer and Int32Array (e.g., const waitBuf = new SharedArrayBuffer(4) and const waitView = new Int32Array(waitBuf)) and have sleepSync(ms: number) use Atomics.wait(waitView, 0, 0, ms) (keeping the early return for ms <= 0) so repeated sleeps reuse the same buffer/view rather than allocating per-call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/daemon/pid.ts`:
- Around line 68-73: The stale-PID cleanup currently calls removePid(pidPath)
unconditionally and may delete a newly written PID file; instead, before
removing check that the current file still contains the stale PID we read
earlier: read the file contents (e.g., fs.readFileSync(pidPath, "utf8") or
equivalent), compare it to the stale pid value you stored (stalePid or pid), and
only call removePid(pidPath) if they match; set stalePidRemoved based on that
conditional removal and handle/read errors safely (try/catch) so you don't
accidentally remove an active process's PID metadata.
In `@src/db/database.ts`:
- Around line 463-467: The checkpointWal method currently interpolates the mode
directly into the PRAGMA SQL; add a runtime whitelist check inside checkpointWal
(before calling db.query) to ensure mode is one of the allowed values (e.g.
normalize to uppercase and allow "PASSIVE","FULL","RESTART"), throw a clear
error if not, and only then use the validated/sanitized mode value when
constructing the PRAGMA string; reference the checkpointWal method and the mode
parameter and keep the rest of the withAdvisoryWriteLock/withRetry flow
unchanged.
In `@tests/db/database.test.ts`:
- Around line 392-413: The race occurs because Bun.spawn is started before the
parent holds the transaction lock; move the child process creation so it happens
after acquiring the lock (i.e., inside the db.transaction block, before the
Atomics.wait) or add an explicit synchronization file/flag: once db.transaction
starts and before Atomics.wait, write the marker (markerPath) or signal the
child, then spawn the child (Bun.spawn) so the child will contend while the
parent holds the lock; reference db.transaction, Atomics.wait, Bun.spawn, and
markerPath when making the change.
In `@tests/integration/platform-worker.test.ts`:
- Around line 486-503: The test currently assumes getMaintenancePreflightStatus
mutates state by removing a stale PID file; change the test to treat preflight
as read-only: after writing the stale PID via getPlatformWorkerPidPath and
calling getMaintenancePreflightStatus, assert workerCheck is defined,
workerCheck.state === "dead", workerCheck.stalePid === 999999, but assert
workerCheck.stalePidRemoved === false and that existsSync(stalePidPath) remains
true (i.e., the file is not deleted). Keep references to
getMaintenancePreflightStatus, getPlatformWorkerPidPath, preflight, workerCheck
and stalePidPath so the assertions are updated in place.
---
Outside diff comments:
In `@src/daemon/pid.ts`:
- Around line 30-39: The readPid function has a TOCTOU race between
existsSync(pidPath) and readFileSync(pidPath) causing readFileSync to throw if
the file is removed/changed; wrap the file read/parse in a try/catch inside
readPid (avoid relying on existsSync), so any FS error (e.g., ENOENT) or parse
errors are caught and the function returns null instead of throwing; reference
the readPid function and the uses of existsSync/readFileSync when making this
change.
---
Duplicate comments:
In `@src/platform-worker.ts`:
- Line 301: The PID file is being written unconditionally via
writePid(state.workerPidPath), which can clobber a live worker's PID; before
calling writePid, check whether state.workerPidPath exists, read its PID, and
verify the process is still alive (e.g., via a helper like isProcessAlive(pid)
or sending signal 0) and whether it represents the same kind of worker; if a
live same-kind process is found, abort startup or log and exit gracefully,
otherwise remove/stale the file and then call writePid(state.workerPidPath) to
create the new PID file.
---
Nitpick comments:
In `@src/db/advisory-lock.ts`:
- Around line 70-73: The sleepSync function currently allocates a new
SharedArrayBuffer and Int32Array on every call which causes allocation churn;
instead create a module-level SharedArrayBuffer and Int32Array (e.g., const
waitBuf = new SharedArrayBuffer(4) and const waitView = new Int32Array(waitBuf))
and have sleepSync(ms: number) use Atomics.wait(waitView, 0, 0, ms) (keeping the
early return for ms <= 0) so repeated sleeps reuse the same buffer/view rather
than allocating per-call.
In `@src/db/database.ts`:
- Around line 331-336: The retry loop currently allocates a new
SharedArrayBuffer for each Atomics.wait call (new Int32Array(new
SharedArrayBuffer(4))) which increases allocations under contention; introduce a
module-level or function-scoped reusable Int32Array (e.g., waitArray) created
once (using new SharedArrayBuffer(4)) and replace the per-attempt allocation in
the retry loop where Atomics.wait is called (inside the code path that computes
delayMs using BASE_DELAY_MS and then calls Atomics.wait) to use that reusable
waitArray; keep all other logic (delayMs calculation and subsequent call to
getSqliteErrorDetails) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06b48340-9204-4996-b726-121cdaf32040
📒 Files selected for processing (7)
src/daemon/pid.tssrc/db/advisory-lock.tssrc/db/database.tssrc/platform-worker.tstests/daemon/pid.test.tstests/db/database.test.tstests/integration/platform-worker.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/daemon/pid.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/integration/platform-worker.test.ts (2)
84-98: Silent JSON parse failures may hide test issues.The
flatMapwith empty array return on parse failure silently drops malformed lines. If a worker emits partial JSON due to a bug, the test would pass with fewer responses than expected rather than failing explicitly.💡 Consider logging dropped lines for debugging
.flatMap((line) => { try { return [ JSON.parse(line) as { ok: boolean; code: string; message?: string; status?: Record<string, unknown>; processed?: number; }, ]; } catch { + console.warn(`[test] Dropped non-JSON line: ${line.slice(0, 100)}`); return []; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/platform-worker.test.ts` around lines 84 - 98, The current flatMap that calls JSON.parse silently swallows parse errors and drops malformed lines; update the catch block in the flatMap (the JSON.parse call within tests/integration/platform-worker.test.ts) to surface failures instead of returning an empty array — e.g., log the raw `line` and rethrow a descriptive Error (or return a single-item object that fails the subsequent assertions) so the test fails loudly when a line cannot be parsed; reference the flatMap/JSON.parse location and ensure the thrown error includes the offending `line` for debugging.
505-576: Ensure complete mock restoration on test failure.The test patches multiple prototype methods but has all restoration in a single
finallyblock. If an earlier restoration throws, subsequent restorations won't execute. Consider wrapping each restoration individually.🛡️ More robust restoration pattern
} finally { - if (previousDaemon === undefined) { - delete process.env.OPEN_MEM_DAEMON; - } else { - process.env.OPEN_MEM_DAEMON = previousDaemon; - } - DaemonManager.prototype.start = originalStart; - DaemonManager.prototype.stop = originalStop; - DaemonManager.prototype.isRunning = originalIsRunning; - DaemonManager.prototype.getStatus = originalGetStatus; - DaemonManager.prototype.signal = originalSignal; - QueueProcessor.prototype.setMode = originalSetMode; + const restorations = [ + () => { process.env.OPEN_MEM_DAEMON = previousDaemon ?? ""; if (previousDaemon === undefined) delete process.env.OPEN_MEM_DAEMON; }, + () => { DaemonManager.prototype.start = originalStart; }, + () => { DaemonManager.prototype.stop = originalStop; }, + () => { DaemonManager.prototype.isRunning = originalIsRunning; }, + () => { DaemonManager.prototype.getStatus = originalGetStatus; }, + () => { DaemonManager.prototype.signal = originalSignal; }, + () => { QueueProcessor.prototype.setMode = originalSetMode; }, + ]; + for (const restore of restorations) { + try { restore(); } catch { /* best-effort */ } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/platform-worker.test.ts` around lines 505 - 576, The test mutates DaemonManager.prototype.start/stop/isRunning/getStatus/signal and QueueProcessor.prototype.setMode but restores them all inside one finally block, so if one restoration throws the rest won't run; change the finally to restore each prototype method in its own safe try/catch (or conditional assignment) so every original (originalStart/originalStop/originalIsRunning/originalGetStatus/originalSignal/originalSetMode) is attempted regardless of errors, e.g., restore DaemonManager.prototype.start in its own try/catch, then stop, isRunning, getStatus, signal, and finally QueueProcessor.prototype.setMode, ensuring seenModes logic remains unchanged.tests/db/database.test.ts (1)
247-296: Consider cleanup robustness for the manually created lock file.The test creates a lock file at
db.writeLockPathto simulate a stale lock. While there's atry/catchfor cleanup in thefinallyblock, if the test fails before reachingfinally, the lock file could persist. ThecleanupPathsarray isn't used here.🛡️ Suggested improvement
test("test_lock_timeout_diagnostics_include_process_role", () => { const dbPath = `/tmp/open-mem-test-${randomUUID()}.db`; cleanupPaths.push(dbPath); const db = createDatabase(dbPath, { processRole: "maintenance" }); const lockPath = db.writeLockPath; + cleanupPaths.push(lockPath); writeFileSync( lockPath,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/db/database.test.ts` around lines 247 - 296, The test test_lock_timeout_diagnostics_include_process_role may leave the manually created lock file behind if the test fails before finally runs; after creating the lock file at db.writeLockPath (variable lockPath) push lockPath into the existing cleanupPaths array so global cleanup will remove it, i.e., add cleanupPaths.push(lockPath) immediately after writing the file (or before) and keep the existing finally unlinkSync block for best-effort immediate cleanup.src/daemon/manager.ts (1)
158-227: Consider potential TOCTOU race in the OS signal path.Between calling
getStatus()at Line 180 and sendingprocess.kill()at Line 200, the daemon process could exit. While this is caught by the outertry/catchand returnsdelivery-failed, the error message might be confusing since it would contain the OS-level error rather than a clear "daemon exited" message.This is a minor edge case since the
delivery-failedstate correctly indicates failure, but you could consider checking liveness again or providing more specific error context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon/manager.ts` around lines 158 - 227, In signal(), avoid the TOCTOU confusion between getStatus() and process.kill(): before sending SIGUSR1 (in the branch where status.pid !== null) re-check liveness (e.g., call this.getStatus() again or use process.kill(pid, 0)) and if the process is gone return a clearer failure (map ESRCH or a missing PID to a specific state like "daemon-exited" or "no-daemon" with appropriate pid/stalePid), otherwise proceed to call process.kill(pid, "SIGUSR1"); also wrap the actual process.kill call in its own try/catch to translate OS errors (ESRCH) into that clearer state instead of a generic delivery-failed with raw OS error.src/db/database.ts (2)
54-65: Potential infinite loop if regex replacement produces same string but with different internal state.The
stripLeadingSqlNoisefunction loops until the string stops changing. While the regex replacement should always make progress or return the same string, consider adding a safety limit.🛡️ Optional safety limit
function stripLeadingSqlNoise(sql: string): string { let normalized = sql; + let iterations = 0; + const maxIterations = 100; for (;;) { + if (++iterations > maxIterations) { + console.warn("[open-mem] stripLeadingSqlNoise exceeded max iterations"); + return normalized.trimStart(); + } const next = normalized.replace(LEADING_SQL_NOISE, ""); if (next === normalized) { return normalized.trimStart(); } normalized = next; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/database.ts` around lines 54 - 65, stripLeadingSqlNoise can theoretically loop indefinitely if replace continues to return a string that appears different to the loop condition; add a safety iteration cap inside the function to prevent an infinite loop: in function stripLeadingSqlNoise(sql: string) use a maxIterations (e.g. 1000) and decrement/count each loop iteration, and if the cap is reached break out and return normalized.trimStart(); keep using LEADING_SQL_NOISE for the replace but ensure the loop exits when iterations exceed the cap (optionally include a debug/warning path if desired).
496-524: Consider validatingmaxErrorsupper bound.While
maxErrorsis floored to at least 1, SQLite'sPRAGMA integrity_check(N)accepts very large values. An extremely largemaxErrors(e.g.,Number.MAX_SAFE_INTEGER) would be valid but potentially expensive. Consider adding an upper bound.💡 Optional upper bound
public integrityCheck(maxErrors = 1): IntegrityCheckResult { - const normalizedMaxErrors = Number.isFinite(maxErrors) ? Math.max(1, Math.floor(maxErrors)) : 1; + const MAX_INTEGRITY_ERRORS = 1000; + const normalizedMaxErrors = Number.isFinite(maxErrors) + ? Math.min(MAX_INTEGRITY_ERRORS, Math.max(1, Math.floor(maxErrors))) + : 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/database.ts` around lines 496 - 524, The integrityCheck method currently only floors maxErrors to >=1 but doesn't cap extremely large values; add an upper bound constant (e.g., MAX_INTEGRITY_ERRORS) and clamp normalizedMaxErrors to Math.min(Math.max(1, Math.floor(maxErrors)), MAX_INTEGRITY_ERRORS) before calling PRAGMA integrity_check, or throw a RangeError if maxErrors exceeds the allowed maximum; update the variable used in the PRAGMA call (normalizedMaxErrors) and consider emitting a warning or clear error when clipping occurs so callers know their value was reduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/build.test.ts`:
- Around line 64-82: The test fails in CI because runner detection is fragile,
the "error" string check is brittle, and docs deps may not be installed; update
the test named "docs build command succeeds after documentation updates" to (1)
use process.execPath directly for the runner (replace the
basename/process.execPath conditional and Bun.which logic with
process.execPath), (2) remove the combined stdout/stderr
`.toLowerCase().not.toContain("error")` assertion and rely only on
`expect(exitCode).toBe(0)` for success, and (3) guard the test when docs
dependencies are missing by skipping in that case (use a preflight like
test.skipIf(!existsSync(resolve(ROOT, "docs/node_modules"))) or ensure docs
dependencies are installed before running the test by running a package manager
install in the docs directory); refer to the test name, the runner variable,
process.execPath, exitCode assertion, and test.skipIf/existsSync/resolve to find
and change the code.
---
Nitpick comments:
In `@src/daemon/manager.ts`:
- Around line 158-227: In signal(), avoid the TOCTOU confusion between
getStatus() and process.kill(): before sending SIGUSR1 (in the branch where
status.pid !== null) re-check liveness (e.g., call this.getStatus() again or use
process.kill(pid, 0)) and if the process is gone return a clearer failure (map
ESRCH or a missing PID to a specific state like "daemon-exited" or "no-daemon"
with appropriate pid/stalePid), otherwise proceed to call process.kill(pid,
"SIGUSR1"); also wrap the actual process.kill call in its own try/catch to
translate OS errors (ESRCH) into that clearer state instead of a generic
delivery-failed with raw OS error.
In `@src/db/database.ts`:
- Around line 54-65: stripLeadingSqlNoise can theoretically loop indefinitely if
replace continues to return a string that appears different to the loop
condition; add a safety iteration cap inside the function to prevent an infinite
loop: in function stripLeadingSqlNoise(sql: string) use a maxIterations (e.g.
1000) and decrement/count each loop iteration, and if the cap is reached break
out and return normalized.trimStart(); keep using LEADING_SQL_NOISE for the
replace but ensure the loop exits when iterations exceed the cap (optionally
include a debug/warning path if desired).
- Around line 496-524: The integrityCheck method currently only floors maxErrors
to >=1 but doesn't cap extremely large values; add an upper bound constant
(e.g., MAX_INTEGRITY_ERRORS) and clamp normalizedMaxErrors to
Math.min(Math.max(1, Math.floor(maxErrors)), MAX_INTEGRITY_ERRORS) before
calling PRAGMA integrity_check, or throw a RangeError if maxErrors exceeds the
allowed maximum; update the variable used in the PRAGMA call
(normalizedMaxErrors) and consider emitting a warning or clear error when
clipping occurs so callers know their value was reduced.
In `@tests/db/database.test.ts`:
- Around line 247-296: The test
test_lock_timeout_diagnostics_include_process_role may leave the manually
created lock file behind if the test fails before finally runs; after creating
the lock file at db.writeLockPath (variable lockPath) push lockPath into the
existing cleanupPaths array so global cleanup will remove it, i.e., add
cleanupPaths.push(lockPath) immediately after writing the file (or before) and
keep the existing finally unlinkSync block for best-effort immediate cleanup.
In `@tests/integration/platform-worker.test.ts`:
- Around line 84-98: The current flatMap that calls JSON.parse silently swallows
parse errors and drops malformed lines; update the catch block in the flatMap
(the JSON.parse call within tests/integration/platform-worker.test.ts) to
surface failures instead of returning an empty array — e.g., log the raw `line`
and rethrow a descriptive Error (or return a single-item object that fails the
subsequent assertions) so the test fails loudly when a line cannot be parsed;
reference the flatMap/JSON.parse location and ensure the thrown error includes
the offending `line` for debugging.
- Around line 505-576: The test mutates
DaemonManager.prototype.start/stop/isRunning/getStatus/signal and
QueueProcessor.prototype.setMode but restores them all inside one finally block,
so if one restoration throws the rest won't run; change the finally to restore
each prototype method in its own safe try/catch (or conditional assignment) so
every original
(originalStart/originalStop/originalIsRunning/originalGetStatus/originalSignal/originalSetMode)
is attempted regardless of errors, e.g., restore DaemonManager.prototype.start
in its own try/catch, then stop, isRunning, getStatus, signal, and finally
QueueProcessor.prototype.setMode, ensuring seenModes logic remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e887d561-ad54-4b7e-acd5-cba7b693d330
📒 Files selected for processing (8)
src/daemon/manager.tssrc/daemon/pid.tssrc/db/database.tssrc/runtime/queue-runtime.tstests/daemon/manager.test.tstests/db/database.test.tstests/e2e/build.test.tstests/integration/platform-worker.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/daemon/pid.ts
- src/runtime/queue-runtime.ts
Additional Comments (3)
In the The fix is to wrap the rollback in a best-effort try-catch so the original error is always re-thrown:
If Windows support is a goal, consider adding an explicit platform guard before reaching this line:
Each call to A simple improvement is to allocate the buffer and its |
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 `@package.json`:
- Line 57: The "docs:build" npm script in package.json currently runs "bun
install --frozen-lockfile" which fails because there is no docs/bun.lock or
docs/bun.lockb; either commit a docs lockfile to docs/ (bun.lock or bun.lockb)
and keep the --frozen-lockfile flag, or remove the --frozen-lockfile option from
the "docs:build" script so bun can install without a pre-existing lockfile;
update the "docs:build" entry accordingly to resolve CI/e2e failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c885e72-a296-437c-91ce-031d8bd0f732
📒 Files selected for processing (2)
package.jsontests/e2e/build.test.ts
Additional Comments (1)
|
Additional Comments (3)
In the fallback Wrap the ROLLBACK in a try/catch to preserve the original error:
A comment says Consider throwing or logging an error inside
The check A more precise fix is to move the |
Additional Comments (4)
The double-check pattern (re-read owner before
This can only be fixed with an atomic compare-and-delete primitive (not available in POSIX). A pragmatic mitigation is to verify post-deletion that the lock can be immediately re-acquired: try {
unlinkSync(lockPath);
// After reaping, caller will retry the O_EXCL open; no further action here
return true;
} catch (error) {
const code = (error as { code?: string }).code;
if (code === "ENOENT") return false; // another reaper beat us, that's fine
throw error;
}The existing behavior already retries the
When the daemon was started in-process (via The caller in const result = state.daemonManager?.signal("PROCESS_NOW");
if (!result?.ok) {
fallbackToInProcess(state);
}A false if (this.subprocess) {
try {
this.subprocess.send(message);
} catch {
// fall through to catch block
throw new Error("IPC send failed");
}
const status = readStatusSafely();
if (!status?.running) {
return {
ok: false,
state: "daemon-dead",
via: "none",
pid: null,
message,
error: "Daemon exited after IPC send",
};
}
return { ok: true, state: "delivered", via: "ipc", pid: status.pid ?? null, message };
}
The Clear the timer first:
With the default public migrate(migrations: Migration[]): void {
// Read applied versions without holding the write lock
this.ensureMigrationTable();
const applied = this.withRetry("migrate.applied_versions", () =>
this.db.query("SELECT version FROM _migrations ORDER BY version").all() as { version: number }[]
);
const appliedVersions = new Set(applied.map((m) => m.version));
const pending = migrations
.filter((m) => !appliedVersions.has(m.version))
.sort((a, b) => a.version - b.version);
// Each transaction already holds the advisory lock via transaction()
for (const migration of pending) {
this.transaction(() => {
this.exec(migration.up);
this.run("INSERT INTO _migrations (version, name) VALUES ($version, $name)", [
{ $version: migration.version, $name: migration.name },
]);
});
}
}This still guarantees each migration is atomic while avoiding holding the cross-process lock unnecessarily across reads and multiple sequential writes. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Additional Comments (3)
Any concurrent write operation from a plugin, daemon, or platform worker that tries to acquire the advisory lock during a long checkpoint will fail with Consider either:
If this fallback path is intentional defensive code (e.g., for non-Bun runtimes), consider adding a warning log so the fallback is visible during debugging: console.warn("[open-mem] transaction(): wrapped.immediate unavailable, using manual BEGIN IMMEDIATE fallback");
this.db.exec("BEGIN IMMEDIATE");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!
Consider adding a guard in start(): boolean {
if (!this.daemonScript) {
throw new Error("DaemonManager is observation-only (daemonScript is empty); start() must not be called");
}
// ... rest of method
} |
Additional Comments (1)
In Node.js (and by extension Bun), Consider using The corresponding sender in process.kill(status.pid, "SIGUSR2"); |
1 participant
Summary
sqlite checkpoint/sqlite integrity), expand operator/docs guidance, and add broad integration and regression test coverage for contention, fallback, and reset workflows.Validation
bun test(936 passed, 0 failed)bun run buildNote
Cursor Bugbot is generating a summary for commit 5c1d426. Configure here.
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests