fix(minimal): add idle-progress timeout to upload to prevent indefinite hang - #920
Conversation
📝 WalkthroughWalkthroughUpload streaming now detects stalled pipe reads, channel writes, finalization, and daemon-unpack drains using idle timeouts. Regression tests cover stalled writers, stalled builders, byte counting, and VCS-root behavior. ChangesUpload stall protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TarBuilder
participant UploadStream
participant SSHChannel
participant Daemon
TarBuilder->>UploadStream: produce tar+zstd bytes
UploadStream->>SSHChannel: read and write with idle timeouts
SSHChannel->>Daemon: transmit upload stream
Daemon-->>SSHChannel: send unpack errors or close
UploadStream->>SSHChannel: drain completion messages with timeout
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
2aca13a to
9516b31
Compare
f622604 to
9313608
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/minimal/src/file_upload.rs (1)
592-595: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood regression coverage for the write-stall path; the read-stall path is untestable as currently designed.
StallingWriter/stream_tar_zstd_bails_when_writer_stallscorrectly exercises the channel-write-stall branch under paused time. Note that a symmetrical test for the "no data from tar builder" branch (abuildclosure that never writes totx) would currently hang the test runner rather than pass — understart_paused = true, the read-timeout timer fires andcopy_futresolves, butbuild_futis left pending with no timer of its own, sotokio::join!(and the paused-clock auto-advance, which needs a pending timer to jump to) can't make further progress. That's independent confirmation of thejoin!issue flagged above rather than a gap in this specific test file — please don't add such a test until the underlying deadlock is addressed, since it would hang CI rather than fail cleanly.Also applies to: 884-993, 1005-1006
🤖 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/file_upload.rs` around lines 592 - 595, Fix the underlying deadlock in the stream_tar_zstd flow rather than adding a read-stall test. Update the coordination of build_fut and copy_fut so a read-timeout from copy_fut also cancels or otherwise terminates a build closure that never writes to tx, allowing the operation to resolve without join! waiting indefinitely. Preserve the existing writer-stall behavior and stream_tar_zstd_bails_when_writer_stalls 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 `@crates/minimal/src/file_upload.rs`:
- Around line 144-146: Update both stall error messages in the upload path’s
bail! calls to use Rust’s backslash-newline continuation instead of an escaped
\n followed by indentation. Preserve the existing message text while ensuring
the rendered error is a single clean line without leading whitespace before the
parenthetical.
- Around line 162-163: Wrap the trailing w.flush() and w.shutdown() calls in the
same IDLE_TIMEOUT-bounded operation used for reads and writes inside the upload
loop. Preserve their existing error contexts while ensuring both finalization
steps cannot hang indefinitely when the peer stops accepting data.
- Around line 59-63: Update stream_via_pipe so a timeout or error from rx.read
can return without waiting for build_fut to complete. Make the tar-building
future independently cancellable, abortable, or spawn it as a task that is
explicitly stopped when the read side exits, while preserving cleanup of the
pipe and existing error propagation.
---
Nitpick comments:
In `@crates/minimal/src/file_upload.rs`:
- Around line 592-595: Fix the underlying deadlock in the stream_tar_zstd flow
rather than adding a read-stall test. Update the coordination of build_fut and
copy_fut so a read-timeout from copy_fut also cancels or otherwise terminates a
build closure that never writes to tx, allowing the operation to resolve without
join! waiting indefinitely. Preserve the existing writer-stall behavior and
stream_tar_zstd_bails_when_writer_stalls coverage.
🪄 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: 21da20a2-db40-4073-b142-439c998256cd
📒 Files selected for processing (2)
crates/minimal/src/client.rscrates/minimal/src/file_upload.rs
| let mut buf = [0u8; 64 * 1024]; | ||
| loop { | ||
| match tokio::time::timeout(IDLE_TIMEOUT, rx.read(&mut buf)).await { | ||
| Ok(Ok(0)) => break, |
There was a problem hiding this comment.
is this how EOF comes through? I always thought it was an error of type EOF, but i googled it and yeah its a 0 byte read. Hmmm.
There was a problem hiding this comment.
yeahhh, it's expected behavior so it comes through via an Ok(...) Unexpected EOF would be io::ErrorKind::UnexpectedEof. As an aside I also kinda hate how much I'm doing these nested result types, it feels very cursed.
| // stall on either the pipe read (tar builder stuck) or the | ||
| // channel write (peer gone, window frozen) surfaces as an | ||
| // error instead of an indefinite hang (#886). | ||
| let mut buf = [0u8; 64 * 1024]; |
There was a problem hiding this comment.
We can turn this down to 4k or 8k, as the BufWriter will only emit 4k blocks anyway.
…te hang When the SSH connection drops mid-upload, russh's `ChannelTx` can block on window availability indefinitely — the peer is gone and no window adjustment ever arrives. The `tokio::io::copy` call in `stream_tar_zstd` had no deadline, so the client hung forever showing "Uploading project files..." with no way to detect the dead peer. Replace `tokio::io::copy` with a manual copy loop that bounds both the pipe read (tar builder stalled) and the channel write (peer gone) with a 30s idle-progress timeout. On timeout, the tar builder task is aborted rather than left detached. A 120s idle backstop is also added to the post-upload channel drain in `upload_workspace_files`. Closes #886.
…ustfmt
The stall-detection logic works — the "upload stalled" message is the
source of the anyhow error, not the top-level context. The test asserted
on `to_string()` which only renders the outer context ("workspace upload
failed after N bytes"), so it never saw "stalled" and failed. Use
`format!("{:#}", err)` to render the full chain. Also apply rustfmt to
the `return Err(...)` expression that `fmt --check` flagged.
9313608 to
c87fb24
Compare
Switch stream_via_pipe to try_join! so a read-timeout cancels the build future (the old join! could deadlock when the builder stalled and never produced bytes). TarZstArchive's Drop already mem::forgets the inner async_tar::Builder, so cancellation is panic-safe. Also guard the trailing flush/shutdown with the idle timeout, fix stray literal \n in the stall error messages, shrink the read buffer to match BufWriter's 4 KiB blocks, and add a read-stall regression test that the try_join! switch makes possible.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/minimal/src/file_upload.rs (1)
104-166: 🩺 Stability & Availability | 🔵 TrivialConsider integration coverage for the stall/timeout behavior, not just mocked unit tests.
All new regression coverage (
StallingWriter,stream_via_pipe_bails_when_builder_stalls, etc.) exercises the timeout logic against in-process mock readers/writers only — there's no exercise of the real SSH channel/daemon path this fix targets (#886's "receive window is full" scenario).As per coding guidelines, "Do not rely only on unit tests for VM/networking behavior; preserve and run the applicable integration and root-integration harnesses" and "When changing VM or daemon paths, run the relevant integration coverage:
just e2eand/orjust test-vm."Also applies to: 879-956
🤖 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/file_upload.rs` around lines 104 - 166, Add integration coverage for the upload stall timeout through the real SSH channel/daemon path, including a scenario where the receive window is full or the peer stops consuming data. Extend the existing upload integration harness around the stream upload flow, rather than relying only on StallingWriter and stream_via_pipe_bails_when_builder_stalls unit tests, and ensure the applicable just e2e or just test-vm coverage exercises this regression.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.
Nitpick comments:
In `@crates/minimal/src/file_upload.rs`:
- Around line 104-166: Add integration coverage for the upload stall timeout
through the real SSH channel/daemon path, including a scenario where the receive
window is full or the peer stops consuming data. Extend the existing upload
integration harness around the stream upload flow, rather than relying only on
StallingWriter and stream_via_pipe_bails_when_builder_stalls unit tests, and
ensure the applicable just e2e or just test-vm coverage exercises this
regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14b12998-2cce-4c78-a0da-f713e0369d4f
📒 Files selected for processing (1)
crates/minimal/src/file_upload.rs
When the SSH connection drops mid-upload, russh's
ChannelTxcan block on window availability indefinitely — the peer is gone and no window adjustment ever arrives. Thetokio::io::copycall instream_tar_zstdhad no deadline, so the client hung forever showing "Uploading project files..." with no way to detect the dead peer.Root cause
stream_tar_zstdusedtokio::io::copyto pump data from the tar builder pipe to the SSH channel writer.tokio::io::copyhas no deadline. russh'sChannelTxblocks on window availability when the SSH receive window is full. If the peer dies without sending a window adjustment (e.g. a vsock reset mid-transfer), the writer parks with no waker and the copy never returns.The post-upload drain loop in
upload_workspace_files(while let Some(msg) = channel.wait().await) also had no timeout — a wedged transport that never delivers aClosewould hang there too.Fix
stream_tar_zstd: Replacedtokio::io::copywith a manual copy loop. Each direction (pipe read + channel write) gets a 30s idle-progress timeout. On timeout, the tar builder task is aborted rather than left detached.upload_workspace_files: Added a 120s idle-progress backstop on thechannel.wait()drain loop — generous enough for a legitimate large unpack, but catches a wedged transport.Closes #886.
Note
Add idle-progress timeouts to upload to prevent indefinite hangs
IDLE_TIMEOUTto the pipe-through loop infile_upload.rs, replacingtokio::io::copywith a manual read/write loop where eachrx.readandw.write_allis individually wrapped intokio::time::timeout.DRAIN_IDLE_TIMEOUTto the post-upload channel drain loop inclient.rs, replacing an unboundedchannel.wait()loop with one that times out if no message arrives.CountingReaderadapter to verify read byte counts.Changes since #920 opened
stream_via_pipefunction to prevent indefinite hangs during upload [65c9764]stream_via_pipe_bails_when_builder_stallsto validate timeout behavior [65c9764]stream_via_pipemanual copy loop [65c9764]stream_via_pipefunction [65c9764]Macroscope summarized c87fb24.
Summary by CodeRabbit