Phase 6.1: TCP half-close (FinWait/CloseWait/LastAck) + LAST_ACK reaping#76
Merged
Phase 6.1: TCP half-close (FinWait/CloseWait/LastAck) + LAST_ACK reaping#76
Conversation
9 bite-sized tasks covering the half-close state machine: - Established → FinWait1 (guest closed first) → LastAck on host EOF - Established → CloseWait (host closed first) → LastAck on guest FIN - LastAck → Closed on guest's final ACK or LAST_ACK_TIMEOUT (60s) Three new pins: tcp_half_close_guest_writes_first (BROKEN_ON_PURPOSE flips at Task 3), tcp_half_close_host_writes_first (passes after Task 3), tcp_last_ack_timeout_reaps_stale_entry (bench-helpers gated, Task 7). Severity: HIGH — current code drops host's pending response on guest shutdown(SHUT_WR), causing silent data loss for HTTP, SMTP, and any protocol with orderly half-close.
Guest sends FIN after data; current code marks state=Closed immediately on guest FIN, so the host's response data is dropped. This pin locks the broken behavior so the Phase 6.1 fix is legible to reviewers. Flips to PASS when Tasks 2–3 implementation lands.
Tasks 2+3+4 implemented together: clippy's -D warnings requires our_fin_sent to be used before committing the field addition, so the relay loop changes from Task 3 and the ACK handler from Task 4 are bundled with Task 2's struct changes. Structural changes (TcpNatEntry): - Add last_state_change: Instant — tracks when state last changed, used for LAST_ACK_TIMEOUT reaping (Task 6). - Add our_fin_sent: bool — prevents re-sending FIN on repeated epoll readiness events for the same half-close transition. - Initialize both fields at all three TcpNatEntry literal sites. FIN handler (handle_tcp_frame): - Established → FinWait1: ACK guest FIN, shutdown(Write) host socket so host sees EOF; stay alive for host's pending response data. - CloseWait → LastAck: host already closed; guest just closed; ACK and wait for guest's final ACK of our FIN. ACK handler (handle_tcp_frame): - LastAck → Closed: guest's final ACK reaps the entry. - Extend ACK-driven consume to FinWait1 state so bytes_in_flight drains and recv_peek eventually sees Ok(0) (host EOF). Relay loop (relay_tcp_nat_data): - Extend data relay to FinWait1 in addition to Established. - Ok(0) arm now dispatches on state: Established → CloseWait, FinWait1 → LastAck (instead of immediate Closed). - FIN-emit logic uses our_fin_sent guard and needs_fin predicate (CloseWait | LastAck) to send FIN exactly once per transition. Test: update tcp_half_close_guest_writes_first to ACK received data segments so the ACK-driven consume path drains the kernel buffer and recv_peek sees EOF. Without ACKs the FinWait1 relay sees the same bytes on every peek and never reaches Ok(0).
Symmetric mirror of tcp_half_close_guest_writes_first: host writes GREETING, shuts down its write side (Established → CloseWait), and waits for guest to reply; guest sends REPLY data + FIN. Also fix two implementation gaps found while writing this pin: 1. Guest→host data forwarding in CloseWait: the payload forward guard was Established-only. CloseWait must also forward guest data (host closed its write side but can still read). Extend the predicate to include CloseWait. 2. shutdown(Write) on CloseWait → LastAck: when the guest sends FIN in CloseWait the host application is blocked on read_to_end. We must call shutdown(Write) on host_stream so the kernel delivers EOF to the host. Without this the host blocks forever.
Add LAST_ACK_TIMEOUT = 60 s (TCP MSL × 2) as a module-scope constant. Merge the LastAck-timeout check into the existing idle-timeout sweep in relay_tcp_nat_data, making a single O(n) pass handle both conditions: - Standard idle-timeout (300 s of inactivity, any state): unchanged. - LastAck-timeout (60 s since last_state_change, LastAck state): reaps entries where the guest's final ACK never arrived, e.g. after a guest crash. Logs at WARN so operators can connect the timeout to a prior half-close sequence without a separate debugging session. The combined sweep avoids a second loop and an extra to_remove Vec allocation. Using last_state_change (set on every state transition) rather than last_activity (set on data relay) gives an accurate 60 s window from the moment we sent our FIN.
…ers) Add a bench-helpers–gated pin that verifies the LAST_ACK_TIMEOUT reaper: 1. Insert a synthetic LastAck entry via the new bench-helpers helper insert_synthetic_lastack_entry (TcpNatState::LastAck, our_fin_sent=true). 2. Back-date last_state_change by 70 s (> LAST_ACK_TIMEOUT = 60 s) via set_synthetic_last_state_change. 3. One drain_to_guest cycle runs the timeout sweep. 4. Assert the entry is gone (tcp_flow_state returns None). Two new bench-helpers methods on SlirpBackend: - insert_synthetic_lastack_entry: seeds a LastAck flow without a full half-close exchange, for timeout reaping tests. - set_synthetic_last_state_change: back-dates last_state_change on an existing entry to simulate an expired LAST_ACK_TIMEOUT. Also widen TcpNatState and tcp_flow_state to pub so the bench-helpers– gated external test can reference them by path. Both were previously pub(crate); they are now observable as part of the bench-helpers surface.
…FinWait2 All TcpNatState variants are now wired into the state machine: - SynReceived, SynSent: handshake paths (unchanged) - Established: normal data transfer (unchanged) - FinWait1: guest half-closed; relay continues forwarding host response - CloseWait: host half-closed; relay continues forwarding guest data - LastAck: both sides closed; waiting for guest's final ACK or timeout - Closed: entry pending removal Remove FinWait2: distinguishing it from FinWait1 would require observing per-segment ACKs from the kernel — the relay does not track those. The relay stays in FinWait1 until host EOF, then jumps to LastAck. If the distinction is needed later, the variant can be re-added. Update the TcpNatState doc comment to document the state machine diagram and the FinWait2 omission rationale, replacing the old per-variant stubs.
Two artifacts from the rebase onto the new #69 tip: 1. bind_port_forward_listeners was called twice in with_security after the rebase — the second call shadowed the first, which was harmless but emitted an unused-variable warning under -D warnings. Drop the duplicate. 2. The bench-helpers helper insert_synthetic_lastack_entry was missing flow_token in its TcpNatEntry initializer (the field was added by the Copilot fix #1 monotonic-token rewrite that landed on #69 after the original phase 6.1 commit chain was written). Allocate via next_flow_token(PROTO_TAG_TCP).
A bare intra-doc-link (`[`set_synthetic_last_state_change`]`) doesn't resolve to a struct method even when both items live in the same `impl` block — rustdoc looks up bare names in the module scope, not the enclosing impl. CI's `-D warnings` then elevates the `broken_intra_doc_links` warning to an error and fails the Documentation job. Switch to `Self::set_synthetic_last_state_change` so rustdoc resolves through the impl.
This was referenced May 6, 2026
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
1 participant
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.
Summary
Adds proper TCP half-close handling to the SLIRP stack. Rebased clean onto
main(post PR #69) — no merge commit.Established → CloseWaitwhen the host EOFs (read returns 0).Established → FinWait1 → LastAckwhen the guest sends FIN.LAST_ACK_TIMEOUTreaping preventsLastAckentries from leaking when a peer never ACKs the final FIN.#[allow(dead_code)]onTcpNatState; removes unusedFinWait2(we never enter it on the relay path).Commits
8 commits cherry-picked from
smoltcp-passt-port-phase6.1-half-closeonto currentmain:docs: Phase 6.1 detailed TDD plan — TCP half-closetest(network): pin tcp_half_close_guest_writes_first (BROKEN_ON_PURPOSE)feat(slirp): FinWait1 → LastAck and Established → CloseWait on host EOFtest(network): pin tcp_half_close_host_writes_firstfeat(slirp): LAST_ACK_TIMEOUT reaping prevents LastAck entry leaktest(network): pin tcp_last_ack_timeout_reaps_stale_entry (bench-helpers)refactor(slirp): drop allow(dead_code) on TcpNatState; remove unused FinWait2fix(slirp): drop duplicate bind + init flow_token in lastack helperTest plan
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo test --test network_baseline -- --test-threads=1— 20/20 (was 18; +2 half-close pins)cargo test --test network_baseline --features bench-helpers -- --test-threads=1— 22/22 (+1 reaping pin)Stacked PRs follow-up
After this lands, Phase 6.2 (async-connect) and Phase 6.3 (window mgmt) will rebase onto post-6.1 main in turn.