Skip to content

test: keep overflow fixtures idle after broken pipes - #305

Merged
HemSoft merged 1 commit into
mainfrom
test/issue-275-overflow-broken-pipes
Nov 5, 2026
Merged

HemSoft merged 1 commit into
mainfrom
test/issue-275-overflow-broken-pipes

Conversation

@HemSoft

@HemSoft HemSoft commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Overflow can close a fixture's output pipes before the runner kills it. A BrokenPipe panic then exits through LLVM's profile writer, and the kill can truncate that write. The recurrence and diagnosis explain the failed unchanged-base collection in PR #303.

Keep finite output strict. Held/overflow fixtures now tolerate BrokenPipe and stay idle until killed. Other write errors abort without entering the profile writer. Add a closed-pipes regression with owned-child cleanup and an instrumented control that requires exactly diverted, sent and no profile.

Acceptance criteria

  • Establish the exit path with bounded inert Linux observations and preserve the original corrupt artifact. Hosted timing remains unobserved.
  • Add a targeted fixture fix and regressions. Restoring the old unwrap makes both checks fail with exit 101; aborting on BrokenPipe also fails.
  • Preserve finite-output, overflow, timeout, process-tree, scoped environment/cwd and exit-status assertions. Production runner, strict profile parsing, coverage scope, caps, deadlines, thresholds and baseline are unchanged.

Definition of done

  • Separate PR from current main d213f24be8c939bdbe345fce371cd7efd0e88ef3; only two fixture files changed.
  • All seven required checks passed on this head in CI run 42112845053, including strict metrics and all three platforms.
  • Authenticated current-head SFL PASS and Codex clean, with zero unresolved threads. Author, editor and complete edit histories checked; formal approvals required: zero.

Validation

  • cargo fmt --all --check and git diff --check: passed.
  • Inert Linux probe, rustc 1a2c4797e, LLVM 23.1.4: candidate plain 5/5 and instrumented 3/3 passed; every coverage set strictly merged with the pinned llvm-profdata. Closed control traced only diverted, sent, without a raw profile.
  • Red checks: strict-write mutant failed 3/3 plain runs and the instrumented regression with child exit 101. Its isolated profile control entered the LLVM write callback. Abort-on-BrokenPipe mutant failed too.
  • Probe limit: real runner and fixture sources in a small offline project, with a core home-dir shim and stubbed agy runner; this is not the repository test binary or cargo-llvm-cov. Stable Clippy passed with probe-only warnings. Full cargo clippy --workspace --all-targets --locked -- -D warnings and cargo test --workspace --locked passed in hosted CI on Windows, macOS and Ubuntu.
  • Evidence retained on Mini at /tmp/chief310-275fix-096c0d89c644, manifest SHA-256 d1fcc4338be8437465d0a0a33b2e61e2b33693b70cdc6c5536f91b2ab7cdb76e.
  • Local commit used a command-only empty hooks path because the hook runs native subprocess fixtures prohibited on Home. No repository hook/config or CI gate changed; no fixtures or app were launched on Home. Hosted security, mutations, metrics and platform gates passed.

Risks and deferred work

This PR's metrics base still contains the old fixture and remains exposed to that race. Candidate code cannot fix its base. No retries, corrupt-profile suppression or baseline substitution are used. After this lands, update PR #303 onto fixed main and verify its actual METRICS_BASE, then advance stacked PR #304 and qualify all required CI and triggered Package jobs. Neither waiting PR is ready to merge yet.

Closes #275

Merge evidence

Checked head 682a3f34e907538f3a3de868540f62abe0fcb62a; all seven GitHub Actions checks passed with integration ID 15368, authenticated SFL/Codex clean, zero unresolved threads and zero formal approvals required. Guarded squash merge landed as 9df7787f0a68e8f23ee5235b89d1ad01014f6a97. Mini and Home main both fast-forwarded to that commit. The original audit report is preserved unchanged.

@HemSoft HemSoft added the model:gpt-6.1-sol PR authored by gpt-6.1-sol label Nov 5, 2026
@HemSoft

HemSoft commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

@sfl review

@HemSoft

HemSoft commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-05T21:30:13.849601Z 682a3f3 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: 682a3f34e9

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

@hemsoft-sfl-reviewer hemsoft-sfl-reviewer 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.

✅ Verdict: PASS • Round 1

Review done by claude-opus-5-5 (adversarial to codex)

The PR changes only test fixtures. The held and overflow fixtures now treat BrokenPipe as the end of their output and stay idle until killed, any other write error aborts, and new closed-pipe regressions run in both the plain and the instrumented profile tests. Approve: the comments match the runner code at the exact SHA, the new checks fail on the old unwrap path, and no Critical, High or Medium defect was found in the full diff.

🔨 Full-Spectrum Review Summary

Effort: high
Review time: 2m 15s
Token count: Unavailable

Pass 🔴 Critical 🟠 High 🟡 Medium ⚪ Low
🔒 Security 0 0 0 0
✅ Accuracy 0 0 0 0
🧰 Quality & Maintainability 0 0 0 0
Total 0 0 0 0

Key Findings

No material defects found.

Coverage

  • Files reviewed:
    • crates/codexbar-providers/tests/command_runner.rs
    • crates/codexbar-providers/tests/command_runner/profile.rs
  • Context read:
    • crates/codexbar-providers/src/command.rs
    • crates/codexbar-providers/src/command/pipe.rs
    • crates/codexbar-providers/Cargo.toml
    • AGENTS.md
  • Verified:
    • Compare base_commit.sha is d213f24be8c939bdbe345fce371cd7efd0e88ef3 and the only commit is 682a3f34e907538f3a3de868540f62abe0fcb62a (ahead 1, behind 0, 2 files changed)
    • Comment says the runner drops both pipes before the kill: confirmed. collect returns Err(Failed) from Capture::drain, which drops both Capture readers before Running::stop calls pipe::stop_group
    • output joins both writer threads inside thread::scope before held calls sent, so a sent marker means both writes returned
    • The old emit(...).unwrap() path would panic on BrokenPipe and exit before sent, so close_readers would fail with its 'exited on closed pipes' panic: the regression catches the old bug
    • emit (finite output) still unwraps, so the finite-stream and cap-boundary assertions stay strict
    • The closed profile helper calls hold before divert, so released finds closed.ready and the lock file. OwnedChild::drop kills and reaps it first
    • The closed profile assertion requires exactly diverted,sent with no .profraw. position and boundary trace every runtime seek and write, so any started profile write would show
    • The new evidence helper keeps the old profile-gathering logic, apart from the borrowed-name tuple type
    • scoped-parent still expects Err(Failed) from held stdout 4MiB+1, and the overflow mode now uses spill; the other mode expectations are unchanged
    • harness = false in Cargo.toml confirms main is the test entry point, so the new close_readers call runs on every platform
    • Check runs on the head SHA: ubuntu, macOS, build and rust-mutations succeeded; Windows, rust-metrics and security-scan were still in progress
  • Not verified:
    • Whether Rust maps the Windows closed-pipe write error (ERROR_NO_DATA) to ErrorKind::BrokenPipe. This lives in the standard library, not this repository; if it does not, the Windows fixture would abort and close_readers would fail. The Windows CI job had not finished
    • Hosted coverage (cargo-llvm-cov) timing and the rust-metrics result, which was still in progress
    • PR-body claims about the mutant runs, Mini evidence and probe results; these are outside the repository
    • Linux-only contained Windows job/drain paths were not re-read; the diff doesn't touch them
  • Tests run: None. Reviewer execution is disabled.

HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 682a3f34e907538f3a3de868540f62abe0fcb62a • SFL v0

@HemSoft
HemSoft merged commit 9df7787 into main Nov 5, 2026
7 checks passed
@HemSoft
HemSoft deleted the test/issue-275-overflow-broken-pipes branch November 5, 2026 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

model:gpt-6.1-sol PR authored by gpt-6.1-sol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent truncated command-runner profiles from failing metrics CI

1 participant