Answer the sender's closing handshake before dropping the socket

docs/implementation-checklist.md

@@ -1,7 +1,7 @@
 # Implementation checklist
 
 Status: **active**
-Current work: **none started — [relay teardown](plans/relay-teardown-drain-plan-2026-08-19.md) is next**
+Current work: **[receiver confirmation](plans/receiver-confirmation-plan-2026-08-19.md) is next**
 Last updated: **2026-08-19**
 
 The tactical view of what is being built and what state it is in. The detailed
@@ -30,20 +30,19 @@ of that order — is in
 ## 1. Relay teardown reset
 
 Plan: [`relay-teardown-drain-plan-2026-08-19.md`](plans/relay-teardown-drain-plan-2026-08-19.md)
-Status: **proposed**
+Status: **done**
 
 A sender intermittently sees `Connection reset by peer` after a transfer that
 actually succeeded. The upload receive task stops reading the socket when the
 sender completes, so the peer's closing handshake reply lands unread and the
 socket is dropped with data queued, producing RST instead of FIN.
 
-- [ ] Phase 1 — keep the receive task alive through teardown
-- [ ] Phase 2 — bound the drain with a named deadline
-- [ ] Phase 3 — check the receiver socket for the same shape
-- [ ] Phase 4 — repeated-run evidence
+- [x] Phase 1 — keep the receive task alive through teardown
+- [x] Phase 2 — bound the drain, in two stages rather than one
+- [x] Phase 3 — check the receiver socket for the same shape
+- [x] Phase 4 — repeated-run evidence
 
-Gate: at least 50 consecutive small-payload CLI transfers with zero resets,
-command and counts recorded in the plan.
+Gate met: 80 runs clean, against 3 failures in 30 on unfixed `main`.
 
 ## 2. Receiver preview and confirmation
 
@@ -96,6 +95,13 @@ Recorded so they are not rediscovered as new ideas. None are committed work.
 - **A direct peer-to-peer data channel** with relay fallback. Larger than
   encryption, needs signaling and a TURN dependency, and does not remove the
   relay path because TURN-blocked networks fall back to it.
+- **The download socket's teardown**, which carries the same shape the upload
+  socket had: its receive task stops reading before the send task writes a
+  `Close`. It produces no user-visible symptom, because the receiving client
+  discards the result of its own close, so it is latent rather than harmless.
+  Fixing it needs a way to show the change worked, which the upload side had
+  and this side does not. See the Phase 3 finding in
+  [`plans/relay-teardown-drain-plan-2026-08-19.md`](plans/relay-teardown-drain-plan-2026-08-19.md).
 - **Prometheus text** from `/metrics`, which currently returns a JSON snapshot.
 - **A first transfer shakeout run** using
   [`validation/transfer-shakeout-template.md`](validation/transfer-shakeout-template.md).

docs/plans/README.md

@@ -36,10 +36,6 @@ None. No implementation work has started.
 
 ## Proposed
 
-- [`relay-teardown-drain-plan-2026-08-19.md`](relay-teardown-drain-plan-2026-08-19.md)
-  — fix the intermittent `Connection reset by peer` a sender sees after a
-  successful transfer. Pre-existing defect, deliberately deferred out of PR #30.
-  Smallest of the three and a prerequisite in practice.
 - [`receiver-confirmation-plan-2026-08-19.md`](receiver-confirmation-plan-2026-08-19.md)
   — show the receiver what it is about to accept and require a y/n before any
   bytes move. Adds a protocol handshake and a new terminal outcome.
@@ -49,15 +45,22 @@ None. No implementation work has started.
 
 ## Done
 
-None yet.
+- [`relay-teardown-drain-plan-2026-08-19.md`](relay-teardown-drain-plan-2026-08-19.md)
+  — completed 2026-08-19: the upload receive task now stays alive through
+  teardown and answers the sender's closing handshake, so a socket is no longer
+  dropped with the peer's reply unread. 80 runs clean against 3 failures in 30
+  before. Two findings recorded rather than fixed: the assertions #30 removed
+  stay out because they guard nothing under per-transfer relay isolation, and
+  the download socket carries the same latent shape without a user-visible
+  symptom.
 
 ## Suggested order (dependencies, not law)
 
-Teardown first. It is a real defect, it needs no protocol change, and the
-confirmation feature adds a new terminal close path — decline — that would
-inherit the same reset bug on the day it ships.
+Teardown first — **done**. It was a real defect, it needed no protocol change,
+and the confirmation feature adds a new terminal close path, decline, that
+would have inherited the same reset bug on the day it shipped.
 
-Confirmation second. It is reviewable without crypto, and it settles what the
+Confirmation next. It is reviewable without crypto, and it settles what the
 receiver sees and when.
 
 Encryption last. It relocates the very metadata fields the confirmation prompt

docs/plans/relay-teardown-drain-plan-2026-08-19.md

@@ -1,8 +1,9 @@
 # Relay teardown drain plan
 
-Status: **proposed**
+Status: **done**
 Created: **2026-08-19**
 Last updated: **2026-08-19**
+Completed: **2026-08-19**
 
 ## Goal
 
@@ -84,39 +85,103 @@ at step 1 — long before the Close frame in step 2 is even sent.
 
 ### Phase 1 — Keep the receive task alive
 
-- [ ] Replace the `break` after `awaiting_receiver`
+- [x] Replace the `break` after `awaiting_receiver`
       ([`upload_ws.rs:451`](../../src/routes/upload_ws.rs#L451)) with a
       transition into a drain state, so `ws_receiver` is not dropped.
-- [ ] In the drain state, keep polling the stream and discard what arrives,
+- [x] In the drain state, keep polling the stream and discard what arrives,
       exiting on the peer's Close frame, on stream end, or on error.
-- [ ] Confirm no other `break` on a success path drops the stream early.
+- [x] Confirm no other `break` on a success path drops the stream early.
+
+Implemented as a `sender_completed` flag plus a drain block after the loop,
+rather than as a state inside it. The loop's many exit paths all mean "stop
+relaying"; only the clean completion also means "stay and finish the
+handshake", so the distinction reads better outside the loop.
 
 ### Phase 2 — Bound the drain
 
-- [ ] Add a named drain-deadline constant to
+- [x] Add a named drain-deadline constant to
       [`config.rs`](../../src/config.rs) with a comment stating what it bounds.
       A second or two is enough for a close handshake on a live connection.
-- [ ] Apply the deadline to the drain loop, so a peer that never replies is
+- [x] Apply the deadline to the drain loop, so a peer that never replies is
       abandoned rather than waited on.
-- [ ] Confirm the existing idle timeout does not fire during a normal drain and
+- [x] Confirm the existing idle timeout does not fire during a normal drain and
       convert a clean finish into a spurious failure.
 
+**Deviation from the plan.** A single short deadline does not work. The plan
+assumed the drain only had to cover a close handshake, but the send task cannot
+write `transfer_complete` until the receiver has finished writing the file,
+which is unbounded work. The wait is therefore two stages: first wait for the
+send task to exit, detected through the event receiver being dropped and
+bounded by the existing idle timeout, then apply
+`WS_CLOSE_DRAIN_TIMEOUT_SECS` to the peer's reply. Both stages are bounded, so
+the property the plan wanted still holds.
+
 ### Phase 3 — Check the receiver side
 
-- [ ] Read the success path in
+- [x] Read the success path in
       [`download_ws.rs`](../../src/routes/download_ws.rs) for the same shape.
-      Initial reading suggests it is less exposed — it does not send a Close on
-      success and its receive task keeps reading — but confirm rather than
-      assume.
-- [ ] If the same defect exists there, fix it in this change; if not, note in
+- [x] If the same defect exists there, fix it in this change; if not, note in
       the plan why it does not apply.
 
+**Finding: the same shape exists, and it is deferred.** The initial reading in
+this plan was wrong. On `ReceiverMessage::Complete` the download receive task
+notifies the sender, completes the session, and breaks, dropping its half of
+the socket; the send task then observes its event channel close and writes a
+`Close`. The receiving client has already sent a `Close` of its own by then, so
+it lands unread exactly as on the upload socket.
+
+It is not user-visible today because the receiving client discards the result
+of its close and returns success immediately, so the reset never reaches a
+person. That makes it latent rather than benign.
+
+It is deliberately not fixed here. The defect produces no observable symptom on
+that socket, so a change to its teardown could not be backed by the kind of
+before-and-after evidence this plan required for the upload side — which is the
+same reason #30 declined to fix the upload side inside a test-stability change.
+It is recorded as its own item in
+[`../implementation-checklist.md`](../implementation-checklist.md).
+
 ### Phase 4 — Evidence
 
-- [ ] Run repeated small-payload CLI transfers and record the reset count.
-- [ ] Attempt to restore the completion-handshake assertions PR #30 removed
+- [x] Run repeated small-payload CLI transfers and record the reset count.
+- [x] Attempt to restore the completion-handshake assertions PR #30 removed
       from the two small-payload tests, or record why they stay out.
-- [ ] Run the full validation command set.
+- [x] Run the full validation command set.
+
+**The defect does not reproduce through separate CLI processes.** A first
+harness that drove `drop send` and `drop recv` as separate processes measured
+zero resets on unfixed `main` across 50 runs. The sender exits the moment it
+reads `transfer_complete`, so the RST arrives at a process that no longer
+touches the socket. Running that harness only against the fix would have
+produced a clean result that meant nothing. It reproduces in the in-process
+harness with two transfers sharing one relay, which is the shape the tests had
+before #30.
+
+Measured with the pre-#30 test file restored:
+
+| Build | Runs | Failures |
+| --- | --- | --- |
+| `main`, unfixed | 30 | 3 |
+| this change | 30 | 0 |
+| this change | 80 | 0 |
+
+The failure text on `main` was
+`transfer should succeed: "IO error: Connection reset by peer (os error 104)"`,
+matching the roughly one-in-six rate #30 reported.
+
+**The removed assertions stay out, for a measured reason.** Restoring them
+while keeping #30's per-transfer relay isolation produced zero failures on
+unfixed `main` as well, across 30 runs: the isolation removes the contention
+that triggers the race, so the assertions would guard nothing. The shape that
+does catch it detects at roughly ten percent per run, which is too weak for CI
+and would reintroduce the flakiness #30 removed. A stronger guard should be a
+targeted test that drives the close handshake directly rather than a
+probabilistic end-to-end race.
+
+Note for any future evidence run: `SESSION_CREATION_LIMIT_PER_MINUTE` is 10 and
+lives in process memory, so a long unbroken run against one relay starts
+failing with a rate-limit error rather than anything meaningful. Restart the
+relay every few transfers.
 
 ## Risks
 
@@ -159,12 +224,13 @@ npm --prefix web audit --audit-level=high
 
 ## Acceptance criteria
 
-- [ ] At least 50 consecutive small-payload CLI transfers with zero resets,
-      with the command and counts recorded.
-- [ ] No new protocol message or status.
-- [ ] The drain has a bounded deadline expressed as a named constant.
-- [ ] The receiver socket has been checked and the finding recorded.
-- [ ] Full validation set passes.
+- [x] At least 50 consecutive runs with zero resets: 80 runs of the in-process
+      harness, against 3 failures in 30 on unfixed `main`.
+- [x] No new protocol message or status.
+- [x] The drain has a bounded deadline expressed as a named constant.
+- [x] The receiver socket has been checked and the finding recorded.
+- [x] Full validation set passes: fmt, clippy with warnings as errors, and 56
+      tests.
 
 ## Kickoff prompt
 
@@ -180,8 +246,10 @@ this plan before opening a pull request.
 
 ## Open questions
 
-- How long should the drain deadline be? A second or two should cover a close
-  handshake on a live connection, but confirm against a slow or lossy link
-  rather than picking a number by feel.
-- Should a drain that hits its deadline be logged at warn or debug? It is
-  benign on a healthy relay but a useful signal if it becomes common.
+- ~~How long should the drain deadline be?~~ Two seconds, applied only to the
+  peer's reply once the send task has already finished. Still worth confirming
+  against a slow or lossy link.
+- ~~Should a drain that hits its deadline be logged at warn or debug?~~ Debug,
+  on the grounds that it is benign on a healthy relay. Revisit if it turns out
+  to be common enough to be worth surfacing.
+- The download socket carries the same latent shape. See the Phase 3 finding.

src/config.rs

@@ -23,6 +23,18 @@ pub const WS_IDLE_TIMEOUT_SECS: u64 = 45;
 pub const SHUTDOWN_DRAIN_DELAY_SECS: u64 = 10;
 pub const SHUTDOWN_MAX_TRANSFER_WAIT_SECS: u64 = 3_500;
 
+/// How long the upload socket keeps reading after the relay has written its
+/// closing frame, waiting for the peer to answer it.
+///
+/// A WebSocket close is a two-way handshake. If the relay stops reading as soon
+/// as it has written its own `Close`, the peer's reply lands in a socket nobody
+/// is draining, and closing a socket with unread data queued sends RST instead
+/// of FIN — which the sender reports as `Connection reset by peer` after a
+/// transfer that actually succeeded. This bounds how long that courtesy lasts,
+/// so a peer that never answers cannot hold the task and its per-IP connection
+/// slot open.
+pub const WS_CLOSE_DRAIN_TIMEOUT_SECS: u64 = 2;
+
 /// The chunk size clients should send. Larger chunks mean fewer WebSocket
 /// frames, fewer wakeups, and fewer control messages per transferred byte, so
 /// a transfer is no longer bottlenecked on per-chunk overhead.

src/routes/upload_ws.rs

@@ -11,14 +11,14 @@ use axum::{
 use futures_util::{SinkExt, StreamExt};
 use tokio::sync::mpsc;
 use tokio::time::{Duration, Instant, interval, timeout};
-use tracing::{Instrument, info, warn};
+use tracing::{Instrument, debug, info, warn};
 
 use crate::{
     app_state::AppState,
     config::{
         MAX_UPLOAD_SIZE_BYTES, MAX_UPLOAD_SIZE_LABEL, PROGRESS_INTERVAL_MS,
-        SENDER_EVENT_CHANNEL_CAPACITY, WS_HEARTBEAT_INTERVAL_SECS, WS_IDLE_TIMEOUT_SECS,
-        WS_MAX_MESSAGE_BYTES, client_ip_from_request,
+        SENDER_EVENT_CHANNEL_CAPACITY, WS_CLOSE_DRAIN_TIMEOUT_SECS, WS_HEARTBEAT_INTERVAL_SECS,
+        WS_IDLE_TIMEOUT_SECS, WS_MAX_MESSAGE_BYTES, client_ip_from_request,
     },
     domain::{
         messages::SenderMessage,
@@ -259,6 +259,9 @@ async fn handle_socket(socket: WebSocket, code: String, state: AppState, client_
             let mut expected_file_size = None;
             let mut bytes_received = 0_u64;
             let mut progress = ProgressThrottle::new();
+            // Set when the sender completes normally, so teardown knows this
+            // socket is owed a closing handshake rather than being abandoned.
+            let mut sender_completed = false;
 
             loop {
                 let result = match timeout(idle_timeout, ws_receiver.next()).await {
@@ -451,6 +454,7 @@ async fn handle_socket(socket: WebSocket, code: String, state: AppState, client_
                                         SenderEvent::Status("awaiting_receiver"),
                                     )
                                     .await;
+                                    sender_completed = true;
                                     break;
                                 }
 
@@ -620,6 +624,52 @@ async fn handle_socket(socket: WebSocket, code: String, state: AppState, client_
                     }
                 }
             }
+
+            // The relay still owes this sender a terminal status and a `Close`
+            // frame, both written by the send task. Dropping `ws_receiver` here
+            // would stop reading a socket that is still open, so the peer's
+            // reply to that `Close` would sit unread, and closing a socket with
+            // data queued sends RST instead of FIN. The sender reports that as
+            // `Connection reset by peer` after a transfer that actually
+            // succeeded.
+            //
+            // The wait cannot be a single short deadline: the send task cannot
+            // write `transfer_complete` until the receiver has finished writing
+            // the file, which is unbounded work. So wait for the send task to
+            // finish first — it drops the event receiver as it exits — and only
+            // then give the peer a brief window to answer. Both stages are
+            // bounded, so a peer that goes quiet cannot hold this task and its
+            // per-IP connection slot open.
+            if sender_completed {
+                if timeout(
+                    Duration::from_secs(WS_IDLE_TIMEOUT_SECS),
+                    sender_tx_for_recv.closed(),
+                )
+                .await
+                .is_err()
+                {
+                    debug!(
+                        session_code = %code_for_recv,
+                        "send task did not finish before the closing handshake wait expired"
+                    );
+                }
+
+                let answered = timeout(Duration::from_secs(WS_CLOSE_DRAIN_TIMEOUT_SECS), async {
+                    while let Some(Ok(message)) = ws_receiver.next().await {
+                        if matches!(message, Message::Close(_)) {
+                            return;
+                        }
+                    }
+                })
+                .await;
+
+                if answered.is_err() {
+                    debug!(
+                        session_code = %code_for_recv,
+                        "sender did not answer the closing handshake before the drain deadline"
+                    );
+                }
+            }
         }
         .instrument(span),
     );