✨ feat(agent): graduate Portwing edge streaming - #630
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
biggest-littlest
left a comment
biggest-littlest
left a comment
There was a problem hiding this comment.
Reviewed the default-on compatibility switch, continuous log lifecycle, bounded stream bridge, and fleet-soak workflow. The full 100%-coverage and real-process soak evidence support graduation.
ALARGECOMPANY
left a comment
ALARGECOMPANY
left a comment
There was a problem hiding this comment.
Independently reviewed the Portwing protocol bridge, cancellation/error paths, disconnect cleanup, stable API/docs, and closed-input CI workflow. Approving through the normal review path.
📝 WalkthroughWalkthroughPortwing is now enabled by default, with Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 9
🧹 Nitpick comments (3)
.github/workflows/quality-portwing-fleet-soak.yml (1)
48-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFloating
ref: mainmakes soak runs non-reproducible.A red PR run cannot be re-run against the same Portwing build. The evidence file does record the resolved commit (
revisions.portwing), so consider pinningrefto a SHA (or a release tag) bumped deliberately, keepingmainonly for the scheduled run.🤖 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 @.github/workflows/quality-portwing-fleet-soak.yml around lines 48 - 54, Update the Portwing checkout step identified by repository: CodesWhat/portwing so pull-request soak runs use a deliberately pinned commit SHA or release tag instead of floating ref: main, while preserving main for scheduled runs. Keep the resolved revision recorded in revisions.portwing.scripts/portwing-fleet-soak.mjs (1)
405-419: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated exec-session startup loop.
Lines 405-419 and 481-496 differ only in the target map. Extract once.
♻️ Proposed refactor
+async function startExecSessions() { + const started = new Map(); + for (const agent of getAgents()) { + const sessionIds = []; + for (let index = 0; index < options.execPerAgent; index += 1) { + sessionIds.push( + await agent.edgeAdapter.startExec('c0000000001', ['sh', '-c', 'cat'], { + tty: true, + outputCallback: () => { + evidence.phases.execOutputFrames += 1; + }, + }), + ); + evidence.phases.execSessionsStarted += 1; + } + started.set(agent, sessionIds); + } + return started; +}Call sites become
const firstExecSessionIds = await startExecSessions();andconst sustainedExecSessionIds = await startExecSessions();.Also applies to: 481-496
🤖 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 `@scripts/portwing-fleet-soak.mjs` around lines 405 - 419, Extract the duplicated exec-session startup loop into a shared async helper, such as startExecSessions, preserving the existing agent iteration, options.execPerAgent count, startExec arguments, output callback, and evidence counters. Replace both firstExecSessionIds and sustainedExecSessionIds initialization blocks with calls to this helper, keeping each resulting map assigned to its respective variable.app/agent/EdgeAgentAdapter.test.ts (1)
819-941: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
as unknown as {...}casts — later tests already call the public method directly.Lines 975, 1000, 1032, 1063, 1141, 1178, 1207, 1228 use
adapter.streamContainerLogs(...), so the optional-method casts plusexpect(typeof streamMethod).toBe('function')/if (!streamMethod) return;are duplicated dead branches that can silently skip assertions. Also, "cancels one live dd log stream without disturbing another request" only opens one stream — either open a second and assert its handlers are untouched, or rename.♻️ Example
- const streamMethod = ( - adapter as unknown as { - streamContainerLogs?: ( - containerId: string, - options: Record<string, unknown>, - handlers: { onChunk: () => void; onEnd: () => void; onError: () => void }, - ) => { cancel: () => void }; - } - ).streamContainerLogs; - - expect(typeof streamMethod).toBe('function'); - if (!streamMethod) return; - - const handle = streamMethod.call( - adapter, + const handle = adapter.streamContainerLogs( 'container-cancel', {}, { onChunk: vi.fn(), onEnd: vi.fn(), onError: vi.fn() }, );Also applies to: 887-887
🤖 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 `@app/agent/EdgeAgentAdapter.test.ts` around lines 819 - 941, Replace the `as unknown as` access patterns for `streamContainerLogs` in the affected tests with direct calls to the public `adapter.streamContainerLogs` method, removing the optional-method guards and early returns so assertions cannot be skipped. Update “cancels one live dd log stream without disturbing another request” to create a second stream and verify its handlers remain untouched, or rename the test to match its single-stream coverage.
🤖 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 `@app/agent/EdgeAgentAdapter.ts`:
- Around line 940-980: Update streamContainerLogs and live stream lifecycle
handling to arm a first-frame/idle deadline using REQUEST_TIMEOUT_MS when
registering each request. On expiry, remove the stream and invoke its terminal
error callback so silent or legacy non-echoing responses cannot remain open
indefinitely. Clear the timer in handleContainerLogChunk, handleContainerLogEnd,
handleContainerLogError, cancelContainerLogStream, and onDisconnect.
In `@app/api/container/log-stream.ts`:
- Around line 330-334: Guard the WebSocket close operations in the backpressure
handling paths around the visible `webSocket.bufferedAmount` checks, including
the corresponding block at 356-371, so exceptions from oversized reasons or
destroyed sockets cannot bypass cleanup. Reuse the file’s existing guarded-close
pattern or introduce a shared helper, and ensure `cleanup(true)` still runs and
the function returns false after a failed close.
- Around line 398-411: Update the legacy WebSocket close paths around the
“Unable to open logs” handling to pass their close-reason strings through
truncateWebSocketCloseReason before closing, including the currently unguarded
path. Replace the edge-path error.message usage with the file’s injected
getErrorMessage helper, preserving the existing close behavior and context.
- Around line 330-371: In app/api/container/log-stream.ts lines 330-371, add a
guarded closeViewer helper that truncates close reasons and catches
webSocket.close failures, then route the backpressure, handleEnd, and
handleError close calls through it so cleanup and resolve always run. In
app/agent/EdgeAgentAdapter.ts lines 1277-1280, delete the stream map entry
before invoking stream.handlers.onError and wrap that callback in try/catch so
exec-session close, removeAgent, and emitAgentDisconnected still execute.
In `@content/docs/current/api/portwing.mdx`:
- Around line 202-236: Update the container log streaming documentation to
remove the unsupported until field from the request example and avoid claiming
that every older agent’s legacy response is converted successfully. State that
legacy response handling requires the agent to echo requestId, matching
streamContainerLogs, the gateway request construction, and
handleContainerLogResponse behavior.
In `@scripts/portwing-fleet-soak.mjs`:
- Around line 128-135: Update the binary validation loop in the portwing fleet
soak setup to handle missing or invalid paths without letting statSync throw
before fail() runs. Ensure both portwing binary and mockdocker binary checks
report the intended “not a file” message through fail(), while preserving the
existing validation for valid paths.
- Around line 270-272: Update the child liveness logic around activeChildren()
to use the existing isRunning predicate, which must treat both exitCode and
signalCode termination as no longer running. Reuse isRunning for the mockDocker
check and cleanup exit-wait check instead of inline exitCode comparisons,
preserving current behavior for running children.
- Around line 434-438: Update closeAdapterSessions() to fail explicitly when the
EdgeAgentAdapter lacks execSessions instead of returning early. Preserve the
existing cleanup behavior when execSessions is present, and use the script’s
established fail() mechanism so the soak cannot pass without exercising exec
cleanup.
- Around line 174-184: Update rssBytes to read RSS from /proc/<pid>/statm,
matching the existing threadCount procfs approach, instead of spawning a
blocking ps process per PID. Make procfs read or parse failures visible to
callers rather than returning 0, and add a sanity assertion around agentRss
baseline/final values so missing RSS data cannot satisfy the aggregate growth
assertion; retain spawnSync if it is still used by gitCommit.
---
Nitpick comments:
In @.github/workflows/quality-portwing-fleet-soak.yml:
- Around line 48-54: Update the Portwing checkout step identified by repository:
CodesWhat/portwing so pull-request soak runs use a deliberately pinned commit
SHA or release tag instead of floating ref: main, while preserving main for
scheduled runs. Keep the resolved revision recorded in revisions.portwing.
In `@app/agent/EdgeAgentAdapter.test.ts`:
- Around line 819-941: Replace the `as unknown as` access patterns for
`streamContainerLogs` in the affected tests with direct calls to the public
`adapter.streamContainerLogs` method, removing the optional-method guards and
early returns so assertions cannot be skipped. Update “cancels one live dd log
stream without disturbing another request” to create a second stream and verify
its handlers remain untouched, or rename the test to match its single-stream
coverage.
In `@scripts/portwing-fleet-soak.mjs`:
- Around line 405-419: Extract the duplicated exec-session startup loop into a
shared async helper, such as startExecSessions, preserving the existing agent
iteration, options.execPerAgent count, startExec arguments, output callback, and
evidence counters. Replace both firstExecSessionIds and sustainedExecSessionIds
initialization blocks with calls to this helper, keeping each resulting map
assigned to its respective variable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ec463793-f503-4f41-9cea-01b1319913ef
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (24)
.github/workflows/quality-portwing-fleet-soak.ymlREADME.de.mdREADME.es.mdREADME.fr.mdREADME.mdREADME.pl.mdREADME.pt-BR.mdREADME.zh-CN.mdapp/agent/EdgeAgentAdapter.test.tsapp/agent/EdgeAgentAdapter.tsapp/api/api.tsapp/api/container/log-stream.test.tsapp/api/container/log-stream.tsapp/api/index.test.tsapp/api/index.tsapp/api/openapi/index.tsapp/api/openapi/paths/portwing.test.tsapp/api/openapi/paths/portwing.tsapp/configuration/index.test.tsapp/configuration/index.tscontent/docs/current/api/index.mdxcontent/docs/current/api/portwing.mdxcontent/docs/current/configuration/agents/index.mdxscripts/portwing-fleet-soak.mjs
| streamContainerLogs( | ||
| containerId: string, | ||
| options: { | ||
| tail?: number; | ||
| since?: string | number; | ||
| follow?: boolean; | ||
| timestamps?: boolean; | ||
| }, | ||
| handlers: ContainerLogStreamHandlers, | ||
| ): ContainerLogStreamHandle { | ||
| if (this.pendingRequests.size + this.liveContainerLogStreams.size >= MAX_PENDING_REQUESTS) { | ||
| handlers.onError(new Error('concurrent request limit reached')); | ||
| return { cancel: () => {} }; | ||
| } | ||
|
|
||
| const requestId = uuidv7(); | ||
| this.liveContainerLogStreams.set(requestId, { containerId, handlers }); | ||
| try { | ||
| this.ws.send( | ||
| JSON.stringify({ | ||
| type: 'dd:container_log_request', | ||
| data: { | ||
| containerId, | ||
| requestId, | ||
| ...options, | ||
| follow: options.follow ?? true, | ||
| stream: true, | ||
| }, | ||
| }), | ||
| ); | ||
| } catch (error: unknown) { | ||
| this.liveContainerLogStreams.delete(requestId); | ||
| handlers.onError(error instanceof Error ? error : new Error(getErrorMessage(error))); | ||
| } | ||
|
|
||
| return { | ||
| cancel: () => { | ||
| this.cancelContainerLogStream(requestId, containerId); | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No first-frame/idle deadline on live streams — a silent or non-echoing agent hangs the viewer forever.
requestContainerLogs arms REQUEST_TIMEOUT_MS; streamContainerLogs arms nothing. Two reachable paths leave the entry in liveContainerLogStreams with no terminal callback:
- Agent never sends chunk/end/error.
- Legacy agent replies
dd:container_log_responsewithout echoingrequestId— per the comment at lines 139-148 that is exactly what legacy agents do, sohandleContainerLogResponsetakes thependingRequestsbranch (no match, dropped) and the live stream is never ended.
Downstream, streamEdgeAgentLogsToWebSocket only resolves/cleans on a handler callback or viewer close, so the gateway promise and socket stay open until disconnect.
🕒 Proposed fix: first-frame deadline
interface LiveContainerLogStream {
containerId: string;
handlers: ContainerLogStreamHandlers;
+ timer: ReturnType<typeof setTimeout>;
} const requestId = uuidv7();
- this.liveContainerLogStreams.set(requestId, { containerId, handlers });
+ const timer = setTimeout(() => {
+ const live = this.liveContainerLogStreams.get(requestId);
+ if (!live) {
+ return;
+ }
+ this.cancelContainerLogStream(requestId, containerId);
+ live.handlers.onError(
+ new Error(`Container log stream for ${containerId} produced no frames within ${REQUEST_TIMEOUT_MS}ms`),
+ );
+ }, REQUEST_TIMEOUT_MS);
+ this.liveContainerLogStreams.set(requestId, { containerId, handlers, timer });Clear timer in handleContainerLogChunk (first frame), handleContainerLogEnd, handleContainerLogError, cancelContainerLogStream, and onDisconnect.
🤖 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 `@app/agent/EdgeAgentAdapter.ts` around lines 940 - 980, Update
streamContainerLogs and live stream lifecycle handling to arm a first-frame/idle
deadline using REQUEST_TIMEOUT_MS when registering each request. On expiry,
remove the stream and invoke its terminal error callback so silent or legacy
non-echoing responses cannot remain open indefinitely. Clear the timer in
handleContainerLogChunk, handleContainerLogEnd, handleContainerLogError,
cancelContainerLogStream, and onDisconnect.
| if ((webSocket.bufferedAmount ?? 0) > MAX_VIEWER_BUFFER_BYTES) { | ||
| webSocket.close(1013, 'Log viewer is too slow'); | ||
| cleanup(true); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded webSocket.close() before cleanup() can strand the gateway promise.
The legacy path in this same file wraps close in try/catch (lines 527-531, 535-539) because ws throws on oversized reasons and on already-destroyed sockets. Here a throw skips cleanup(), so resolve() never fires (handleUpgrade awaits it at 613-625), viewer listeners stay attached, and handle.cancel() is never sent. The throw also unwinds into the adapter callback site (handleContainerLogEnd, onDisconnect).
🛡️ Proposed fix: single guarded close helper
+ const closeViewer = (code: number, reason: string) => {
+ try {
+ webSocket.close(code, truncateWebSocketCloseReason(reason));
+ } catch {
+ /* socket already closed */
+ }
+ };
+
const emitMessages = (messages: DockerLogMessage[]): boolean => {
@@
if ((webSocket.bufferedAmount ?? 0) > MAX_VIEWER_BUFFER_BYTES) {
- webSocket.close(1013, 'Log viewer is too slow');
+ closeViewer(1013, 'Log viewer is too slow');
cleanup(true);
@@
const handleEnd = () => {
if (cleaned) {
return;
}
if (emitMessages(decoder.flush())) {
- webSocket.close(1000, 'Stream ended');
+ closeViewer(1000, 'Stream ended');
}
cleanup(false);
};
const handleError = (error: Error) => {
if (cleaned) {
return;
}
- webSocket.close(1011, truncateWebSocketCloseReason(`Log stream error (${error.message})`));
+ closeViewer(1011, `Log stream error (${error.message})`);
cleanup(false);
};Also applies to: 356-371
🤖 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 `@app/api/container/log-stream.ts` around lines 330 - 334, Guard the WebSocket
close operations in the backpressure handling paths around the visible
`webSocket.bufferedAmount` checks, including the corresponding block at 356-371,
so exceptions from oversized reasons or destroyed sockets cannot bypass cleanup.
Reuse the file’s existing guarded-close pattern or introduce a shared helper,
and ensure `cleanup(true)` still runs and the function returns false after a
failed close.
| if ((webSocket.bufferedAmount ?? 0) > MAX_VIEWER_BUFFER_BYTES) { | ||
| webSocket.close(1013, 'Log viewer is too slow'); | ||
| cleanup(true); | ||
| return false; | ||
| } | ||
| try { | ||
| webSocket.send( | ||
| JSON.stringify({ | ||
| ...message, | ||
| displayTs: formatLogDisplayTimestamp(message.ts), | ||
| }), | ||
| ); | ||
| } catch { | ||
| cleanup(true); | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| }; | ||
|
|
||
| const handleChunk = (chunk: ContainerLogStreamChunk) => { | ||
| if (cleaned) { | ||
| return; | ||
| } | ||
| emitMessages(decoder.push({ type: chunk.stream, payload: chunk.logs })); | ||
| }; | ||
| const handleEnd = () => { | ||
| if (cleaned) { | ||
| return; | ||
| } | ||
| if (emitMessages(decoder.flush())) { | ||
| webSocket.close(1000, 'Stream ended'); | ||
| } | ||
| cleanup(false); | ||
| }; | ||
| const handleError = (error: Error) => { | ||
| if (cleaned) { | ||
| return; | ||
| } | ||
| webSocket.close(1011, truncateWebSocketCloseReason(`Log stream error (${error.message})`)); | ||
| cleanup(false); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Exceptions escaping a log-stream handler callback break cleanup on both sides. The gateway closes the viewer socket unguarded inside onEnd/onError/backpressure paths, and ws.close() throws on oversized reasons or a dead socket; the adapter then invokes those callbacks inline in its teardown loop, so one throw aborts both cleanups.
app/api/container/log-stream.ts#L330-L371: route all threewebSocket.close(...)calls through a guardedcloseViewer(code, reason)helper (try/catch +truncateWebSocketCloseReason) socleanup()andresolve()always run.app/agent/EdgeAgentAdapter.ts#L1277-L1280: delete the map entry first and wrapstream.handlers.onError(...)in try/catch so exec-session close,removeAgent, andemitAgentDisconnectedstill execute.
📍 Affects 2 files
app/api/container/log-stream.ts#L330-L371(this comment)app/agent/EdgeAgentAdapter.ts#L1277-L1280
🤖 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 `@app/api/container/log-stream.ts` around lines 330 - 371, In
app/api/container/log-stream.ts lines 330-371, add a guarded closeViewer helper
that truncates close reasons and catches webSocket.close failures, then route
the backpressure, handleEnd, and handleError close calls through it so cleanup
and resolve always run. In app/agent/EdgeAgentAdapter.ts lines 1277-1280, delete
the stream map entry before invoking stream.handlers.onError and wrap that
callback in try/catch so exec-session close, removeAgent, and
emitAgentDisconnected still execute.
| function truncateWebSocketCloseReason(reason: string): string { | ||
| if (Buffer.byteLength(reason, 'utf8') <= MAX_WEBSOCKET_CLOSE_REASON_BYTES) { | ||
| return reason; | ||
| } | ||
|
|
||
| let truncated = ''; | ||
| for (const character of reason) { | ||
| if (Buffer.byteLength(truncated + character, 'utf8') > MAX_WEBSOCKET_CLOSE_REASON_BYTES) { | ||
| break; | ||
| } | ||
| truncated += character; | ||
| } | ||
| return truncated; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Apply the new truncation helper to the legacy close paths too.
Unable to open logs (…) at line 466 is neither truncated nor guarded — a long Docker error (>123 bytes) throws RangeError out of the handler; line 536 is guarded but still untruncated, so the viewer loses the reason. Route both through truncateWebSocketCloseReason. Minor related nit: the edge path uses error.message while the rest of the file uses the injected getErrorMessage.
🤖 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 `@app/api/container/log-stream.ts` around lines 398 - 411, Update the legacy
WebSocket close paths around the “Unable to open logs” handling to pass their
close-reason strings through truncateWebSocketCloseReason before closing,
including the currently unguarded path. Replace the edge-path error.message
usage with the file’s injected getErrorMessage helper, preserving the existing
close behavior and context.
| ```json | ||
| { | ||
| "type": "dd:container_log_request", | ||
| "data": { | ||
| "requestId": "log-8f31", | ||
| "containerId": "8d3f…", | ||
| "stream": true, | ||
| "follow": true, | ||
| "tail": 200, | ||
| "timestamps": true, | ||
| "since": 0, | ||
| "until": 0 | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| A current Portwing agent responds with zero or more chunk frames, preserving the Docker stream: | ||
|
|
||
| ```json | ||
| { | ||
| "type": "dd:container_log_chunk", | ||
| "data": { | ||
| "requestId": "log-8f31", | ||
| "containerId": "8d3f…", | ||
| "stream": "stderr", | ||
| "logs": "2026-07-28T12:00:00Z warning\n" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| The agent finishes with `dd:container_log_end` (`requestId`, `containerId`, optional `reason`) or `dd:container_log_error` (`requestId`, `containerId`, `error`). When the browser closes its log WebSocket, Drydock sends `dd:container_log_cancel` with the same `requestId` and `containerId`; Portwing cancels the Docker request promptly. | ||
|
|
||
| `requestId` is mandatory and correlates concurrent viewers. Drydock caps each downstream viewer at 1 MiB of queued data, while Portwing uses a bounded controller send queue and admits at most 128 live log streams. A slow consumer is closed instead of allowing unbounded memory growth. | ||
|
|
||
| Mixed-version fleets remain compatible. An older Portwing agent ignores `stream` and returns the legacy `dd:container_log_response`; Drydock renders that response as one stdout chunk and then ends the viewer. A newer agent still uses the legacy response for requests that omit `stream: true`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Doc/code mismatch: until is not sent on stream requests, and the legacy claim is broader than the code.
streamContainerLogs options are tail | since | follow | timestamps only (app/agent/EdgeAgentAdapter.ts:940-947), and the gateway passes exactly those (app/api/container/log-stream.ts:383-388). Also, handleContainerLogResponse only converts a legacy response into a chunk+end when the agent echoes requestId — an agent that omits the echo leaves the viewer hanging (see the separate comment on EdgeAgentAdapter.ts).
📝 Suggested edits
"tail": 200,
"timestamps": true,
- "since": 0,
- "until": 0
+ "since": 0
}
}-Mixed-version fleets remain compatible. An older Portwing agent ignores `stream` and returns the legacy `dd:container_log_response`; Drydock renders that response as one stdout chunk and then ends the viewer. A newer agent still uses the legacy response for requests that omit `stream: true`.
+Mixed-version fleets remain compatible. An older Portwing agent ignores `stream` and returns the legacy `dd:container_log_response`; when that response echoes the `requestId`, Drydock renders it as one stdout chunk and then ends the viewer. A newer agent still uses the legacy response for requests that omit `stream: true`.As per path instructions, "Documentation content. Review for accuracy against the code, not prose style."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```json | |
| { | |
| "type": "dd:container_log_request", | |
| "data": { | |
| "requestId": "log-8f31", | |
| "containerId": "8d3f…", | |
| "stream": true, | |
| "follow": true, | |
| "tail": 200, | |
| "timestamps": true, | |
| "since": 0, | |
| "until": 0 | |
| } | |
| } | |
| ``` | |
| A current Portwing agent responds with zero or more chunk frames, preserving the Docker stream: | |
| ```json | |
| { | |
| "type": "dd:container_log_chunk", | |
| "data": { | |
| "requestId": "log-8f31", | |
| "containerId": "8d3f…", | |
| "stream": "stderr", | |
| "logs": "2026-07-28T12:00:00Z warning\n" | |
| } | |
| } | |
| ``` | |
| The agent finishes with `dd:container_log_end` (`requestId`, `containerId`, optional `reason`) or `dd:container_log_error` (`requestId`, `containerId`, `error`). When the browser closes its log WebSocket, Drydock sends `dd:container_log_cancel` with the same `requestId` and `containerId`; Portwing cancels the Docker request promptly. | |
| `requestId` is mandatory and correlates concurrent viewers. Drydock caps each downstream viewer at 1 MiB of queued data, while Portwing uses a bounded controller send queue and admits at most 128 live log streams. A slow consumer is closed instead of allowing unbounded memory growth. | |
| Mixed-version fleets remain compatible. An older Portwing agent ignores `stream` and returns the legacy `dd:container_log_response`; Drydock renders that response as one stdout chunk and then ends the viewer. A newer agent still uses the legacy response for requests that omit `stream: true`. |
🤖 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 `@content/docs/current/api/portwing.mdx` around lines 202 - 236, Update the
container log streaming documentation to remove the unsupported until field from
the request example and avoid claiming that every older agent’s legacy response
is converted successfully. State that legacy response handling requires the
agent to echo requestId, matching streamContainerLogs, the gateway request
construction, and handleContainerLogResponse behavior.
Source: Path instructions
| for (const [name, path] of [ | ||
| ['portwing binary', options.portwingBin], | ||
| ['mockdocker binary', options.mockDockerBin], | ||
| ]) { | ||
| if (!statSync(path).isFile()) { | ||
| fail(`${name} is not a file: ${path}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
statSync throws before fail() can report.
A missing binary path surfaces as a raw ENOENT stack instead of the intended message.
🛠️ Proposed fix
for (const [name, path] of [
['portwing binary', options.portwingBin],
['mockdocker binary', options.mockDockerBin],
]) {
- if (!statSync(path).isFile()) {
+ let stats;
+ try {
+ stats = statSync(path);
+ } catch (error) {
+ fail(`${name} is not readable: ${path} (${error.code ?? error.message})`);
+ }
+ if (!stats.isFile()) {
fail(`${name} is not a file: ${path}`);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const [name, path] of [ | |
| ['portwing binary', options.portwingBin], | |
| ['mockdocker binary', options.mockDockerBin], | |
| ]) { | |
| if (!statSync(path).isFile()) { | |
| fail(`${name} is not a file: ${path}`); | |
| } | |
| } | |
| for (const [name, path] of [ | |
| ['portwing binary', options.portwingBin], | |
| ['mockdocker binary', options.mockDockerBin], | |
| ]) { | |
| let stats; | |
| try { | |
| stats = statSync(path); | |
| } catch (error) { | |
| fail(`${name} is not readable: ${path} (${error.code ?? error.message})`); | |
| } | |
| if (!stats.isFile()) { | |
| fail(`${name} is not a file: ${path}`); | |
| } | |
| } |
🤖 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 `@scripts/portwing-fleet-soak.mjs` around lines 128 - 135, Update the binary
validation loop in the portwing fleet soak setup to handle missing or invalid
paths without letting statSync throw before fail() runs. Ensure both portwing
binary and mockdocker binary checks report the intended “not a file” message
through fail(), while preserving the existing validation for valid paths.
| function rssBytes(pid) { | ||
| if (!pid) { | ||
| return 0; | ||
| } | ||
| const result = spawnSync('ps', ['-o', 'rss=', '-p', String(pid)], { encoding: 'utf8' }); | ||
| if (result.status !== 0) { | ||
| return 0; | ||
| } | ||
| const kibibytes = Number(result.stdout.trim()); | ||
| return Number.isFinite(kibibytes) ? kibibytes * 1024 : 0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ps per pid per tick: blocking sampler, and a silent 0 makes the RSS assertion vacuous.
Two problems: the 1s sampler does spawnSync('ps') once per agent (24 blocking spawns/second on the scheduled run), which perturbs the very controller heap it measures; and a non-zero ps status returns 0, so agentRss.baseline/final become 0 and aggregate agent RSS growth bounded passes with no data. Read /proc/<pid>/statm (same source style as threadCount) and let a read failure be visible.
🛠️ Proposed fix
-function rssBytes(pid) {
- if (!pid) {
- return 0;
- }
- const result = spawnSync('ps', ['-o', 'rss=', '-p', String(pid)], { encoding: 'utf8' });
- if (result.status !== 0) {
- return 0;
- }
- const kibibytes = Number(result.stdout.trim());
- return Number.isFinite(kibibytes) ? kibibytes * 1024 : 0;
-}
+const PAGE_SIZE = 4096;
+
+function rssBytes(pid) {
+ if (!pid) {
+ return 0;
+ }
+ // statm field 2 is resident set size in pages.
+ const pages = Number(readFileSync(`/proc/${pid}/statm`, 'utf8').split(' ')[1]);
+ if (!Number.isFinite(pages)) {
+ fail(`unable to read RSS for pid ${pid}`);
+ }
+ return pages * PAGE_SIZE;
+}spawnSync then becomes unused in the import at line 3 unless gitCommit keeps it (it does). Additionally, record a sanity assertion so a zeroed baseline can never pass silently:
recordAssertion(
'aggregate agent RSS growth bounded',
- evidence.resources.agentRss.growth <= options.rssGrowthThresholdBytes,
+ evidence.resources.agentRss.baseline > 0 &&
+ evidence.resources.agentRss.growth <= options.rssGrowthThresholdBytes,
`${evidence.resources.agentRss.growth} <= ${options.rssGrowthThresholdBytes} bytes`,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function rssBytes(pid) { | |
| if (!pid) { | |
| return 0; | |
| } | |
| const result = spawnSync('ps', ['-o', 'rss=', '-p', String(pid)], { encoding: 'utf8' }); | |
| if (result.status !== 0) { | |
| return 0; | |
| } | |
| const kibibytes = Number(result.stdout.trim()); | |
| return Number.isFinite(kibibytes) ? kibibytes * 1024 : 0; | |
| } | |
| const PAGE_SIZE = 4096; | |
| function rssBytes(pid) { | |
| if (!pid) { | |
| return 0; | |
| } | |
| // statm field 2 is resident set size in pages. | |
| const pages = Number(readFileSync(`/proc/${pid}/statm`, 'utf8').split(' ')[1]); | |
| if (!Number.isFinite(pages)) { | |
| fail(`unable to read RSS for pid ${pid}`); | |
| } | |
| return pages * PAGE_SIZE; | |
| } |
🤖 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 `@scripts/portwing-fleet-soak.mjs` around lines 174 - 184, Update rssBytes to
read RSS from /proc/<pid>/statm, matching the existing threadCount procfs
approach, instead of spawning a blocking ps process per PID. Make procfs read or
parse failures visible to callers rather than returning 0, and add a sanity
assertion around agentRss baseline/final values so missing RSS data cannot
satisfy the aggregate growth assertion; retain spawnSync if it is still used by
gitCommit.
| function activeChildren() { | ||
| return [...children.values()].filter((child) => child.exitCode === null); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Liveness check misses signal-terminated children.
Node leaves exitCode === null when a child dies from a signal (signalCode is set instead). An OOM-killed or crashed agent therefore stays in activeChildren(), is sampled as a live 0-RSS process, and in cleanup() (lines 582-587) the exit wait falls through to the 3s timeout. Same test at line 574 for mockDocker.
🛠️ Proposed fix
+function isRunning(child) {
+ return child.exitCode === null && child.signalCode === null;
+}
+
function activeChildren() {
- return [...children.values()].filter((child) => child.exitCode === null);
+ return [...children.values()].filter(isRunning);
}Then reuse isRunning at lines 574 and 582 instead of the inline exitCode comparisons.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function activeChildren() { | |
| return [...children.values()].filter((child) => child.exitCode === null); | |
| } | |
| function isRunning(child) { | |
| return child.exitCode === null && child.signalCode === null; | |
| } | |
| function activeChildren() { | |
| return [...children.values()].filter(isRunning); | |
| } |
🤖 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 `@scripts/portwing-fleet-soak.mjs` around lines 270 - 272, Update the child
liveness logic around activeChildren() to use the existing isRunning predicate,
which must treat both exitCode and signalCode termination as no longer running.
Reuse isRunning for the mockDocker check and cleanup exit-wait check instead of
inline exitCode comparisons, preserving current behavior for running children.
| const slowAdapter = slowAgent.edgeAdapter; | ||
| const serverSocket = slowAdapter?.ws?._socket; | ||
| if (!serverSocket?.pause || !serverSocket?.resume) { | ||
| fail('production EdgeAgentAdapter WebSocket does not expose a pausable socket'); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the internals the soak reaches into still exist on EdgeAgentAdapter.
fd -t f 'EdgeAgentAdapter.ts' --exec ast-grep outline {} --items all
rg -nP --type=ts '\b(execSessions|_socket)\b' -C2 -g '!**/*.test.ts'Repository: CodesWhat/drydock
Length of output: 2970
🏁 Script executed:
#!/bin/bash
set -u
echo "== target references =="
rg -n --type=ts --type=mjs '\bedgeAdapter\b|\.execSessions\b|\.ws\b|_socket\b|closeAdapterSessions\b' -C 2
echo
echo "== soak relevant sections =="
if [ -f scripts/portwing-fleet-soak.mjs ]; then
nl -ba scripts/portwing-fleet-soak.mjs | sed -n '180,225p;420,480p'
fi
echo
echo "== EdgeAgentAdapter relevant sections =="
nl -ba app/agent/EdgeAgentAdapter.ts | sed -n '1,180p;380,560p;930,1020p'Repository: CodesWhat/drydock
Length of output: 357
🏁 Script executed:
#!/bin/bash
set -u
echo "== target references =="
rg -n --glob '*.ts' --glob '*.mjs' '\bedgeAdapter\b|\.execSessions\b|\.ws\b|_socket\b|closeAdapterSessions\b' -C 2 || true
echo
echo "== soak relevant sections =="
awk 'NR>=180 && NR<=225 || NR>=420 && NR<=480 { printf "%s\t%s\n", NR, $0 }' scripts/portwing-fleet-soak.mjs || true
echo
echo "== EdgeAgentAdapter relevant sections =="
awk 'NR>=1 && NR<=180 || NR>=380 && NR<=560 || NR>=930 && NR<=1020 { printf "%s\t%s\n", NR, $0 }' app/agent/EdgeAgentAdapter.ts || trueRepository: CodesWhat/drydock
Length of output: 179
Guard closeAdapterSessions() against upstream API changes.
EdgeAgentAdapter still exposes ws and execSessions, but closeAdapterSessions() returns early when execSessions is missing. That allows the slow-consumer/exec-cleanup soaks to pass without exercising anything if that shape changes.
🛠️ Minimum guard for `closeAdapterSessions`
function closeAdapterSessions(adapter, sessionIds) {
const sessions = adapter?.execSessions;
if (!(sessions instanceof Map)) {
- return;
+ fail('EdgeAgentAdapter no longer exposes an execSessions Map');
}🤖 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 `@scripts/portwing-fleet-soak.mjs` around lines 434 - 438, Update
closeAdapterSessions() to fail explicitly when the EdgeAgentAdapter lacks
execSessions instead of returning early. Preserve the existing cleanup behavior
when execSessions is present, and use the script’s established fail() mechanism
so the soak cannot pass without exercising exec cleanup.
Standard release-identity prep for **v1.6.0-rc.9** (same-day follow-up to rc.8), replicating the rc.8 prep (#626) pattern exactly: - CHANGELOG: `[Unreleased]` → `## [1.6.0-rc.9] — 2026-07-28` (keeps #630's Portwing edge-streaming entries, adds the two #631 UI fixes under Fixed), fresh empty `[Unreleased]`, link chain updated - 17-file identity bump: README badge/highlights, demo mocks, site-config/site-content, API doc samples, quickstart tag matrix, updates highlights section, pinned identity test constants (RC=1.6.0-rc.9, PREV=1.6.0-rc.8) - Version files stay at base `1.6.0` per the RC ritual Verified: 133/133 scripts tests, non-empty `extract-changelog-entry` body for v1.6.0-rc.9, no stale rc.8 identity outside deliberate history. Full local pre-push gate green. rc.9 delta over rc.8 = #630 (Portwing edge streaming default-on) + #631 (row-overlay chip clip fix, registry-error tag-cell fix). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Changelog - ✨ Added v1.6.0-rc.9 highlights covering Portwing edge log streaming and two UI fixes. - 🔧 Changed release identity references across documentation, demos, site content, API samples, quickstart data, and tests from rc.8 to rc.9. - 🔧 Changed changelog compare-link chain and added a fresh `[Unreleased]` section. - 🔧 Retained version files at base version `1.6.0`. - ✅ Verified 133 script tests, changelog extraction, stale rc.8 checks, and the full pre-push gate. ## Concerns - No runtime code changes are included. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Wholesale-tree sync of `dev/v1.6` @ `3dc5f7b5` onto `main` ahead of the **v1.6.0-rc.9** cut. Delta over rc.8 (`bcbbf712`): - #630 — Portwing edge log streaming default-on (continuous WebSocket bridging, 1 MiB per-viewer caps, fleet-soak workflow) - #631 — UI: row-overlay chip unclipped at the pinned-column edge; registry errors no longer replace the tag in the tag column - #633 — rc.9 release-identity prep (CHANGELOG + 17-file bump) Head branch is a throwaway (`sync/main-20260728-rc9`) per the sync-PR pattern; the server-side commit's tree is exactly `dev/v1.6^{tree}` so `git diff origin/main origin/dev/v1.6` is empty after merge. Dispatch `release-cut.yml --ref main -f release_tag=v1.6.0-rc.9` once merged. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Changelog - ✨ Added continuous Portwing container-log streaming with cancellation, backpressure limits, legacy compatibility, and fleet-soak testing. - 🔧 Changed Portwing to enabled-by-default; `DD_EXPERIMENTAL_PORTWING=false` is now the emergency disable switch. - 🐛 Fixed pinned-column row-overlay clipping and registry errors overwriting container tags. - 🔧 Updated release identity and documentation from `v1.6.0-rc.8` to `v1.6.0-rc.9`. - ✨ Added release notes, API documentation, soak evidence reporting, and related test coverage. ## Concerns - Dispatch the release workflow with `release_tag=v1.6.0-rc.9` after merging. - Confirm the fleet-soak workflow succeeds and produces its evidence artifact. - Review Portwing memory, viewer-buffer, concurrency, and reconnect thresholds for production suitability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Summary
Validation
Depends on CodesWhat/portwing#71 (merged).
Changelog
DD_EXPERIMENTAL_PORTWING=falseas the emergency opt-out.Concerns