diff --git a/docs/adr-002-additional-rust-components.md b/docs/adr-002-additional-rust-components.md index 301b5a72..0663d988 100644 --- a/docs/adr-002-additional-rust-components.md +++ b/docs/adr-002-additional-rust-components.md @@ -356,7 +356,12 @@ spawn, pipe wiring, and lifecycle coordination into Rust. flushing, and callback semantics if dispatch predicates are too broad. - Raw file descriptor ownership remains subtle. A Rust helper must not close a descriptor still owned by an asyncio transport unless the Python side has - explicitly transferred that responsibility. + explicitly transferred that responsibility. The borrow half of this contract + is now centralized in the `with_borrowed_reader` RAII helper, which wraps the + reconstructed handle in `ManuallyDrop` so a caller-owned reader FD is never + closed on any exit path, including panic-unwind (issue `#125`); the writer FD + is still deliberately consumed so it closes to signal EOF. See the + developers' guide, "Rust FD-borrow ownership contract". - A native subprocess fast path duplicates timeout and termination behaviour, which is correctness-sensitive and platform-dependent. - Pseudo-terminal and terminal sinks can block differently from `/dev/null` or diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 1dae57cc..3fdea007 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1098,6 +1098,38 @@ function the splice and read/write paths previously shared. New failure conditions get a variant here rather than a stringly-typed `io::Error::other(...)`. +## Rust FD-borrow ownership contract + +The pump and consume entry points in `rust/cuprum-rust/src/lib.rs` divide the +descriptors they touch into a *borrowed* reader and a *consumed* writer, and +centralize the borrow half in one helper, `with_borrowed_reader`. The helper +rebuilds a `StreamHandle` from the caller-owned raw descriptor, wraps it in +`ManuallyDrop`, and runs the caller's closure against it. `ManuallyDrop` +suppresses the close on *every* exit path — a normal return and unwinding from +a panicking operation alike — so a descriptor the Python side still owns is +never closed by Rust. `pump_stream` and `consume_stream` both route their +reader through the helper, keeping the "borrow this FD without owning it" rule +in a single place. + +This supersedes an earlier pattern that reconstructed the handle and called +`std::mem::forget` after the inner operation returned. Because a panic unwinds +past the trailing `forget`, that pattern dropped — and therefore closed — the +caller-owned descriptor on the unwind path, exposing the Python transport to a +double close of the same FD (the `#125` panic-unwind hazard). `ManuallyDrop` +holds regardless of how the scope exits, so no drop guard or `forget` call is +required. + +There is deliberately no borrowed *writer* variant. The writer FD handed to +`pump_stream` is consumed: it must close on drop — including during unwinding — +so downstream readers observe EOF. Reconstruct the writer with +`stream_from_raw` (which yields an owning handle) and let it drop; reserve +`with_borrowed_reader` for descriptors whose ownership stays with the caller. +The helper's safety contract obliges the caller to guarantee `fd` is a valid +open descriptor (or Windows handle) for the duration of the call and that +ownership remains with the caller; in return the helper guarantees it never +closes `fd`. `rust/cuprum-rust/src/lib_tests.rs` regression-tests both halves: +the borrowed FD stays open after a normal operation and after a panicking one. + ## Rust splice-loop and drain contract The Linux zero-copy path in `rust/cuprum-rust/src/splice.rs` follows one diff --git a/docs/execplans/4-2-1-rust-pump-stream.md b/docs/execplans/4-2-1-rust-pump-stream.md index 92a421a1..8d12bab7 100644 --- a/docs/execplans/4-2-1-rust-pump-stream.md +++ b/docs/execplans/4-2-1-rust-pump-stream.md @@ -312,8 +312,26 @@ Dependencies: - No new Python or Rust dependencies expected; if a new crate (for example `libc`) is required, stop and escalate per tolerances. +## Addendum: FD-borrow ownership contract (issue `#125`) + +The borrowed-reader mitigation planned above (keeping the reader FD open by +`std::mem::forget`-ing the `File` wrapper) has been superseded by an RAII +helper. `with_borrowed_reader` in `rust/cuprum-rust/src/lib.rs` reconstructs +the reader handle, wraps it in `ManuallyDrop`, and runs the pump closure +against it, so the caller-owned reader FD is never closed on any exit path — +including unwinding from a panicking operation. The trailing-`forget` pattern +was skipped on unwind, closing the caller-owned descriptor and exposing the +Python transport to a double close; `ManuallyDrop` holds regardless of how the +scope exits. The writer FD is still deliberately consumed and closes on drop to +signal EOF. The canonical description of this contract now lives in the +developers' guide, "Rust FD-borrow ownership contract", with regression +coverage in `rust/cuprum-rust/src/lib_tests.rs`. + ## Revision note (required when editing an ExecPlan) Initial draft authored on 2026-01-28 to plan 4.2.1 implementation. Updated on 2026-01-28 to mark completion, record decisions, and document validation -results (including the extended timeout for `make nixie`). +results (including the extended timeout for `make nixie`). Updated on +2026-07-28 to add the FD-borrow ownership contract addendum recording the +`with_borrowed_reader`/`ManuallyDrop` helper that superseded the planned +`std::mem::forget` mitigation (issue `#125`). diff --git a/docs/roadmap.md b/docs/roadmap.md index a125025f..ab7de7f4 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -443,10 +443,16 @@ rather than gating the acceleration work. ### 8.1. Close the Rust pump file-descriptor close race -This step removes a silent failure the baseline observed on the shipped pump -path. See tee-hotpath-profiling-baseline-2026-06-12.md §"Incidental findings" -item 1, adr-002-additional-rust-components.md (FD ownership risk), and issues -`#124` / `#125`. +This step aims to remove a silent failure the baseline observed on the shipped +pump path. See tee-hotpath-profiling-baseline-2026-06-12.md §"Incidental +findings" item 1 and adr-002-additional-rust-components.md (FD ownership risk). + +The panic-unwind FD ownership hazard tracked under `#125` is **resolved**. +`with_borrowed_reader` now wraps the borrowed reader in `ManuallyDrop`, so a +caller-owned reader FD stays open on both success and panic-unwind; +`pump_stream` and `consume_stream` route through the helper, and +`rust/cuprum-rust/src/lib_tests.rs` regression-tests that the borrowed FD +survives both normal and panicking operations. - [ ] 8.1.1. Fix the silent `OSError: [Errno 9] Bad file descriptor` raised from `_UnixWritePipeTransport._call_connection_lost` during Rust pump shutdown.