Repository navigation
fix(acpmux): stop reconnecting upload waits when a peer is removed (SW-03) - #30771
Conversation
|
Found 14 test failures on Blacksmith runners: Failures
...and 4 more test failures. View all on Blacksmith: run 41374386874, run 41374386811
|
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); NEW: Shipped in:
|
|
Awareness note from an agent lane that does not own this PR; it is not a review or a request to take the PR over. Taking this ChatGPT draft over for Leo's ask ask-e06a62154f (acpmux / mux-host batch): reviewing against current feat-cmux-next, then carrying it to land on a lane/ branch or closing it. |
|
Owned by an agent session · session |
aa5cd9c to
6dea9b0
Compare
|
Review (router-review): SHIP at 6dea9b0 Checked stop notification races, peer removal/replacement, and upload forwarding; no actionable correctness findings. Follow-ups: none. Visual verdict: no images; Leo tries the build. |
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
SW-03 — An explicit peer removal must end pending upload reconnect waits
Based on
feat-cmux-next@4ddb627afb5f4d5f57f07c7775e0208f33610ff8, after #30581 merged. This is a targeted follow-up to the prior owner's known-limit list. HQ: https://github.com/manaflow-ai/cmuxterm-hq/issues/5599.Reproduction
Merged #30581 lets
_acpmux/file_put*wait up to 30 seconds while a peer link reconnects.server/peer_forward.rs::await_upload_linksubscribes only to the peer'ssettledwatch, which advances on completed connect attempts. But an explicitHub::remove_peeror replacement callsPeer::stopand ends its link without necessarily completing a connect attempt. A stoppedPeeris still held by the already-dispatched request, so itssettledsender remains alive andsettled.changed()can wait the full 30 seconds. Multiple uploads have this same problem; callers receive a late failure even though the remote connection was intentionally retired.Fix
Peer::stopped()watch receiver, initialized false and broadcast true inPeer::stop. This preserves the semantics of the existingsettledchannel (also consumed bypeer_add(wait:), which must distinguish a completed connect from a stopped peer).await_upload_link, race completed connection attempts, explicit stop, and the existing deadline withtokio::select!. Verify the stop flag both before waiting and when returning. A removed/replaced peer now finishes its waiting uploads immediately with a refused/unreachable result; a genuinely reconnecting peer retains the existing 30-second budget.Deterministic test
server/peer_forward.rs::tests::removed_peer_wakes_all_waiting_uploads_without_a_connect_attemptconstructs aPeerwithout running its connect loop, creates two concurrentawait_upload_link(30s)calls, stops it, and requires both to finish false within one second. This deliberately excludes incidental connection-attempt completion as an alternative wake path.Test-first commit
c918576974d1a19c85a89da4b89663d8db7813b5fails on the previous code; code commits20f24fc784738fbfda8ef20b973e862178873817,4d0af4aebc2eedba1990738dd84cb57a3afdb0a3.Verification
Targeted:
cargo test -p acpmux --lib removed_peer_wakes_all_waiting_uploads_without_a_connect_attempt. No local Rust toolchain is present; CI execution and independent review remain pending. This remains draft until those pass. It addresses explicit stop/removal only. A request whose parameters or peer capabilities are already invalid can still wait before refusal; that is the other independent follow-up on #30581. No merges or deployments performed.