v1.3.0: wall-clock timeout for the engine (default 20 min, tunable) - #8
Conversation
New input timeout-minutes (default 20). Wraps the `ocr review` call in
run_pass with GNU `timeout --kill-after=10s ${N}m`: SIGTERM at N minutes,
SIGKILL 10s later if the engine ignored the term. This caps how long a
single review pass can run — previously there was no wall-clock guard,
so a large-PR + slow-model combo could burn 40+ minutes before failing
closed on subtask timeouts (observed at 43min on a 71-file PR).
On timeout (rc 124 or 137), run_pass drops a marker in $RUNNER_TEMP;
run_review reads it and prints a wall-clock-specific error, distinct
from the existing 'no usable result' error so log readers can tell
which mode failed and whether to bump timeout-minutes or investigate
the engine.
Accepts decimals so tests can force a timeout quickly (e.g. "0.5" = 30s).
Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
README inputs table gets a row explaining the wall-clock guard, the distinct fail-close error, decimals for testing, and the exhaustive-mode caveat (per-pass budget → worst case timeout-minutes × 3). Example workflow gets a commented `# timeout-minutes: "20"` line so consumers see it as a tunable when they copy the file.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐳 Orca-Code-Review
Found 2 issues in this PR: 🟠 1 P1 · ⚪ 1 P3.
Reviewed via OrcaRouter — Route Smarter. Ship Safer. Spend Less.
| timeout --kill-after=10s "${TIMEOUT_MIN}m" \ | ||
| ocr review --from "$BASE" --to "$HEAD" \ | ||
| --background "$(cat "$BACKGROUND")" --format json \ | ||
| > "$1" 2> "$REVIEW_LOG" || rc=$? |
There was a problem hiding this comment.
🟠 P1 The code updates introduce a timeout command to control execution duration, but there may still be a potential race condition between the termination signal and the timeout. This could lead to unpredictable states if other processes or instructions are not correctly managing concurrent operations. Review external/environment interactions to ensure proper sequence and completion.
| PRs on shipped models; bump for very large diffs or slow-per-call | ||
| models where per-file review takes longer. | ||
| required: false | ||
| default: "20" |
There was a problem hiding this comment.
⚪ P3 Ensure consistency in spelling and naming conventions for YAML keys, like timeout-minutes, to maintain readability and maintainability.
![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: 9ee16f8955
ℹ️ 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".
| if [ "$rc" = "124" ] || [ "$rc" = "137" ]; then | ||
| echo "::warning::Engine wall-clock timeout after ${TIMEOUT_MIN} minute(s) (rc=$rc)." | ||
| printf 'wall-clock timeout after %sm\n' "$TIMEOUT_MIN" > "$RUNNER_TEMP/wallclock-timeout" |
There was a problem hiding this comment.
When exhaustive is enabled, a timeout from pass 2 or 3 writes the wall-clock marker here, but the caller at the extra-pass branch treats any non-policy run_pass failure as best-effort and only warns before continuing with the earlier result. That means a run can hit the new wall-clock ceiling and still publish/pass using an incomplete exhaustive review, despite the new input documenting that timeouts fail closed and that each exhaustive pass has its own budget; timeout exits 124 on expiry per timeout --help, so this is exactly the path those passes take.
Useful? React with 👍 / 👎.
Post-merge review of the two self-review findingsThe self-review check on this PR reported FAILURE with 1 P1 + 1 P3. I squash-merged without opening the findings, which was a process mistake — the correct flow is Judgment: both findings are false positives. v1.3.0 does not need to be reverted. P1 — action.yml:758 (
|
1 participant
Orca-Code-Review — push 1
Tier: STRONG (final pass) — blocked
❌ 1 finding blocks merge
Summary
timeout-minutesinput (default20). Wrapsocr reviewin GNUtimeout --kill-after=10s: SIGTERM at N minutes, SIGKILL 10s later. On timeout the run fails closed with a distinctwall-clock timeout after Nmerror (separate from the existingno usable resultfail, so log readers can tell which mode tripped).# timeout-minutes: "20"in the example workflow.Motivation
Without a wall-clock guard, a large-PR + slow-model combo has been observed running 43 minutes before failing closed on subtask timeouts. A hard ceiling makes the failure mode predictable (fail fast, clear message) and stops burning quota on runs that were going to fail anyway.
Behavior change
Consumers whose reviews previously took 20+ minutes will now fail closed at 20 minutes instead of running to completion (or eventual engine failure). The error message tells them to bump
timeout-minuteson the workflow. Inexhaustivemode each engine pass has its own budget, so worst-case whole-review wall time =timeout-minutes × 3.Test plan
Verified end-to-end on
ZhenghuaBao/Code-Review-TestPR #7:timeout-minutes: "0.1"(6s) → engine killed at 6s, rc=124, warning + wall-clock-specific error printed, step failed closedtimeout-minutes: "2"(2m) → review completed normally in ~29s, gate ran, no wall-clock artifacts in logaction.ymland example workflow🤖 Generated with Claude Code