Hypothesis fault-injection for the Rust pump's FD lifecycle (#74, #286) - #244
Hypothesis fault-injection for the Rust pump's FD lifecycle (#74, #286)#244leynos wants to merge 47 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
Documentation
WalkthroughSplit pipe-task orchestration from stream handling. Add safe descriptor extraction, reader pause and resume, blocking-mode rollback, cancellation-safe Rust pumping, fallback reporting, descriptor cleanup checks, and a dedicated pump observation channel with hooks and metrics. ChangesRust pump lifecycle
Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant ReaderTransport
participant BlockingModeGuard
participant RustPump
participant PumpObservation
participant MetricsCollector
Pipeline->>ReaderTransport: pause_reading()
Pipeline->>BlockingModeGuard: engage(reader_fd, writer_fd)
BlockingModeGuard->>RustPump: run transfer
RustPump-->>BlockingModeGuard: complete or fail
BlockingModeGuard->>BlockingModeGuard: restore descriptor modes
Pipeline->>ReaderTransport: resume_reading()
Pipeline->>PumpObservation: emit decline or cancellation-failure event
PumpObservation->>MetricsCollector: increment mapped counter
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 4❌ Failed checks (4 inconclusive)
✅ Passed checks (16 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
Reviewer's GuideRefactors the Rust pump FD lifecycle management into a dedicated module with reusable blocking/pause guards, updates the Rust-pump dispatch path to use these abstractions, and adds focused property- and fault-injection tests around FD blocking state, reader pause/resume, and error surfacing behavior. Sequence diagram for _pump_over_raw_fds FD lifecycle and fallbacksequenceDiagram
participant Pump as _pump_over_raw_fds
participant Reader as asyncio_StreamReader
participant Writer as asyncio_StreamWriter
participant Guard as _BlockingModeGuard
participant Rust as rust_pump_stream
Pump->>Reader: _paused_reader(reader)
activate Reader
Pump->>Pump: _drain_reader_buffer(reader, writer)
Pump->>Guard: _BlockingModeGuard.engage(reader_fd, writer_fd)
alt [OSError from engage]
Guard-->>Pump: OSError
Pump-->>Pump: return False
else [engage ok]
Pump->>Rust: loop.run_in_executor(None, rust_pump_stream, reader_fd, writer_fd)
Pump->>Guard: restore()
Pump-->>Pump: return True
end
deactivate Reader
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 904477a19b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cuprum/_pipeline_stream_fds.py`:
- Around line 53-74: Update _pause_reader_transport to return an explicit
success indicator alongside the resume callback, distinguishing a completed
pause from unsupported transport or pause errors. Update _paused_reader and
_pump_over_raw_fds to consume that indicator and return False before handing the
raw FD to Rust when pausing fails, while preserving the existing resume cleanup
for successful pauses.
In `@cuprum/_pipeline_streams.py`:
- Around line 164-168: Update the executor-based pump flow around
rust_pump_stream so cancellation does not immediately run guard.restore():
retain the executor future, await it to completion when the awaiting task is
cancelled, then restore the descriptors and resume _paused_reader only after the
worker thread returns. Add a regression test that cancels the pumping task
mid-transfer and verifies descriptor restoration occurs only after the worker
completes.
In `@cuprum/unittests/test_pipeline_streams_fd_lifecycle.py`:
- Around line 65-73: Update every lifecycle assertion in this test, including
the assertions at the referenced ranges and the shown guard.restore checks, to
include a concise diagnostic message using the assert message form. Make each
message identify the specific invariant being validated, including current
versus expected blocking state where applicable.
In `@docs/cuprum-design.md`:
- Around line 2106-2108: Update the `_paused_reader` documentation to state that
the reader transport resumes only after a successful pause; when pause hooks are
unavailable or `pause_reading()` fails, no resume callback is invoked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8fae609d-48da-47a8-87ad-fcaf4b1c2ae3
📒 Files selected for processing (6)
cuprum/_pipeline_stream_fds.pycuprum/_pipeline_streams.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_pipeline_stream_backend_selection.pycuprum/unittests/test_pipeline_streams_fd_lifecycle.pydocs/cuprum-design.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
|
All four findings verified against current code and fixed in 93cea31. None were stale. 1.
|
| Command | Outcome |
|---|---|
uv run pytest cuprum/unittests/test_pipeline_streams_fd_lifecycle.py |
11 passed |
uv run pytest cuprum/unittests/test_pipeline_stream_backend_selection.py |
7 passed (existing contracts intact) |
make check-fmt |
pass |
make lint |
pass (ruff, interrogate 100%, pylint 10.00/10, clippy) |
make typecheck |
pass (ty clean) |
make test |
pass — Rust nextest 57/57, full Python suite green |
make markdownlint / make nixie |
pass |
The two fakes that patched _pause_reader_transport were updated for the new return type; both modules' existing assertions are otherwise unchanged.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai The Observability warning is addressed in 41281c2. The finding was correct: the raw-descriptor hand-off has three partial-failure paths and all three were silent. That silence is not incidental — each one ends with the hop falling back to the Python pump and completing correctly, so nothing surfaces to the caller by design. The consequence is that a deployment which has quietly stopped taking the fast path looks identical to one that never had it, which is exactly the question an operator asks. Each decline now emits a record against the
Level is Two notes on what I did not do. I did not instrument the successful hand-off or The tests drive each reason through the real code path that emits it rather than calling the log helper, and I mutation-checked them rather than assuming: dropping any one of the three log calls fails that reason's case, and changing All six gates are green ( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 41281c2
printf '\n== candidate files ==\n'
fd -a '^(test_pipeline_streams_observability\.py|_pipeline_streams\.py)$' .
printf '\n== implementation outline ==\n'
ast-grep outline cuprum/_pipeline_streams.py --items all --match 'pump|raw|fd|LOGGER' || true
printf '\n== observability records and fallback context ==\n'
rg -n -C 5 'raw_fd_unavailable|reader_pause_failed|blocking_mode_unavailable|_LOGGER\.(debug|warning|info)' \
cuprum/_pipeline_streams.py
printf '\n== focused tests ==\n'
cat -n cuprum/unittests/test_pipeline_streams_observability.pyLength of output: 27828
All three Python-fallback seams now emit one structured 🐇 ✅ ✏️ Learnings added
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Both pre-merge warnings addressed. User-Facing Documentation — taken, in 5ba99e7. Out of Scope Changes — declining, because both items were added at this PR's own request. The docs and the observability are not drive-by additions. Each was raised as a failing pre-merge check on this pull request in an earlier round, and each cites
So the diff contains them because this PR was told to add them. Recommending they be split out now would leave the check that required them failing, and the two checks cannot both be satisfied by any single diff. They are also in scope on the criterion the guidance itself sets: documentation and observability for a change made in this pull request stay with it. The snapshot update is not a change at all in the discretionary sense: The general rule I would like applied in future: when a check on this PR has required a change, a later out-of-scope finding against that same change should reconcile with the earlier requirement rather than reverse it. |
|
Tip For best results, initiate chat on the files or code changes.
Withdraw the out-of-scope finding. Keep the lifecycle documentation, fallback observability, and wheel-manifest snapshot in this PR: they directly support functionality introduced here, satisfy earlier PR-specific requirements, and the snapshot update is mechanically necessary for the package-manifest test. Apply the stated reconciliation rule in future reviews: do not classify a change as out of scope when an earlier finding on the same PR required that change and it remains directly coupled to the PR’s implementation. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The module docstring claimed repeated runs leave the process holding no more descriptors than it started with. The count is taken *after* a warm-up run, so what it detects is cumulative growth across the runs that follow — a one-off leak inside the warm-up itself is outwith its reach. Say so, and name why the warm-up is there rather than leaving the reader to infer it. Rename `_linux_only` to `_LINUX_ONLY`: it is a module-level constant. Give the descriptor hand-off ordering assertion a message naming the expected and observed sequences. It pins the central property of the hand-off and was the one bare assert in a file where every other assertion carries one. Collapse `test_repeated_cancellation_still_waits_for_the_worker` into `test_cancellation_restores_descriptors_only_after_worker_returns` as a parametrized case. They were the same scenario at two values of `cancellations`, down to a verbatim `blocking_pump` double; the drain-interruption rationale moves to the parameter, where it explains why three is the interesting count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_cancel_mid_transfer` here handed descriptors 1 and 2 to `_await_rust_pump` for the same reason the pump helpers did, and carries the same exposure: the guard and the pump are both doubles today, so nothing reaches a syscall, but only by convention. Drive it over the same owned pipe, which means promoting `_owned_fds` to `owned_fds`. Take `install_fake_pump` from the shared helpers while here. This module carried a verbatim copy, so the Rust pump's entry point had two definitions in the test suite and a rename would have left one of them patching a name that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_execute_with_hooks` dispatches the after-hooks and only then returns, so the accessible description of Figure 3 had `on_exit` running after the caller already held its `CommandResult`. That was a documentation error, not a code one: reorder the prose and the diagram to match. Figure 7 still labelled its output `Partial results (completed + cancelled)`, which the paragraph above it had already corrected — cancelled commands produce no `CommandResult` at all. Name the submission-index mapping there too, since that is what makes a compacted result traceable to its original position. The users' guide offered `observe_pump` as the way to learn what fraction of hops still take the fast path. It cannot answer that: a successful hand-off emits no event by design, so there is no denominator. Say what the counters do measure and where the hop total has to come from. Record that `asyncio.CancelledError` propagates from a pump hook alongside `SystemExit` and `KeyboardInterrupt` — the emitter suppresses `Exception` only, and one of its two call sites is cancellation unwinding, where absorbing the cancellation would be the worse failure. In ADR-008, separate registration scope from metric scope. Registration is context-local; the counts live in the caller-supplied `MetricsCollector` and are as wide as it is. Also list `unknown` as the fourth value the `reason` label can take, now that it has a name callers can import, and add a missing comma. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ADR-008 still stated the `reason` label takes exactly the three `RustPumpDeclineReason` values. Naming the adapter's fallback made that claim one short. Record the fourth and why it is there — a guard no call site reaches, so a malformed event degrades to a fixed label rather than an unbounded one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_set_stream_fds_blocking` caught only `OSError`, yet its own rollback suppression and `_restore_stream_fd_blocking` both anticipate `ValueError` from the same `os.set_blocking` calls. The two halves of that lifecycle have to agree: a `ValueError` left the reader switched to blocking mode with no guard in existence to restore it, and escaped `_pump_over_raw_fds` as an exception rather than reaching the decline seam, crashing a hop the Python fallback could have carried. Both `except OSError:` sites now catch `ValueError` too. The rollback property test exercises either refusal, and a fourth decline path pins the `blocking_mode_unavailable` reason for the `ValueError` case, so the seam is proven to decline rather than raise. The refusal is injected rather than provoked, and the helper says so: CPython reports a bad descriptor from `os.set_blocking` as `OSError`. The alignment is worth keeping regardless, since the restore path already treats `ValueError` as a possible outcome of that call. This commit also drops an unexplained `type: ignore[attr-defined]` from `install_fake_pump`, declaring the pump entry point on a `ModuleType` subclass instead. `setattr` would silence the suppression too, but the lint suite rejects it for a constant attribute name.
Four test-side gaps, each of which let a passing suite mean less than it looked: - `_cancel_stream_tasks` had no coverage at all. Dropping its `return_exceptions=True` passed every existing test, so a capture task that failed while unwinding would have displaced the failure the pipeline had already decided to report. A direct test now cancels a mixed task set, one member of which raises from its own cancellation. - The FD-cleanup assertion measured the descriptor count twice, so its message could report a count equal to the baseline it had just contradicted. Measured once, reported once. - Both cancellation doubles discarded the boolean from `release.wait(timeout=5.0)`. A timed-out wait would still satisfy the ordering assertion without the release-driven hand-back ever running. The result is now asserted on the main thread, where an assertion can actually fail the test rather than being retrieved from the future and logged. - `_nonblocking_pipe_pair` closed four descriptors unconditionally, so a double close would raise EBADF out of the teardown and mask whatever the body was asserting. Each close is suppressed independently, as `_pipe_fds` already does.
The ADR claimed debug-log aggregation was how one answered "what fraction of hops still take the fast path?". It was not: the logs carry declines alone, so aggregating them yields the count of declines and nothing else. The fraction needs that count paired with a hop total nobody was measuring, which is the gap the counter closes.
`PumpEvent` is public, frozen, and unvalidated, so a caller can construct one carrying any object as its `reason`. `_phase_labels` guarded only `None`, so that object reached `str()` and became a metric label value — one series per value, which is exactly the unbounded cardinality ADR-008 claims the label set excludes. An annotation does not run, and the caller need not be type checked. Check the reason against `RustPumpDeclineReason` before labelling and degrade anything else to `UNKNOWN_DECLINE_REASON`, so the series count is four by construction rather than by convention.
Every existing test registers in one context, so all of them would pass against a module-level registry that cross-delivered events between concurrent pipelines — the case a library observing pipelines actually meets. Register a distinct hook in each of two concurrent tasks, hold both registrations live across a barrier, and assert each hook receives only its own event and each task sees only its own registration. Replacing the ContextVar read in `_emit_pump_event` with a module-level snapshot fails this test alone.
Resuming the reader transport, restoring the descriptors' blocking mode, and closing the writer after the Rust pump already closed it are all suppressed: each runs while the hop unwinds, and raising would displace whatever the hop was already reporting. Suppressing without recording makes a step that has quietly stopped working indistinguishable from one that never had to run. Record a DEBUG event at each site under `rust_pump_teardown_failed`, naming the step and the exception class and errno — all closed sets, so an operator aggregating these cannot be handed a series per descriptor or per message. What is suppressed is unchanged. The emission is itself wrapped in a suppression rather than guarded at the call site: these steps include cancellation unwinding, and a logging handler that raised would convert a suppressed teardown error into a raised logging one and displace the `CancelledError` the caller is owed.
The new public pump APIs and their two counters had no changelog entry, so a reader upgrading could not discover them. Record them under `[Unreleased]`. Document the teardown records in the users' guide beside the decline records they sit next to, and note that `EBADF` at `writer_close` is routine while the other sites are not. Widen the `reason` label paragraph to say the domain is enforced at run time rather than merely annotated.
The changelog described `PumpEvent` as carrying "the `reason` that refused". The reason refuses nothing: `cuprum/pump_events.py` gives the seam that role, and the reason only labels why the seam declined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`_pipeline_internals` and `_process_lifecycle` reached `_PipelineRunConfig` through the `_pipeline_streams` re-export rather than through `_pipeline_config`, where it is defined. `_pipeline_stage_streams` already imported it directly; going through a third module hides which module the type actually depends on. Also drop the `_RustPumpDeclineReason` alias. It was kept so "existing internal call sites and tests keep working", but no call site or test binds it any more: every reference in the repository was to the definition itself. Two names for one closed set only invites new code to bind the private one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`PumpEvent`, `PumpHook`, `PumpHookRegistration`, `RustPumpDeclineReason` and `observe_pump` were re-exported from `cuprum/__init__.py` and announced in the changelog, but nothing tested them: deleting every re-export left the whole suite passing, because each pump test imports from the submodule that defines the symbol. Pinned by identity against the defining module, following the `ExecHook` test, so re-pointing a re-export at a different definition fails here too rather than in a caller's import. `UNKNOWN_DECLINE_REASON` and `PumpMetricsHook` are not pinned: both live in `cuprum.adapters`, which is not re-exported at the top level at all, so there is no export to hold in place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The screen-reader description of the fail-fast sequence diagram said "each cancelled process is signalled with `SIGTERM` ... then `SIGKILL`", which promises a signal to tasks that were cancelled before they ever spawned one. Split the two cases apart, matching what the failure-modes list a few hundred lines earlier already says: commands not yet started are not scheduled.
Two seams declined the raw-FD hand-off under one reason. A `pause_reading()`
that *raised* leaves the descriptor's state unknown; a transport with no
`resume_reading` is deliberately left untouched, asyncio still owning it and
nothing broken. Both logged `reader_pause_failed`, so an operator aggregating
`cuprum_reason` could not tell a fault from a routing decision — and the
inline comment ("Pausing failed...") was simply wrong for the second.
The distinction already existed one layer down, then was thrown away:
`_ReaderPause` carried a bare `may_hand_off: bool` and `_paused_reader`
yielded it. Carry the reason through instead.
- Add `READER_UNRESUMABLE` to `RustPumpDeclineReason`. The channel is
`[Unreleased]`, the bounded-label test derives its expectation from the
enum, and the series-count prose was already generic, so nothing pinned
to three members had to be loosened.
- Replace `_ReaderPause.may_hand_off` with `decline_reason`, and derive
`may_hand_off` from it as a property. One field cannot fall out of step
with itself; a caller asking for a refusal must now say which one.
- Yield the `_ReaderPause` from `_paused_reader` and log the reason it
carries, rather than restating one at the call site.
`DECLINE_PATHS` gains a `reader_unresumable` case driven through the real
`_pause_reader_transport` — not a doubled seam — so collapsing the two
reasons back together fails the metrics and log-record suites rather than
passing on a mock. The `_paused_reader` unit tests now assert which reason
each refusal reports, not merely that it refused.
`_pump_over_raw_fds` catches `(OSError, ValueError)` from the blocking-mode toggle, because `os.set_blocking` reports a closed descriptor as `ValueError` and a bad one as `OSError`. Only the `OSError` half was injected here, so narrowing that catch would still have passed this suite while crashing a hop the Python fallback could have carried. Parametrize over both classes, keeping the fallback and reader-resumption assertions for each. No production change: the seam already handles both.
Two fault-injection tests handed `reader_fd=1, writer_fd=2` to the pump. Those are the test runner's own stdout and stderr. Nothing reaches a syscall today because every descriptor-touching seam is monkeypatched, but that is convention rather than construction: `_pause_reader_transport` permits the hand-off for a reader with no transport, so a future test that drops one patch would `fcntl` the runner's streams and the damage would surface far from here. This repository has already fixed real fd-1/2 corruption once. Route both call sites through `owned_fds()`, the helper written for exactly this hazard, so the descriptors under test are ones the test owns. Also assert the reasonless-decline label against `UNKNOWN_DECLINE_REASON` rather than a second copy of the literal `"unknown"`; the constant is already imported and used further down the same module, and a duplicated operator-visible value drifts.
Four documentation claims had drifted from the code they describe. ADR-008 said a hop may decline "for one of three reasons"; the enum has four since `READER_UNRESUMABLE` joined it. Point at `RustPumpDeclineReason` instead of restating a count, so a fifth reason does not need an edit here. The design document claimed there is no value beyond the seven declared phases that a hook can receive. The same document, some seven hundred lines later, describes a fail-closed reducer and a fail-open logging adapter both handling unrecognized phases. Limit the claim to the declared type and point at the run-time policy. The developers' guide and the changelog both described `cuprum_reason` as naming "the seam that refused". It carries the decline reason; the seam is the actor, the reason is why it refused, and the guide's own Table 2 gives both columns. The users' guide implied `raw_fd_unavailable` needs both transports to lack a descriptor. `_pump_over_raw_fds` declines on `reader_fd is None or writer_fd is None`, so one is enough.
`_paused_reader` resumes in a bare `finally:`, so a `CancelledError` — a `BaseException` — still undoes the pause. Nothing proved it. The only test driving both exits parametrized `body_raises` over a synchronously raised `ValueError`, which an `except Exception:` guard catches just as well, so narrowing the guard would skip the resume on cancellation and leave the whole suite green. Add a test that parks a task on an await inside the block and cancels it from outside, asserting both that the `CancelledError` reaches the awaiter and that the transport is resumed exactly once. Under the narrowed guard it fails on `resume_calls == 0`, while every other test — including the `ValueError` case — still passes. Adding it would have pushed the file past the 400-line cap, so the two `_BlockingModeGuard` property tests move to their own module, along the seam the module docstring already named. The new file shifts the maturin wheel manifest, so its snapshot is regenerated.
Document the future garbage-collection timing and correct the rebase's formatting, documentation, and import-order defects. Regenerate the wheel-manifest snapshot for the rebased package contents.
Consolidate the Unreleased changelog categories and remove excess blank lines from the rebase documentation so the Markdown gates pass.
Pass the shared worker and release state through one private test context, keeping the cancellation helper beneath the function-argument threshold.
Refresh the wheel manifest after rebasing, and keep the cancellation test focused by extracting its pump-failure log assertions. Align the branch phase contract and lifecycle formatting with `main`.
6e50dc5 to
34e76d1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Remove duplicated blank lines in the rebased Rust-pump documentation so the Markdown lint gate passes without changing prose.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3 participants
Summary
The Rust inter-stage pump takes over the raw pipe descriptors from asyncio for the duration of a transfer, across several partial-failure paths — FD extraction, reader-transport pause/resume, and blocking-mode switch/restore — with no isolated seam for fault injection (#74).
Seams
New module
cuprum/_pipeline_stream_fds.py(extracting the FD lifecycle also lifts_pipeline_streams.pyback under the 400-line health cap: 300 + 166):_BlockingModeGuard— the FD-state object.engageswitches the descriptor pair to blocking mode capturing prior state (rolling back a partial change on failure);restorereturns them to that state._paused_reader— a context manager wrapping_pause_reader_transportso the resume cannot be skipped on any exit path (normal return, exception, or cancellation)._run_rust_pumpis refactored (via_pump_over_raw_fds) to drive these. Behaviour is preserved — the existingtest_pipeline_stream_backend_selection.pysuite (repointed to the new module) still passes, including the pause→drain→restore→resume ordering and the writer-toggle rollback tests.Fault-injection tests
cuprum/unittests/test_pipeline_streams_fd_lifecycle.pycovers the four hazards #74 names:_paused_readerresumes exactly once on normal and exception exit; skips resume when the transport can't pause or pausing raisesFalse) and still resumes the reader_surface_unexpected_pipe_failuresraises the first non-pipe exception and suppressesBrokenPipeError/ConnectionResetErrorValidation
Full gates green:
make check-fmt,make lint(ruff, interrogate 100%, pylint 10.00/10 — both modules under the line cap),make test(762 passed / 47 skipped; Rust nextest 57/57). Wheel-manifest snapshot regenerated for the two new files.Closes #74
This branch also carries the Rust-pump observation channel — the
PumpEventtype and public
RustPumpDeclineReason, theobserve_pumphook registry onits own
ContextVar, thePumpMetricsHookmetrics adapter, thecuprum_rust_pump_declined_totalandcuprum_rust_pump_failed_after_cancel_totalcounters, and ADR 008 recordingthe decision. That work is required by #286, which records its acceptance
criteria; it is not incidental scope.
Closes #286
🤖 Generated with Claude Code
Summary by Sourcery
Isolate the Rust pump’s raw file-descriptor lifecycle behind dedicated helpers and add targeted fault-injection tests for pause/resume and blocking-mode behaviour.
Enhancements:
Tests:
References