feat: SSE heartbeat and stale-client cleanup strategy (Closes #17) - #114
feat: SSE heartbeat and stale-client cleanup strategy (Closes #17)#114laurentketterle-hub wants to merge 4 commits into
Conversation
- Periodic heartbeat events every 30s - Safe broadcast with try/catch error handling - Stale client eviction after 90s inactivity - Client tracking with write-error thresholds - Integration hooks for server.js Closes Flamki#17 Signed-off-by: laurentketterle-hub <laurentketterle-hub@users.noreply.github.com>
Signed-off-by: laurentketterle-hub <laurentketterle-hub@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults 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)
📝 WalkthroughWalkthroughChangesThe PR adds bounded SSE client tracking, guarded event broadcasting, write-error eviction, stale-client cleanup, periodic heartbeats, lifecycle controls, logging, and connection statistics. Tests cover tracking, broadcasting, cleanup, and heartbeat control. SSE heartbeat lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SSEClient
participant setupHeartbeat
participant createClientTracker
participant safeBroadcast
participant clients
SSEClient->>createClientTracker: register connection
createClientTracker->>clients: store client metadata
setupHeartbeat->>safeBroadcast: broadcast heartbeat
safeBroadcast->>SSEClient: write serialized event
SSEClient-->>safeBroadcast: success or write error
safeBroadcast->>createClientTracker: update errors or remove client
setupHeartbeat->>createClientTracker: remove stale clients
setupHeartbeat-->>setupHeartbeat: expose statistics and stop control
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/sse-heartbeat.test.js (1)
102-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the observable heartbeat lifecycle.
This test does not let the interval run. It does not verify a heartbeat event,
clientCount, stale cleanup, or thatstopHeartbeat()prevents later writes.Add timer-controlled tests for one emitted heartbeat and no heartbeat after shutdown.
🤖 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 `@tests/sse-heartbeat.test.js` around lines 102 - 123, Expand the “starts and stops heartbeat” coverage around setupHeartbeat to use controlled timers, advance time until one heartbeat event is emitted, and assert the observable event and clientCount behavior. Then call stopHeartbeat(), advance the timers again, and verify no further heartbeat or client writes occur while retaining the existing getStats assertions.
🤖 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 `@src/sse-heartbeat.js`:
- Around line 3-141: Run the configured Prettier formatter and commit the
resulting formatting changes in src/sse-heartbeat.js (lines 3-141) and
tests/sse-heartbeat.test.js (lines 13-121); no behavioral changes are required.
- Around line 171-175: Update the heartbeat and stale-client handling around
safeBroadcast, clientTracker.getStaleClients, and the response lifecycle so
heartbeat writes do not refresh client staleness. Track activity separately
using only actual client-originated signals, and remove dead responses from
tracking on both close and error events.
- Around line 122-125: Update the write-error handling around
clientTracker.markError so any thrown res.write() immediately marks the response
for eviction by adding it to deadClients on the first failure. Remove or bypass
the markError-based delay unless writeErrors is explicitly retained for separate
metrics or a non-fatal policy.
- Around line 159-161: Update the SSE request-close cleanup associated with
client registration so closing a response also removes it from clientTracker,
either by exposing and invoking removeClient(res) from the server close handler
or by wrapping the existing cleanup. Add a test covering tracker removal when
the request closes, while preserving the existing sseClients cleanup behavior.
- Around line 154-156: Update src/sse-heartbeat.js lines 154-156 in the
sseClients.push override to honor clientTracker.addClient(res): reject and close
responses at the 100-client limit before sending the initial connection event,
and only append accepted responses. Add coverage in tests/sse-heartbeat.test.js
lines 22-29 by creating 100 clients, verifying client 101 is rejected, and
asserting the response array remains capped at 100 entries.
---
Nitpick comments:
In `@tests/sse-heartbeat.test.js`:
- Around line 102-123: Expand the “starts and stops heartbeat” coverage around
setupHeartbeat to use controlled timers, advance time until one heartbeat event
is emitted, and assert the observable event and clientCount behavior. Then call
stopHeartbeat(), advance the timers again, and verify no further heartbeat or
client writes occur while retaining the existing getStats assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4727c532-f547-4aa3-a341-fa94cec1b9e6
📒 Files selected for processing (2)
src/sse-heartbeat.jstests/sse-heartbeat.test.js
| const shouldRemove = clientTracker ? clientTracker.markError(res) : true | ||
| if (shouldRemove) { | ||
| deadClients.push(res) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Evict a response after the first thrown write.
At Line 122, the first two thrown writes do not add the response to deadClients. The test at tests/sse-heartbeat.test.js:93-97 therefore leaves badRes in clients.
A thrown res.write() indicates a broken connection. Remove that response immediately. Keep writeErrors only if it supports a separate metric or a non-fatal failure policy.
🤖 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 `@src/sse-heartbeat.js` around lines 122 - 125, Update the write-error handling
around clientTracker.markError so any thrown res.write() immediately marks the
response for eviction by adding it to deadClients on the first failure. Remove
or bypass the markError-based delay unless writeErrors is explicitly retained
for separate metrics or a non-fatal policy.
| sseClients.push = function (res) { | ||
| clientTracker.addClient(res) | ||
| return originalPush(res) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce and test the 100-client limit. addClient() can reject a response, but Line 155 ignores that result and Line 156 still appends it. The connection limit is therefore bypassed and excess responses are not tracked.
src/sse-heartbeat.js#L154-L156: do not append a rejected response. Reject and close the SSE request before sending its initial connection event.tests/sse-heartbeat.test.js#L22-L29: add 100 clients, assert that client 101 is rejected, and assert that the response array does not exceed 100 entries.
📍 Affects 2 files
src/sse-heartbeat.js#L154-L156(this comment)tests/sse-heartbeat.test.js#L22-L29
🤖 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 `@src/sse-heartbeat.js` around lines 154 - 156, Update src/sse-heartbeat.js
lines 154-156 in the sseClients.push override to honor
clientTracker.addClient(res): reject and close responses at the 100-client limit
before sending the initial connection event, and only append accepted responses.
Add coverage in tests/sse-heartbeat.test.js lines 22-29 by creating 100 clients,
verifying client 101 is rejected, and asserting the response array remains
capped at 100 entries.
| // Wrap the existing cleanup on close to also remove from tracker | ||
| // Note: server.js already has req.on('close', ...) but we also track | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove tracker entries when the request closes.
src/server.js:122-139 removes a closed response from sseClients only. Its Map entry remains in clientTracker. Later stale cleanup skips that entry because sseClients.indexOf(res) is -1.
Expose tracker removal during registration, or update the server close handler to call removeClient(res). Also add this lifecycle case to the tests.
🤖 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 `@src/sse-heartbeat.js` around lines 159 - 161, Update the SSE request-close
cleanup associated with client registration so closing a response also removes
it from clientTracker, either by exposing and invoking removeClient(res) from
the server close handler or by wrapping the existing cleanup. Add a test
covering tracker removal when the request closes, while preserving the existing
sseClients cleanup behavior.
| safeBroadcast(sseClients, clientTracker, heartbeatEvent) | ||
|
|
||
| // 2. Evict stale clients | ||
| const staleClients = clientTracker.getStaleClients(staleTimeoutMs) | ||
| for (const { res, entry } of staleClients) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not use heartbeat writes as stale-client activity.
The heartbeat at Line 171 updates lastWrite for every successful response before Line 174 checks staleness. A response that accepts writes can never become stale under this rule.
Track a separate lifecycle signal. For dead peers, remove responses on close and error. If client activity is required, update a separate timestamp only from an actual client-originated signal.
🤖 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 `@src/sse-heartbeat.js` around lines 171 - 175, Update the heartbeat and
stale-client handling around safeBroadcast, clientTracker.getStaleClients, and
the response lifecycle so heartbeat writes do not refresh client staleness.
Track activity separately using only actual client-originated signals, and
remove dead responses from tracking on both close and error events.
Signed-off-by: laurentketterle-hub <laurentketterle-hub@users.noreply.github.com>
Signed-off-by: laurentketterle-hub <laurentketterle-hub@users.noreply.github.com>
1 participant
Summary
Adds SSE heartbeat and stale-client cleanup strategy per #17.
Changes
New:
src/sse-heartbeat.jsgetStats()for monitoringNew:
tests/sse-heartbeat.test.jsIntegration
In
server.js, wrap the existing SSE setup:Acceptance Criteria
Closes #17
Signed-off-by: laurentketterle-hub laurentketterle-hub@users.noreply.github.com
Summary by CodeRabbit
New Features
Tests