fix(minimald): surface daemon RPC errors to client and detect upload connection drops - #919
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR replaces stream-based SSH channel I/O with russh message draining, requests subsystem replies, propagates daemon errors through extended data stream 1, and requires explicit EOF or close signals for successful upload completion. ChangesSSH channel error propagation and completion handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
0chroma
force-pushed
the
0chroma/fix-upload-rpc-errors-901
branch
from
July 22, 2026 20:25
fcd22b3 to
ccf1170
Compare
…connection drops Two defects in the SSH channel handling left the client with opaque failures or, worse, false success when the daemon-side operation failed. Defect 1: `upload_workspace_files` treated `channel.wait()` returning `None` as success, but `None` also means the SSH connection dropped mid-unpack. Now tracks `Eof`/`Close` messages to distinguish a clean post-unpack close from a connection loss, bailing on the latter. Defect 2: `handle_channel` on the daemon returned early on handler errors without writing anything, so the client saw an opaque "EOF while parsing a value" instead of the actual error. Now funnels all error paths (JSON parse, handler, serialization) through a single match that writes the error to SSH extended data (stream 1) before closing the channel. The client's `oneshot_rpc` is rewritten to use `channel.wait()` instead of `into_stream()`, collecting data and extended-data separately so daemon-side errors are legible. Also changes `request_subsystem(false, ...)` to `true` so unknown subsystems surface as a Failure instead of the client writing into a channel nobody serves. Closes #901.
…uild
The prior commit added a second `use tokio::io::{AsyncReadExt, AsyncWriteExt}`
that duplicated imports already on the line above, raising E0252 and
failing every build-dependent CI lane (tests, e2e, build-linux, macOS
artifacts) plus a rustfmt diff. Those traits are no longer used at module
scope after `handle_channel` was rewritten to call `Channel` methods
directly, so drop the redundant line and trim the surviving import to the
`AsyncRead`/`ReadBuf` it still needs. Also collapse the `data_bytes(...).await`
call onto one line for `cargo fmt --check`.
0chroma
force-pushed
the
0chroma/fix-upload-rpc-errors-901
branch
from
July 24, 2026 15:59
f276224 to
ef3070a
Compare
0chroma
marked this pull request as ready for review
July 24, 2026 16:48
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minimal/src/client.rs (1)
499-521: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame refused-subsystem hang applies here.
stream_uploadrequests the subsystem withwant_reply = true(Line 483) but the drain loop has noChannelMsg::Failurearm. If the daemon refuses the subsystem, the failure reply is ignored, the channel stays open, andwait()blocks indefinitely instead of surfacing a clear error. Mirror the handling indownload_diag_bundle(Lines 593-597).🔒️ Proposed fix
while let Some(msg) = channel.wait().await { match msg { russh::ChannelMsg::ExtendedData { data, ext: 1 } => { append_daemon_error(&mut err, &data); } russh::ChannelMsg::Eof | russh::ChannelMsg::Close => { saw_close = true; } + russh::ChannelMsg::Failure => anyhow::bail!( + "daemon refused the {subsystem} subsystem (likely a CLI/daemon version skew)" + ), _ => {} } }🤖 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/minimal/src/client.rs` around lines 499 - 521, Update the stream_upload channel-draining match to handle russh::ChannelMsg::Failure, mirroring download_diag_bundle: surface a clear subsystem-refusal error and terminate the wait loop instead of ignoring the failure and blocking indefinitely. Preserve the existing ExtendedData, Eof, Close, and post-loop error handling.
🧹 Nitpick comments (1)
crates/minimald/src/rpc.rs (1)
46-89: 🧹 Nitpick | 🔵 TrivialReminder: exercise the daemon RPC path with integration coverage.
This changes the daemon-side oneshot RPC channel contract; please run the applicable integration harness rather than relying on the module unit tests alone.
As per coding guidelines: "When changing VM or daemon paths, run the relevant integration coverage:
just e2eand/orjust test-vm" and "Do not rely only on unit tests for VM/networking behavior."🤖 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/minimald/src/rpc.rs` around lines 46 - 89, Exercise the daemon RPC path after updating handle_channel, using the applicable integration coverage rather than only module unit tests. Run just e2e and/or just test-vm as appropriate, and verify the oneshot RPC success and error paths remain functional.Source: Coding guidelines
🤖 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/minimal/src/client.rs`:
- Around line 240-268: Update the response-draining loop in oneshot_rpc to
explicitly handle russh::ChannelMsg::Failure and return an error immediately
when the subsystem request is refused. Preserve collection of Data and
ExtendedData messages, but do not let the fallback arm swallow Failure; ensure
the error identifies the requested subsystem using R::NAME.
---
Outside diff comments:
In `@crates/minimal/src/client.rs`:
- Around line 499-521: Update the stream_upload channel-draining match to handle
russh::ChannelMsg::Failure, mirroring download_diag_bundle: surface a clear
subsystem-refusal error and terminate the wait loop instead of ignoring the
failure and blocking indefinitely. Preserve the existing ExtendedData, Eof,
Close, and post-loop error handling.
---
Nitpick comments:
In `@crates/minimald/src/rpc.rs`:
- Around line 46-89: Exercise the daemon RPC path after updating handle_channel,
using the applicable integration coverage rather than only module unit tests.
Run just e2e and/or just test-vm as appropriate, and verify the oneshot RPC
success and error paths remain functional.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c34b77e6-8f95-4d02-8581-4163acb27959
📒 Files selected for processing (3)
crates/minimal/src/client.rscrates/minimald/src/rpc.rscrates/minimald/src/test_harness.rs
Comment on lines
+240
to
+268
| // want_reply = true so an unknown subsystem (CLI/daemon version | ||
| // skew) surfaces as a Failure instead of the client writing into a | ||
| // channel nobody serves (#901). | ||
| channel | ||
| .request_subsystem(false, R::NAME) | ||
| .request_subsystem(true, R::NAME) | ||
| .await | ||
| .with_context(|| format!("request subsystem {}", R::NAME))?; | ||
|
|
||
| let body = serde_json::to_vec(&request).context("serialize request")?; | ||
| channel.data_bytes(body).await.context("write request")?; | ||
| channel.eof().await.context("shutdown write half")?; | ||
|
|
||
| let mut rpc = channel.into_stream(); | ||
| rpc.write_all(&body).await.context("write request")?; | ||
| rpc.shutdown().await.context("shutdown write half")?; | ||
|
|
||
| // Drain the channel with wait() rather than into_stream() so that | ||
| // extended-data (stream 1) — where the daemon writes handler errors | ||
| // (#901) — is visible instead of silently discarded by the stream's | ||
| // AsyncRead impl. | ||
| let mut resp_buf = Vec::with_capacity(256); | ||
| rpc.read_to_end(&mut resp_buf) | ||
| .await | ||
| .context("read response")?; | ||
| let mut err_buf = Vec::new(); | ||
| while let Some(msg) = channel.wait().await { | ||
| match msg { | ||
| russh::ChannelMsg::Data { data } => { | ||
| resp_buf.extend_from_slice(&data); | ||
| } | ||
| russh::ChannelMsg::ExtendedData { data, ext: 1 } => { | ||
| err_buf.extend_from_slice(&data); | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle ChannelMsg::Failure — the loop currently ignores a refused subsystem and can hang.
The comment at Lines 240-242 states want_reply = true so an unknown subsystem "surfaces as a Failure instead of the client writing into a channel nobody serves," but the drain loop's _ => {} arm swallows ChannelMsg::Failure. Since request_subsystem does not consume its reply (it arrives via wait(), as open_exec_channel and download_diag_bundle both demonstrate), and a bare refusal does not close the channel (see the note at Lines 588-593), wait() will block indefinitely on a CLI/daemon version skew instead of surfacing the failure. oneshot_rpc has no surrounding timeout, so this is an unbounded hang on exactly the path this PR aims to fix.
🔒️ Proposed fix: bail on subsystem refusal
while let Some(msg) = channel.wait().await {
match msg {
russh::ChannelMsg::Data { data } => {
resp_buf.extend_from_slice(&data);
}
russh::ChannelMsg::ExtendedData { data, ext: 1 } => {
err_buf.extend_from_slice(&data);
}
+ russh::ChannelMsg::Failure => anyhow::bail!(
+ "daemon refused the {} subsystem (likely a CLI/daemon version skew)",
+ R::NAME
+ ),
_ => {}
}
}📝 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.
Suggested change
| // want_reply = true so an unknown subsystem (CLI/daemon version | |
| // skew) surfaces as a Failure instead of the client writing into a | |
| // channel nobody serves (#901). | |
| channel | |
| .request_subsystem(false, R::NAME) | |
| .request_subsystem(true, R::NAME) | |
| .await | |
| .with_context(|| format!("request subsystem {}", R::NAME))?; | |
| let body = serde_json::to_vec(&request).context("serialize request")?; | |
| channel.data_bytes(body).await.context("write request")?; | |
| channel.eof().await.context("shutdown write half")?; | |
| let mut rpc = channel.into_stream(); | |
| rpc.write_all(&body).await.context("write request")?; | |
| rpc.shutdown().await.context("shutdown write half")?; | |
| // Drain the channel with wait() rather than into_stream() so that | |
| // extended-data (stream 1) — where the daemon writes handler errors | |
| // (#901) — is visible instead of silently discarded by the stream's | |
| // AsyncRead impl. | |
| let mut resp_buf = Vec::with_capacity(256); | |
| rpc.read_to_end(&mut resp_buf) | |
| .await | |
| .context("read response")?; | |
| let mut err_buf = Vec::new(); | |
| while let Some(msg) = channel.wait().await { | |
| match msg { | |
| russh::ChannelMsg::Data { data } => { | |
| resp_buf.extend_from_slice(&data); | |
| } | |
| russh::ChannelMsg::ExtendedData { data, ext: 1 } => { | |
| err_buf.extend_from_slice(&data); | |
| } | |
| _ => {} | |
| } | |
| } | |
| // want_reply = true so an unknown subsystem (CLI/daemon version | |
| // skew) surfaces as a Failure instead of the client writing into a | |
| // channel nobody serves (`#901`). | |
| channel | |
| .request_subsystem(true, R::NAME) | |
| .await | |
| .with_context(|| format!("request subsystem {}", R::NAME))?; | |
| let body = serde_json::to_vec(&request).context("serialize request")?; | |
| channel.data_bytes(body).await.context("write request")?; | |
| channel.eof().await.context("shutdown write half")?; | |
| // Drain the channel with wait() rather than into_stream() so that | |
| // extended-data (stream 1) — where the daemon writes handler errors | |
| // (`#901`) — is visible instead of silently discarded by the stream's | |
| // AsyncRead impl. | |
| let mut resp_buf = Vec::with_capacity(256); | |
| let mut err_buf = Vec::new(); | |
| while let Some(msg) = channel.wait().await { | |
| match msg { | |
| russh::ChannelMsg::Data { data } => { | |
| resp_buf.extend_from_slice(&data); | |
| } | |
| russh::ChannelMsg::ExtendedData { data, ext: 1 } => { | |
| err_buf.extend_from_slice(&data); | |
| } | |
| russh::ChannelMsg::Failure => anyhow::bail!( | |
| "daemon refused the {} subsystem (likely a CLI/daemon version skew)", | |
| R::NAME | |
| ), | |
| _ => {} | |
| } | |
| } |
🤖 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/minimal/src/client.rs` around lines 240 - 268, Update the
response-draining loop in oneshot_rpc to explicitly handle
russh::ChannelMsg::Failure and return an error immediately when the subsystem
request is refused. Preserve collection of Data and ExtendedData messages, but
do not let the fallback arm swallow Failure; ensure the error identifies the
requested subsystem using R::NAME.
twitchyliquid64
approved these changes
Jul 24, 2026
norrietaylor
added a commit
that referenced
this pull request
Jul 29, 2026
…C transport (#1011) * fix(minimal): gate `min stop` on the daemon being down, not on the RPC transport `min stop` succeeds when the daemon is stopped, so judge it by that rather than by the liveness of the transport that carried the request. On a VM target minimald IS the guest's pid-1: an accepted Shutdown tears the VM — and with it the SSH transport — down as a direct consequence of succeeding. The client drains the channel and only then decodes what it collected, so a reply lost on the way out decodes an empty buffer and fails with "decode response for shutdown". Since #919 stopped swallowing RPC and connection errors that surfaces as a non-zero exit for a stop that did exactly what was asked — the assertion the nightly session-e2e-soak trips. A failed Shutdown RPC no longer bails on the spot. It falls through to the same wait an accepted shutdown runs, then re-asks the liveness probe the command opened with: a native minimald has no lifecycle state to poll, so its wait returns at once saying nothing, and only the probe can answer for that backend. A daemon confirmed down is a successful stop; one still there returns the original RPC error, context intact. Only that failure arm probes. An accepted shutdown is still judged by its wait alone, because the daemon acknowledges before it has finished going down and asking again there would race its own teardown. SessionsLive keeps its message and its non-zero exit, and the already-down fast path is untouched. The e2e's stop assertion now keeps stderr instead of discarding it: it threw away the one piece of text that says why a stop failed, which is why this needed a hypothesis rather than a log. Closes gominimal/inbox#363 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(minimal): address review feedback Surface the RPC error the recovered stop suppresses, and make the observation behind that recovery testable. - The failed-RPC-but-daemon-down arm exited 0 silently, discarding the error that explains what went wrong. Print it on stderr, which is what the e2e script now captures and what scripted callers' >/dev/null leaves intact. - Extract the liveness observation into daemon_confirmed_stopped so the real probe -- not a stub -- is exercised by a test, and document that it is a VM-backend recovery in practice: minimald holds its listener through SHUTDOWN_GRACE, so the native probe reports "running". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in the SSH channel handling left the client with opaque failures or, worse, false success when the daemon-side operation failed.
Defect 1 — Upload reports success when connection drops mid-unpack
upload_workspace_filesdrained the channel withwhile let Some(msg) = channel.wait().await.channel.wait()returnsNonefor two reasons: (a) daemon closed after successful unpack, (b) SSH connection dropped. Only (a) means success. In case (b), the error buffer was empty and the function returnedOk(()), causingcmd_activateto proceed with an empty workspace.Fix: Track
ChannelMsg::Eof/ChannelMsg::Close— russh only delivers those on a clean close. If neither appeared, bail with a connection-dropped error.Defect 2 — Daemon RPC errors never reach the client
handle_channelreturned early on handler errors, writing nothing to the channel. Theserve_*wrappers logged to the daemon's own stderr. The client'sread_to_endgot 0 bytes, surfacing as an opaque "EOF while parsing a value."Fix (server): All error paths (JSON parse, handler, serialization) are funneled through a single match that writes the error to SSH extended data (stream 1) before closing — same mechanism
serve_stream_workspace_filesalready uses.Fix (client):
oneshot_rpcrewritten to usechannel.wait()instead ofinto_stream()(whoseAsyncReadsilently discardsExtendedData), collecting data and extended-data separately. If extended data is present, bail with that error.request_subsystem(false, ...)changed totrueso unknown subsystems surface as a Failure.Closes #901.
Note
Surface daemon RPC errors to client and detect upload connection drops in minimald
Client.oneshot_rpcnow reads SSH channel messages viawait()to capture extended-data (stream 1); if the daemon writes an error there, the client returns it as an error rather than seeing an opaque EOF.Client.uploadnow tracks whether an explicitEoforClosewas received and fails with "upload stream ended unexpectedly" if the connection drops mid-transfer, preventing false success on mid-unpack disconnects.ServeOneshot.handle_channelin rpc.rs now writes handler/decode errors as human-readable text on SSH extended-data stream 1 before closing the channel, instead of silently closing.oneshot_rpc_surfaces_handler_errors_as_extended_dataasserts that invalid JSON sent to a subsystem produces an error on extended-data with no response bytes.Macroscope summarized ef3070a.