Repository navigation
fix: session capture v2 — PR review fixes - #825
Conversation
…atch fallback - Don't mark backfill 'complete' if stopped early — use 'paused' so it resumes on next daemon start instead of being skipped forever - Handle JSONL lines >64KB: scan forward to find next newline and skip the oversized line instead of getting stuck at same offset - Check startFilewatch() return value — on failure, fall back to 60s polling via setInterval so live sessions are still captured
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 341723be47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Fall back to polling ingest every 60s as degraded mode | ||
| const { ingestFileFull, discoverAllJsonlFiles, buildWorkerMap } = await import('./session-capture.js'); | ||
| deps.log({ timestamp: deps.now().toISOString(), level: 'warn', event: 'filewatch_failed_fallback_polling' }); | ||
| setInterval(async () => { |
There was a problem hiding this comment.
Track and clear fallback polling interval on stop
When filewatch fails, this creates a new polling setInterval but does not retain its handle, so stop() cannot clear it. In degraded mode, the daemon leaves an active timer behind after shutdown; the callback returns early when running is false, but the interval itself still keeps the Node event loop alive and can block clean process/test exit. Store this timer like heartbeatTimer/orphanTimer and clear it in stop().
Useful? React with 👍 / 👎.
| const skipBuf = Buffer.alloc(Math.min(bytesAvailable, chunkSize * 4)); | ||
| const { bytesRead: skipRead } = await fh.read(skipBuf, 0, skipBuf.length, effectiveOffset); | ||
| const skipStr = skipBuf.subarray(0, skipRead).toString('utf-8'); | ||
| const nlPos = skipStr.indexOf('\n'); | ||
| if (nlPos === -1) { |
There was a problem hiding this comment.
Scan oversized lines until real newline or EOF
This branch caps the forward scan to chunkSize * 4 bytes and treats nlPos === -1 as if the remainder of the file is one line. If a JSONL line exceeds that cap (for example, a very large tool payload) and a newline exists later, the function returns newOffset: fileSize and skips valid trailing records instead of advancing to the actual next newline. The skip scan needs to continue across the file until a newline is found or true EOF is reached.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces a polling fallback mechanism in the scheduler daemon for cases where file watching fails, updates the backfill process to handle paused states, and implements logic to skip oversized lines during file ingestion. Feedback focuses on the stability of the new polling logic in scheduler-daemon.ts, recommending the use of a recursive setTimeout instead of setInterval to avoid overlapping executions and resource leaks. Additionally, it is suggested to process files in chunks during the fallback polling to prevent high memory usage when encountering large session files.
| setInterval(async () => { | ||
| if (!running) return; | ||
| try { | ||
| const files = await discoverAllJsonlFiles(); | ||
| const workerMap = await buildWorkerMap(captureSql); | ||
| for (const f of files) { | ||
| await ingestFileFull(captureSql, f.sessionId, f.jsonlPath, f.projectPath, 0, { | ||
| parentSessionId: f.parentSessionId, | ||
| isSubagent: f.isSubagent, | ||
| workerMap, | ||
| }); | ||
| } | ||
| } catch { | ||
| /* best-effort fallback */ | ||
| } | ||
| }, config.heartbeatIntervalMs); |
There was a problem hiding this comment.
The use of setInterval here has a few issues that could impact the daemon's stability:
- Resource Leak: The interval timer is never cleared when the daemon stops because its ID is not stored. This will cause the polling to continue running in the background, leaking resources.
- Overlapping Executions: If the polling work takes longer than
config.heartbeatIntervalMs,setIntervalwill schedule another execution before the current one finishes. This can lead to overlapping runs and potential race conditions. - Silent Errors: The empty
catchblock hides any errors that occur during polling, making it difficult to debug issues with this fallback mechanism.
I recommend refactoring this to use a recursive setTimeout loop. This pattern ensures that one polling operation completes before the next one is scheduled. You'll also need to store the timer ID so it can be cleared in the stop() function, and add logging within the catch block.
| await ingestFileFull(captureSql, f.sessionId, f.jsonlPath, f.projectPath, 0, { | ||
| parentSessionId: f.parentSessionId, | ||
| isSubagent: f.isSubagent, | ||
| workerMap, | ||
| }); |
There was a problem hiding this comment.
The ingestFileFull function reads the entire remaining content of a file into memory. If a session file becomes very large, this could lead to excessive memory consumption and potentially crash the daemon process.
A more robust approach would be to process files in chunks, similar to the logic in session-backfill.ts. I recommend replacing the call to ingestFileFull with a loop that calls ingestFile with a defined chunk size, iterating until the file is fully processed. Per repository guidelines, it is acceptable to use a hardcoded numeric limit for the chunk size in this fallback logic to prevent performance issues.
References
- It is acceptable to use hardcoded numeric limits (magic numbers) in non-critical fallback logic, especially when they serve as intentional caps to prevent performance issues like excessive I/O.
Housekeeping pass after the bulk migration (PR #1130). Per brainstorm.md "Poured (shipped)" and "Killed" sections, 30 wishes are no longer active and have been moved under .genie/wishes/_archive/ to reduce clutter in the active backlog. Shipped (27): - agent-stability-hardening (PR #1112) - brain-foundation, brain-embeddings, brain-intelligence, brain-observability, brain-identity-impl, brain-init-skill (all 6 brain-obsidian children, @khal-os/brain v1.22.0) - env-defaults-local-mode - genie-layout-migration, genie-model-resolution, genie-onboarding-flow (onboarding overhaul) - omni-lifecycle-hardening (PR #359), omni-skill-upgrade, omni-version-unify - os-services-orphan-leak (PR #272 + #274) - pgserve-daemon-ownership (PR #827) - remove-openclaw - rlmx-v02, rlmx-v03-cag, rlmx-v04-gemini3 (npm v0.260331.5) - session-capture-v2 (PR #825) - v4-database-layer, v4-hook-cli-safety, v4-message-routing, v4-session-executor, v4-spawn-resilience, v4-team-lifecycle (v4 Stability Sprint) Killed (3): - brain-obsidian (parent spec — all 6 children shipped, reference only) - genie-app (superseded by genie-app-v1 / genie-studio) - session-ingester-perf (superseded by session-capture-v2) Also updates brainstorm.md links to point at wishes/_archive/<name>/WISH.md and fixes a duplicate v4 Stability Sprint entry. Active backlog drops from 47 → 20 wishes + 1 backup folder.
Housekeeping pass after the bulk migration (PR #1130). Per brainstorm.md "Poured (shipped)" and "Killed" sections, 30 wishes are no longer active and have been moved under .genie/wishes/_archive/ to reduce clutter in the active backlog. Shipped (27): - agent-stability-hardening (PR #1112) - brain-foundation, brain-embeddings, brain-intelligence, brain-observability, brain-identity-impl, brain-init-skill (all 6 brain-obsidian children, @khal-os/brain v1.22.0) - env-defaults-local-mode - genie-layout-migration, genie-model-resolution, genie-onboarding-flow (onboarding overhaul) - omni-lifecycle-hardening (PR #359), omni-skill-upgrade, omni-version-unify - os-services-orphan-leak (PR #272 + #274) - pgserve-daemon-ownership (PR #827) - remove-openclaw - rlmx-v02, rlmx-v03-cag, rlmx-v04-gemini3 (npm v0.260331.5) - session-capture-v2 (PR #825) - v4-database-layer, v4-hook-cli-safety, v4-message-routing, v4-session-executor, v4-spawn-resilience, v4-team-lifecycle (v4 Stability Sprint) Killed (3): - brain-obsidian (parent spec — all 6 children shipped, reference only) - genie-app (superseded by genie-app-v1 / genie-studio) - session-ingester-perf (superseded by session-capture-v2) Also updates brainstorm.md links to point at wishes/_archive/<name>/WISH.md and fixes a duplicate v4 Stability Sprint entry. Active backlog drops from 47 → 20 wishes + 1 backup folder.
Summary
Follow-up fixes for session capture v2 after Codex PR review:
completeon early stop (daemon shutdown); markpausedso it resumes on next start\nand skipsfs.watchfails (path missing, recursive unsupported), falls back to 60s polling so live sessions are still capturedTest plan
bun run typecheck— PASSbun run lint— PASS (warnings only, complexity)bun run dead-code— PASS