Repository navigation
fix(cli): backpressure sandbox uploads - #4448
Conversation
Stream archive uploads over a bounded SSH subsystem whose credit follows extraction stdin progress. Preserve the existing input budget and tar destination semantics, validate completion, and clean up cancelled transfers. Add protocol, lifecycle, and large-upload regression coverage without changing russh. Signed-off-by: Shiju <shiju@nvidia.com>
Keep cancellation active through upload setup and retry delays, and terminate cancelled SSH probes. Preserve protocol failures through output completion and report captured diagnostics on early extraction failure. Remove redundant upload validation and extend the affected regressions. Signed-off-by: Shiju <shiju@nvidia.com>
|
Label |
PR Review StatusThe initial review found no blocking findings in the credited upload protocol, CLI cancellation and failure handling, supervisor integration, or regression coverage. The published upload docs explain the upgrade requirement and partial destinations after interruption. @shiju-nv, I checked your reproduction and application-layer credit explanation on #4430 against the patch: the upload window follows boundary stdin consumption while the existing input guard remains in place. Blocking findings: None. Gator metadata
|
johntmyers
added
gator:watch-pipeline
Replace the framed upload protocol with SSH's own streams. The client sends one JSON plan line, the raw archive and EOF. The supervisor's stdout carries only cumulative totals of bytes written to extraction stdin, extraction output goes to stderr, and the exit status reports the result. Extraction starts inline through the ordinary exec path. This removes the frame codec, SHA-256 digests, the CLI sha2 dependency and the supervisor upload state machine while keeping the 1 MiB credit window, the 4 MiB input guard, cancellation and error reporting. A failed archive now stops SSH before closing stdin, so EOF never marks a truncated archive complete. Signed-off-by: Shiju <shiju@nvidia.com>
PR Review StatusThe follow-up review found no blocking regressions in the change from custom upload frames to credited SSH streams. I checked bounded plan parsing, consumption-based credit, extraction output isolation, final byte totals and exit status, cancellation, and the updated regression coverage. Blocking findings: None. Required Branch Checks, Helm Lint, Trivy Changes, and DCO pass on this head. The required E2E suite is running; Gator will inspect its results next cycle. Gator metadata
|
johntmyers
added
gator:watch-pipeline
- Apply the forward multiplexing overrides to the upload ssh command, so a user ControlMaster cannot carry an upload over another session's master connection. - Kill ssh before closing its stdin when a transfer fails locally, and include ssh stderr in transport errors. - Share ACK_INTERVAL_BYTES from core. Assert at compile time that CLI chunks fit the credit left after an ack and that the window stays below the supervisor's pending-input guard. - Log the real reason when the supervisor rejects an upload. - Test that a single-packet upload reports its final total. Signed-off-by: Shiju <shiju@nvidia.com>
BlockedThe latest head has merge conflicts with Action required: @shiju-nv, resolve the merge conflicts and push an updated head. Please also inspect the Protobuf Compatibility job and address any PR-caused failure. Gator metadata
|
…kpressure Signed-off-by: Shiju <shiju@nvidia.com> # Conflicts: # crates/openshell-cli/src/ssh.rs
johntmyers
added
gator:blocked
Signed-off-by: Shiju <shiju@nvidia.com>
PR Review StatusThe follow-up review found no blocking regressions in the upload SSH connection ownership, cancellation cleanup, shared credit cadence, or merge integration. The added test checks that uploads open the subsystem without a TTY. @shiju-nv, I checked the updated head: the merge conflicts are resolved and Protobuf Compatibility now passes. Branch Checks, Helm Lint, Trivy Changes, and DCO also pass; the required E2E suite is running. Blocking findings: None. Gator metadata
|
johntmyers
added
gator:watch-pipeline
johntmyers
added
the
gator:approval-needed
johntmyers
added
gator:merge-ready
and removed
gator:approval-needed
Monitoring CompleteMonitoring is complete because this PR has merged. The final Gator state was merge-ready, and the latest review reported no blocking findings. I removed the active Gator metadata
|
2 participants
Summary
Large
sandbox uploadtransfers can outrun tar extraction and terminate with exit status 74 when queued stdin reaches 4 MiB. Stream built-in uploads with consumption credit so slow extraction makes the CLI wait while memory remains bounded.This revision replaces the earlier framed protocol with SSH's own streams. It removes the frame codec, SHA-256 digests, the CLI
sha2dependency and the supervisor upload state machine, about 39% of the production code, while keeping the same credit window and failure behavior.Related Issue
Closes #4430.
Changes
openshell-upload-v1SSH subsystem. The CLI sends one JSON plan line, the raw tar stream and EOF. The supervisor's stdout carries only cumulative totals of bytes written to extraction stdin, and the CLI keeps at most 1 MiB beyond the last total outstanding. Extraction output arrives as stderr and the SSH exit status is the result; success requires exit status 0 and a final total equal to the bytes sent. No russh change is required.ForkAfterAuthentication, as forwards do, so the ssh exit status reports extraction. Like forwards, this needs OpenSSH 8.7 or later.Testing
Formatting, scoped Clippy with warnings denied, and the focused
openshell-core,openshell-cliandopenshell-supervisor-processlibrary tests pass on the merged head (82 CLI, 2 core, 65 supervisor). Mutations that remove CLI credit gating, the supervisor credit check or the final total, route extractor stdout into the credit stream, or send EOF after a failed archive each fail a focused test.Hosted CI passed on
3aeda041a, including the Podmanupload_backpressureE2E (large_upload_preserves_content_and_metadata_outside_workdir). Later commits change only ssh arguments, compile-time checks, diagnostics and tests; CI for the current head covers them.Not covered by tests: the caller-side order that kills ssh before stdin closes, a rejection after the result is reported, and the supervisor's 5 s drain timeout.
Checklist