Skip to content

docs: mark #125 FD-ownership hazard resolved in roadmap 8.1 (#125) - #232

Open
leynos wants to merge 6 commits into
mainfrom
issue-125-rust-extract-fd-borrow-raii-helper-to-centralise-mem-forget-ownership-contract
Open

docs: mark #125 FD-ownership hazard resolved in roadmap 8.1 (#125)#232
leynos wants to merge 6 commits into
mainfrom
issue-125-rust-extract-fd-borrow-raii-helper-to-centralise-mem-forget-ownership-contract

Conversation

@leynos

@leynos leynos commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Summary

Roadmap Section 8.1 previously bundled issues #124 and #125 into a single outstanding "pump file-descriptor close race" work item. The #125 panic-unwind FD ownership hazard is now fixed in code and regression-tested, so this documentation-only change stops presenting it as outstanding implementation work.

Closes #125.

Changes

  • Record #125 as resolved: with_borrowed_reader wraps the borrowed reader in ManuallyDrop, keeping a caller-owned reader FD open on both success and panic-unwind; pump_stream/consume_stream route through the helper; and rust/cuprum-rust/src/lib_tests.rs regression-tests that the borrowed FD survives normal and panicking operations.
  • Rescope task 8.1.1 (the silent Bad file descriptor shutdown close race) to the still-outstanding #124.

Verification

make markdownlint and make nixie both pass (docs-only diff).

References

🤖 Generated with Claude Code

Summary by Sourcery

Documentation:

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

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

Walkthrough

Changes

Roadmap status update

Layer / File(s) Summary
Record resolved FD ownership work
docs/roadmap.md
Mark Phase 8.1 as resolved, document ManuallyDrop protection and helper routing, reference regression tests, and retain the shutdown Bad file descriptor fix in the checklist.

Suggested labels: Roadmap

Poem

Keep borrowed descriptors bright,
Through calm returns and panic flight.
Mark the ownership race as done,
Leave shutdown’s error for one.

🚥 Pre-merge checks | ✅ 18 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Developer Documentation ⚠️ Warning Roadmap marks #125 resolved, but with_borrowed_reader/ManuallyDrop is not documented in docs/developers-guide.md or an ADR, and 8.1.1 remains unchecked. Document the helper contract in docs/developers-guide.md and the relevant ADR/execplan, then check off the completed roadmap item while leaving 8.1.1 open.
Linked Issues check ❓ Inconclusive The diff only updates roadmap documentation, so the linked code requirements cannot be verified from this PR alone. Review the actual code changes for #125 and confirm the RAII helper, unwind safety, and regression tests are in place.
✅ Passed checks (18 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the roadmap docs change and includes the linked issue reference.
Description check ✅ Passed The description stays on-topic and accurately describes the roadmap update.
Out of Scope Changes check ✅ Passed No unrelated changes appear beyond the roadmap status update and task rescope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Testing (Overall) ✅ Passed Docs-only diff; no new behaviour or tests introduced. Existing lib_tests.rs exercises real FD ownership on success and panic, and would catch close-on-unwind bugs.
User-Facing Documentation ✅ Passed PASS: Treat this as docs-only; no user-facing behaviour changed, and docs/users-guide.md already covers the Rust stream backend.
Module-Level Documentation ✅ Passed Only docs/roadmap.md changed; no modules were added or edited, so the module-docstring rule is not applicable here.
Testing (Unit And Behavioural) ✅ Passed Treat the PR as passing: the regression tests cover both successful use and panic-unwind FD invariants, and this docs-only change needs no behavioural or E2E test.
Testing (Property / Proof) ✅ Passed Docs-only roadmap update; no new invariant or proof obligation is introduced, and existing unwind regression tests already cover the FD-ownership hazard.
Testing (Compile-Time / Ui) ✅ Passed Docs-only change; no compile-time/UI output was altered, and the crate already ships a trybuild compile-test harness.
Unit Architecture ✅ Passed PASS: This is a docs-only roadmap update; only docs/roadmap.md changed, so no code paths, dependencies, or side-effect boundaries were altered.
Domain Architecture ✅ Passed PASS: The PR is docs-only, touching docs/roadmap.md and not altering any domain, adapter, or infrastructure code or boundaries.
Observability ✅ Passed Docs-only roadmap update; no operational behaviour, logging, metrics, tracing, or alerts were introduced or altered.
Security And Privacy ✅ Passed Verify the diff: only docs/roadmap.md changed, and it only rewords roadmap status with issue refs and a helper summary; no secrets, auth, permissions, or data exposure added.
Performance And Resource Use ✅ Passed Only docs/roadmap.md changed; no executable code, loops, allocations, I/O, or hot-path behaviour were introduced.
Concurrency And State ✅ Passed PASS — keep this docs-only: the Rust helper already makes FD ownership explicit with unwind-safe ManuallyDrop and panic/success regression tests.
Architectural Complexity And Maintainability ✅ Passed PASS: The PR is docs-only; it adds no new layers or dependencies, and the existing Rust helper/test setup already centralises the FD contract.
Rust Compiler Lint Integrity ✅ Passed Only docs/roadmap.md changed; no Rust code, lint suppressions, or suspect clones were added, so compiler-lint integrity is unaffected.
📋 Issue Planner

Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).

View plan for ticket: #125

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-125-rust-extract-fd-borrow-raii-helper-to-centralise-mem-forget-ownership-contract

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

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

@leynos
leynos marked this pull request as ready for review July 26, 2026 15:09

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Please try again later or upgrade to continue using Sourcery

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 49f363b521

ℹ️ 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".

Comment thread docs/roadmap.md Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@docs/roadmap.md`:
- Around line 458-460: Update the introductory roadmap description for step
8.1.1 to state that it targets or will remove the Rust pump shutdown failure,
matching the unchecked item and outstanding issue `#124`; do not describe the
failure as already removed.
🪄 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: 9c116b5c-264d-4723-996a-356377769327

📥 Commits

Reviewing files that changed from the base of the PR and between aa1fd32 and 49f363b.

📒 Files selected for processing (1)
  • docs/roadmap.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)

Comment thread docs/roadmap.md Outdated
@buzzybee-df12

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ 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 coderabbitai Bot removed the Issue label Jul 27, 2026
codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

leynos and others added 6 commits July 28, 2026 23:00
Section 8.1 previously presented issues #124 and #125 as a single
outstanding pump FD close-race work item. The #125 panic-unwind FD
ownership hazard is now fixed in code and regression-tested:
with_borrowed_reader wraps the borrowed reader in ManuallyDrop so a
caller-owned reader FD stays open on success and panic-unwind, and
rust/cuprum-rust/src/lib_tests.rs asserts the borrowed FD survives both
normal and panicking operations.

Split the section so 8.1.1 scopes the still-outstanding shutdown
close-race work to #124, and record #125 as resolved.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Section 8.1 introduction stated the step "removes" the silent Rust
pump shutdown failure, but that #124 close-race work (task 8.1.1) is
still unchecked. Reword to "aims to remove" so the intro describes the
outstanding intent rather than a completed outcome. The resolved #125
panic-unwind status and the 8.1.1 checklist item are unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Issue #124 covers the splice-loop dedup and drain_reader EINTR fix, not
the _UnixWritePipeTransport._call_connection_lost bad-FD shutdown race.
No dedicated issue tracks that race, so remove the incorrect #124 links
from the Section 8.1 intro and task 8.1.1 and note it remains an
unlinked, outstanding item. The resolved #125 borrowed-reader
panic-unwind status is preserved unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Record the with_borrowed_reader RAII helper contract in the three places
that describe Rust FD ownership:

- developers-guide.md: new "Rust FD-borrow ownership contract" section
  (canonical) covering the ManuallyDrop borrow, the consumed-writer
  asymmetry, the safety contract, and the panic-unwind regression tests.
- adr-002: note the raw-FD ownership known-risk is now mitigated by the
  helper, cross-referencing the developers' guide.
- execplan 4-2-1: addendum recording that the helper superseded the
  planned std::mem::forget mitigation, plus the required revision note.

Uses the repo's typos-enforced "centralize" spelling. make markdownlint
and make nixie pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The unchecked 8.1.1 checklist item already conveys that the pump
shutdown close race is outstanding, so the explicit sentence saying so
was redundant. Remove it; the intro now just cites the baseline finding
and ADR-002.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebasing onto main brought in the repository-wide Markdown reformat, so
run the new #125 ownership-contract prose through mdformat-all to match
the surrounding wrap width. Also drop a stray duplicate blank line the
entity-aware merge driver left before the new developers' guide heading.

Content is unchanged; this is wrapping only. Unrelated pre-existing
format drift elsewhere in the docs tree is deliberately left alone so
this branch stays scoped to the #125 documentation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lodyai
lodyai Bot force-pushed the issue-125-rust-extract-fd-borrow-raii-helper-to-centralise-mem-forget-ownership-contract branch from 735bcd4 to 9235555 Compare July 28, 2026 21:09
codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

@codescene-access codescene-access Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No quality gates enabled for this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rust: extract FD-borrow RAII helper to centralise mem::forget ownership contract

2 participants