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 48 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)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
`_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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/unittests/test_pipeline_streams_fd_lifecycle.py`:
- Around line 339-350: Update both test call sites, including
test_pump_over_raw_fds_falls_back_when_pause_fails and the shown _run_rust_pump
test, to obtain reader and writer descriptors from owned_fds() instead of
literals 1 and 2. Wrap each invocation in the helper’s required with context so
the pump uses only test-owned pipe descriptors.
In `@cuprum/unittests/test_pump_metrics_adapter.py`:
- Around line 293-301: Update
test_a_decline_without_a_reason_falls_back_to_a_bounded_label to compare the
reason label against the existing UNKNOWN_DECLINE_REASON constant instead of the
duplicated "unknown" literal, matching the established usage elsewhere in the
module.
In `@docs/adr-008-rust-pump-observation-channel.md`:
- Around line 15-17: Update the ADR sentence near the inter-stage pipe hand-off
to state that a hop may decline that hand-off for one of four reasons,
preserving the following cancellation and worker-failure statement.
In `@docs/cuprum-design.md`:
- Around line 780-781: Update the statement describing the seven `ExecPhase`
values to limit completeness to the declared type, and acknowledge that
consumers need a policy for malformed or future runtime phase values; do not
claim hooks can never receive other values.
In `@docs/developers-guide.md`:
- Around line 497-500: The documentation wording in docs/developers-guide.md
lines 497-500 must describe cuprum_reason as naming the reason for the decline,
not the refusing seam. Apply the corresponding wording change in CHANGELOG.md
lines 21-22 so it says the field is labelled with the decline reason; no code
changes are required.
In `@docs/users-guide.md`:
- Around line 1359-1364: Update the raw_fd_unavailable description in the
cuprum_reason table to state that at least one asyncio transport may lack a raw
descriptor, matching the decline behavior of _try_rust_pump().
🪄 Autofix
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: b4dae902-d32c-4590-9799-b4f73c20ab1a
📒 Files selected for processing (31)
CHANGELOG.mdcuprum/__init__.pycuprum/_pipeline_internals.pycuprum/_pipeline_pipe_tasks.pycuprum/_pipeline_stream_fds.pycuprum/_pipeline_streams.pycuprum/_pipeline_wait.pycuprum/_process_lifecycle.pycuprum/_token_registration.pycuprum/adapters/pump_metrics.pycuprum/context/registration.pycuprum/context/state.pycuprum/pump_events.pycuprum/pump_observation.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/_rust_pump_test_helpers.pycuprum/unittests/test_pipeline_fd_cleanup.pycuprum/unittests/test_pipeline_pipe_tasks.pycuprum/unittests/test_pipeline_stream_backend_selection.pycuprum/unittests/test_pipeline_streams_cancellation.pycuprum/unittests/test_pipeline_streams_fd_lifecycle.pycuprum/unittests/test_pipeline_streams_observability.pycuprum/unittests/test_public_api.pycuprum/unittests/test_pump_metrics_adapter.pycuprum/unittests/test_pump_observation.pydocs/adr-008-rust-pump-observation-channel.mddocs/contents.mddocs/cuprum-design.mddocs/developers-guide.mddocs/execplans/4-3-1-parametrize-existing-stream-unit-tests.mddocs/users-guide.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)
`make spelling` regenerates `typos.toml` from the shared base plus `typos.local.toml`, but the inline-code-span exemption lived only in the generated file — hand-edited in, so every regeneration deleted it and three backtick-quoted identifiers (`artifact`, `color`) failed the gate on a clean checkout. The pattern now lives in the overlay's `[patterns] ignore`, which the generator merges and preserves. Verified non-vacuous: a seeded violation outside a code span still fails the gate. Closes #294. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The dispatch-order test passed literal `reader_fd=1, writer_fd=2` to `_run_rust_pump` — the test runner's own stdout and stderr. CodeRabbit flagged the same pattern in the FD lifecycle tests; this site was not flagged, but it is the identical trap. It is inert today only because `_set_stream_fds_blocking` and `_restore_stream_fd_blocking` are both monkeypatched here. That is convention, not construction: `_pause_reader_transport` permits the hand-off for a reader with no transport, so dropping either patch reaches `fcntl` on descriptors 1 and 2 for real. This repository has already fixed that corruption once, which is why "inert today by monkeypatch" is not a good enough guarantee to leave in place. Route it through `owned_fds()` like the others, so the descriptors under test are ones the test owns. No literal descriptor arguments to the pump helpers remain in the repository.
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.
leynos
changed the title
`_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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai All three pre-merge findings are addressed at Testing (Overall) — no test cancels an active
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== PR head and claimed commit =='
gh pr view 244 --repo leynos/cuprum --json headRefOid,commits,statusCheckRollup \
--jq '{
headRefOid,
commits: [.commits[-12:][] | {oid: .oid, messageHeadline: .messageHeadline}],
checks: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]
}'
git show -s --format='commit=%H%nparents=%P%nsubject=%s' baa96d47
git merge-base --is-ancestor baa96d47 HEAD && echo 'baa96d47_is_ancestor_of_HEAD=true' || echo 'baa96d47_is_ancestor_of_HEAD=false'
printf '%s\n' '== changed files in claimed commit =='
git show --stat --oneline baa96d47
printf '%s\n' '== relevant source and test outline =='
ast-grep outline \
cuprum/_pipeline_stream_fds.py \
cuprum/unittests/test_pipeline_streams_fd_lifecycle.py \
cuprum/unittests/test_pipeline_streams_blocking_mode.py \
--items all --match 'paused_reader|resumes_when|BlockingModeGuard|always_resumes' || true
printf '%s\n' '== cancellation test and context-manager cleanup =='
rg -n -C 10 \
'def _paused_reader|async def _paused_reader|finally:|test_paused_reader_resumes_when_the_block_is_cancelled|test_paused_reader_always_resumes_a_pausable_transport' \
cuprum/_pipeline_stream_fds.py \
cuprum/unittests/test_pipeline_streams_fd_lifecycle.py \
cuprum/unittests/test_pipeline_streams_blocking_mode.py
printf '%s\n' '== maturin snapshot references =='
rg -n -C 2 'test_pipeline_streams_blocking_mode|test_pipeline_streams_fd_lifecycle' \
cuprum/unittests/__snapshots__/test_maturin_build.ambr
printf '%s\n' '== tracking issue and cited work items =='
gh issue view 297 --repo leynos/cuprum --json number,state,title,url,body \
--jq '{number,state,title,url,body}'
gh issue view 219 --repo leynos/cuprum --json number,state,title,url,body \
--jq '{number,state,title,url,body}' || true
gh pr view 293 --repo leynos/cuprum --json number,state,title,url,body,mergedAt \
--jq '{number,state,title,url,mergedAt,body}' || trueLength of output: 55601
Withdraw the opt-in pump tracing-span finding. Issue Do not report all CI as green. The current PR head still has two failed GitHub checks:
✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai — re-posting the standing rebuttals on this PR. The earlier ones omitted your mention 1. 2. Syrupy snapshots for log/metric records — declined, on repository precedent. 3. Out of Scope — resolved by a requiring issue, per your own alternative remedy. 4. Pump tracing span — deferred to #297, not refused. A span is not a cheap opt-in here. 5. Decline reasons are enum-bounded, and the count is now four. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== current PR head =='
gh pr view 244 --repo leynos/cuprum --json headRefOid,title,body \
--jq '{headRefOid,title,body}'
printf '%s\n' '== implementation outlines =='
ast-grep outline \
cuprum/_token_registration.py \
cuprum/context/registration.py \
cuprum/pump_observation.py \
cuprum/pump_events.py \
cuprum/adapters/pump_metrics.py \
--items all --match 'TokenRegistration|PumpHookRegistration|RustPumpDeclineReason|PumpEvent|decline' || true
printf '%s\n' '== shared token lifecycle and decline reasons =='
rg -n -C 5 \
'_TokenRegistrationBase|class _TokenRegistration|class PumpHookRegistration|def detach|READER_UNRESUMABLE|class RustPumpDeclineReason' \
cuprum/_token_registration.py cuprum/context/registration.py \
cuprum/pump_observation.py cuprum/pump_events.py
printf '%s\n' '== retry and bounded-domain coverage =='
rg -n -C 4 \
'test_failed_cross_context_detach_can_be_retried|READER_UNRESUMABLE|decline_labels_stay_inside|non_enum_reason' \
cuprum/unittests -g '*.py' || true
printf '%s\n' '== documentation bounded-domain wording =='
rg -n -C 3 \
'READER_UNRESUMABLE|RustPumpDeclineReason|unknown|decline reason' \
docs/adr-008-rust-pump-observation-channel.md docs/users-guide.md docs/developers-guide.md || true
printf '%s\n' '== issue scopes =='
gh issue view 286 --repo leynos/cuprum --json number,state,title,url,body \
--jq '{number,state,title,url,body}'
gh issue view 297 --repo leynos/cuprum --json number,state,title,url,body \
--jq '{number,state,title,url,body}'Length of output: 39523
🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/developers-guide.md (1)
526-533: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winName the garbage-collection timing.
When
_report_pump_outcome_after_canceldoes not retrieve the worker future's
exception, the unretrieved-exception warning can appear when that future is
garbage-collected. Replace “at collection time” with “when the future is
garbage-collected”. This avoids confusing result collection with garbage
collection.Suggested wording
- would resurface at collection time as an unretrieved-exception warning, + would resurface when the future is garbage-collected as an + unretrieved-exception warning,🤖 Prompt for 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. In `@docs/developers-guide.md` around lines 526 - 533, Update the documentation paragraph describing _report_pump_outcome_after_cancel to replace “at collection time” with “when the future is garbage-collected,” preserving the existing unretrieved-exception warning context.
🤖 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.
Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 526-533: Update the documentation paragraph describing
_report_pump_outcome_after_cancel to replace “at collection time” with “when the
future is garbage-collected,” preserving the existing unretrieved-exception
warning context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b82eae02-573d-4426-bad7-1dd0d9298315
📒 Files selected for processing (11)
CHANGELOG.mdcuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_pipeline_stream_backend_selection.pycuprum/unittests/test_pipeline_streams_blocking_mode.pycuprum/unittests/test_pipeline_streams_fd_lifecycle.pycuprum/unittests/test_pump_metrics_adapter.pydocs/adr-008-rust-pump-observation-channel.mddocs/cuprum-design.mddocs/developers-guide.mddocs/users-guide.mdtypos.local.toml
🔗 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)
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: