Repository navigation
fix(ai-vault): bound OpenCode SQLite and parse-cache scans - #10708
AmethystLiang wants to merge 10 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pull request adds bounded JSON cache persistence with byte and structural admission checks, newest-path deduplication, suffix trimming, and expanded persistence tests. It introduces an abortable OpenCode SQLite scan context, FIFO worker transport with timeout, cancellation, fault recovery, and idle teardown, and threads that context through discovery and parsing. Candidate parsing manages SQLite deadlines and cached candidates, scan outcomes record metrics and skipped-work issues, and scanner helpers handle history reads, execution-host assignment, and session merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/ai-vault/session-parse-cache-persistence.test.ts (2)
259-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoundary depths are implicit — the test writes 28/29, not 32/33.
The wrapper (root object →
entries→ entry pair → entry object) contributes the remaining 4 levels, so 28/29 land on the 32/33 boundary. That coupling is invisible: any change to the persisted file shape shifts the effective depth and the test would keep passing while no longer probing the limit.♻️ Derive the session depth from the limit and the wrapper depth
+ // Wrapper adds 4 levels: file object → entries → entry pair → entry object. + const WRAPPER_DEPTH = 4 + const maxSessionDepth = SESSION_PARSE_CACHE_JSON_LIMITS.nestingDepth - WRAPPER_DEPTH - await writeNestedCache(28) + await writeNestedCache(maxSessionDepth) initSessionParseCachePersistence({ filePath: cacheFile, appVersion: APP_VERSION }) expect((await coldParseStats(transcript)).reused).toBe(1) simulateRestart(cacheFile) - await writeNestedCache(29) + await writeNestedCache(maxSessionDepth + 1)Requires exporting/importing
SESSION_PARSE_CACHE_JSON_LIMITS.
608-619: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock teardown after assertions leaves
console.debugstubbed on failure. Both writer-admission tests restore the spy on the line afterexpect(...), so a failed assertion aborts beforemockRestore()and silently swallows debug output in every later test in the file. Move teardown into a lifecycle hook.
src/main/ai-vault/session-parse-cache-persistence.test.ts#L608-L619: drop the inlinedebugSpy.mockRestore()and rely on anafterEach(() => vi.restoreAllMocks())(or setrestoreMocks: truein the Vitest config).src/main/ai-vault/session-parse-cache-persistence.test.ts#L641-L652: apply the same change to the byte-admission test.src/main/ai-vault/session-scanner-opencode-sqlite-coexistence.test.ts (1)
228-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the advance from the exported deadline constant.
45_000is duplicated across three tests and silently tracksOPENCODE_SQLITE_SCAN_DEADLINE_MS. Importing the constant (and advancing by+ 1) keeps these tests correct if the budget changes.Also applies to: 282-282, 315-315
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a55dd7d6-a841-4466-a6dd-18e6574ebd77
📒 Files selected for processing (21)
src/main/ai-vault/session-parse-cache-persistence.test.tssrc/main/ai-vault/session-parse-cache-persistence.tssrc/main/ai-vault/session-scanner-agent-parser.tssrc/main/ai-vault/session-scanner-antigravity-history.tssrc/main/ai-vault/session-scanner-execution-host.tssrc/main/ai-vault/session-scanner-opencode-sources.tssrc/main/ai-vault/session-scanner-opencode-sqlite-candidate-phase.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-candidate-phase.tssrc/main/ai-vault/session-scanner-opencode-sqlite-coexistence.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-discovery.tssrc/main/ai-vault/session-scanner-opencode-sqlite-scan-context.tssrc/main/ai-vault/session-scanner-opencode-sqlite-scan-outcome.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-scan-outcome.tssrc/main/ai-vault/session-scanner-opencode-sqlite-worker-client.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-worker-client.tssrc/main/ai-vault/session-scanner-opencode-sqlite-worker-spawn.tssrc/main/ai-vault/session-scanner-opencode-sqlite-worker-transport.tssrc/main/ai-vault/session-scanner-parse-cache.tssrc/main/ai-vault/session-scanner-session-merge.tssrc/main/ai-vault/session-scanner-source-discovery.tssrc/main/ai-vault/session-scanner.ts
|
@coderabbitai[bot] resolved |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/ai-vault/session-parse-cache-persistence.ts (1)
112-130: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winLimit-rejected cache files are never dropped, so the cost repeats every launch.
A file that trips
SESSION_PARSE_CACHE_MAX_BYTESorSESSION_PARSE_CACHE_JSON_LIMITSthrows into the outercatch, bypassing thermat Line 126. Until a save happens to succeed, each launch re-reads up to 64 MiB and re-scans it; if serialization returnsnullthe stale file stays forever. Treating capacity rejection like unreadable content makes it self-healing.🛠️ Sketch: drop the file on capacity rejection too
try { const { buffer } = await readNodeFileWithinLimit( current.filePath, SESSION_PARSE_CACHE_MAX_BYTES ) const raw = buffer.toString('utf8') await assertSessionParseCacheJsonWithinLimitsCooperatively(raw, SESSION_PARSE_CACHE_JSON_LIMITS) const entries = parsePersistedFile(JSON.parse(raw), current.appVersion) if (entries) { seedSessionParseCache(entries) return } // Unreadable content, not a missing file: drop it so it can't be re-read // every launch while the next save replaces it. await rm(current.filePath, { force: true }).catch(() => {}) - } catch { + } catch (err) { + if (isCacheAdmissionRejection(err)) { + await rm(current.filePath, { force: true }).catch(() => {}) + } // Why: a missing/corrupt/foreign cache file must never fail the scan; // worst case is exactly today's cold scan. }
🧹 Nitpick comments (8)
src/main/ai-vault/session-parse-cache-snapshot-serialization.ts (1)
156-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the duplicated suffix search into one generic helper.
The sync and async variants share identical search logic; the sync path can delegate by treating
serializeas a thunk, or both can call one generator/step function. Keeps the trimming invariant defined in one place.src/main/ai-vault/session-parse-cache-persistence.ts (1)
238-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
handle.write(piece)to make the sequential-append intent explicit.This is correct today: for file handles,
writeFileis an alias ofappendFileand cannot change the mode set byopen(), and it is only unsafe to callfilehandle.writeFile()multiple times without awaiting each promise — which this loop does. Still,writeFilereads as "replace the file", sohandle.writedocuments the streaming append better.src/main/ai-vault/session-scanner-parse-cache.ts (1)
172-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value"Read-only" is convention only — the iterator yields live cache objects.
cache.entries()hands out the mutable in-memory entries (withresume) under a narrower type, so a future caller mutating a yielded entry would silently corrupt cache state.Readonly<PersistedSessionParseCacheEntry>in the signature would encode the intent at the type level.src/main/ai-vault/session-scanner-opencode-sqlite-worker-transport.ts (1)
57-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the standard
causeoption and setnameconsistently. Declaring acauseclass field shadows the built-inError.cause; passing it throughsuper(message, { cause })keepsinstanceof/serialization behaviour standard.OpenCodeSqliteWorkerTimeoutErroralso leavesnameas'Error'while the fault error sets it, which makes log triage inconsistent.♻️ Proposed adjustment
-export class OpenCodeSqliteWorkerTimeoutError extends Error {} -export class OpenCodeSqliteWorkerFaultError extends Error { - constructor( - message: string, - readonly cause?: Error - ) { - super(message) - this.name = 'OpenCodeSqliteWorkerFaultError' - } -} +export class OpenCodeSqliteWorkerTimeoutError extends Error { + name = 'OpenCodeSqliteWorkerTimeoutError' +} +export class OpenCodeSqliteWorkerFaultError extends Error { + constructor(message: string, cause?: Error) { + super(message, { cause }) + this.name = 'OpenCodeSqliteWorkerFaultError' + } +}src/main/ai-vault/session-scanner-opencode-sqlite-discovery.ts (1)
50-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winA successful list is discarded when a sibling source aborts the shared context. The post-await check only tests
isTerminated, so if the worker already returned a complete list and another source then tripped the shared context, this leg falls back to parse-cache candidates and reports the listing as cancelled — surfacing staler data than it holds.Preferring the fresh result when it is non-empty keeps the good leg's work:
♻️ Proposed adjustment
- if (!args.context.isTerminated) { + if (!args.context.isTerminated || candidates.length > 0) { return candidates }src/main/ai-vault/session-scanner-opencode-sqlite-worker-backoff.test.ts (1)
21-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
FakeOpenCodeSqliteWorkerhere too.SilentWorkerduplicates the shared fake worker’s request-recording, event-listener,terminate,unref, andpostMessagebehavior, so protocol changes can drift between scan tests.src/main/ai-vault/session-scanner-opencode-sqlite-worker-handle.ts (1)
45-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
isSpawned. It is declared but has no tracked consumers; keeping only the supported handle surface avoids unnecessary API bloat.src/main/ai-vault/fake-opencode-sqlite-worker.ts (1)
1-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep this fake in the test file or a test-scoped module.
fake-opencode-sqlite-worker.tsis only imported by the OpenCode SQLite worker client test, yet lives insrc/main/ai-vault/as a production source module, so production code can import it or builds can include it. Move it into the test or an existing fixture/helper module instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5362c8c1-aa30-4aaa-9102-122ea00a7fda
📒 Files selected for processing (26)
src/main/ai-vault/fake-opencode-sqlite-worker.tssrc/main/ai-vault/session-parse-cache-persistence.test.tssrc/main/ai-vault/session-parse-cache-persistence.tssrc/main/ai-vault/session-parse-cache-snapshot-serialization.tssrc/main/ai-vault/session-scanner-agent-parser.tssrc/main/ai-vault/session-scanner-candidate-parsing.tssrc/main/ai-vault/session-scanner-opencode-sources.tssrc/main/ai-vault/session-scanner-opencode-sqlite-cached-candidates.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-cached-candidates.tssrc/main/ai-vault/session-scanner-opencode-sqlite-candidate-phase.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-candidate-phase.tssrc/main/ai-vault/session-scanner-opencode-sqlite-coexistence.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-discovery.tssrc/main/ai-vault/session-scanner-opencode-sqlite-scan-context.tssrc/main/ai-vault/session-scanner-opencode-sqlite-scan-cooldown.tssrc/main/ai-vault/session-scanner-opencode-sqlite-scan-outcome.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-scan-outcome.tssrc/main/ai-vault/session-scanner-opencode-sqlite-worker-backoff.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-worker-client.test.tssrc/main/ai-vault/session-scanner-opencode-sqlite-worker-client.tssrc/main/ai-vault/session-scanner-opencode-sqlite-worker-handle.tssrc/main/ai-vault/session-scanner-opencode-sqlite-worker-transport.tssrc/main/ai-vault/session-scanner-parse-cache.tssrc/main/ai-vault/session-scanner.tssrc/shared/json-text-structure-limit.test.tssrc/shared/json-text-structure-limit.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/ai-vault/session-scanner-opencode-sources.ts
- src/main/ai-vault/session-scanner-opencode-sqlite-worker-client.test.ts
Greptile SummaryThis PR addresses two production scan findings: bounded OpenCode SQLite scan execution via a per-scan
Confidence Score: 5/5This PR is safe to merge. The deadline, circuit-breaker, and parse-cache bounding logic is well-isolated, the arm/pause/disarm reference counts are correctly balanced in all tested paths, and every failure mode degrades gracefully to a cold scan. The core deadline and cache-bounding machinery is thoroughly unit-tested across 37 files and 257 tests. No correctness defects were found in the main execution paths. Two minor issues exist: a same-named constant exported at two different byte sizes across two modules (a naming hazard for future callers, not a runtime bug today), and a behavioral difference where an in-memory cache that empties after a parse now preserves the prior on-disk snapshot rather than replacing it with an empty entries file — a narrow edge case that produces stale cache reads at worst. Files Needing Attention: session-parse-cache-snapshot-serialization.ts and session-parse-cache-persistence.ts both export SESSION_PARSE_CACHE_MAX_BYTES with different values (64 MiB vs 16 MiB); any future import of the serialization-module constant in a persistence context would silently use the wrong limit.
|
| Filename | Overview |
|---|---|
| src/main/ai-vault/session-scanner-opencode-sqlite-scan-context.ts | New file: reference-counted arm/pause/disarm deadline clock, AbortController signal, and per-scan circuit-breaker counters; logic is careful and well-tested. |
| src/main/ai-vault/session-parse-cache-snapshot-serialization.ts | New file: cooperative serialization with binary-search trim; exports SESSION_PARSE_CACHE_MAX_BYTES = 64 MiB, which differs from the persistence module's 16 MiB constant of the same name — latent naming hazard for future callers. |
| src/main/ai-vault/session-parse-cache-persistence.ts | Adds 16 MiB read cap, cooperative structural-token validation, and temp-file sync before rename; empty-entry snapshot path now preserves prior file instead of writing an empty array. |
| src/main/ai-vault/session-scanner-opencode-sqlite-worker-transport.ts | Adds per-scan context abort listener that drains only that scan's queued calls; arm/pause/settle interactions appear correctly balanced. |
| src/main/ai-vault/session-scanner-opencode-sqlite-candidate-phase.ts | New file: tracks worker-bound SQLite candidates per batch to arm/pause the budget clock only while actual SQLite work is in flight; disarms early when no SQLite candidates exist. |
| src/main/ai-vault/session-scanner-opencode-sqlite-scan-cooldown.ts | New file: simple exponential-backoff process-wide cooldown for hard SQLite failures; module-level state with test reset function. |
| src/main/ai-vault/session-scanner-opencode-sqlite-discovery.ts | Wraps listing in a budget-aware helper that falls back to parse-cache candidates when the scan context is already terminated; empty-list terminated-context path correctly marks list cancelled. |
| src/main/ai-vault/session-scanner-opencode-sqlite-scan-outcome.ts | New file: maps termination reason and list-cancelled state to user-facing issue strings; clears process-wide backoff only when a scan with a SQLite source completes without omitted work. |
| src/main/ai-vault/session-scanner-opencode-sqlite-cached-candidates.ts | New file: replays warm parse-cache rows for cooldown/terminated scans, deduplicating by sessionId with mtime and DB-path-rank tiebreaking. |
| src/main/ai-vault/session-scanner-candidate-parsing.ts | Extracted from session-scanner.ts; adds opencodeSqliteScanContext threading through parse loop and suppresses terminated-error results cleanly. |
| src/shared/json-text-structure-limit.ts | New shared utility: character-level JSON structural-token and nesting-depth counter; correctly ignores tokens inside string literals and handles escape sequences. |
Sequence Diagram
sequenceDiagram
participant Scanner as scanAiVaultSessions
participant Ctx as OpenCodeSqliteScanContext
participant Disc as discoverOpenCodeSessions
participant Budget as listWithinScanBudget
participant Worker as SQLiteWorkerTransport
participant Phase as OpenCodeSqliteCandidatePhase
participant Cache as ParseCache
participant Persist as session-parse-cache-persistence
Scanner->>Cache: ensureSessionParseCacheLoaded()
Cache->>Persist: readNodeFileWithinLimit (16 MiB)
Persist-->>Cache: buffer - assertStructureLimits - JSON.parse - seed
Scanner->>Ctx: new OpenCodeSqliteScanContext()
alt cooldown active
Scanner->>Ctx: enterCooldown() - abort signal
end
Scanner->>Disc: discoverAiVaultSessionSources(context)
Disc->>Budget: listWithinScanBudget(context)
alt context.isTerminated
Budget->>Cache: cachedOpenCodeSqliteCandidates()
Budget-->>Disc: cached candidates + markSqliteListCancelled
else normal path
Budget->>Ctx: armDeadline() - start 45s timer
Budget->>Worker: listOpenCodeSqliteSessionsViaWorker(context)
Worker-->>Budget: candidates
Budget->>Ctx: pauseDeadline() - bank remaining time
Budget-->>Disc: candidates
end
Scanner->>Phase: new OpenCodeSqliteCandidatePhase(context)
loop parse batches
Phase->>Phase: prepareBatch() - filter terminated SQLite, arm if worker-bound
Phase->>Worker: parse via transport
Worker-->>Phase: AiVaultSession or null
Phase->>Ctx: trackBatch() - pauseDeadline when last SQLite promise settles
alt deadline fires mid-batch
Ctx->>Ctx: abort signal - transport drains scan queue
end
end
Phase->>Ctx: disarmDeadline()
Scanner->>Ctx: recordOpenCodeSqliteScanOutcome()
alt clean scan with SQLite source
Ctx->>Cooldown: noteOpenCodeSqliteScanProgress()
else crash/timeout/unavailable
Ctx->>Cooldown: noteOpenCodeSqliteScanHardFailure()
end
Scanner->>Persist: scheduleSessionParseCachePersist()
Persist->>Persist: serializeSnapshotCooperatively (binary-search trim, 16 MiB)
Persist->>Persist: writeFile pieces - sync() - rename atomically
Reviews (2): Last reviewed commit: "fix(ai-vault): defer list-failure diagno..." | Re-trigger Greptile
Review follow-ups on the OpenCode SQLite scan budget. A per-call timeout was counted as a worker death, so a database merely slower than the 15s call timeout tripped the crash circuit and was reported to the user as "its background scanner kept crashing" — every scan, forever. Timeouts now have their own counter and termination reason, and their own message. Because those reads are synchronous node:sqlite, terminate() cannot preempt a thread parked in a query; spawning the replacement immediately stacked a second thread on the same core. Respawn now waits for the old thread to actually die, with a grace period so a never-returning query cannot wedge the transport. Crash loops, timeout loops, and failed spawns now arm a process-wide backoff. Deadline expiry deliberately does not: it still banks parses in the cache, so the next scan resumes further down the list, while the hard failures cannot be retried into success and this scan re-runs every cache TTL. Also: - one spawn failure no longer drains queued calls belonging to other concurrent scans off the shared transport - cache-served rows no longer arm the budget clock they never spend - the termination cause leads the scan issue instead of being masked by the cancelled-listing message; a source with no DB stays quiet - an over-capacity cache snapshot trims to its newest entries instead of abandoning the save and silently disabling #9210 persistence for good - parse stats count completed work, not work the deadline threw away - the scan context is required down the production parse path Splits the worker lifecycle and candidate parsing into their own modules to stay under max-lines.
Replay cached OpenCode SQLite sessions instead of blanking them when the scanner is paused by backoff. The SQLite listing is the only producer of synthetic candidates, so a paused scan used to lose every SQLite-backed session for the duration of the pause — even ones already parsed and cached. Also adds a separate worker unavailability circuit to back off after a few failed spawn attempts, distinct from crash/timeout loops. Only reports OpenCode pause to users with an actual install, and backs off hard when the budget expires without any worker answers. Optimizes JSON structure validation using charCodeAt for scanning large snapshots.
- Serialize cache snapshots cooperatively to avoid event-loop blocking on large snapshots (64 MB). - Fall back to cached OpenCode SQLite candidates when list operations timeout or fail, instead of losing all progress. - Distinguish parse responses (cacheable progress) from list responses (not cacheable) when deciding whether to backoff. - Deduplicate cached candidates by session ID across DB path aliases, using DB path order to break ties.
Replace hardcoded values with their corresponding constants (SESSION_PARSE_CACHE_JSON_LIMITS.nestingDepth, OPENCODE_SQLITE_SCAN_DEADLINE_MS) to reduce brittleness. Centralize console.debug spy teardown in afterEach hook to prevent mock-state pollution between tests.
Session parse cache now fsyncs before atomic rename for durability. Track parse cache hits to prevent unnecessary OpenCode scan backoffs when completed live work serves rows from the cache. Defer single list-timeout/fault diagnostics to outcome reporting instead of escalating to circuit breakers; sibling sources can now safely race without cascading failures.
7eaa15d to
3b5dda0
Compare
* fix(opencode): keep scan budgets across queue waits and batches Reuse the scan-owned lifetime proposed in #10708 by @AmethystLiang with the existing shared worker queue. * test(opencode): check nonempty session fixtures and lint scoped controls * Derive OpenCode scan deadline message from its budget --------- Co-authored-by: Neil Parker <nwparker@MacBook-Pro-3.localdomain> Co-authored-by: OpenCode issue campaign <codex@localhost>
Summary
This PR addresses both release-scan findings for p2-ai-vault-perf:
Release-scan reference: /private/tmp/prod-release-worker-reports/FINAL.md (findings #8924 and #9474).
Test plan and results
Design: docs/p2-ai-vault-perf-design.md
Round-five review: docs/p2-ai-vault-perf-code-review-round-5.md (SHIP; no P0/P1/P2 findings).