Skip to content

fix(queue): preserve terminal pre-execution failures - #2213

Open
seonghobae wants to merge 3 commits into
codex/pr1142-current-main-successorfrom
codex/queue-health-terminal-preexecution-failure
Open

seonghobae wants to merge 3 commits into
codex/pr1142-current-main-successorfrom
codex/queue-health-terminal-preexecution-failure

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Parent owner: #1150 exact 42bb922f03bf75aed1bc1931d9fbaf04a5433e20. Incident evidence: #712 and OriginWeave exact-head Actions specimens. Current exact head is ecbdc3507148e2ab922deba8432db87c129cf433.

Finding

The canonical read-only queue-health collector preserves startup_failure and pre-runner cancelled diagnostics, but parent #1150 drops a distinct terminal class: a current-head run can end completed/failure with a materialized failed job that has no runner assignment and steps=[]. Because failure is excluded from TERMINAL_DIAGNOSTIC_STATUSES, that run is filtered before job evidence is fetched. If such a job is supplied offline, the core report otherwise labels the row generic terminal evidence.

That state must remain fail-closed and distinct from a product/security test that actually obtained a runner and executed steps.

RED → minimal causal repair

Test-first 5de066e45979a0c4b910191a9388590b3f75f065 adds one behavior-level regression fixture. On the parent implementation it is deterministically RED because the completed/failure run is omitted before its job endpoint is read. The contract requires current-head completed/failure + job runner_id=0 + empty runner + steps=[] to survive collection and report a distinct non-passing terminal-pre-execution state.

Production successor b016348d1ed35304eb2504d30093922dbf2d42ae makes the bounded owner repair only in scripts/ci/actions_queue_health.py: completed failure joins terminal diagnostics; only a current-head failed job with no runner and zero executed steps is reclassified as terminal_pre_execution_failure. A normal failed job with a runner or executed steps remains the ordinary terminal product/security failure path.

Focused review then proposed that row relabeling undercounted summary.terminal_job_count. Direct source verification showed that claim does not reproduce: _CORE_BUILD_REPORT computes the terminal aggregate while the row still has the core terminal state, and the wrapper does not recompute or decrement that aggregate after relabeling. ecbdc3507148e2ab922deba8432db87c129cf433 therefore adds only the explicit regression assertion terminal_job_count == 1; no production change was made for an invalid finding.

Fresh compare remains ahead-only / behind 0. Effective paths are only:

  • scripts/ci/actions_queue_health.py
  • tests/test_actions_queue_health_terminal_preexecution.py

No workflow trigger, runner selector, token/credential, cancellation/rerun behavior, leaf repository, ruleset, required check, or success conclusion is changed.

Acceptance

Ready status is used only to admit the unchanged exact head to hosted checks and independent review; it is not merge readiness. Do not merge until applicable exact-head checks are terminal and valid findings are resolved. The collector must preserve repository/PR/head/run/job identity and must never translate this pre-execution terminal failure into success, ordinary executed test failure, or pending runner capacity. If source changes are needed, return the PR to Draft while repairing. No self-approval, bypass, force update, destructive rebase, blind rerun, workflow/gate weakening, merge, tag, or release is authorized by this child.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: c02bed7e-881b-41ac-a508-7be42bd15f67

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

@coderabbitai review

Please review exact head b016348d1ed35304eb2504d30093922dbf2d42ae against parent #1150 exact 42bb922f03bf75aed1bc1931d9fbaf04a5433e20. Focus on whether completed/failure + materialized zero-step/no-runner jobs are preserved as fail-closed terminal pre-execution evidence without reclassifying ordinary runner-backed failures or weakening existing startup/cancelled semantics.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

parent='42bb922f03bf75aed1bc1931d9fbaf04a5433e20'
head='b016348d1ed35304eb2504d30093922dbf2d42ae'

git cat-file -e "${parent}^{commit}"
git cat-file -e "${head}^{commit}"
git diff --check "$parent" "$head"
git diff --stat "$parent" "$head"
git diff --find-renames --find-copies --unified=80 "$parent" "$head" -- \
  scripts/ci/actions_queue_health.py \
  tests/test_actions_queue_health_terminal_preexecution.py

printf '\n--- collector outline ---\n'
ast-grep outline scripts/ci/actions_queue_health.py --items all --view expanded

printf '\n--- regression-test outline ---\n'
ast-grep outline tests/test_actions_queue_health_terminal_preexecution.py --items all --view expanded

Length of output: 20956


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- terminal collection and report implementation ---'
sed -n '99,368p' scripts/ci/actions_queue_health.py
sed -n '1,360p' scripts/ci/actions_queue_health_core.py

printf '%s\n' '--- related test definitions and contexts ---'
rg -n -i -C 12 \
  'startup_failure|cancelled_before_runner_assignment|terminal_pre_execution|runner_assigned|steps_count|conclusion.*failure|status.*completed' \
  tests scripts/ci \
  -g '*.py'

printf '%s\n' '--- all changed paths and commit metadata ---'
git diff --name-status 42bb922f03bf75aed1bc1931d9fbaf04a5433e20 b016348d1ed35304eb2504d30093922dbf2d42ae
git show -s --format='commit=%H%nparents=%P%nsubject=%s' b016348d1ed35304eb2504d30093922dbf2d42ae
git merge-base --is-ancestor 42bb922f03bf75aed1bc1931d9fbaf04a5433e20 b016348d1ed35304eb2504d30093922dbf2d42ae
printf 'parent_is_ancestor=%s\n' "$?"

Length of output: 201418


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- entrypoint terminal collection ---'
sed -n '200,368p' scripts/ci/actions_queue_health.py

printf '%s\n' '--- core symbol map ---'
ast-grep outline scripts/ci/actions_queue_health_core.py --items all --view expanded

printf '%s\n' '--- core normalisation and report blocks ---'
rg -n -C 6 '^def (_normalise_run|build_report|_normalise_job|_run_is_pending|_job_is_pending)\b|execution_state|is_pending|runner_assigned|steps_count' \
  scripts/ci/actions_queue_health_core.py

printf '%s\n' '--- focused queue-health test file inventory ---'
fd -t f -E '__pycache__' 'actions_queue_health' tests | sort

printf '%s\n' '--- focused queue-health test contract locations ---'
rg -n -C 8 'startup_failure|cancelled_before_runner_assignment|runner_assigned|steps_count|terminal.*failure|execution_state' \
  tests/test_actions_queue_health*.py

Length of output: 53197


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- core job classification and report construction ---'
sed -n '267,740p' scripts/ci/actions_queue_health_core.py

printf '%s\n' '--- queue-health report contract tests ---'
sed -n '1,260p' tests/test_actions_queue_health_contract.py
sed -n '820,1010p' tests/test_actions_queue_health.py

printf '%s\n' '--- all queue-health failed-job fixture assertions ---'
rg -n -i -C 10 \
  'conclusion["'"'"']?\s*:\s*["'"'"']failure|conclusion.*FAILURE|terminal_job_count|execution_state.*terminal|runner_id.*[1-9]|steps.*\[[^]]' \
  tests/test_actions_queue_health*.py

Length of output: 62990


@seonghobae I found one summary-accounting defect.

  • scripts/ci/actions_queue_health.py, Line 469: The code changes execution_state from the core value "terminal" to "terminal_pre_execution_failure". The core summary computes terminal_job_count from execution_state == "terminal" before this override. Therefore, a report that contains only this terminal failure reports terminal_job_count: 0. This loses the terminal classification in aggregate data and can make terminal-failure monitoring undercount. Keep the distinct execution state, but update terminal_job_count to include "terminal_pre_execution_failure", or compute the count before the override. Add an assertion for terminal_job_count == 1 to tests/test_actions_queue_health_terminal_preexecution.py.

The collector otherwise has the required behavior:

  • It includes failure in the completed current-head terminal diagnostic query.
  • It fetches job evidence before report classification.
  • It requires current-head identity, failed run, failed job, no runner assignment, and exactly zero steps.
  • It leaves runner-backed or stepped failures as the normal "terminal" path.
  • It does not change the startup failure or cancelled predicates.
  • The target state remains non-pending and non-successful.

I did not execute repository tests because the review environment permits source inspection only.

You are interacting with an AI system.

@seonghobae
seonghobae marked this pull request as draft September 15, 2026 14:09

Copy link
Copy Markdown
Contributor Author

Review finding verification on successor ecbdc3507148e2ab922deba8432db87c129cf433: the aggregate undercount claim is not reproduced by the current call order. _CORE_BUILD_REPORT(...) constructs rows with the core _job_state, where this failed job is still execution_state="terminal", and computes summary["terminal_job_count"] from those core rows before the wrapper re-labels the returned row as terminal_pre_execution_failure. The wrapper does not recompute or decrement that existing summary field, so a single specimen retains terminal_job_count == 1.

I added that exact aggregate invariant as a regression assertion in ecbdc350...; no production change is warranted for this review comment. The original owner fix remains b016348d.... Please re-review the current head and flag any different causal defect; do not infer a RED that the actual report construction order does not produce.

@seonghobae
seonghobae marked this pull request as ready for review September 15, 2026 14:11

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Re-review current exact ecbdc3507148e2ab922deba8432db87c129cf433. The only successor after your prior review is the aggregate invariant assertion summary["terminal_job_count"] == 1; source inspection of _CORE_BUILD_REPORT shows it computes that count while the row is still core terminal, before the wrapper relabels the returned row. Please verify the current head rather than transferring the previous undercount finding.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base='42bb922f03bf75aed1bc1931d9fbaf04a5433e20'
fix='b016348d1ed35304eb2504d30093922dbf2d42ae'
head='ecbdc3507148e2ab922deba8432db87c129cf433'

git cat-file -e "${base}^{commit}"
git cat-file -e "${fix}^{commit}"
git cat-file -e "${head}^{commit}"
git merge-base --is-ancestor "$base" "$head"
printf '%s\n' '--- exact commits ---'
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$fix" "$head"
printf '%s\n' '--- paths base..head ---'
git diff --name-status "$base" "$head"
printf '%s\n' '--- successor diff fix..head ---'
git diff --check "$fix" "$head"
git diff --unified=80 "$fix" "$head" -- tests/test_actions_queue_health_terminal_preexecution.py scripts/ci/actions_queue_health.py
printf '%s\n' '--- full scoped diff base..head ---'
git diff --check "$base" "$head"
git diff --unified=50 "$base" "$head" -- scripts/ci/actions_queue_health.py tests/test_actions_queue_health_terminal_preexecution.py

printf '%s\n' '--- entrypoint outline ---'
ast-grep outline scripts/ci/actions_queue_health.py --items all --view expanded
printf '%s\n' '--- core outline ---'
ast-grep outline scripts/ci/actions_queue_health_core.py --items all --view expanded

printf '%s\n' '--- report construction and wrapper relabeling references ---'
rg -n -C 12 '_CORE_BUILD_REPORT|terminal_job_count|terminal_pre_execution_failure|TERMINAL_DIAGNOSTIC_STATUSES|startup_failure|cancelled_before_runner_assignment' \
  scripts/ci/actions_queue_health.py scripts/ci/actions_queue_health_core.py tests/test_actions_queue_health_terminal_preexecution.py

printf '%s\n' '--- relevant implementation slices ---'
sed -n '180,520p' scripts/ci/actions_queue_health.py
sed -n '500,720p' scripts/ci/actions_queue_health_core.py
printf '%s\n' '--- regression fixture ---'
cat -n tests/test_actions_queue_health_terminal_preexecution.py

Length of output: 79380


@seonghobae Re-review of exact head ecbdc3507148e2ab922deba8432db87c129cf433 is complete.

The prior terminal_job_count finding is withdrawn. scripts/ci/actions_queue_health.py calls _CORE_BUILD_REPORT(...) before Line 469 relabels the row. The core report computes summary["terminal_job_count"] while the row has execution_state == "terminal". The wrapper does not recompute or decrement that value. The added assertion correctly verifies this behavior.

I found no different causal defect in the scoped changes. The collector preserves the current-head completed/failure specimen, fetches its job evidence, and classifies only the zero-step, no-runner failed job as terminal_pre_execution_failure. The runner-backed or stepped failure path remains terminal. The existing startup-failure and cancelled predicates remain unchanged.

I performed source inspection only. I did not execute repository tests.

You are interacting with an AI system.

seonghobae commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Fresh exact-head queue evidence on ecbdc3507148e2ab922deba8432db87c129cf433, updated after the dispatch job finally ran: Semgrep and Security Scan are SUCCESS, and CodeQL language detection ran successfully on hosted runner 1001987520. Compatibility jobs python 104501557063 (runner 1001989212) and actions 104501557079 (runner 1001989220) both executed earlier and failed specifically at Release runner or enforce current-head CodeQL verdict after their verdict-read step. The same run's coordinator Dispatch current-head CodeQL scan job 104577757456, which was still pre-runner queued when this note was first posted, was later admitted on runner 1001991159 at 2026-09-16 01:12:55Z and completed SUCCESS at 01:13:04Z.

This is stronger evidence than generic runner starvation: the exact workflow allowed the compatibility shards to consume/reject a missing verdict before the coordinator that creates the current-head dispatch receipt was able to execute, and the coordinator then succeeded later. The overall CodeQL PR run remains FAILURE because those earlier shard failures were not converted into a terminal current-head verdict in this run. Treat this as central dispatch/verdict ordering/retry-control evidence, separate from #2213's completed/failure + runner_id=0 + steps=[] classifier repair. Preserve the exact head/run/job identities; no blind rerun or OriginWeave consumer workaround is warranted.

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