Repository navigation
fix(minimald): bound wedged transport writes with an inactivity deadline - #2303
Merged
norrietaylor merged 3 commits intoOct 18, 2026
Conversation
russh bounds every session-loop transport write only by the inactivity timer, and minimald set it to None, so a write that never completed parked the session loop forever. Keepalives are dispatched from that same loop, so they stopped firing too: an attach whose transport wedged went dark in both directions and survived every keepalive budget. Arm the timer at the full keepalive budget instead. A healthy idle attach answers each keepalive and resets the clock, so it is never reaped; a wedged one is torn down one interval past the budget. Add a mechanism test that wedges the write side of an otherwise-live russh session and proves the session future now resolves with InactivityTimeout instead of parking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Warning Review limit reached
This review includes 3 billable files and costs up to $0.75. Or wait 34 minutes for your next included review. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly and concisely describes the main change: adding an inactivity deadline to bound wedged transport writes. It follows the required Conventional Commit format. Description check The description thoroughly explains the defect, fix, behavior, and verification results. It includes equivalent Summary and Testing content, although it does not include the template's Checklist headi… Full details: Description check
-
Explanation
The description thoroughly explains the defect, fix, behavior, and verification results. It includes equivalent Summary and Testing content, although it does not include the template's Checklist heading. This omission is non-critical because the description addresses documentation updates and does not identify a breaking change.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the keepalive beat
A stalled write waits, then meets its fate
The server ends the quiet wait
The session returns its timeout
I hop along, the test is green!
Comment @coderabbitai help to get the list of available commands.
…deadline Share the wedgeable-transport setup between the wedge test and a new healthy-idle test that answers keepalives for several inactivity windows, widen the test deadlines so KEX and auth cannot trip them on a loaded runner, and assert the production russh config arms INACTIVITY_TIMEOUT.
Member
|
/review |
Contributor
Member
|
/review |
norrietaylor
approved these changes
Oct 18, 2026
Member
There was a problem hiding this comment.
Approved on ARCH PASS at d3aace9.
norrietaylor
deleted the
inbox-patch/fix/wedged-transport-write-inactivity-deadline-ec4a433782b5e15f
branch
October 18, 2026 07:37
norrietaylor
pushed a commit
that referenced
this pull request
Nov 4, 2026
…ine (#2303) russh bounds every session-loop transport write only by the inactivity timer, and minimald set it to None, so a write that never completed parked the session loop forever. Keepalives are dispatched from that same loop, so they stopped firing too: an attach whose transport wedged went dark in both directions and survived every keepalive budget. The timer is now armed one interval past the keepalive budget, so a healthy idle attach is never reaped and a wedged one is torn down. Backport of #2303. Adapted to the line: the test module gains the small accept-all client handler main's connection tests already had. (cherry picked from commit 02f3af9)
Merged
norrietaylor
pushed a commit
that referenced
this pull request
Nov 5
8000
, 2026
…ine (#2303) russh bounds every session-loop transport write only by the inactivity timer, and minimald set it to None, so a write that never completed parked the session loop forever. Keepalives are dispatched from that same loop, so they stopped firing too: an attach whose transport wedged went dark in both directions and survived every keepalive budget. The timer is now armed one interval past the keepalive budget, so a healthy idle attach is never reaped and a wedged one is torn down. Backport of #2303. Adapted to the line: the test module gains the small accept-all client handler main's connection tests already had. (cherry picked from commit 02f3af9)
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
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.
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABQrDSRg
The defect
An established SSH attach over vsock could go dark in both directions and stay that way far past every keepalive budget. The mechanism: russh bounds every session-loop transport write only by the inactivity timer, and
minimaldsetinactivity_timeout: None. A transport write that never completed (a wedged vsock peer, a full buffer nobody drains) therefore parked the session loop forever. Keepalives are dispatched from that same loop, so they stopped firing too — the connection was dead in both directions yet outlived the entire keepalive budget, because the only deadline that could reclaim it was disarmed.The fix
Arm the inactivity timer instead of disabling it — at the full keepalive budget (interval × (max + 1) = 2h at the current 30min/3 settings):
inactivity_timeout: Some(INACTIVITY_TIMEOUT)inbuild_russh_config, replacingNone, with the rationale rewritten to state both halves: why it must not reap a healthy idle attach, and whyNoneis not safe.russh::Error::InactivityTimeout, which propagates throughConnectionError::Protocol, tearing the connection down one interval past the budget.diag.rsthat assertedinactivity_timeout: Noneas a design invariant are updated to the new truth.The SSH-id handshake phase is unaffected: it completes eagerly inside
from_stream, which is already bounded separately byHANDSHAKE_DEADLINE(30s).Verification
Gate commands, all with
--manifest-path target/Cargo.toml:cargo fmt --all -- --check— pass.cargo clippy --workspace --all-targets --locked -- -D warnings— pass.cargo test --locked -p minimald --lib a_wedged— the new mechanism test passes:connection::tests::a_wedged_transport_write_ends_the_session_on_the_inactivity_deadline ... ok. The test handshakes a real russh client against the server through a flippable write-wedged transport, then proves the session future resolves withConnectionError::Protocol(russh::Error::InactivityTimeout)on the inactivity deadline — while 8000 under the oldinactivity_timeout: Nonethe same future never resolves and the outer timeout fires instead (regression guard).keepalive_maxis set far above the inactivity budget in the test so the resolution is by the inactivity deadline, not by exhausting keepalives.net::answererflake that rotates between runs also reproduces identically on the pristine baseline. Net effect of this change: +1 passing test, nothing broken.CI lanes that cover this change:
native-daemon-e2e(ci-linux-native),test-kvm(ci-linux-kvm), andmacos e2e(ci-macos) all trigger oncrates/minimald/**and run live-session e2e for minimald, which this sandbox cannot host.Full diff audit performed: 3 files, 162 insertions, 10 deletions — every hunk traces to the defect (the config change + its comment, the two stale-comment truth updates, the one new mechanism test).
Summary by CodeRabbit