Optimize broadcast handling and debounce WS session reloads#222
Conversation
…t under high concurrency When 22 Claude processes fire hooks simultaneously, the processEvent transaction held the SQLite write lock while executing 27 synchronous broadcast() calls + 37 DB reads to prepare broadcast data, blocking the Node.js event loop and degrading sessions API from 30ms to 6+ min. Changes: - Add server/lib/broadcast-queue.js: deferred broadcast queue with enqueue/flush/dedup — same-type same-ID broadcasts auto-merge - Replace all broadcast() inside processEvent with enqueue(), removing ~27 redundant DB reads for broadcast data, drastically reducing transaction lock hold time - setImmediate(flush) after transaction commits — async broadcast, non-blocking hook response - Raise busy_timeout from 5s to 60s for better write-concurrency tolerance Result: sessions API from 6+ min to 30-350ms Co-Authored-By: Claude <noreply@anthropic.com>
Without debouncing, every session_updated/new_event WS message triggered an immediate load() call. With 22+ concurrent Claude processes issuing hooks, the server received dozens of redundant API requests per second, saturating the event loop and making the sessions page unresponsive. Add 800ms debounce — bursty WS messages collapse into a single load() call, matching the pattern already used by SessionOverview.
|
I have read the CLA Document and I hereby sign the CLA txj seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
There was a problem hiding this comment.
Code Review
This pull request introduces a deferred broadcast queue to decouple WebSocket notifications from SQLite transactions, reducing database lock hold times and deduplicating redundant broadcasts. It also adds a debounced reload mechanism on the client side to handle rapid bursts of events. The review feedback highlights several critical areas for improvement in the queue implementation: preserving the chronological order of events during deduplication, wrapping asynchronous database reads in try-catch blocks to prevent server crashes, adding defensive checks for null or undefined references, and clearing the queue if a transaction rolls back to prevent stale event broadcasts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Deduplicate: for DB-backed types, keep only the last entry per (type, id). | ||
| // Inline-data types ("new_event") are never deduped. | ||
| const seen = new Map(); | ||
| const deduped = []; | ||
|
|
||
| for (let i = 0; i < items.length; i++) { | ||
| const item = items[i]; | ||
| const key = dedupeKey(item); | ||
| if (key) { | ||
| seen.set(key, i); | ||
| } else { | ||
| // Inline data — always keep | ||
| deduped.push(item); | ||
| } | ||
| } | ||
|
|
||
| // Append the last occurrence of each dedup-able key | ||
| for (const idx of seen.values()) { | ||
| deduped.push(items[idx]); | ||
| } |
There was a problem hiding this comment.
The current deduplication logic pushes all DB-backed events to the end of the queue, which alters the original chronological order of events. For example, a session_created event could be sent after a subsequent new_event (like SessionStart), which can break client-side state assumptions or cause UI glitches.
We can preserve the exact chronological order of all events by first finding the last index of each dedup-able key, and then filtering the original array in-place.
// Deduplicate: for DB-backed types, keep only the last entry per (type, id)
// while preserving the original chronological order of all events.
const seen = new Map();
for (let i = 0; i < items.length; i++) {
const key = dedupeKey(items[i]);
if (key) {
seen.set(key, i);
}
}
const deduped = [];
for (let i = 0; i < items.length; i++) {
const item = items[i];
const key = dedupeKey(item);
if (key) {
if (seen.get(key) === i) {
deduped.push(item);
}
} else {
// Inline data — always keep
deduped.push(item);
}
}There was a problem hiding this comment.
Fixed in this PR. The deduplication logic now uses a two-pass approach: first pass records the last index for each dedup key in a Map (overwriting keeps the last occurrence), then second pass filters items to only those indices (plus keyless inline items), preserving the original insertion order. DB-backed events are no longer appended to the end — they remain at their original position in the queue.
There was a problem hiding this comment.
Fixed. The deduplication logic now uses a two-pass approach: first pass records the last index per dedup key in a seen Map (overwriting keeps the last occurrence), second pass filters items to only those kept indices (plus keyless inline items), preserving the original insertion order. DB-backed events remain at their original position in the queue — they are no longer appended to the end. (Verified: 631 server tests pass.)
| // Send broadcasts (DB reads happen here, outside the transaction) | ||
| for (const item of deduped) { | ||
| const data = loadData(item); | ||
| if (data !== undefined && data !== null) { | ||
| broadcast(item.type, data); | ||
| } | ||
| } |
There was a problem hiding this comment.
Since flush() is executed asynchronously outside the main request-response cycle (via setImmediate), any unhandled exception thrown during loadData(item) (e.g., due to database lock or query failure) will result in an uncaught exception that crashes the entire Node.js process.
Wrapping the broadcast loop in a try-catch block ensures that a single failed broadcast or database read does not bring down the server.
// Send broadcasts (DB reads happen here, outside the transaction)
for (const item of deduped) {
try {
const data = loadData(item);
if (data !== undefined && data !== null) {
broadcast(item.type, data);
}
} catch (err) {
console.error(`[BROADCAST-QUEUE] Failed to broadcast ${item.type}:`, err);
}
}There was a problem hiding this comment.
Fixed. The broadcast loop in flush() now wraps each item dispatch in an individual try-catch. If loadData() or broadcast() throws for one item, the error is logged via console.error and the loop continues processing remaining items. No unhandled exception can escape to crash the process.
There was a problem hiding this comment.
Fixed. The broadcast loop in flush() now wraps each item dispatch in an individual try-catch. If loadData() or broadcast() throws for one item, the error is logged via console.error and the loop continues processing remaining items. No unhandled exception can escape to crash the process.
| function dedupeKey(item) { | ||
| const { type, ref } = item; | ||
| if (ref.data !== undefined) return null; // inline payload, don't dedup | ||
| if (ref.sessionId) return `${type}:session:${ref.sessionId}`; | ||
| if (ref.agentId) return `${type}:agent:${ref.agentId}`; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
If ref is null or undefined, accessing ref.data will throw a TypeError and crash the execution. Adding a defensive guard at the beginning of dedupeKey prevents potential crashes.
| function dedupeKey(item) { | |
| const { type, ref } = item; | |
| if (ref.data !== undefined) return null; // inline payload, don't dedup | |
| if (ref.sessionId) return `${type}:session:${ref.sessionId}`; | |
| if (ref.agentId) return `${type}:agent:${ref.agentId}`; | |
| return null; | |
| } | |
| function dedupeKey(item) { | |
| const { type, ref } = item; | |
| if (!ref) return null; | |
| if (ref.data !== undefined) return null; // inline payload, don't dedup | |
| if (ref.sessionId) return `${type}:session:${ref.sessionId}`; | |
| if (ref.agentId) return `${type}:agent:${ref.agentId}`; | |
| return null; | |
| } |
There was a problem hiding this comment.
Fixed. Added a defensive guard at the top of dedupeKey(): if (!ref) return null; — a null/undefined ref is treated as non-dedup-able (same as inline data), preventing TypeError on ref.data access.
There was a problem hiding this comment.
Fixed. Added a defensive guard at the top of dedupeKey(): if (!ref) return null; — a null/undefined ref is treated as non-dedup-able (same as inline data), preventing TypeError on ref.data access.
| function loadData(item) { | ||
| const { type, ref } = item; | ||
|
|
||
| // Inline payload — no DB read needed | ||
| if (ref.data !== undefined) return ref.data; |
There was a problem hiding this comment.
Similarly, if ref is null or undefined, accessing ref.data in loadData will throw a TypeError. Adding a defensive guard here ensures robust execution.
| function loadData(item) { | |
| const { type, ref } = item; | |
| // Inline payload — no DB read needed | |
| if (ref.data !== undefined) return ref.data; | |
| function loadData(item) { | |
| const { type, ref } = item; | |
| if (!ref) return null; | |
| // Inline payload — no DB read needed | |
| if (ref.data !== undefined) return ref.data; |
There was a problem hiding this comment.
Fixed. Added a defensive guard at the top of loadData(): if (!ref) return null; — a null/undefined ref returns null immediately, preventing TypeError on ref.data access.
There was a problem hiding this comment.
Fixed. Added a defensive guard at the top of loadData(): if (!ref) return null; — a null/undefined ref returns null immediately, preventing TypeError on ref.data access.
| // Flush deferred broadcasts outside the transaction — reduces lock hold time | ||
| // and deduplicates redundant session/agent updates across a single hook event. | ||
| setImmediate(() => broadcastQueue.flush()); |
There was a problem hiding this comment.
If processEvent throws an error and the transaction rolls back, any items already enqueued in broadcastQueue during the transaction will remain in the queue because flush() is never called. These stale items will then be incorrectly flushed during the next successful event.
Consider clearing the queue if an error occurs (e.g., by wrapping processEvent in a try-catch block in the route handler to call broadcastQueue.clear(), or clearing the queue at the start of processEvent).
There was a problem hiding this comment.
Fixed. The /event route now wraps processEvent in a try-catch. If the transaction throws (rollback), broadcastQueue.clear() is called to purge any items that were enqueued inside the failed transaction, preventing stale broadcasts from being sent on the next flush cycle. The error is then re-thrown so the existing Express error handling still applies.
There was a problem hiding this comment.
Fixed. The /event route now wraps processEvent in a try-catch. If the transaction throws (rollback), broadcastQueue.clear() is called to purge any items that were enqueued inside the failed transaction, preventing stale broadcasts from being sent on the next flush cycle. The error is then re-thrown so the existing Express error handling still applies.
In the Conversation tab's agent tree, Main Agent now always appears at the top, and subagents are sorted by started_at descending so the most recent ones are shown first — making it easy to track what's happening right now. Children within each parent node follow the same newest-first order.
…s newest-first The /api/sessions/:id/transcripts endpoint sorted subagents by started_at ascending (oldest first). Flip to descending so the most recent subagents appear at the top of the dropdown, matching the agent tree sort order in the Dashboard tab.
Session detail API returned ALL events (up to 100k rows, 41MB) even though the frontend never used them — it loads events via the paginated /api/events endpoint. This single change cuts session-detail response from 28s/41MB to 0.09s/41KB. Also cache the liveness probe (ps + lsof) result for 30s. These synchronous commands run every watchdog tick (15s) and with 22+ claude processes they block the event loop for seconds, starving HTTP requests. - Remove events from GET /api/sessions/:id response - CLI (ccam session <id>) fetches recent events from /api/events instead - Cache probeLiveCwds() for 30s, export _clearCache for tests - Update tests to match new response shape
The probe cache introduced in the previous commit persists across test cases, causing the container-disabling test to return stale cached results. Clear the cache in beforeEach.
theraihanrakibb
left a comment
There was a problem hiding this comment.
Good, well-motivated optimization — the "22+ concurrent Claude processes saturating the event loop" rationale is clear, and the broadcast-queue.js deferral + dedup plus the 800 ms Sessions.tsx debounce are the right shape. Tests were updated too (_clearProbeCache for isolation). A few notes:
1. flush() reorders inline events before DB-backed updates (worth a look): in flush(), the first loop pushes every inline new_event item (in source order) and only records indices for DB-backed types; the second loop appends DB-backed items after. So within one flush a new_event is broadcast before its session_updated/agent_updated, whereas the original code emitted in source order. Clients may render an event for a session before receiving the session's updated state. Consider preserving source order while still deduping (keep the last index but emit in original sequence), or explicitly flush inline events after the DB-backed ones.
2. run_status not debounced: Sessions.tsx debounces session_updated and new_event(Stop/SessionEnd) but calls load() immediately for run_status. Route it through the same timer for consistency.
3. Dedup drops distinct create/update signals (edge): for DB-backed keys only the last per (type, id) survives, so a session_created + later session_updated for the same session in one batch collapses to just the update. Confirm the client doesn't rely on the create event specifically.
4. busy_timeout 5s to 60s: reasonable band-aid for contention, but it masks lock contention rather than fixing it — a one-line comment noting it's intentional would help.
(Verified /api/events exists, so the ccam.js change fetching events separately is correctly backed.)
- Rewrite dedup logic to use two-pass Map filter: first pass records last index per key, second pass filters in original order (fixes HIGH-priority review comment about DB-backed events being pushed to end of queue, breaking chronological order) - Wrap flush() broadcast loop in try-catch so a single failed DB read or broadcast does not crash the process - Add null-ref defensive guards in dedupeKey() and loadData() Addresses review comments hoangsonww#1-hoangsonww#4 on PR hoangsonww#222. Co-Authored-By: Claude <noreply@anthropic.com>
When processEvent throws and the SQLite transaction rolls back, any items enqueued during the transaction would remain in the broadcast queue and be incorrectly flushed on the next successful event. Wrap processEvent in try-catch and call broadcastQueue.clear() on failure. Addresses review comment hoangsonww#5 on PR hoangsonww#222. Co-Authored-By: Claude <noreply@anthropic.com>
The busy_timeout increase from 5s to 60s is a temporary mitigation for lock contention under heavy concurrent writes. Add a comment explaining this is a band-aid and the root cause should be addressed separately. Addresses theraihanrakibb's review suggestion on PR hoangsonww#222. Co-Authored-By: Claude <noreply@anthropic.com>
Route run_status through the same debounce timer as session_updated and new_event instead of immediately calling loadDashboardRuns(). This prevents rapid bursts of run_status messages from overwhelming the server under heavy concurrent Claude session activity. Addresses theraihanrakibb's review suggestion on PR hoangsonww#222. Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Changes
Type of Change
How to Test
Checklist
🖋️ CLA Assistantbot will prompt me on my first PR)npm test)npm run format:check)Screenshots