Skip to content

test(reliability): bound benchmark Git metadata lookup - #666

Closed
seonghobae wants to merge 1 commit into
mainfrom
fix/benchmark-git-metadata-timeout-555
Closed

test(reliability): bound benchmark Git metadata lookup#666
seonghobae wants to merge 1 commit into
mainfrom
fix/benchmark-git-metadata-timeout-555

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Reliability defect

Advances #555 with a disjoint local-metadata subprocess slice. Protected main a7be9eb8ca0b807364173f8471fc300e3cf9350f calls git rev-parse HEAD in scripts/build_benchmark_report.py::_source_commit() without any deadline and catches all failures as "unknown". A hung local Git metadata child can therefore stall benchmark evidence indefinitely, and a timeout must not be silently converted into successful provenance evidence.

Intentional RED

Exact test-only head: 7887a80fe4e251e9f0db92355fcd5b6e0127f16a, directly based on current protected main.

tests/test_benchmark_git_metadata_timeout.py reaches the production _source_commit() boundary with subprocess.run replaced by a deterministic TimeoutExpired. It requires:

  • one named package-owned GIT_METADATA_TIMEOUT_SECONDS deadline in (0, 30];
  • the deadline to be passed to subprocess.run;
  • timeout to fail closed with the stable message source commit lookup timed out; and
  • child stdout/stderr content to remain absent from the public error.

Current protected-main code is expected to fail at the actual boundary because it passes no timeout and broadly converts the timeout into "unknown". Setup/import/fixture/changelog failure is not valid RED.

Minimum GREEN after RED proof

  1. Add the smallest named local-Git-metadata deadline and pass it through subprocess.run(timeout=...).
  2. Catch subprocess.TimeoutExpired explicitly and raise stable non-reflective RuntimeError("source commit lookup timed out") without embedding command/output/path content.
  3. Preserve existing successful commit lookup and current non-timeout fallback behavior unless test evidence proves a separate defect.
  4. This git rev-parse operation does not spawn a package-owned descendant tree; do not add process-group machinery without evidence.
  5. Add focused boundary tests, concise reliability doctoring and an authoritative changelog fragment after GREEN; render/check managed CHANGELOG.md before Ready.
  6. Require exact changed-production statement/branch coverage and one unchanged final head with full applicable Python/Rust/PyO3/package/GPU/fuzz/Security/SAST/current-head review evidence.

This slice is intentionally disjoint from #578, which owns ignored Rust scientific-study subprocesses, and from #657, which owns PR-governance GitHub CLI retry behavior. Canonical cross-cutting documentation remains #604/#621. No numerical formula, dependency/workflow, package version/release or reviewer-authority change belongs here.

Keep Draft until RED→GREEN history and all final gates are complete.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4c0580f4-018e-442a-9af4-a32db09fdc40

📥 Commits

Reviewing files that changed from the base of the PR and between 384516b and e8440f5.

📒 Files selected for processing (2)
  • scripts/build_benchmark_report.py
  • tests/test_benchmark_git_metadata_timeout.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Exact bounded GREEN implementation for Draft #666. Immediately before any write, refetch source head 7887a80fe4e251e9f0db92355fcd5b6e0127f16a, protected main a7be9eb8ca0b807364173f8471fc300e3cf9350f, scripts/build_benchmark_report.py, the fail-first test, and PR comments; abort if either ref moved or another writer appeared.

Fail-first evidence is valid: CI run 31413008925, Python job 93535310561 completed with exactly 1 failed, 2948 passed, 2 skipped; the sole failure is tests/test_benchmark_git_metadata_timeout.py::test_source_commit_timeout_is_bounded_and_fails_closed because _source_commit() swallowed TimeoutExpired and returned without raising. Rust/PyO3, package/reinstall/release acceptance, GPU, fuzz, Security Scan 31413009299, and SAST 31413008949 are green.

Implement only the root-cause fix:

  1. Add one documented package-owned GIT_METADATA_TIMEOUT_SECONDS with a short local-metadata deadline in (0, 30] and pass it as timeout= to the existing subprocess.run.
  2. Catch subprocess.TimeoutExpired before the existing non-timeout fallback and raise exactly RuntimeError("source commit lookup timed out") with no child stdout/stderr/command/path reflection. Preserve successful lookup and the current non-timeout "unknown" fallback.
  3. Strengthen focused tests only where needed for success, ordinary non-timeout fallback, deadline forwarding, and non-reflection; do not add process-group machinery for this single local git rev-parse call.
  4. Add concise reliability doctoring plus an authoritative docs/changelog.d fragment only after focused GREEN; render/check managed CHANGELOG.md with the repository renderer.
  5. Run focused tests, git diff --check, then the full applicable suite. Keep Draft. Do not touch canonical docs: establish canonical product and architecture baseline #604 docs, reliability: bound ignored Rust subprocess deadlines #578 scientific subprocess work, fix(governance): lighten PR history snapshot to avoid GraphQL 502 #657 GitHub-CLI retry logic, numerical formulas, dependencies/workflows, package version/release, or reviewer authority.

Stop writing if exact head/base or writer state changes. After the final source mutation, leave fresh exact-head CI/Security/SAST/review/Ready/merge to the maintainer loop.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Fresh single-writer rebind for Draft #666 after protected main advanced. The earlier handoff 5243634147 was explicitly bound to main a7be9eb8ca0b807364173f8471fc300e3cf9350f; it has no receipt/reaction and is stale now. Immediately before any write, refetch and proceed only if source head is still exactly 7887a80fe4e251e9f0db92355fcd5b6e0127f16a, protected main is exactly 36407d6bee04da3bf2310b6377c9fe1c80e4b982, scripts/build_benchmark_report.py still contains the unbounded _source_commit() call, the fail-first test remains the sole branch change, and no other writer has appeared. Abort rather than race if any identity moves.

The RED history remains diagnostic evidence: the public _source_commit() boundary swallowed subprocess.TimeoutExpired. Revalidate that current protected main did not independently repair this exact script path, then implement only the minimum GREEN if the defect persists: define documented GIT_METADATA_TIMEOUT_SECONDS in (0, 30], pass it to subprocess.run(..., timeout=...), catch subprocess.TimeoutExpired before the ordinary fallback and raise exact non-reflective RuntimeError("source commit lookup timed out") from None, while preserving successful stripped commit output and the historical "unknown" fallback for ordinary non-timeout Git/OS failures. Do not add process-group machinery or touch report semantics, acquisition claims, dependencies/workflows, package version/release, or canonical #604/#621 docs.

Run the focused timeout contract first, then applicable full validation on one unchanged source head. If product GREEN, add concise #555 reliability doctoring and an authoritative changelog fragment, render/check managed CHANGELOG.md, and recreate CI/Security/SAST/current-head review evidence against the live base. Keep Draft until all gates are terminal.

@seonghobae
seonghobae force-pushed the fix/benchmark-git-metadata-timeout-555 branch from 7887a80 to dd6f981 Compare August 11, 2026 05:01
@seonghobae
seonghobae marked this pull request as ready for review August 11, 2026 05:01
@seonghobae
seonghobae force-pushed the fix/benchmark-git-metadata-timeout-555 branch 5 times, most recently from 996852d to 2685090 Compare August 11, 2026 05:56
@seonghobae

Copy link
Copy Markdown
Contributor Author

Superseded by unified Git metadata timeout fix PR (see fix/git-metadata-timeouts-unified).

@seonghobae seonghobae closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant