From 5cbde541c0b0e1ebc65e99f0b1b369c22dff8e84 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 26 Jul 2026 16:47:07 +0200 Subject: [PATCH 1/6] docs: mark #125 FD-ownership hazard resolved in roadmap 8.1 Section 8.1 previously presented issues #124 and #125 as a single outstanding pump FD close-race work item. The #125 panic-unwind FD ownership hazard is now fixed in code and regression-tested: with_borrowed_reader wraps the borrowed reader in ManuallyDrop so a caller-owned reader FD stays open on success and panic-unwind, and rust/cuprum-rust/src/lib_tests.rs asserts the borrowed FD survives both normal and panicking operations. Split the section so 8.1.1 scopes the still-outstanding shutdown close-race work to #124, and record #125 as resolved. Co-Authored-By: Claude Fable 5 --- docs/roadmap.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index a125025f..568edee8 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -445,11 +445,19 @@ rather than gating the acceleration work. 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`. +item 1, adr-002-additional-rust-components.md (FD ownership risk), and issue +`#124`. + +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. + `_UnixWritePipeTransport._call_connection_lost` during Rust pump shutdown + (`#124`). - Success: the `echo-devnull-nocb-s4-rust` scenario logs no bad-FD errors across repeated runs, FD ownership at pump teardown is documented, and a regression test reproduces the close race and asserts clean shutdown. From 0f0b20660561401c067d3c0401ee97de113cad73 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 28 Jul 2026 13:01:56 +0200 Subject: [PATCH 2/6] docs: frame roadmap 8.1 intro as intent for outstanding #124 work The Section 8.1 introduction stated the step "removes" the silent Rust pump shutdown failure, but that #124 close-race work (task 8.1.1) is still unchecked. Reword to "aims to remove" so the intro describes the outstanding intent rather than a completed outcome. The resolved #125 panic-unwind status and the 8.1.1 checklist item are unchanged. Co-Authored-By: Claude Fable 5 --- docs/roadmap.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index 568edee8..2a7acb8a 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -443,8 +443,8 @@ 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" +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, adr-002-additional-rust-components.md (FD ownership risk), and issue `#124`. From b0840a579d231d1de3d8a9fadbb7878eefc3a807 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 28 Jul 2026 13:03:34 +0200 Subject: [PATCH 3/6] docs: stop attributing the pump shutdown race to #124 in roadmap 8.1 Issue #124 covers the splice-loop dedup and drain_reader EINTR fix, not the _UnixWritePipeTransport._call_connection_lost bad-FD shutdown race. No dedicated issue tracks that race, so remove the incorrect #124 links from the Section 8.1 intro and task 8.1.1 and note it remains an unlinked, outstanding item. The resolved #125 borrowed-reader panic-unwind status is preserved unchanged. Co-Authored-By: Claude Fable 5 --- docs/roadmap.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index 2a7acb8a..d29f8586 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -444,9 +444,9 @@ rather than gating the acceleration work. ### 8.1. Close the Rust pump file-descriptor close race 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, adr-002-additional-rust-components.md (FD ownership risk), and issue -`#124`. +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). +No dedicated issue tracks this shutdown race yet; it remains outstanding. The panic-unwind FD ownership hazard tracked under `#125` is **resolved**. `with_borrowed_reader` now wraps the borrowed reader in `ManuallyDrop`, so a @@ -456,8 +456,7 @@ caller-owned reader FD stays open on both success and panic-unwind; 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 - (`#124`). + `_UnixWritePipeTransport._call_connection_lost` during Rust pump shutdown. - Success: the `echo-devnull-nocb-s4-rust` scenario logs no bad-FD errors across repeated runs, FD ownership at pump teardown is documented, and a regression test reproduces the close race and asserts clean shutdown. From db87c8fc6ad2fc09d79673e2f6902821c65b7fb0 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 28 Jul 2026 13:07:30 +0200 Subject: [PATCH 4/6] docs: document the FD-borrow ownership contract for #125 Record the with_borrowed_reader RAII helper contract in the three places that describe Rust FD ownership: - developers-guide.md: new "Rust FD-borrow ownership contract" section (canonical) covering the ManuallyDrop borrow, the consumed-writer asymmetry, the safety contract, and the panic-unwind regression tests. - adr-002: note the raw-FD ownership known-risk is now mitigated by the helper, cross-referencing the developers' guide. - execplan 4-2-1: addendum recording that the helper superseded the planned std::mem::forget mitigation, plus the required revision note. Uses the repo's typos-enforced "centralize" spelling. make markdownlint and make nixie pass. Co-Authored-By: Claude Fable 5 --- docs/adr-002-additional-rust-components.md | 7 ++++- docs/developers-guide.md | 33 ++++++++++++++++++++++ docs/execplans/4-2-1-rust-pump-stream.md | 20 ++++++++++++- 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/docs/adr-002-additional-rust-components.md b/docs/adr-002-additional-rust-components.md index 301b5a72..b71b6e56 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..5e900a71 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1098,6 +1098,39 @@ 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..fe2d00c4 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`). From 0e8279f3a45d35e9e0ffa058d09741052f95983d Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 28 Jul 2026 22:57:32 +0200 Subject: [PATCH 5/6] docs: drop redundant outstanding-status sentence from roadmap 8.1 The unchecked 8.1.1 checklist item already conveys that the pump shutdown close race is outstanding, so the explicit sentence saying so was redundant. Remove it; the intro now just cites the baseline finding and ADR-002. Co-Authored-By: Claude Opus 5 (1M context) --- docs/roadmap.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index d29f8586..ab7de7f4 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -446,7 +446,6 @@ rather than gating the acceleration work. 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). -No dedicated issue tracks this shutdown race yet; it remains outstanding. The panic-unwind FD ownership hazard tracked under `#125` is **resolved**. `with_borrowed_reader` now wraps the borrowed reader in `ManuallyDrop`, so a From 92355553ed753ce9bebb467ba646a72c06a23cce Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 28 Jul 2026 23:08:49 +0200 Subject: [PATCH 6/6] docs: normalize FD-borrow contract prose after rebase onto main Rebasing onto main brought in the repository-wide Markdown reformat, so run the new #125 ownership-contract prose through mdformat-all to match the surrounding wrap width. Also drop a stray duplicate blank line the entity-aware merge driver left before the new developers' guide heading. Content is unchanged; this is wrapping only. Unrelated pre-existing format drift elsewhere in the docs tree is deliberately left alone so this branch stays scoped to the #125 documentation. Co-Authored-By: Claude Opus 5 (1M context) --- docs/adr-002-additional-rust-components.md | 4 ++-- docs/developers-guide.md | 27 +++++++++++----------- docs/execplans/4-2-1-rust-pump-stream.md | 24 +++++++++---------- 3 files changed, 27 insertions(+), 28 deletions(-) diff --git a/docs/adr-002-additional-rust-components.md b/docs/adr-002-additional-rust-components.md index b71b6e56..0663d988 100644 --- a/docs/adr-002-additional-rust-components.md +++ b/docs/adr-002-additional-rust-components.md @@ -360,8 +360,8 @@ spawn, pipe wiring, and lifecycle coordination into Rust. 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". + 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 5e900a71..3fdea007 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1098,7 +1098,6 @@ 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 @@ -1106,11 +1105,11 @@ 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. +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 @@ -1122,14 +1121,14 @@ 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. +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 diff --git a/docs/execplans/4-2-1-rust-pump-stream.md b/docs/execplans/4-2-1-rust-pump-stream.md index fe2d00c4..8d12bab7 100644 --- a/docs/execplans/4-2-1-rust-pump-stream.md +++ b/docs/execplans/4-2-1-rust-pump-stream.md @@ -316,22 +316,22 @@ Dependencies: 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`. +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`). Updated on 2026-07-28 -to add the FD-borrow ownership contract addendum recording the +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`).