Preserve partial capture when a timed-out run's readers have not reached EOF (#292)

[leynos]

Why

The Typecheck and test (Python 3.15a) CI job moved from Python 3.15.0b2 to
3.15.0rc1 and now fails on main itself
(run 31058537773),
and therefore on every pull request. Four parametrizations of
test_non_positive_timeout_at_public_boundary fail — the capture=True cases
for both run() and run_sync(), at timeouts 0 and -1.0:

E  AssertionError: a capturing run must surface partial stdout as a string,
   got output=None stderr=None

This is the blocker for CI on #243 and #244.

The mechanism

_run_subprocess_with_streams spawns a stdout and a stderr consumer task, waits
for the exit code, and on expiry terminates the process and calls
_drain_stream_consumers. That helper cancelled every reader not already
done() and mapped a cancelled reader to None.

That mapping was only ever harmless because of a scheduling coincidence. The
process is dead by then, so its pipes are closed, and on 3.12 through 3.14 the
readers observe end-of-file and complete before the drain inspects them.
Nothing in the code enforced that ordering. Under 3.15.0rc1 the process exit is
observed before the pending end-of-file events on the stdout and stderr pipes
have been processed, so both readers are still parked in read() when the drain
runs. It cancels them and reports None for both streams — exactly the
two-None signature in the failure.

Note the scope: this is not confined to non-positive timeouts. The ordinary
positive-timeout path drains through the same helper, so on rc1 a
run(timeout=5) against a command that produced output and then hung would
raise TimeoutExpired with that output discarded. The non-positive cases are
merely where the suite happens to look.

The fix

_drain_stream_consumers now takes a capture flag, and a capturing drain does
two things a non-capturing one does not.

• It waits up to _CAPTURE_EOF_GRACE_S (0.25 s) for the readers to reach
  end-of-file before cancelling them. The process is already dead, so
  end-of-file is imminent rather than hypothetical, and a reader cancelled one
  scheduling turn short of it loses the capture it was about to deliver. The
  window is bounded because a grandchild that inherited the pipe can wedge a
  reader indefinitely, and teardown must never wait on that.
• It reports a reader that still has no text as the empty string rather than
  None, so the documented "a capturing run reports its streams as text"
  contract holds by construction rather than by scheduling luck.

The cancellation and stdin-failure paths pass capture=False: they discard the
drained text, so they pay neither the window nor the contract and teardown stays
as prompt as before.

Both halves are version-neutral. Nothing here is conditioned on the interpreter
version, and 3.12 to 3.14 behaviour is unchanged — on those versions the readers
were already finished, so the window returns immediately and the fallback never
fires. test_safe_cmd_run.py's assertions are untouched; they state the intended
contract and the fix is what now satisfies it.

Regression tests

New file cuprum/unittests/test_timeout_capture_contract.py. Rather than
simulating rc1's scheduling, it withholds end-of-file outright, which reproduces
the worst case on every interpreter and so fails on the old code unconditionally:

• a capturing drain reports the empty string for readers that never reach
  end-of-file;
• a capturing drain lets readers a few scheduling turns from end-of-file deliver
  their capture;
• a non-capturing drain still reports None;
• driving the public boundary with the stream consumers replaced by readers that
  never observe end-of-file, a capturing run_sync(timeout=0) still reports both
  streams as text — this covers the wiring that tells the drain a run is
  capturing.

Mutation proof: reverting both halves of the fix in
cuprum/_subprocess_execution.py (immediate cancel, None on absence) fails
three of the four, and the public-boundary one fails with
got output=None stderr=None — byte-for-byte the CI symptom, on Python 3.14.

Validation

rc1 cannot be run locally: uv python install 3.15.0rc1 reports
No download found for request: cpython-3.15rc1-linux-x86_64-gnu, as
python-build-standalone has not published it. So the rc1 job cannot be confirmed
green before merge; CI is the oracle. Instead:

• the mechanism was derived by reading the drain path and confirmed against the
  CI failure's exact two-None signature;
• the regression tests reproduce the symptom without needing rc1 at all, by
  withholding end-of-file;
• the suite passes on 3.13, 3.14, and 3.15.0b2;
• the full gate set is green — check-fmt, lint, typecheck, test,
  markdownlint, nixie, and cs delta origin/main HEAD ("No issues found!").
  mbake format --check Makefile fails identically on origin/main, whose
  Makefile this branch does not touch.

References

• Closes #292
• Failing job on main:
  run 31058537773

Summary by Sourcery

Ensure capturing subprocess runs always report captured stdout and stderr as text on timeout instead of dropping partial output based on event-loop scheduling.

Bug Fixes:

• Preserve partial stdout/stderr capture for timed-out capturing runs by waiting briefly for readers to reach EOF and treating missing text as empty strings rather than None, fixing failures on newer Python versions.

Enhancements:

• Extend the stream-drain helper with a capture-aware mode that differentiates capturing and non-capturing runs while keeping teardown behaviour unchanged for non-capturing paths.

Documentation:

• Document the capture-aware drain behaviour and its role in preserving timeout output across Python versions in the developers guide.

Tests:

• Add regression tests that simulate readers that never reach EOF or reach it late to validate the capture contract for timeouts and the capture-aware drain behaviour at both helper and public API boundaries.

Chores:

• Update changelog to record the timeout capture fix and reference the associated issue.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Summary

Fix timed-out capturing subprocess runs that return None for stdout or stderr.

• Wait up to 0.25 seconds for captured readers to reach EOF.
• Return empty strings when captured output is unavailable.
• Preserve partial output after reader cancellation.
• Settle reader tasks and log suppressed failures or expired grace periods at DEBUG.
• Keep non-capturing and stdin-failure teardown prompt.
• Add property-based, regression, logging, and public timeout-boundary tests.
• Split stream and drain helpers to match ADR-007.
• Update user and developer documentation, the changelog, and packaging snapshots for issue #292.
• Validate formatting, linting, type checking, tests, documentation checks, and supported Python versions.

Walkthrough

Preserve captured output during timeout cleanup. Allow a bounded EOF grace period. Report empty strings for captured streams without text. Discard output on non-capturing cleanup paths.

Changes

Timeout capture handling

Layer / File(s)	Summary
Extract shared stream text handling cuprum/_stream_text.py, cuprum/_streams.py, cuprum/_testing.py	Move decoding, chunk writing, line emission, and newline handling into shared helpers. Preserve buffered text when a capturing read is cancelled.
Implement capture-aware draining cuprum/_subprocess_drain.py	Add bounded EOF handling, centralised consumer settlement, capture-aware decoding, and DEBUG logging for discarded reader failures.
Route cleanup paths by capture mode cuprum/_subprocess_execution.py	Use capture-enabled draining for timeout cleanup. Use non-capturing draining for cancellation and stdin-writer failures.
Validate and document the timeout contract cuprum/unittests/*, docs/*.md, CHANGELOG.md, typos.local.toml	Test delayed EOF, partial output, empty captured streams, cancellation, logging, and non-capturing drains. Update packaging expectations, documentation, the changelog, and typo configuration.

Sequence Diagram(s)

sequenceDiagram
  participant SubprocessExecution
  participant StreamConsumers
  participant TimeoutExpired
  SubprocessExecution->>StreamConsumers: Drain streams after timeout with capture enabled
  StreamConsumers->>StreamConsumers: Wait for bounded EOF grace period
  StreamConsumers-->>SubprocessExecution: Return settled stdout and stderr
  SubprocessExecution->>TimeoutExpired: Set output and stderr

Loading

Possibly related PRs

• leynos/cuprum#158: Both PRs modify stream-consumer draining and timeout cleanup.
• leynos/cuprum#223: Both PRs modify subprocess timeout stream-draining behaviour and its tests.
• leynos/cuprum#226: Both PRs modify _drain_stream_consumers and subprocess timeout cleanup.

Suggested labels: Issue

Suggested reviewers: codescene-access, codescene-delta-analysis

Poem

Wait for EOF within the bound,
Preserve partial output found.
Return "" when streams are bare,
Discard output when capture is not there,
Keep timeout results sound.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error, 2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Unit Architecture	❌ Error	_await_eof_grace directly uses asyncio.wait(..., timeout=0.25); the elapsed-time dependency is not injectable, and tests rely on real sleeping.	Inject a narrow timer/wait dependency at the drain boundary, or isolate an explicit scheduler abstraction; test grace behaviour without real-time sleeps.
Developer Documentation	⚠️ Warning	The guide documents the new modules, but the design document and ADR-007 still assign consumer wiring to _subprocess_execution.py and omit _subprocess_drain.py.	Update docs/cuprum-design.md §8.1.5 and ADR-007 to record the new drain boundary, capture-aware semantics, and changed contract.
Observability	⚠️ Warning	The 0.25 s EOF grace changes teardown latency and adds failure states, but only DEBUG logs were added; existing metrics and tracing receive no drain outcome or grace timing.	Emit bounded-cardinality drain/grace outcomes through the existing ExecEvent, metrics, and tracing paths, correlated by exec_id; retain stream and error-type fields and exclude captured payloads.
Performance And Resource Use	❓ Inconclusive	Initial inspection found a bounded 0.25 s EOF wait and existing capture buffering; inspect call paths and allocation behaviour before deciding.	Verify all drain paths, task-settlement behaviour, and whether new text helpers add material per-chunk or per-line overhead.

✅ Passed checks (16 passed)

Check name	Status	Explanation
Title check	✅ Passed	Preserve partial capture for timed-out runs and link issue #292 in the title.
Description check	✅ Passed	Ensure the description clearly explains the timeout capture defect, implementation, tests, documentation, and validation.
Linked Issues check	✅ Passed	Satisfy [#292] by preserving captured output, returning strings, bounding EOF waits, and retaining prompt non-capturing teardown.
Out of Scope Changes check	✅ Passed	Keep the refactoring, tests, documentation, changelog, and typo configuration aligned with the timeout capture fix.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Testing (Overall)	✅ Passed	Tests substantively cover delayed and absent EOF, empty-string and partial capture, cancellation settlement, non-capturing semantics, public timeout wiring, failure preservation, logging, and moved...
User-Facing Documentation	✅ Passed	Mark this check PASS: docs/users-guide.md documents partial timeout output, empty strings under capture=True, bounded EOF handling, cancellation retention, and None for non-capturing runs.
Module-Level Documentation	✅ Passed	All ten changed Python modules have top-level docstrings; new stream and drain modules describe their purpose, utility, and relationships to the execution components.
Testing (Unit And Behavioural)	✅ Passed	Keep this coverage: property tests exercise consumer states and invariants; logging tests cover failures and grace expiry; public run()/run_sync() tests verify timeout payloads and process cleanup.
Testing (Property / Proof)	✅ Passed	Hypothesis tests vary both stdout/stderr across completed, pending, failing and partial states plus arbitrary text, and assert settlement, capture semantics and primary-error preservation; determin...
Testing (Compile-Time / Ui)	✅ Passed	Accept: no Rust/TypeScript compile-time changes exist; focused tests assert stable timeout and log semantics, while the wheel-layout snapshot covers packaged-file output.
Domain Architecture	✅ Passed	The change is confined to subprocess and stream infrastructure; no domain model or domain logic is modified, so this domain-segregation check is not applicable.
Security And Privacy	✅ Passed	Keep this check passing: the patch adds no secrets or privilege changes, retains parameterized subprocess execution, and logs only stream operation, error type, reader count, and timeout metadata.
Concurrency And State	✅ Passed	The drain has explicit task ownership, bounded EOF ordering, cancellation handling, and gather-based settlement; tests cover delayed EOF, partial capture, failures, cancellation, ordering, and no p...
Architectural Complexity And Maintainability	✅ Passed	Keep the change: _subprocess_drain centralizes three teardown paths, _stream_text isolates pure helpers, modules stay under 400 lines, and runtime imports remain acyclic.
Rust Compiler Lint Integrity	✅ Passed	The PR changes no Rust source or Rust configuration; the Rust-scoped diff is empty and added lines contain no Rust lint suppressions, artificial anchors, or clone calls.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-python-315rc1-timeout-capture

---

_{Comment @coderabbitai help to get the list of available commands.}

[sourcery-ai]

Reviewer's Guide

Ensure timed-out capturing subprocess runs reliably surface partial stdout/stderr as text instead of None, by making the drain helper capture-aware and adding regression tests and documentation for the behavior.

Sequence diagram for capturing drain on subprocess timeout

sequenceDiagram
    participant Runner as _run_subprocess_with_streams
    participant Subprocess
    participant StdoutConsumer as stdout_consumer_task
    participant StderrConsumer as stderr_consumer_task
    participant Drain as _drain_stream_consumers

    Runner->>Subprocess: start subprocess
    Subprocess->>StdoutConsumer: produce stdout
    Subprocess->>StderrConsumer: produce stderr

    Runner->>Runner: _wait_for_exit_code_within_timeout
    alt timeout
        Runner->>Runner: _cancel_stdin_writer
        Runner->>Drain: _drain_stream_consumers(consumers, capture=execution.capture)
        opt capture is True
            Drain->>Drain: asyncio.wait(consumers, timeout=_CAPTURE_EOF_GRACE_S)
        end
        Drain->>StdoutConsumer: _cancel_pending_consumers
        Drain->>StderrConsumer: _cancel_pending_consumers
        StdoutConsumer-->>Drain: stdout_result
        StderrConsumer-->>Drain: stderr_result
        Drain->>Drain: _decode_consumer_result(result, capture=True)
        Drain-->>Runner: stdout_text ("" if no text), stderr_text ("" if no text)
        Runner->>Runner: _handle_stream_timeout(stdout_text, stderr_text)
    else cancellation or stdin failure
        Runner->>Runner: _cancel_stdin_writer
        Runner->>Drain: _drain_stream_consumers(consumers, capture=False)
        Drain->>StdoutConsumer: _cancel_pending_consumers
        Drain->>StderrConsumer: _cancel_pending_consumers
        StdoutConsumer-->>Drain: stdout_result
        StderrConsumer-->>Drain: stderr_result
        Drain->>Drain: _decode_consumer_result(result, capture=False)
        Drain-->>Runner: stdout_text=None, stderr_text=None
        Runner-->>Runner: propagate error without captured output
    end

Loading

File-Level Changes

Change	Details	Files
Make stream consumer draining capture-aware so timed-out capturing runs preserve partial output and never return None for their streams.	Introduce _CAPTURE_EOF_GRACE_S and use asyncio.wait to give consumers a short window to reach EOF before cancellation on capturing drains. Add _decode_consumer_result helper to normalize consumer outcomes to either text or None based on the capture flag, treating exceptions and missing output as empty string in capturing mode. Extend _drain_stream_consumers to accept a capture flag, apply the EOF grace window when capturing, and decode both stdout/stderr via _decode_consumer_result. Update _run_subprocess_with_streams to pass execution.capture into _drain_stream_consumers on timeout, and capture=False on cancellation and stdin-writer failure paths so those teardown paths remain fast and non-capturing. Export _decode_consumer_result from the subprocess execution module’s public surface for tests and internal use.	cuprum/_subprocess_execution.py
Document the capture-aware drain behavior and the Python 3.15.0rc1 scheduling change that motivated it, and record the fix in the changelog.	Describe the capture flag, EOF grace window, and empty-string vs None semantics in the developers guide’s subprocess timeout section, including guidance on when to pass capture=False. Add a changelog entry explaining that timed-out capturing runs now preserve partial capture and return text instead of None, referencing issue Python 3.15.0rc1: capturing run loses partial capture on timeout, reporting None #292 and Python 3.15.0rc1 behavior.	docs/developers-guide.md CHANGELOG.md
Adapt existing timeout tests to the new _drain_stream_consumers signature and semantics.	Update helper-based timeout tests to call _drain_stream_consumers with capture=False wherever drained text is intentionally discarded. Ensure property tests for consumer draining treat the new capture parameter as non-capturing in teardown paths and still assert the same structural properties of cancellation and draining.	cuprum/unittests/test_subprocess_timeout.py cuprum/unittests/test_subprocess_timeout_properties.py
Add regression tests that pin the capture contract for timed-out runs and validate behavior when readers never or only-late observe EOF.	Create helpers that simulate consumers which never reach EOF and consumers that reach EOF after several scheduling turns, to exercise worst-case scheduling independent of Python version. Add tests that a capturing drain reports empty strings for permanently wedged readers, preserves text for readers that reach EOF a few turns later, and leaves None for non-capturing drains. Monkeypatch _consume_stream to simulate readers that never see EOF and drive the public run_sync(timeout=0, capture=True) boundary, asserting TimeoutExpired carries empty-string stdout/stderr rather than None. Ensure new tests reference the shared timeout helpers (python_catalogue, python_interpreter, child_argv) and integrate into the unittest suite.	cuprum/unittests/test_timeout_capture_contract.py

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#292	Ensure that for capturing runs (run() and run_sync()), timeouts always surface stdout and stderr as strings (empty string acceptable) instead of None, including when readers are cancelled before seeing EOF (e.g., under Python 3.15.0rc1).	✅
#292	Make the stream-drain logic robust rather than timing-dependent by giving stdout/stderr consumer tasks a short bounded window to reach EOF before cancellation, so partial captured output is preserved on timeout in a version-neutral way.	✅
#292	Update tests and documentation to codify and guard the capture-on-timeout contract (capturing runs report text, not None) so regressions like the Python 3.15.0rc1 behavior are prevented.	✅

Possibly linked issues

• Python 3.15.0rc1: capturing run loses partial capture on timeout, reporting None #292: The PR changes drain semantics so timed-out capturing runs preserve or default to empty-string output, fixing the rc1 regression.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Actionable comments posted: 2

🤖 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/_subprocess_execution.py`:
- Around line 120-125: Make _drain_stream_consumers cancellation-safe by
ensuring both consumer tasks are explicitly cancelled and awaited when the
capture EOF-grace wait is interrupted, before re-propagating CancelledError from
the enclosing run. Preserve normal result handling when the grace period
completes, and add a regression covering cancellation of a timed-out capturing
run during that period that verifies both readers are settled.

In `@cuprum/unittests/test_timeout_capture_contract.py`:
- Around line 143-151: Update consume_forever to explicitly consume the required
on_line callback parameter before awaiting, while keeping its name and signature
unchanged for _spawn_stream_consumers compatibility. Use del on_line or a
narrowly scoped suppression so Ruff no longer reports ARG001.

🪄 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: c4e7b5cd-1cee-4717-aa2d-f0fc2da15bf1

📥 Commits

Reviewing files that changed from the base of the PR and between 7f6ec92 and 480f62e.

📒 Files selected for processing (7)

• CHANGELOG.md
• cuprum/_subprocess_execution.py
• cuprum/unittests/__snapshots__/test_maturin_build.ambr
• cuprum/unittests/test_subprocess_timeout.py
• cuprum/unittests/test_subprocess_timeout_properties.py
• cuprum/unittests/test_timeout_capture_contract.py
• docs/developers-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)

[buzzybee-df12]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

🤖 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/_subprocess_execution.py`:
- Around line 92-105: Update _decode_consumer_result to use structural pattern
matching with match/case for dispatching result values, replacing the current
isinstance and None checks. Preserve the existing behavior: BaseException or
None returns an empty string when capture is true and None otherwise, while a
string result is returned unchanged.
- Around line 144-147: Update _settle_consumers and _decode_consumer_result so
cancellation of a reader that already buffered output preserves and returns that
partial data instead of becoming an empty string. Add a regression test covering
output written before an inherited pipe delays EOF, and verify the captured
output remains available after consumer cancellation.

In `@cuprum/unittests/test_timeout_capture_contract.py`:
- Around line 84-86: Complete the sentence in the timeout-capture contract
description by changing “the pipe's EOF events are:” to “the pipe's EOF events
are processed:”.

🪄 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: 83f1d1fa-3ee1-47e1-a44c-b71ed93e25a1

📥 Commits

Reviewing files that changed from the base of the PR and between 7f6ec92 and ce47ba5.

📒 Files selected for processing (7)

• CHANGELOG.md
• cuprum/_subprocess_execution.py
• cuprum/unittests/__snapshots__/test_maturin_build.ambr
• cuprum/unittests/test_subprocess_timeout.py
• cuprum/unittests/test_subprocess_timeout_properties.py
• cuprum/unittests/test_timeout_capture_contract.py
• docs/developers-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)

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[pandalump]

@coderabbitai Responses to the pre-merge checks, evaluated against head f991a5c7.

Unit Architecture (Error) — split response

(a) Injecting the EOF-grace policy: declined, as scope.

The precedent for injection exists and I want to be honest that it does — ExecutionContext.cancel_grace (cuprum/sh.py) is exactly that shape, and an output_grace field beside it would be a small, well-precedented addition. Two reasons not to add it now:

• No caller or test needs a non-default value. A knob whose only value is its default is a public API surface bought on speculation, and it would have to be honoured forever.
• The two windows are not the same kind of thing. cancel_grace is a policy about someone else's process — how long to let a command clean up before SIGKILL, which legitimately varies with what you are running. The EOF window is an internal scheduler-turn budget: how many turns a dead process's pipe needs before its buffered end-of-file is visible. That depends on the interpreter, not the environment, which is why it is measured in a quarter second and not in units an operator would reason about.

If a concrete case turns up — a workload where the readers reliably need longer, or a test that must drive the window — that is a good argument for the field, and I will add it. Please point at one if you have it.

(b) BaseException collapsed to "": answered, with the history.

I checked whether the swallowing is ours before deciding what to do about it.

It predates this PR. await asyncio.gather(*consumers, return_exceptions=True) with the result discarded unexamined arrived in 8f997b10 (#22) and was factored into _drain_stream_consumers by d7b2e797 (#226) — both well before this branch's merge base, 7f6ec929.

What did arrive with this PR, in 480f62e4, is narrower and worth stating: before it, a broken capturing reader decoded to None while a genuinely empty capture decoded to "", so the two were still distinguishable at that boundary. _decode_consumer_result merges both into "" to honour "a capturing run reports its streams as text". That is an intentional trade, but it removes the last discriminator, so your "indistinguishable" reading is fair on the current code even though the underlying swallow is inherited.

Rather than redesign the settlement path while a failure is propagating through it, the drain now says what it discards. cuprum/_subprocess_drain.py logs stream_consumer_failed at DEBUG for any settled consumer whose result is an exception other than a plain CancelledError, in the shape _log_suppressed_stream_close_error established: exc_info, plus structured extra carrying cuprum_operation (which stream) and cuprum_error_type. A plain cancellation stays unrecorded — every teardown cancels something, and recording it would make the record routine enough to ignore. Covered by test_subprocess_drain_logging.py, including the negative case; removing the record fails those tests.

Testing (Property / Proof) (Warning) — added

Correct, and cheap to fix. test_capturing_drain_reports_every_consumer_as_text generates capture=True across the completed/pending/failing states and asserts the string-not-None contract plus the text each state should leave behind. It samples a fourth state, partial — a reader cancelled holding a buffer — which runs the production _drain rather than a double, over Hypothesis-generated text, so the partial-preservation half of the contract is proved rather than asserted. Reverting the fix fails it.

Observability (Warning) — minimal DEBUG added, metrics declined

There was no logging in this module at all, which was a fair hit. Two DEBUG records now, both on the cuprum._subprocess_drain logger and both in the existing cuprum_-prefixed extra shape:

• stream_consumer_failed, described above.
• capture_eof_grace_expired, counting readers still parked when the window closed (cuprum_pending_readers, cuprum_timeout_s), mirroring stream_reader_drain_timeout in _streams.py. That record is how a wedged pipe — commonly a grandchild holding it open — shows up at all.

Metrics and trace events are declined here deliberately. Emitting them belongs with the ExecEvent contract work in #285 and #286, which owns naming, cardinality, and the exit-telemetry gaps you note (timeout reason, drain latency). Adding a counter from a teardown helper ahead of that contract would prejudge it, and would need its own issue.

User-Facing Documentation (Inconclusive) — resolved by documenting it

Your read was right that the guide was silent on the new behaviour. docs/users-guide.md now states that exc.output / exc.stderr are always strings under capture=True (empty when a stream produced nothing), that a bounded window lets readers observe end-of-file after the process dies, and that a reader cancelled at the window's close still yields what it had read.

---

Gates on f991a5c7: check-fmt, lint (pylint 10.00/10), typecheck (All checks passed!, zero diagnostics), test (862 Python passed / 52 skipped, 104 Rust passed), markdownlint, nixie, mbake validate, and cs delta origin/main HEAD ("No issues found!") — all green.

One structural note worth flagging: cuprum/_streams.py was at 399 lines and cuprum/_subprocess_execution.py at 415 behind a too-many-lines suppression, so neither could absorb these changes. Both were split along seams that already existed — cuprum/_stream_text.py and cuprum/_subprocess_drain.py, as pure moves in a separate commit — and the over-long drain property module was split likewise. Every touched module is now under the 400-line ceiling and the suppression is gone.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== HEAD =='
git rev-parse HEAD
git status --short
printf '%s\n' '== Changed files against merge base =='
base="$(git merge-base origin/main HEAD)"
git diff --name-status "$base"...HEAD

printf '%s\n' '== Relevant module outlines =='
fd -a '(_subprocess_drain|_subprocess_execution|_stream_text)\.py$' cuprum -x ast-grep outline {} --items all --type function --view compact

printf '%s\n' '== Drain implementation and logging references =='
rg -n -C 5 \
  '(_drain_stream_consumers|_settle_consumers|_decode_consumer_result|stream_consumer_failed|capture_eof_grace_expired|_log_suppressed_stream_close_error)' \
  cuprum

printf '%s\n' '== Tests and user documentation references =='
rg -n -C 3 \
  '(test_capturing_drain_reports_every_consumer_as_text|test_subprocess_drain_logging|output.*stderr|EOF|end-of-file|capture_eof_grace_expired)' \
  cuprum/unittests docs/users-guide.md

printf '%s\n' '== Public call sites of the drain =='
rg -n -C 3 '\b_drain_stream_consumers\s*\(' cuprum

Length of output: 1458

---

🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '== Relevant files =='
fd -a '(_subprocess_drain|_subprocess_execution|_stream_text)\.py$' cuprum
fd -a '(test_subprocess_drain_logging|test_timeout_capture_contract|test_subprocess_timeout|test_subprocess_timeout_properties)\.py$' cuprum/unittests

printf '%s\n' '== Implementation =='
rg -n -C 8 \
  '(_drain_stream_consumers|_settle_consumers|_decode_consumer_result|stream_consumer_failed|capture_eof_grace_expired|_log_suppressed_stream_close_error)' \
  cuprum/_subprocess_drain.py cuprum/_subprocess_execution.py 2>&1 || true

printf '%s\n' '== Drain call sites =='
rg -n -C 4 '\b_drain_stream_consumers\s*\(' cuprum 2>&1 || true

printf '%s\n' '== Regression and logging tests =='
rg -n -C 6 \
  '(test_capturing_drain_reports_every_consumer_as_text|test_subprocess_drain_logging|stream_consumer_failed|capture_eof_grace_expired|partial)' \
  cuprum/unittests 2>&1 || true

printf '%s\n' '== User documentation =='
rg -n -C 5 \
  '(TimeoutExpired|exc\.output|exc\.stderr|end-of-file|EOF|capture=True|capture)' \
  docs/users-guide.md 2>&1 || true

printf '%s\n' '== Recent commits and changed-file names =='
git log --oneline -12 2>&1 || true
git diff --name-status HEAD~12..HEAD 2>&1 || true

Length of output: 75550

---

@pandalump Close the pre-merge findings.

The fixed internal EOF grace remains appropriate. Do not add output_grace without a concrete caller requirement.

Keep the stream_consumer_failed DEBUG record. It preserves diagnostic information before the captured-stream contract maps unavailable text to "".

Keep the capture-aware drain split. The timeout path waits for EOF. The cancellation and stdin-writer-failure paths retain prompt teardown.

The property test covers completed, pending, failing, and partial consumers. The logging tests cover failure records, cancellation exclusion, and EOF-grace expiry. The user guide now states the capture=True timeout contract.

I verified these changes at f991a5c78e07b1e051203fd8e10d353acf03b270. I did not execute the stated validation commands in the sandbox.

---

✏️ Learnings added

Learnt from: pandalump
URL: https://github.com/leynos/cuprum/pull/293

Timestamp: 2026-08-07T13:00:41.141Z
Learning: In Cuprum, the capture EOF grace in `cuprum/_subprocess_drain.py` is an internal fixed scheduler-turn budget, not a public `ExecutionContext` policy. Do not add an `output_grace` configuration field unless a concrete caller requirement needs a non-default value.

---

Learnt from: pandalump
URL: https://github.com/leynos/cuprum/pull/293

Timestamp: 2026-08-07T13:00:41.141Z
Learning: In Cuprum, timeout capture in `cuprum/_subprocess_drain.py` must return text for each captured stream. A settled reader failure is logged at DEBUG as `stream_consumer_failed` with `cuprum_operation` and `cuprum_error_type`, then decodes to `""`; plain `asyncio.CancelledError` is not logged.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

_{You are interacting with an AI system.}

[codescene-access]

No quality gates enabled for this code.

[pandalump]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

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/unittests/test_subprocess_drain_logging.py`:
- Around line 45-48: Remove the explicit `return None` from
`_never_reaches_eof`; leave the function ending after the awaited event so its
`None` result is implicit and complies with R501.

In `@cuprum/unittests/test_subprocess_drain_properties.py`:
- Around line 59-71: Replace the conditional dispatch in the consumer-kind
helper with a match statement on kind, preserving the existing completed,
pending, failing, and partial behavior and the unsupported-kind ValueError
fallback.
- Around line 200-208: Update the run_case test around _drain_while_raising to
yield control to the event loop after creating the consumer tasks and before
invoking the drain, allowing the failing consumer to reach its exception first.
Preserve the primary TimeoutError assertion and ensure the test still verifies
cleanup without letting the scenario remain cancellation-only.

In `@cuprum/unittests/test_timeout_capture_contract.py`:
- Around line 208-218: Replace the four asyncio.sleep(0) polling turns in both
capturing and non-capturing cancellation tests with a blocked-read barrier.
Update the reader stub to expose an asyncio.Event when its post-buffer read
begins waiting, then await that event before calling drain.cancel() so
cancellation is tested only after _drain has consumed the buffer and blocked on
EOF.

🪄 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: 2fe3d3b7-9b34-4977-9233-de1287ea813b

📥 Commits

Reviewing files that changed from the base of the PR and between ce47ba5 and f991a5c.

📒 Files selected for processing (14)

• cuprum/_stream_text.py
• cuprum/_streams.py
• cuprum/_subprocess_drain.py
• cuprum/_subprocess_execution.py
• cuprum/_testing.py
• cuprum/unittests/__snapshots__/test_maturin_build.ambr
• cuprum/unittests/test_subprocess_drain_logging.py
• cuprum/unittests/test_subprocess_drain_properties.py
• cuprum/unittests/test_subprocess_timeout.py
• cuprum/unittests/test_subprocess_timeout_properties.py
• cuprum/unittests/test_timeout_capture_contract.py
• docs/developers-guide.md
• docs/users-guide.md
• typos.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)

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the explicit None return.

Delete Line 48. _never_reaches_eof has no non-None return path, so the
explicit return None violates R501.

Proposed fix

 async def _never_reaches_eof() -> str | None:
     """Block as a reader does on a pipe whose EOF never arrives."""
     await asyncio.Event().wait()
-    return None

Based on learnings and coding guidelines, functions that only return None
must use implicit None or a bare return, not return None.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	async def _never_reaches_eof() -> str | None:
	"""Block as a reader does on a pipe whose EOF never arrives."""
	await asyncio.Event().wait()
	return None
	async def _never_reaches_eof() -> str | None:
	"""Block as a reader does on a pipe whose EOF never arrives."""
	await asyncio.Event().wait()

🤖 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 `@cuprum/unittests/test_subprocess_drain_logging.py` around lines 45 - 48,
Remove the explicit `return None` from `_never_reaches_eof`; leave the function
ending after the awaited event so its `None` result is implicit and complies
with R501.

Sources: Coding guidelines, Learnings

[coderabbitai]

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the multi-branch dispatch with match.

Replace the four imperative branches with match kind cases. This helper
dispatches a fixed set of consumer states.

Proposed refactor

-    if kind == "completed":
-        await asyncio.sleep(0)
-        return text
-    if kind == "pending":
-        await asyncio.Event().wait()
-        return text
-    if kind == "failing":
-        await asyncio.sleep(0)
-        raise _ConsumerFailureError
-    if kind == "partial":
-        return await _partial_capture(text)
-    msg = f"unsupported consumer kind: {kind!r}"
-    raise ValueError(msg)
+    match kind:
+        case "completed":
+            await asyncio.sleep(0)
+            return text
+        case "pending":
+            await asyncio.Event().wait()
+            return text
+        case "failing":
+            await asyncio.sleep(0)
+            raise _ConsumerFailureError
+        case "partial":
+            return await _partial_capture(text)
+        case _:
+            msg = f"unsupported consumer kind: {kind!r}"
+            raise ValueError(msg)

As per path instructions, “Prefer structural pattern matching over
isinstance() or imperative decomposition.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if kind == "completed":
	await asyncio.sleep(0)
	return text
	if kind == "pending":
	await asyncio.Event().wait()
	return text
	if kind == "failing":
	await asyncio.sleep(0)
	raise _ConsumerFailureError
	if kind == "partial":
	return await _partial_capture(text)
	msg = f"unsupported consumer kind: {kind!r}"
	raise ValueError(msg)
	match kind:
	case "completed":
	await asyncio.sleep(0)
	return text
	case "pending":
	await asyncio.Event().wait()
	return text
	case "failing":
	await asyncio.sleep(0)
	raise _ConsumerFailureError
	case "partial":
	return await _partial_capture(text)
	case _:
	msg = f"unsupported consumer kind: {kind!r}"
	raise ValueError(msg)

🤖 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 `@cuprum/unittests/test_subprocess_drain_properties.py` around lines 59 - 71,
Replace the conditional dispatch in the consumer-kind helper with a match
statement on kind, preserving the existing completed, pending, failing, and
partial behavior and the unsupported-kind ValueError fallback.

Source: Path instructions

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Exercise failed consumers before cleanup.

Yield the event loop before raising primary. At Line 208, the finally block
drains newly created tasks before a "failing" consumer reaches its exception.
The test therefore verifies cancellation only and does not prove that a
completed reader failure cannot replace the primary error.

Proposed fix

             consumers = (
                 asyncio.create_task(_make_consumer(stdout_kind, "out")),
                 asyncio.create_task(_make_consumer(stderr_kind, "err")),
             )
+            for _ in range(4):
+                await asyncio.sleep(0)
             primary = TimeoutError("primary")
             with pytest.raises(TimeoutError) as exc_info:
                 await _drain_while_raising(primary, consumers)

As per coding guidelines, new behavioural coverage must be substantive and
non-vacuous.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	async def run_case() -> None:
	"""Drain failing consumers while a TimeoutError is propagating."""
	consumers = (
	asyncio.create_task(_make_consumer(stdout_kind, "out")),
	asyncio.create_task(_make_consumer(stderr_kind, "err")),
	)
	primary = TimeoutError("primary")
	with pytest.raises(TimeoutError) as exc_info:
	await _drain_while_raising(primary, consumers)
	async def run_case() -> None:
	"""Drain failing consumers while a TimeoutError is propagating."""
	consumers = (
	asyncio.create_task(_make_consumer(stdout_kind, "out")),
	asyncio.create_task(_make_consumer(stderr_kind, "err")),
	)
	for _ in range(4):
	await asyncio.sleep(0)
	primary = TimeoutError("primary")
	with pytest.raises(TimeoutError) as exc_info:
	await _drain_while_raising(primary, consumers)

🤖 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 `@cuprum/unittests/test_subprocess_drain_properties.py` around lines 200 - 208,
Update the run_case test around _drain_while_raising to yield control to the
event loop after creating the consumer tasks and before invoking the drain,
allowing the failing consumer to reach its exception first. Preserve the primary
TimeoutError assertion and ensure the test still verifies cleanup without
letting the scenario remain cancellation-only.

Source: Coding guidelines

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Replace turn-count polling with a blocked-read barrier.

Do not use four asyncio.sleep(0) turns as proof that _drain consumed
_PARTIAL_TEXT and reached its next read(). A scheduler can leave the task
unstarted. The capturing test can then raise CancelledError, and the
non-capturing test can pass without exercising cancellation during a blocked
read.

Expose an asyncio.Event from the reader stub when its post-buffer read() is
waiting. Await that event before calling drain.cancel() in both tests.

Based on the PR objective, avoid relying on event-loop scheduling.

Also applies to: 235-243

🤖 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 `@cuprum/unittests/test_timeout_capture_contract.py` around lines 208 - 218,
Replace the four asyncio.sleep(0) polling turns in both capturing and
non-capturing cancellation tests with a blocked-read barrier. Update the reader
stub to expose an asyncio.Event when its post-buffer read begins waiting, then
await that event before calling drain.cancel() so cancellation is tested only
after _drain has consumed the buffer and blocked on EOF.