-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: add opt-in bandwidth monitoring behind BANDWIDTH_MONITOR_ENABLED #1094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
509d908
dd3be82
432b526
c3fc217
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import type { | |
| UpsertProviderMcpServerInput, | ||
| } from '@/shared/types.js'; | ||
| import { AppError, asyncHandler, createApiSuccessResponse } from '@/shared/utils.js'; | ||
| import { isBandwidthMonitorEnabled } from '@/modules/websocket/services/bandwidth-monitor.service.js'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Use the WebSocket module barrel. Line 20 violates the cross-module import rule. Export As per static analysis, this import violates 🧰 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 AgentsSource: Linters/SAST tools |
||
|
|
||
| const router = express.Router(); | ||
|
|
||
|
|
@@ -666,6 +667,22 @@ router.get( | |
| offset = parsedOffset; | ||
| } | ||
|
|
||
| // Optional, opt-in bandwidth monitoring (BANDWIDTH_MONITOR_ENABLED=true). | ||
| // Identifies callers of the unbounded (limit=null) path, useful for | ||
| // catching bandwidth regressions where a full-history fetch slips back in. | ||
| // Logs enough of the request to fingerprint the source. | ||
| 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(' <- ')}` | ||
|
Comment on lines
+674
to
+682
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 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.
Based on static analysis: “Avoid logging sensitive data.” 🧰 Tools🪛 ast-grep (0.45.0)[warning] 674-682: Avoid logging sensitive data (log-sensitive-data-typescript) [warning] 674-682: Logging request-derived input unsanitized allows log forging (CRLF injection); strip newlines / encode the value before logging. (log-injection-typescript) 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
| ); | ||
| } | ||
|
|
||
| const result = await sessionsService.fetchHistory(sessionId, { | ||
| limit, | ||
| offset, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| import { WebSocket } from 'ws'; | ||
|
|
||
| /** | ||
| * Optional, opt-in bandwidth monitoring. Tracks bytes sent by every response | ||
| * the server emits (HTTP responses and every WebSocket channel: shell, chat, | ||
| * desktop notifications, plugin proxy), bucketed by a coarse key, and logs | ||
| * totals on an interval so they can be correlated against client-side data | ||
| * usage readings. Disabled by default - enable with BANDWIDTH_MONITOR_ENABLED=true. | ||
| */ | ||
|
|
||
| export function isBandwidthMonitorEnabled(): boolean { | ||
| return process.env.BANDWIDTH_MONITOR_ENABLED === 'true'; | ||
| } | ||
|
|
||
| type BandwidthBucket = { | ||
| messages: number; | ||
| bytes: number; | ||
| }; | ||
|
|
||
| const buckets = new Map<string, BandwidthBucket>(); | ||
| const monitorStartedAt = Date.now(); | ||
|
|
||
| function bucketFor(key: string): BandwidthBucket { | ||
| let bucket = buckets.get(key); | ||
| if (!bucket) { | ||
| bucket = { messages: 0, bytes: 0 }; | ||
| buckets.set(key, bucket); | ||
| } | ||
| return bucket; | ||
| } | ||
|
|
||
| export function recordBandwidth(key: string, byteLength: number): void { | ||
| if (!isBandwidthMonitorEnabled()) { | ||
| return; | ||
| } | ||
| const bucket = bucketFor(key); | ||
| bucket.messages += 1; | ||
| bucket.bytes += byteLength; | ||
| } | ||
|
|
||
| export function estimateByteLength(data: unknown): number { | ||
| if (typeof data === 'string') { | ||
| return Buffer.byteLength(data, 'utf8'); | ||
| } | ||
| if (Buffer.isBuffer(data)) { | ||
| return data.length; | ||
| } | ||
| if (data instanceof ArrayBuffer) { | ||
| return data.byteLength; | ||
| } | ||
| if (ArrayBuffer.isView(data)) { | ||
| return data.byteLength; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| let loggingStarted = false; | ||
|
|
||
| export function startBandwidthLogging(intervalMs = 10_000): void { | ||
| if (!isBandwidthMonitorEnabled() || loggingStarted) { | ||
| return; | ||
| } | ||
| loggingStarted = true; | ||
|
|
||
| const timer = setInterval(() => { | ||
| const elapsedSec = ((Date.now() - monitorStartedAt) / 1000).toFixed(1); | ||
| const entries = Array.from(buckets.entries()).sort((a, b) => b[1].bytes - a[1].bytes); | ||
| const totalBytes = entries.reduce((sum, [, bucket]) => sum + bucket.bytes, 0); | ||
| const breakdown = entries | ||
| .map(([key, bucket]) => `${key}: ${bucket.messages} msgs / ${(bucket.bytes / 1024).toFixed(1)}KB`) | ||
| .join(' | '); | ||
|
|
||
| console.log( | ||
| `[bandwidth-monitor] elapsed=${elapsedSec}s TOTAL=${(totalBytes / 1024).toFixed(1)}KB${breakdown ? ' | ' + breakdown : ''}` | ||
| ); | ||
| }, intervalMs); | ||
|
|
||
| timer.unref(); | ||
| } | ||
|
|
||
| type TaggedWebSocket = WebSocket & { _bandwidthRoute?: string }; | ||
|
|
||
| let webSocketSendPatched = false; | ||
|
|
||
| /** | ||
| * Monkey-patches WebSocket.prototype.send once so every outbound WS frame on | ||
| * every channel (shell, chat, desktop-notifications, plugin proxy) is | ||
| * counted, without needing to instrument each service individually. No-op | ||
| * unless bandwidth monitoring is enabled. | ||
| */ | ||
| export function patchWebSocketBandwidthTracking(): void { | ||
| if (!isBandwidthMonitorEnabled() || webSocketSendPatched) { | ||
| return; | ||
| } | ||
| webSocketSendPatched = true; | ||
|
|
||
| const originalSend = WebSocket.prototype.send; | ||
|
|
||
| WebSocket.prototype.send = function patchedSend( | ||
| this: TaggedWebSocket, | ||
| data: Parameters<typeof originalSend>[0], | ||
| ...rest: unknown[] | ||
| ) { | ||
| try { | ||
| const route = this._bandwidthRoute ?? 'ws:unknown'; | ||
| recordBandwidth(route, estimateByteLength(data)); | ||
| } catch { | ||
| // Monitoring must never break real traffic. | ||
| } | ||
|
|
||
| // @ts-expect-error - forwarding the library's overloaded variadic signature as-is | ||
| return originalSend.call(this, data, ...rest); | ||
| }; | ||
| } | ||
|
|
||
| export function tagWebSocketRoute(ws: WebSocket, route: string): void { | ||
| if (!isBandwidthMonitorEnabled()) { | ||
| return; | ||
| } | ||
| (ws as TaggedWebSocket)._bandwidthRoute = `ws:${route}`; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import pty, { type IPty } from 'node-pty'; | |
| import { WebSocket, type RawData } from 'ws'; | ||
|
|
||
| import { parseIncomingJsonObject } from '@/shared/utils.js'; | ||
| import { isBandwidthMonitorEnabled } from '@/modules/websocket/services/bandwidth-monitor.service.js'; | ||
|
|
||
| type ShellIncomingMessage = { | ||
| type?: string; | ||
|
|
@@ -297,6 +298,43 @@ export function handleShellConnection( | |
| let urlDetectionBuffer = ''; | ||
| const announcedAuthUrls = new Set<string>(); | ||
|
|
||
| // Optional, opt-in bandwidth monitoring (BANDWIDTH_MONITOR_ENABLED=true). | ||
| // Counters are always tracked (cheap); only the periodic/close logging is gated. | ||
| const diag = { | ||
| outputMessages: 0, | ||
| outputBytes: 0, | ||
| replayMessages: 0, | ||
| replayBytes: 0, | ||
| resizeMessages: 0, | ||
| inputMessages: 0, | ||
| inputBytes: 0, | ||
| startedAt: Date.now(), | ||
| }; | ||
| if (isBandwidthMonitorEnabled()) { | ||
| const diagLogInterval = setInterval(() => { | ||
| const elapsedSec = ((Date.now() - diag.startedAt) / 1000).toFixed(1); | ||
| console.log( | ||
| `[bandwidth-monitor shell] key=${ptySessionKey} elapsed=${elapsedSec}s ` + | ||
| `output: ${diag.outputMessages} msgs / ${(diag.outputBytes / 1024).toFixed(1)}KB | ` + | ||
| `replay: ${diag.replayMessages} msgs / ${(diag.replayBytes / 1024).toFixed(1)}KB | ` + | ||
| `resize: ${diag.resizeMessages} msgs | ` + | ||
| `input: ${diag.inputMessages} msgs / ${diag.inputBytes}B` | ||
| ); | ||
| }, 5000); | ||
| ws.on('close', () => { | ||
| clearInterval(diagLogInterval); | ||
| const elapsedSec = ((Date.now() - diag.startedAt) / 1000).toFixed(1); | ||
| console.log( | ||
| `[bandwidth-monitor shell] FINAL key=${ptySessionKey} elapsed=${elapsedSec}s ` + | ||
| `output: ${diag.outputMessages} msgs / ${(diag.outputBytes / 1024).toFixed(1)}KB | ` + | ||
| `replay: ${diag.replayMessages} msgs / ${(diag.replayBytes / 1024).toFixed(1)}KB | ` + | ||
| `resize: ${diag.resizeMessages} msgs | ` + | ||
| `input: ${diag.inputMessages} msgs / ${diag.inputBytes}B | ` + | ||
| `TOTAL SENT: ${((diag.outputBytes + diag.replayBytes) / 1024).toFixed(1)}KB` | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| ws.on('message', async (rawMessage) => { | ||
| try { | ||
| const data = parseShellMessage(rawMessage); | ||
|
|
@@ -360,12 +398,13 @@ export function handleShellConnection( | |
|
|
||
| if (existingSession.buffer.length > 0) { | ||
| existingSession.buffer.forEach((bufferedData) => { | ||
| ws.send( | ||
| JSON.stringify({ | ||
| type: 'output', | ||
| data: bufferedData, | ||
| }) | ||
| ); | ||
| const payload = JSON.stringify({ | ||
| type: 'output', | ||
| data: bufferedData, | ||
| }); | ||
| diag.replayMessages += 1; | ||
| diag.replayBytes += Buffer.byteLength(payload, 'utf8'); | ||
| ws.send(payload); | ||
|
Comment on lines
+401
to
+407
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Count every outbound shell frame before printing The counters include replay and PTY output only. The reconnect frame at Lines 392-397, plus error, welcome, exit, and Also applies to: 531-537 🤖 Prompt for AI Agents |
||
| }); | ||
| } | ||
|
|
||
|
|
@@ -489,12 +528,13 @@ export function handleShellConnection( | |
| emitAuthUrl(bestUrl, true); | ||
| } | ||
|
|
||
| session.ws.send( | ||
| JSON.stringify({ | ||
| type: 'output', | ||
| data: outputData, | ||
| }) | ||
| ); | ||
| const outputPayload = JSON.stringify({ | ||
| type: 'output', | ||
| data: outputData, | ||
| }); | ||
| diag.outputMessages += 1; | ||
| diag.outputBytes += Buffer.byteLength(outputPayload, 'utf8'); | ||
| session.ws.send(outputPayload); | ||
| } | ||
| }); | ||
|
|
||
|
|
@@ -552,13 +592,17 @@ export function handleShellConnection( | |
| } | ||
|
|
||
| if (data.type === 'input') { | ||
| const inputStr = readString(data.data); | ||
| diag.inputMessages += 1; | ||
| diag.inputBytes += Buffer.byteLength(inputStr, 'utf8'); | ||
| if (shellProcess) { | ||
| shellProcess.write(readString(data.data)); | ||
| shellProcess.write(inputStr); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (data.type === 'resize') { | ||
| diag.resizeMessages += 1; | ||
| if (shellProcess) { | ||
| shellProcess.resize(readNumber(data.cols, 80), readNumber(data.rows, 24)); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: 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:
🌐 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:
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 bothfinishandclosesorecordBandwidthstill runs, including when Node has already handed data off to the OS.🤖 Prompt for AI Agents