feat(cli): attach terminals through fleet nodes - #1484
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds authenticated ChangesFleet node terminal attachment
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LocalAgent
participant FleetNodeProxy
participant BrokerRuntime
participant Worker
User->>LocalAgent: run attach --node
LocalAgent->>FleetNodeProxy: startFleetNodeAttachProxy
FleetNodeProxy->>BrokerRuntime: create authenticated terminal session
BrokerRuntime->>Worker: request PTY snapshot
Worker-->>BrokerRuntime: return snapshot and stream output
BrokerRuntime-->>FleetNodeProxy: send terminal messages
FleetNodeProxy-->>LocalAgent: expose broker-compatible terminal traffic
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
crates/broker/src/runtime/worker_events.rs (1)
6-29: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAdd an early return for the common no-session case.
This function runs for every PTY chunk from every worker. Most brokers have no terminal session attached, yet each call still iterates the map and builds a
Vec. When a session does exist,chunkis cloned once per session even though the last send could take ownership.An
if terminal_sessions.is_empty() { return; }guard at the top removes the scan on the hot path at no cost.🤖 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 `@crates/broker/src/runtime/worker_events.rs` around lines 6 - 29, Add an early return at the start of publish_terminal_output when terminal_sessions is empty, before collecting session IDs, so the common no-session path avoids scanning and allocating while preserving existing behavior for attached sessions.crates/broker/src/terminal_control.rs (2)
218-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the wire round-trip test to the remaining message variants.
The test covers
terminal.opendecoding andterminal.outputencoding. The CLI also sendsterminal.input,terminal.resize, andterminal.close, and consumesterminal.ready,terminal.input_ack,terminal.error, andterminal.closed. A rename or field change in any of those variants would break the CLI without failing a test. Add assertions for each variant tag and for theskip_serializing_ifbehaviour on the optionaloffset,code, andmessagefields.🤖 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 `@crates/broker/src/terminal_control.rs` around lines 218 - 244, Extend terminal_wire_round_trips_without_control_frames to cover decoding or encoding for terminal.input, terminal.resize, terminal.close, terminal.ready, terminal.input_ack, terminal.error, and terminal.closed, asserting each expected wire tag. Also verify optional offset, code, and message fields are omitted when absent and included when present, preserving the existing terminal.open and terminal.output checks.
186-215: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSend a WebSocket close frame before shutdown.
When
TerminalControlCommand::Shutdownorcommand_rx.recv()returnsNone, sendMessage::Close(None)before returning.🤖 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 `@crates/broker/src/terminal_control.rs` around lines 186 - 215, Update the TerminalControlCommand::Shutdown and command_rx.recv() == None branch in the terminal control loop to send a WebSocket Message::Close(None) through sink before returning. Preserve the existing immediate return behavior after the close-frame send.packages/cli/src/cli/commands/local-agent.test.ts (1)
99-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining
--nodebranches.Two branches in the
--nodedispatch have no test:
- The broker-override rejection at
packages/cli/src/cli/commands/local-agent.tslines 553-564 (--nodecombined with--broker-url,--api-key, or--state-dir).- The failure path at lines 572-575, where
deps.attachNodethrows and the command must report the message and exit with 1.Both are small additions that follow the shape of the
--ssh-hostconflict test below.Note also that this test asserts
expect.objectContaining({ json: true })reachesattachNode, but the defaultattachNodeimplementation discards those flags. See the separate comment onlocal-agent.tslines 85-107.🤖 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 `@packages/cli/src/cli/commands/local-agent.test.ts` around lines 99 - 113, Add tests for the remaining --node dispatch branches in the local-agent command: verify broker-related options (--broker-url, --api-key, or --state-dir) are rejected with the same conflict behavior as the --ssh-host tests, and verify an attachNode failure reports its error message and exits with status 1. Extend the harness as needed to make deps.attachNode throw, while preserving the existing successful --node assertions.crates/broker/src/runtime/init.rs (1)
309-315: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a deeper terminal queue than the control queue.
The terminal channel uses the same depth (256) as
fleet_control. The two lanes have very different traffic shapes: node control carries occasional heartbeats and actions, while the terminal lane carries every PTY chunk for every attached session.publish_terminal_outputincrates/broker/src/runtime/worker_events.rsends the session whentry_sendfails, so a short output burst from a verbose agent can terminate a healthy attach.Raising the depth (or reading it from an env var next to
node_max_agents) keeps the bound while making a transient burst survivable.🤖 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 `@crates/broker/src/runtime/init.rs` around lines 309 - 315, Increase the bounded capacity of the terminal control and event channels created in the runtime initialization block, keeping the fleet/control queue at its existing depth. Choose a deeper fixed capacity or reuse an environment-configured value alongside node_max_agents, while preserving the existing publish_terminal_output behavior and bounded-queue semantics.
🤖 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 `@CHANGELOG.md`:
- Line 12: Update the trailing clause of the adjacent --ssh-host changelog
bullet to describe --node as supporting the implemented authenticated terminal
attach behavior, removing the contradictory statement that it remains reserved
for canonical fleet-native attach. Preserve the concise impact-first style and
the existing release-section structure.
In `@crates/broker/src/runtime/fleet.rs`:
- Around line 111-142: Remove the inserted terminal_sessions entry for
session_id when send_to_worker returns Err in the PTY snapshot path, before
emitting snapshot_failed. In event_loop.rs
(crates/broker/src/runtime/event_loop.rs:207-215), add a WorkerName cleanup
helper that removes matching terminal_sessions and terminal_snapshot_requests
entries, invoke it alongside resize_owners.remove and pty_observability.remove
in worker release and reap paths, and add deadline-based cleanup of
terminal_snapshot_requests in the reap_tick arm.
- Around line 166-176: Update the BASE64 decoding branch in the terminal input
handler to reject data_base64 whose encoded length exceeds the safe bound of 64
* 1024 * 4 / 3 + 4 before calling BASE64.decode. Preserve the existing
invalid_input terminal error and return behavior, then retain the decoded-length
check as a defense-in-depth validation.
In `@crates/broker/src/runtime/init.rs`:
- Around line 249-251: Ensure terminal URL derivation in the initialization flow
fails loudly when fleet_ws_url does not contain the expected control path.
Update the logic around terminal_ws_url to assert that the substitution
occurred, or reuse a relaycast node_terminal_ws_url helper if available, while
preserving the requirement that terminal traffic uses a distinct websocket
endpoint.
In `@crates/broker/src/runtime/worker_events.rs`:
- Around line 944-960: Update the terminal output handling around
publish_terminal_output to reuse the chunk and offset values already extracted
when constructing stream_event, rather than rereading payload fields. Only call
publish_terminal_output when the extracted chunk is non-empty, while preserving
the existing offset and terminal session arguments.
- Around line 860-885: Update the terminal snapshot parsing that constructs
TerminalToCloud::Ready so offset is optional and defaults to 0 when absent,
while keeping screen, rows, and cols required for a valid snapshot. Preserve the
existing malformed-response Error path when any mandatory field is missing or
invalid.
In `@crates/broker/src/terminal_control.rs`:
- Around line 151-159: Update the invalid token-header branch in the terminal
reconnect loop to multiply reconnect_delay before sleeping and continuing,
matching the backoff behavior of the other failure arms. Preserve the existing
warning and retry flow.
In `@packages/cli/src/cli/commands/local-agent.ts`:
- Around line 572-575: Update the error handling in the catch block of the
local-agent command to always emit a consistent “Error: ” prefix, including for
plain Error instances, while avoiding a duplicated prefix when the message
already begins with it. Preserve the existing non-Error string conversion and
exit behavior, and keep the change scoped to the deps.error call.
In `@packages/cli/src/cli/lib/attach-fleet-node.ts`:
- Around line 283-288: The output replay loop must not silently discard events
when the socket closes or exceeds MAX_BUFFERED_BYTES. Update the outputHistory
handling around workerStreamEvent to retain the unsent remainder, or emit a
pty_input_error with an explicit truncation code before clearing it, so users
are informed when replay output is incomplete.
- Around line 167-168: Authenticate the loopback server created in
attach-fleet-node.ts by minting a random token at startup, passing it to attach
clients through brokerUrl/apiKey, and rejecting requests that lack the token
before handling HTTP or WebSocket routes. Apply this to the request flow around
createServer and the /api/input/<agent>/stream endpoint, while preserving the
broker’s existing view-mode input rejection.
- Around line 66-86: Update readBody to settle its promise when the
IncomingMessage emits either 'error' or 'aborted', resolving with the same
empty-object fallback used for invalid JSON. Preserve the existing data
accumulation and end-handler parsing behavior while ensuring await
readBody(request) cannot remain pending after a request failure.
- Around line 169-188: Update the snapshot wait logic in the GET `/snapshot`
handler to retain the timeout handle and clear it whenever `ready` settles,
including when it wins the `Promise.race`; preserve the existing timeout error
response and successful snapshot response behavior.
- Around line 385-402: Update the terminal websocket reconnect flow around the
socket error/close handlers and terminal.ready processing to track
reconnectAttempts, apply exponential backoff capped at 30 seconds, and stop
reconnecting after a bounded maximum. When a resumed connection fails, surface
the failure instead of ignoring it, and reject the attach with a
FleetNodeAttachError using node_unreachable once the cap is reached. Reset
reconnectAttempts to zero when a terminal.ready frame is received, while
preserving the existing reconnect guards and stopped-socket checks.
---
Nitpick comments:
In `@crates/broker/src/runtime/init.rs`:
- Around line 309-315: Increase the bounded capacity of the terminal control and
event channels created in the runtime initialization block, keeping the
fleet/control queue at its existing depth. Choose a deeper fixed capacity or
reuse an environment-configured value alongside node_max_agents, while
preserving the existing publish_terminal_output behavior and bounded-queue
semantics.
In `@crates/broker/src/runtime/worker_events.rs`:
- Around line 6-29: Add an early return at the start of publish_terminal_output
when terminal_sessions is empty, before collecting session IDs, so the common
no-session path avoids scanning and allocating while preserving existing
behavior for attached sessions.
In `@crates/broker/src/terminal_control.rs`:
- Around line 218-244: Extend terminal_wire_round_trips_without_control_frames
to cover decoding or encoding for terminal.input, terminal.resize,
terminal.close, terminal.ready, terminal.input_ack, terminal.error, and
terminal.closed, asserting each expected wire tag. Also verify optional offset,
code, and message fields are omitted when absent and included when present,
preserving the existing terminal.open and terminal.output checks.
- Around line 186-215: Update the TerminalControlCommand::Shutdown and
command_rx.recv() == None branch in the terminal control loop to send a
WebSocket Message::Close(None) through sink before returning. Preserve the
existing immediate return behavior after the close-frame send.
In `@packages/cli/src/cli/commands/local-agent.test.ts`:
- Around line 99-113: Add tests for the remaining --node dispatch branches in
the local-agent command: verify broker-related options (--broker-url, --api-key,
or --state-dir) are rejected with the same conflict behavior as the --ssh-host
tests, and verify an attachNode failure reports its error message and exits with
status 1. Extend the harness as needed to make deps.attachNode throw, while
preserving the existing successful --node assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3650f50e-e0a3-416e-b229-9964b9b812ab
📒 Files selected for processing (11)
CHANGELOG.mdcrates/broker/src/lib.rscrates/broker/src/runtime/event_loop.rscrates/broker/src/runtime/fleet.rscrates/broker/src/runtime/init.rscrates/broker/src/runtime/mod.rscrates/broker/src/runtime/worker_events.rscrates/broker/src/terminal_control.rspackages/cli/src/cli/commands/local-agent.test.tspackages/cli/src/cli/commands/local-agent.tspackages/cli/src/cli/lib/attach-fleet-node.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Independent code review (workflow Correctness (fix before merge):
Cleanup (confirmed, lower severity, at your discretion): Requesting fixes for #1-5 before this goes to Khaliq for the required review. #6-10 at your discretion given time. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/broker/src/runtime/event_loop.rs`:
- Around line 280-285: Update TerminalSession readiness to track the active
connection generation, and reset or invalidate it when fleet.rs handles
disconnect/reconnect rather than preserving ready=true. On each new connection,
replay a Ready snapshot for every retained session before releasing queued
Output, and discard or gate commands belonging to the prior connection. Add a
disconnect/reconnect test asserting Ready precedes all resumed output.
In `@packages/cli/src/cli/commands/local-agent.test.ts`:
- Line 146: Update the test around the error mock assertion to explicitly verify
that error was called exactly once, while retaining the existing
toHaveBeenCalledWith check for the expected message.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6666821d-37e1-4a14-af55-d18a266c543c
📒 Files selected for processing (11)
CHANGELOG.mdcrates/broker/src/runtime/api.rscrates/broker/src/runtime/event_loop.rscrates/broker/src/runtime/fleet.rscrates/broker/src/runtime/init.rscrates/broker/src/runtime/maintenance.rscrates/broker/src/runtime/worker_events.rscrates/broker/src/terminal_control.rspackages/cli/src/cli/commands/local-agent.test.tspackages/cli/src/cli/commands/local-agent.tspackages/cli/src/cli/lib/attach-fleet-node.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.md
- crates/broker/src/runtime/init.rs
- crates/broker/src/terminal_control.rs
- packages/cli/src/cli/commands/local-agent.ts
- crates/broker/src/runtime/fleet.rs
- packages/cli/src/cli/lib/attach-fleet-node.ts
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/cli/src/cli/lib/attach-fleet-node.ts (4)
452-458: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFully stop remote transport during shutdown.
close()closesremoteonly when its state isOPEN. If shutdown occurs while the initial or resumed socket isCONNECTING, that socket can establish after the proxy has closed.The reconnect timer is also not stored or cleared. Shutdown during backoff therefore keeps the timer active and can delay process exit. Store and clear the reconnect timer. Terminate any non-closed remote socket during
close().Also applies to: 466-480
🤖 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 `@packages/cli/src/cli/lib/attach-fleet-node.ts` around lines 452 - 458, Update the reconnect logic in connect and close so the reconnect timeout is stored and cleared during shutdown, preventing delayed reconnect attempts. Ensure close() terminates remote whenever it is not already CLOSED, including CONNECTING sockets, and prevent pending reconnect callbacks from initiating a new connection after shutdown.
154-164: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake terminal readiness connection-scoped.
readyresolves only once. After a reconnect,/snapshotcan return the oldsnapshotbefore the new WebSocket sends itsterminal.readyframe. Existing/wsclients also do not receive the new ANSI snapshot because theterminal.readyhandler only updates local state.The resize route has the same gap. It checks only
remote.readyState, butcrates/broker/src/runtime/fleet.rsrejects resize requests untilsession.readyis true at Lines [275-281]. The proxy can therefore return HTTP 200 withapplied: truewhile the broker rejects the resize.Create readiness state per connection generation. Gate snapshot and resize requests on the current generation. Resynchronize existing event sockets when the new snapshot arrives.
Also applies to: 253-274, 394-410
🤖 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 `@packages/cli/src/cli/lib/attach-fleet-node.ts` around lines 154 - 164, Replace the single-use readiness promise around ready, resolveReady, rejectReady, and readySettled with readiness state tied to each WebSocket connection generation. Update snapshot and resize handlers to await the current generation’s terminal-ready state, and report resize success only after the broker accepts the request. In the terminal.ready handling and existing /ws event-socket flow, publish the newly received ANSI snapshot so reconnecting clients are resynchronized instead of retaining stale data.
288-298: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRetain output when no event socket accepts it.
The code checks
eventSockets.sizebefore callingbroadcast. If the set contains a closed or backpressured socket,broadcastsends to no client, but the current chunk is not added tooutputHistory. The next/wsconnection cannot replay that chunk.Make
broadcastreport whether an event socket accepted the frame. Retain or explicitly report the chunk when no event socket accepts it.Also applies to: 411-423
🤖 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 `@packages/cli/src/cli/lib/attach-fleet-node.ts` around lines 288 - 298, Update broadcast in attach-fleet-node.ts to return whether at least one open, non-backpressured event socket successfully accepts the encoded frame, while preserving removal of unusable sockets. Use that result at both broadcast call sites (including the later occurrence) to retain the current chunk in outputHistory or explicitly report it when no socket accepts it, rather than relying only on eventSockets.size.
437-443: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnd the local session when the broker sends
terminal.closed.This branch reports only to
inputSockets. A client connected only to/wsreceives no terminal-closed signal. The reconnect handler then treats the terminal closure as a transient transport failure and retries the resume URL.Reject initial readiness when necessary. Notify or close
eventSockets. Suppress reconnect attempts for a terminal closure. Use a terminal-specific error instead of eventually reportingnode_unreachable.🤖 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 `@packages/cli/src/cli/lib/attach-fleet-node.ts` around lines 437 - 443, Update the terminal.closed branch in the attach-fleet-node session flow to terminate the local session, not only broadcast pty_input_error through inputSockets. Reject initial readiness when closure occurs, notify or close eventSockets for /ws clients, and mark the closure as terminal-specific so the reconnect handler suppresses resume attempts and does not report node_unreachable.
🤖 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.
Outside diff comments:
In `@packages/cli/src/cli/lib/attach-fleet-node.ts`:
- Around line 452-458: Update the reconnect logic in connect and close so the
reconnect timeout is stored and cleared during shutdown, preventing delayed
reconnect attempts. Ensure close() terminates remote whenever it is not already
CLOSED, including CONNECTING sockets, and prevent pending reconnect callbacks
from initiating a new connection after shutdown.
- Around line 154-164: Replace the single-use readiness promise around ready,
resolveReady, rejectReady, and readySettled with readiness state tied to each
WebSocket connection generation. Update snapshot and resize handlers to await
the current generation’s terminal-ready state, and report resize success only
after the broker accepts the request. In the terminal.ready handling and
existing /ws event-socket flow, publish the newly received ANSI snapshot so
reconnecting clients are resynchronized instead of retaining stale data.
- Around line 288-298: Update broadcast in attach-fleet-node.ts to return
whether at least one open, non-backpressured event socket successfully accepts
the encoded frame, while preserving removal of unusable sockets. Use that result
at both broadcast call sites (including the later occurrence) to retain the
current chunk in outputHistory or explicitly report it when no socket accepts
it, rather than relying only on eventSockets.size.
- Around line 437-443: Update the terminal.closed branch in the
attach-fleet-node session flow to terminate the local session, not only
broadcast pty_input_error through inputSockets. Reject initial readiness when
closure occurs, notify or close eventSockets for /ws clients, and mark the
closure as terminal-specific so the reconnect handler suppresses resume attempts
and does not report node_unreachable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d906a686-2069-44b7-a729-b1952564089b
📒 Files selected for processing (5)
crates/broker/src/runtime/event_loop.rscrates/broker/src/runtime/fleet.rscrates/broker/src/runtime/worker_events.rspackages/cli/src/cli/commands/local-agent.tspackages/cli/src/cli/lib/attach-fleet-node.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/broker/src/runtime/worker_events.rs
- crates/broker/src/runtime/event_loop.rs
- packages/cli/src/cli/commands/local-agent.ts
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/broker/src/runtime/fleet.rs">
<violation number="1" location="crates/broker/src/runtime/fleet.rs:160">
P1: A terminal write timeout can corrupt the shared worker command stream instead of failing only this attach: cancellation may leave a partial JSON frame, and the next worker command is then consumed as an invalid line. Avoid cancelling a partially written framed stdin operation—serialize it behind a dedicated writer and reset/terminate the worker when a write cannot complete.</violation>
<violation number="2" location="crates/broker/src/runtime/fleet.rs:263">
P2: Near the terminal queue reserve, a failed snapshot/input/resize reports two `terminal.closed` frames: `send_terminal` queues an `output_backpressure` close when the error is rejected, then `fail_terminal_session` queues another close with the real code. Make failure reporting emit at most one close and preserve the original failure reason.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // from worker_events to the terminal lane, so this never | ||
| // stalls heartbeat/action processing behind a PTY snapshot. | ||
| let request_id = format!("terminal_snapshot_{}", Uuid::new_v4().simple()); | ||
| match tokio::time::timeout( |
There was a problem hiding this comment.
P1: A terminal write timeout can corrupt the shared worker command stream instead of failing only this attach: cancellation may leave a partial JSON frame, and the next worker command is then consumed as an invalid line. Avoid cancelling a partially written framed stdin operation—serialize it behind a dedicated writer and reset/terminate the worker when a write cannot complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/runtime/fleet.rs, line 160:
<comment>A terminal write timeout can corrupt the shared worker command stream instead of failing only this attach: cancellation may leave a partial JSON frame, and the next worker command is then consumed as an invalid line. Avoid cancelling a partially written framed stdin operation—serialize it behind a dedicated writer and reset/terminate the worker when a write cannot complete.</comment>
<file context>
@@ -135,17 +157,18 @@ impl BrokerRuntime {
- match self
- .workers
- .send_to_worker(
+ match tokio::time::timeout(
+ TERMINAL_WORKER_WRITE_TIMEOUT,
+ self.workers.send_to_worker(
</file context>
| .count() | ||
| >= TERMINAL_INPUT_MAX_IN_FLIGHT_PER_SESSION | ||
| { | ||
| self.send_terminal(TerminalToCloud::Error { |
There was a problem hiding this comment.
P2: Near the terminal queue reserve, a failed snapshot/input/resize reports two terminal.closed frames: send_terminal queues an output_backpressure close when the error is rejected, then fail_terminal_session queues another close with the real code. Make failure reporting emit at most one close and preserve the original failure reason.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/runtime/fleet.rs, line 263:
<comment>Near the terminal queue reserve, a failed snapshot/input/resize reports two `terminal.closed` frames: `send_terminal` queues an `output_backpressure` close when the error is rejected, then `fail_terminal_session` queues another close with the real code. Make failure reporting emit at most one close and preserve the original failure reason.</comment>
<file context>
@@ -224,18 +253,33 @@ impl BrokerRuntime {
+ .count()
+ >= TERMINAL_INPUT_MAX_IN_FLIGHT_PER_SESSION
+ {
+ self.send_terminal(TerminalToCloud::Error {
+ session_id,
+ code: "input_backpressure".into(),
</file context>
3 participants
Closes #1449
Depends on cloud #2995.
Implements the Relay half of canonical --node terminal attach:
Checks: cargo fmt --check; cargo check -p agent-relay-broker; cargo test -p agent-relay-broker terminal_wire_round_trips_without_control_frames --lib. CLI focused Vitest launched: packages/cli/src/cli/commands/local-agent.test.ts.