Skip to content

Commit cb1747c

Browse files
vmarkushinclaude
andcommitted
docs(review): ledger pass-70 nightly-soak reporter cancellation finding (P2, deferred)
Codex review pass 70 (after a ChatGPT-backend outage forced two retries) raised a single P2: the report-soak-failures job's `if: !cancelled()` guard may skip the reporter when a needed soak job is cancelled by its own job-level timeout, dropping the intended timeout signal. Deferred rather than blindly changed — the exact GitHub Actions cancellation semantics need validation with a throwaway workflow run, and the current guard was chosen deliberately to avoid false regression reports on a manual run cancellation. Recorded in the follow-up ledger. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 9954ebf commit cb1747c

1 file changed

Lines changed: 24 additions & 0 deletions

File tree

.claude/vmark-pr-review-follow-ups.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,30 @@ from every pass is either fixed in the PR or recorded here.
55

66
## Unresolved
77

8+
- **nightly-soak `report-soak-failures` may skip on a job-level-timeout
9+
cancellation** (`.github/workflows/nightly-soak.yml` reporter job `if:` guard)
10+
- Impact/evidence: Codex pass 70 P2 (2026-09-03). The reporter is guarded by
11+
`if: !cancelled() && ...`; Codex argues that when a needed soak job is
12+
cancelled — including by its own job-level `timeout-minutes` (the likeliest
13+
shape of a reintroduced hang/livelock) — the reporter is skipped, so the
14+
`needs.*.result == "cancelled"` branch never fires and a hung setup/build
15+
produces neither an issue comment nor a step summary. Suggested remedy: an
16+
`always()`-style dependency guard that still excludes a genuine user
17+
cancellation of the whole run.
18+
- First/most-recent pass: Codex pass 70 (2026-09-03).
19+
- Why unaddressed: the exact GitHub Actions semantics here are subtle and
20+
NOT verifiable from this repo — whether `cancelled()` is true in a dependent
21+
job when only an upstream job hits its own `timeout-minutes` (vs. a
22+
workflow-level cancellation) determines whether the current guard is already
23+
correct, and the fix must distinguish a job-timeout from a user run
24+
cancellation (the current `!cancelled()` was chosen deliberately, per its
25+
own comment, to keep a manual run cancellation from posting a bogus hang
26+
report). Getting it wrong reintroduces false regression reports or keeps the
27+
missed-signal gap. It needs validation with a throwaway workflow run, which
28+
is out of scope for a blind edit in this review loop. (Same reporter area as
29+
the pass-? F6 hardening already in the PR.)
30+
- Suggested Linear title: "nightly-soak: ensure report-soak-failures fires on a soak job's job-level timeout without reporting user cancellations"
31+
832
- **Startup reconciliation of an execution node whose head diverges from the
933
persisted L2 head is unsolved and was REVERTED** (`crates/node/src/args.rs`
1034
startup head-repair loop; the sign-failure fail-stop in

0 commit comments

Comments
 (0)