feat: add opt-in bandwidth monitoring behind BANDWIDTH_MONITOR_ENABLED - #1094
feat: add opt-in bandwidth monitoring behind BANDWIDTH_MONITOR_ENABLED#1094centerionware wants to merge 4 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 (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change adds an opt-in bandwidth monitor for HTTP responses and WebSocket traffic. It records byte totals by route and activity type, logs periodic summaries, adds shell connection diagnostics, and logs unbounded message-history requests when enabled. ChangesBandwidth monitoring
Sequence Diagram(s)sequenceDiagram
participant WebSocketServer
participant BandwidthMonitor
participant ShellWebSocket
WebSocketServer->>BandwidthMonitor: initialize tracking and logging
WebSocketServer->>BandwidthMonitor: tag WebSocket route
ShellWebSocket->>BandwidthMonitor: record replay, output, input, and resize bytes
BandwidthMonitor->>BandwidthMonitor: aggregate route totals
BandwidthMonitor->>BandwidthMonitor: log periodic and final totals
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Temporary logging to measure actual bytes sent per shell WebSocket connection (live output, buffer replay, resize, input), broken down by message type, logged every 5s and on close. For investigating unexpectedly high mobile data usage on the shell/terminal screen.
Shell-WS-only instrumentation showed ~1KB/s, far too low to explain the reported usage, so this widens measurement to everything the server sends: every WebSocket channel (shell, chat, desktop notifications, plugin proxy) via a WebSocket.prototype.send patch, and every HTTP response (API, static assets, SPA shell) via an early response-wrapping middleware. Logs a bucketed summary every 10s as [DIAG net].
The tab-visibility fix on main didn't stop the growth in live testing even while parked on the Shell tab, and static analysis found no other in-repo caller of the unbounded path. Logging request headers (user-agent, referer, x-forwarded-for, remote addr) on every limit=null hit to fingerprint whatever is actually calling it, and exposing a build marker on /health to rule out a stale image serving pre-fix code.
Turns the ad-hoc instrumentation used to diagnose siteboon#1087's bandwidth bug into a permanent, disabled-by-default feature: bucketed HTTP + WebSocket byte counters logged on an interval, per-connection shell WS bandwidth breakdown, and fingerprinting of any unbounded messages-endpoint hit. All of it is gated behind BANDWIDTH_MONITOR_ENABLED=true so it costs nothing when unset.
4944434 to
c3fc217
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: 5
🤖 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 `@server/index.ts`:
- Around line 153-162: Update the response listener around res.on('finish') to
use a shared one-time guard registered on both finish and close, ensuring
recordBandwidth runs exactly once for completed or early-closed responses,
including after data has been handed to the OS.
In `@server/modules/providers/provider.routes.ts`:
- Line 20: Export isBandwidthMonitorEnabled from the WebSocket module barrel in
server/modules/websocket/index.ts, then update the provider routes import to
reference the barrel instead of the bandwidth-monitor service implementation.
Preserve the existing symbol usage and behavior.
- Around line 674-682: Remove sensitive metadata from the bandwidth-monitor
diagnostic in server/modules/providers/provider.routes.ts lines 674-682: omit
raw IP and forwarded-address fields, redact referer query strings, and use only
a privacy-reviewed hashed identifier if correlation is necessary. In
server/modules/websocket/services/shell-websocket.service.ts lines 317-333,
replace ptySessionKey with a generated connection identifier and exclude the
project path, session ID, and command-derived suffix from logs.
In `@server/modules/websocket/services/shell-websocket.service.ts`:
- Around line 401-407: Update the shell websocket send flow around the existing
replay send and all other outbound frames, including reconnect, error, welcome,
exit, and auth_url messages. Introduce one helper that records each frame’s
category and byte size before invoking ws.send, then route every shell outbound
send through it so TOTAL SENT includes all server-to-client traffic while
preserving existing payloads.
In `@src/stores/useSessionStore.ts`:
- Around line 674-684: Update refreshFromServer to record when a refresh request
arrives while the session already has an in-flight promise, using a per-session
dirty flag or generation counter. After performRefreshFromServer completes,
issue one bounded follow-up refresh when a newer request was recorded, keep the
inFlightRefreshRef entry until the follow-up finishes, and resolve all callers
only after the latest refresh completes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea8a1711-873b-4202-be24-ccbc910672ac
📒 Files selected for processing (12)
.env.exampleserver/index.tsserver/modules/providers/provider.routes.tsserver/modules/websocket/services/bandwidth-monitor.service.tsserver/modules/websocket/services/shell-websocket.service.tsserver/modules/websocket/services/websocket-server.service.tssrc/components/chat/hooks/useChatRealtimeHandlers.tssrc/components/chat/hooks/useChatSessionState.tssrc/components/chat/types/types.tssrc/components/chat/view/ChatInterface.tsxsrc/components/main-content/view/MainContent.tsxsrc/stores/useSessionStore.ts
| res.on('finish', () => { | ||
| const ext = path.extname(req.path); | ||
| const segments = req.path.split('/').filter(Boolean); | ||
| const bucketKey = ext | ||
| ? `http:static:${ext}` | ||
| : segments[0] === 'api' | ||
| ? `http:api:${segments.slice(0, 2).join('/')}` | ||
| : `http:page:${segments[0] || '/'}`; | ||
| recordBandwidth(bucketKey, bytesSent); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)server/index\.ts$|package\.json$|package-lock\.json$|pnpm-lock\.yaml$|yarn\.lock$' || true
echo "== server/index outline =="
if [ -f server/index.ts ]; then
wc -l server/index.ts
ast-grep outline server/index.ts --match 'recordBandwidth' --view plain || true
echo "== relevant lines =="
sed -n '1,240p' server/index.ts | cat -n
fi
echo "== usages of recordBandwidth/bytesSent =="
rg -n "recordBandwidth|bytesSent|finish|close" server/index.ts || trueRepository: siteboon/claudecodeui
Length of output: 11345
🌐 Web query:
Node.js http.ServerResponse events close finish premature termination client aborted documentation
💡 Result:
In Node.js, the http.ServerResponse object behaves as a Writable stream. To identify a premature termination (such as a client aborting the request), you should monitor the close event and verify that the response did not finish normally [1][2]. Event definitions: - finish: Emitted when the last segment of the response headers and body have been handed off to the operating system for transmission [3][4]. It does not guarantee that the client received the data [3][5]. - close: Emitted when the response is completed or when the underlying connection is terminated prematurely (before the response is fully sent) [3]. Detecting Premature Termination: Because the close event is emitted regardless of whether the response finished successfully or was terminated, you must check the status of the response to distinguish between these cases [6][7]. The standard way to detect a premature termination is to check if the response was not properly finalized [1][2]: const server = http.createServer((req, res) => { res.on('close', => { if (!res.writableEnded) { console.log('Response was aborted by the client'); } else { console.log('Response closed normally'); } }); }); Alternatively, you can check res.writableFinished, which will be true only if the response finished successfully [3][4]. Note on Modern Practices: - The 'aborted' event on requests and responses has been largely deprecated or discouraged in favor of checking the 'close' event alongside the stream's state [7][2]. - For cancelling asynchronous work (like database queries) when a client disconnects, you can now use the req.signal property, which provides an AbortSignal that is aborted when the underlying socket closes or the request is destroyed [8]. - Be aware that in recent Node.js versions, there has been an ongoing effort to align stream behaviors. Generally, a "clean abort" (where the connection closes without an explicit error) will emit 'close' without emitting an 'error' event [1][2]. Always prioritize checking!res.writableEnded or!res.writableFinished within the 'close' event listener to reliably detect interruptions [2].
Citations:
- 1: http: emit error on aborted ServerResponse nodejs/node#63448
- 2: Fix HTTP/2 RST_STREAM behaviour, add auto-drain, deprecate 'aborted', fix related compat API issues nodejs/node#63249
- 3: https://nodejs.org/api/http.html
- 4: https://beta.docs.nodejs.org/http.html
- 5: https://node.readthedocs.io/en/latest/api/http/
- 6: http: always emit close on req and res nodejs/node#20611
- 7: https://stackoverflow.com/questions/60041727/how-to-detect-aborted-ajax-request-in-nodejs-express-js
- 8: nodejs/node@bf1aebc
🌐 Web query:
Node.js http.ServerResponse close finish both emitted response finished normally documentation
💡 Result:
In Node.js, the 'finish' and 'close' events on an http.ServerResponse serve distinct purposes regarding the lifecycle of an HTTP response [1][2]. 'finish' event The 'finish' event is emitted when the response has been sent [1][3]. Specifically, this occurs when the last segment of the response headers and body has been handed off to the operating system for transmission over the network [1][4]. It does not guarantee that the client has received the data; it only signifies that Node.js has completed its local processing and handed the data to the system [1][2]. After this event, no further events are emitted on the response object [3][5]. 'close' event The 'close' event indicates that the response has completed, or that the underlying connection was terminated prematurely before the response was finished [1][6]. In modern Node.js versions, this event is emitted at the end of every response, regardless of whether it finished successfully or was interrupted [6][2]. Comparison While both events are relevant to the end of a request/response cycle, their primary distinction lies in what they signify [2]: - 'finish' tracks the successful hand-off of the outgoing data to the OS [1][2]. - 'close' tracks the final state of the response object or its underlying connection [6][2]. When a response completes normally, both events are typically emitted [6]. Comparing these two events can be useful for diagnostics; for example, if a 'close' event occurs without a corresponding 'finish', it may indicate that the connection was terminated prematurely [2].
Citations:
- 1: https://nodejs.org/api/http.html
- 2: https://www.thenodebook.com/http/http-server-lifecycle
- 3: https://node.readthedocs.io/en/stable/api/http/
- 4: https://beta.docs.nodejs.org/http.html
- 5: https://node.readthedocs.io/en/latest/api/http/
- 6: doc: http.ServerResponse close event nodejs/node#30489
Record bandwidth for responses that close early.
res.on('finish') misses responses after some chunks are written if the connection closes before the response finishes. Use a one-time guarded listener on both finish and close so recordBandwidth still runs, including when Node has already handed data off to the OS.
🤖 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 `@server/index.ts` around lines 153 - 162, Update the response listener around
res.on('finish') to use a shared one-time guard registered on both finish and
close, ensuring recordBandwidth runs exactly once for completed or early-closed
responses, including after data has been handed to the OS.
| UpsertProviderMcpServerInput, | ||
| } from '@/shared/types.js'; | ||
| import { AppError, asyncHandler, createApiSuccessResponse } from '@/shared/utils.js'; | ||
| import { isBandwidthMonitorEnabled } from '@/modules/websocket/services/bandwidth-monitor.service.js'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the WebSocket module barrel.
Line 20 violates the cross-module import rule. Export isBandwidthMonitorEnabled from server/modules/websocket/index.ts and import it through that barrel.
As per static analysis, this import violates boundaries/dependencies.
🧰 Tools
🪛 ESLint
[error] 20-20: Cross-module imports must go through that module's barrel file (server/modules//index.ts or index.js).
(boundaries/dependencies)
🤖 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 `@server/modules/providers/provider.routes.ts` at line 20, Export
isBandwidthMonitorEnabled from the WebSocket module barrel in
server/modules/websocket/index.ts, then update the provider routes import to
reference the barrel instead of the bandwidth-monitor service implementation.
Preserve the existing symbol usage and behavior.
Source: Linters/SAST tools
| if (limit === null && isBandwidthMonitorEnabled()) { | ||
| console.log( | ||
| `[bandwidth-monitor messages] UNBOUNDED hit sessionId=${sessionId} ` + | ||
| `ua=${JSON.stringify(req.headers['user-agent'] ?? null)} ` + | ||
| `referer=${JSON.stringify(req.headers['referer'] ?? req.headers['origin'] ?? null)} ` + | ||
| `remoteAddr=${req.socket.remoteAddress ?? 'unknown'} ` + | ||
| `xForwardedFor=${JSON.stringify(req.headers['x-forwarded-for'] ?? null)} ` + | ||
| `authHeaderPresent=${Boolean(req.headers['authorization'])} ` + | ||
| `stack=${new Error('trace').stack?.split('\n').slice(1, 5).join(' <- ')}` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove raw identifiers and sensitive metadata from bandwidth diagnostics.
The monitoring logs persist data that can identify users or expose secrets. Opt-in logging does not remove the exposure risk.
server/modules/providers/provider.routes.ts#L674-L682: omit raw IP addresses and forwarded addresses, redact referer query strings, and use a privacy-reviewed hashed identifier if caller correlation is required.server/modules/websocket/services/shell-websocket.service.ts#L317-L333: replaceptySessionKeywith a generated connection identifier. Do not log the project path, session ID, or command-derived suffix.
Based on static analysis: “Avoid logging sensitive data.”
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 674-682: Avoid logging sensitive data
Context: console.log(
[bandwidth-monitor messages] UNBOUNDED hit sessionId=${sessionId} +
ua=${JSON.stringify(req.headers['user-agent'] ?? null)} +
referer=${JSON.stringify(req.headers['referer'] ?? req.headers['origin'] ?? null)} +
remoteAddr=${req.socket.remoteAddress ?? 'unknown'} +
xForwardedFor=${JSON.stringify(req.headers['x-forwarded-for'] ?? null)} +
authHeaderPresent=${Boolean(req.headers['authorization'])} +
stack=${new Error('trace').stack?.split('\n').slice(1, 5).join(' <- ')}
)
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
[warning] 674-682: Logging request-derived input unsanitized allows log forging (CRLF injection); strip newlines / encode the value before logging.
Context: console.log(
[bandwidth-monitor messages] UNBOUNDED hit sessionId=${sessionId} +
ua=${JSON.stringify(req.headers['user-agent'] ?? null)} +
referer=${JSON.stringify(req.headers['referer'] ?? req.headers['origin'] ?? null)} +
remoteAddr=${req.socket.remoteAddress ?? 'unknown'} +
xForwardedFor=${JSON.stringify(req.headers['x-forwarded-for'] ?? null)} +
authHeaderPresent=${Boolean(req.headers['authorization'])} +
stack=${new Error('trace').stack?.split('\n').slice(1, 5).join(' <- ')}
)
Note: [CWE-117] Improper Output Neutralization for Logs. OWASP A09:2021 Security Logging and Monitoring Failures.
(log-injection-typescript)
📍 Affects 2 files
server/modules/providers/provider.routes.ts#L674-L682(this comment)server/modules/websocket/services/shell-websocket.service.ts#L317-L333
🤖 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 `@server/modules/providers/provider.routes.ts` around lines 674 - 682, Remove
sensitive metadata from the bandwidth-monitor diagnostic in
server/modules/providers/provider.routes.ts lines 674-682: omit raw IP and
forwarded-address fields, redact referer query strings, and use only a
privacy-reviewed hashed identifier if correlation is necessary. In
server/modules/websocket/services/shell-websocket.service.ts lines 317-333,
replace ptySessionKey with a generated connection identifier and exclude the
project path, session ID, and command-derived suffix from logs.
Source: Linters/SAST tools
| const payload = JSON.stringify({ | ||
| type: 'output', | ||
| data: bufferedData, | ||
| }); | ||
| diag.replayMessages += 1; | ||
| diag.replayBytes += Buffer.byteLength(payload, 'utf8'); | ||
| ws.send(payload); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Count every outbound shell frame before printing TOTAL SENT.
The counters include replay and PTY output only. The reconnect frame at Lines 392-397, plus error, welcome, exit, and auth_url frames, do not update these counters. The final TOTAL SENT value therefore underreports server-to-client shell traffic. Route every outbound shell send through one helper that records its category before calling ws.send.
Also applies to: 531-537
🤖 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 `@server/modules/websocket/services/shell-websocket.service.ts` around lines
401 - 407, Update the shell websocket send flow around the existing replay send
and all other outbound frames, including reconnect, error, welcome, exit, and
auth_url messages. Introduce one helper that records each frame’s category and
byte size before invoking ws.send, then route every shell outbound send through
it so TOTAL SENT includes all server-to-client traffic while preserving existing
payloads.
| const refreshFromServer = useCallback((sessionId: string): Promise<void> => { | ||
| const existing = inFlightRefreshRef.current.get(sessionId); | ||
| if (existing) { | ||
| return existing; | ||
| } | ||
|
|
||
| const promise = performRefreshFromServer(sessionId).finally(() => { | ||
| inFlightRefreshRef.current.delete(sessionId); | ||
| }); | ||
| inFlightRefreshRef.current.set(sessionId, promise); | ||
| return promise; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Queue a follow-up refresh when a new signal arrives during an in-flight refresh.
Lines 675-678 return the existing request without recording the newer signal. If refresh A starts, then a later complete or external update requests refresh B, A can return a transcript from before B's event. The map entry is then removed and no request fetches B's state.
Track a per-session dirty flag or generation counter. After the active request completes, run one more bounded refresh when another request arrived while it was in flight. Resolve callers only after that follow-up refresh completes.
🤖 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/stores/useSessionStore.ts` around lines 674 - 684, Update
refreshFromServer to record when a refresh request arrives while the session
already has an in-flight promise, using a per-session dirty flag or generation
counter. After performRefreshFromServer completes, issue one bounded follow-up
refresh when a newer request was recorded, keep the inFlightRefreshRef entry
until the follow-up finishes, and resolve all callers only after the latest
refresh completes.
1 participant
Summary
BANDWIDTH_MONITOR_ENABLED=true(see.env.example); when unset, all instrumentation is a no-op with effectively zero overhead (the HTTP byte-counting middleware isn't even mounted).server/modules/websocket/services/bandwidth-monitor.service.tscentralizes bucketed byte counters for HTTP responses and every WebSocket channel (shell, chat, desktop notifications, plugin proxy), logged on a 10s interval as[bandwidth-monitor] ....[bandwidth-monitor shell] ....limit=null) sessions-messages endpoint, logged as[bandwidth-monitor messages] UNBOUNDED hit ..., useful for catching future regressions of the bug fixed in fix: unbounded session transcript refetch causes severe bandwidth usage on long sessions #1087.Why
While diagnosing severe mobile data usage on the Shell tab (multiple GB in under an hour), it took bespoke server-wide instrumentation to actually trace the bytes back to the unbounded chat-history refetch fixed in #1087. Keeping a lightweight, opt-in version of that instrumentation around makes it easy to reproduce that kind of investigation in the future without hand-patching the server again.
Test plan
BANDWIDTH_MONITOR_ENABLED=trueon a live deployment; confirmed[bandwidth-monitor],[bandwidth-monitor shell], and[bandwidth-monitor messages]log lines appear and totals track real client-side data usage.Summary by CodeRabbit
BANDWIDTH_MONITOR_ENABLEDconfiguration option, disabled by default.