Desktop: broker owns the /events SSE live mirror (Phase 3 complete) - #74
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds ChangesSSE live-mirror endpoint
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a65cc87b97
ℹ️ 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".
| }) | ||
| // One held read-only connection so data_version is comparable across polls (it's per-connection). | ||
| let db = null | ||
| try { db = new DatabaseSync(path.join(sopDir, 'state.db'), { readOnly: true }); db.exec('PRAGMA busy_timeout = 2000') } catch (_) { db = null } |
There was a problem hiding this comment.
When /events is opened on a fresh SOP dir before state.db exists, this read-only open falls into the catch and leaves db null for the lifetime of the stream. Later forwarded writes/imports can create and populate state.db, but dataVersion() will keep returning 0 and the DB contents are not part of signals(), so the live mirror never emits the new plate/run data until the browser reconnects. Retry/open the reader when the DB appears, or create/migrate it the way the FastAPI stream did.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the held data_version connection now opens LAZILY (ensureDb) and re-opens on error, so a state.db that does not exist at connect (fresh SOP dir) or is recreated mid-stream gets picked up -- data_version then observes the new commits and re-emits, instead of pinning to 0 / a stale handle. (snapshot()/signals() already re-read fresh each poll, so they pick up the appeared db; this closes the data_version side.) Added a snapshot-resilient-to-missing-db test.
| let lastSig = signals(sopDir) | ||
| for (const frame of snapshot(sopDir)) res.write(frame) // initial snapshot |
There was a problem hiding this comment.
If state.db exists but the schema is not ready or readable yet (for example during first creation/migration, or an older/partial DB), signals() or snapshot() can throw no such table before any timer-level catch runs. Because this happens inside the HTTP handler without the served-read try/catch, one /events request can terminate the broker process instead of failing or retrying the stream.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in the hardening push that landed alongside this review: the initial snapshot/signals read is now inside a try/catch in createEventStream (with the close/error cleanup attached BEFORE the first read, and the broker call defensively wrapped), so a "no such table" during first-creation/migration tears the stream down cleanly instead of crashing the broker.
|
Adversarial self-review hardening (folded into the latest push):
|
aa42c97 to
78805b0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@desktop/sse.js`:
- Line 70: The snapshot function calls at both locations (the for loop iterating
over snapshot(sopDir) and the other snapshot call around lines 77-80) lack error
handling, which means any thrown exceptions during store or liveness reads will
propagate up and crash the broker process. Wrap both snapshot(sopDir) calls in
try-catch blocks to gracefully handle any errors that occur during snapshot
generation, ensuring exceptions are logged but do not terminate the process or
handler execution.
- Around line 55-66: The catch block on line 62 silently swallows all exceptions
when creating the DatabaseSync instance by setting db to null, which causes
dataVersion() to always return 0 and disables change-triggered refreshes.
Instead of using catch (_) { db = null }, fail fast by re-throwing the error or
responding with an appropriate error status code to the client. This ensures
that database connection failures are properly surfaced rather than silently
degrading the SSE stream's functionality.
🪄 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
Run ID: ca7b7385-8ae2-4ed6-b655-01da216a8c62
📒 Files selected for processing (4)
desktop/broker.jsdesktop/package.jsondesktop/sse.jsdesktop/sse.test.js
The broker now serves the /events live-mirror stream itself, instead of forwarding it -- the last read/mirror piece. Mirrors dashboard_app.event_stream: a snapshot on connect (plate, inflight, pending, queue, runs -- each a BARE array, the SSE shape, using the Node store + liveness), a fresh snapshot whenever SQLite's data_version moves OR a file/ liveness signal changes, and a 10s heartbeat. Holds one read-only connection so data_version is comparable across polls; tears down on client disconnect. Verified against the live FastAPI: the initial snapshot frames (all five events) match byte-for-byte, and adding a task re-emits an updated plate frame (change detection works). - desktop/sse.js (+ tests): snapshot(), signals() (run/session liveness + pending/queue mtimes -- the changes a DB write wouldn't move), createEventStream(). - desktop/broker.js: GET /events is token-gated then streamed (before the JSON SERVED map). With this the broker owns the WHOLE live mirror (every read + the stream). Still forwarded: settings (env-detected terminal) and the action/write endpoints (Phase 4, next). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
78805b0 to
65e4748
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
desktop/sse.js (1)
101-104:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard periodic snapshot emission in the timer callback.
Line 103 still calls
snapshot(sopDir)without a local guard insidesetInterval. If store/liveness read throws there, the outertryat Lines 90-110 will not catch it (async callback), which can surface as an uncaught exception.Suggested fix
if (dv !== lastDv || sig !== lastSig) { lastDv = dv; lastSig = sig - for (const frame of snapshot(sopDir)) res.write(frame) // all frames on any change + try { + for (const frame of snapshot(sopDir)) res.write(frame) // all frames on any change + } catch (_) { + cleanup() + try { res.end() } catch (_) { /* already closed */ } + return + } }🤖 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 `@desktop/sse.js` around lines 101 - 104, The snapshot(sopDir) call on line 103 inside the setInterval callback is not guarded by a try-catch block, which means if it throws an error, it will not be caught by the outer try block at Lines 90-110 and will become an uncaught exception. Wrap the snapshot(sopDir) call and the subsequent res.write(frame) loop in a try-catch block within the setInterval callback to handle any errors that may occur during periodic snapshot emission, ensuring errors are properly caught and handled without crashing the process.
🤖 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.
Duplicate comments:
In `@desktop/sse.js`:
- Around line 101-104: The snapshot(sopDir) call on line 103 inside the
setInterval callback is not guarded by a try-catch block, which means if it
throws an error, it will not be caught by the outer try block at Lines 90-110
and will become an uncaught exception. Wrap the snapshot(sopDir) call and the
subsequent res.write(frame) loop in a try-catch block within the setInterval
callback to handle any errors that may occur during periodic snapshot emission,
ensuring errors are properly caught and handled without crashing the process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7390e8ad-17e8-45ee-964c-7d20d423c73f
📒 Files selected for processing (4)
desktop/broker.jsdesktop/package.jsondesktop/sse.jsdesktop/sse.test.js
✅ Files skipped from review due to trivial changes (1)
- desktop/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- desktop/broker.js
- desktop/sse.test.js
1 participant
The broker now serves the
/eventslive-mirror stream itself instead of forwarding it — the last read/mirror piece, so Phase 3 is complete: the broker owns the entire live mirror.What it does
Mirrors
dashboard_app.event_stream:plate,inflight,pending,queue,runs, each a bare array (the SSE frame shape, distinct from the GET endpoints'{key: ...}wrapper), built from the Node store + liveness;data_versionmoves or a file/liveness signal changes (run liveness, session liveness,pending/+queue/mtimes — the changes a DB write wouldn't move);Holds one read-only connection so
data_versionis comparable across polls; tears down the connection + timer on client disconnect.Verification
Against the live FastAPI: the initial snapshot frames (all five events) match, and adding a task re-emits an updated
plateframe (change detection confirmed). 33node --test.Remaining
settings(env-detected terminal) stays forwarded — permanent.🤖 Generated with Claude Code
Summary by CodeRabbit
401on missing/invalid tokens) and safer handling of streaming failures.sse.js.