DanielLeens commented on PR #11840:
URL: https://github.com/apache/seatunnel/pull/11840#issuecomment-5315289525
# What Problem Does This PR Solve?
On 2026-08-17, Apache PR #11834 showed the required `Build` check stuck
reporting `queued` for a long time, even though the linked fork run already had
62 jobs `completed`, 18 `in_progress`, and 7 `queued`. Root cause:
`.github/workflows/update_build_status.yml` (a `*/15 * * * *` cron job) copies
the **top-level** `status`/`conclusion` from `GET
/repos/{owner}/{repo}/actions/runs/{run_id}` verbatim into the Apache-side
`Build` check run via `PATCH /check-runs/{id}`. For reusable-workflow runs
(this repo's `Build` → `backend.yml` fan-out is ~87 jobs), GitHub can leave the
run-level `status` at `queued` long after child jobs are actively running or
finished, so the check-run patch degenerates into `queued -> queued` and the PR
appears permanently stuck even though CI is actually progressing (or already
done).
This PR adds a job-level fallback: when the top-level run is `queued`, it
lists the real jobs via `GET /actions/runs/{run_id}/jobs` and derives a more
accurate `status`/`conclusion` from them before patching the check run, and
switches the check run's `details_url` field from a non-existent API property
to the run's real `html_url`.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Scope note: this PR does not touch which check names are required for branch
protection, and it does not add/remove/rename any workflow job
(`build_main.yml`'s `Build` → `backend.yml` job graph is untouched). The single
file changed is `.github/workflows/update_build_status.yml` (+118/-8), a
maintenance cron job that reconciles the *live status* of the already-existing
`Build` check run against the real fork-side workflow run.
**Before** (`update_build_status.yml`, old lines 158-159):
```js
status: workflowRun.status,
details_url: workflowRun.details_url
```
**After** (fed by `normalizeWorkflowRun`, lines 151-183):
```js
status: normalizedWorkflowRun.status,
details_url: normalizedWorkflowRun.details_url // = workflowRun.html_url
```
Key findings, all backed by cross-checking the actual GitHub REST response
shapes against the other workflow that originates this data
(`notify_test_workflow.yml`, which creates the check run with `output.text =
JSON.stringify({owner, repo, run_id})` targeting the **fork** repo, and
`details_url: actions_url` where `actions_url` is manually built as
`https://github.com/<fork>/actions/runs/<run_id>`):
1. **A real, previously-silent bug is fixed as a side effect.** The GitHub
"Get a workflow run" response object has no `details_url` field — only
`html_url`. So `workflowRun.details_url` at the old code was always
`undefined`. Since `buildCheckRun.details_url` (from the Checks API) is never
`undefined`, the `sameDetailsUrl` check was always false, meaning the cron job
unconditionally re-PATCHed the `Build` check run for *every* eligible open PR
on *every* 15-minute tick, regardless of whether anything had actually changed.
Switching to `workflowRun.html_url`, which matches the format of `actions_url`
set at check-run creation time, makes `sameDetailsUrl` a meaningful,
usually-true comparison, so the no-op-skip logic will finally work as intended.
This is a genuine efficiency/correctness fix beyond what the PR description
explicitly claims credit for — worth calling out in the PR description for
reviewers/history, but not a blocker.
2. **`normalizeWorkflowRun` correctness**: only branches when
`workflowRun.status === 'queued'`; all other statuses pass through unchanged
except for the `details_url` fix above — correct, minimal-blast-radius design.
- `jobs.length === 0 || jobs.every(job => job.status === 'queued')` →
leaves status as `queued`, i.e., no regression versus current behavior when the
job list itself confirms nothing has started.
- `jobs.every(job => job.status === 'completed')` → `status =
'completed'`, `conclusion = deriveCompletedConclusion(...)`.
- Otherwise (mixed) → `status = 'in_progress'`, `conclusion = null`. This
is the case that fixes the reported #11834 symptom (62 completed / 18
in_progress / 7 queued → correctly reported as `in_progress`, not stuck
`queued`).
3. **`deriveCompletedConclusion` — conclusion-vocabulary mapping is
necessary and correct for its primary purpose.** Job-level `conclusion` values
from the Jobs API can include `startup_failure`, which is not a legal value for
the Checks API's `conclusion` enum. Without this mapping, a `startup_failure`
job conclusion passed straight to `PATCH /check-runs` would be rejected by
GitHub with a 422. The function correctly folds `startup_failure` into
`failure`. Priority order implemented: `failure/startup_failure` > `timed_out`
> `action_required` > `cancelled` > all-`skipped` > `neutral`(+skipped only) >
default `success`.
4. **Minor gap — `stale` job conclusion is unhandled.** If a completed job's
conclusion is `stale` and no other job conclusion matches an earlier branch,
`deriveCompletedConclusion` falls through to the default `return 'success'`,
silently reporting a stale job as a successful build. `stale` is a legal Checks
API conclusion value that could have been mapped explicitly. This is a
low-probability edge case, and any resulting mis-patch is non-fatal (see
finding 6 below), but it is a real, unaddressed edge in an otherwise careful
mapping function.
5. **Theoretical `conclusion: null` PATCH failure.** If all jobs are
`completed` but somehow none have a non-empty string `conclusion`,
`deriveCompletedConclusion` returns `fallbackConclusion`, which is
`workflowRun.conclusion` — typically `null` for a top-level run still reporting
`queued`. That `null` would then be sent as `patchParams.conclusion` while
`status: 'completed'`, which the Checks API PATCH endpoint requires to be a
valid conclusion string, not `null`. This is a very low-probability scenario
but is a latent gap in the fallback path.
6. **No cascading failure risk.** Findings 4-5 above are contained: the
whole `processPullRequest` body is wrapped by an outer `try { ... } catch
(error) { console.error(...) }`, so a bad PATCH for one PR just logs and moves
on — it does not fail the workflow run or block other PRs in the batch. Given
this safety net, findings 4-5 are correctness nits, not availability risks.
## 1.2 Compatibility Impact
- **No branch-protection required-check-name changes.** The check run
created by `notify_test_workflow.yml` is still literally named `'Build'`
(unchanged), and `build_main.yml`'s trigger/job structure is untouched. Branch
protection rules referencing the `Build` status check are unaffected.
- **Net compatibility effect is positive, not neutral.** Because of the
`details_url` bug fix above, currently in-flight PRs whose `Build` check is
genuinely stuck `queued` at the top level should start reflecting real progress
after this merges, rather than requiring maintainers to manually re-run/monitor
them. I traced the `details_url` values from both origin points and confirmed
they use the same URL format, so the comparison will behave sanely for existing
check runs immediately after merge — no first-run flood of unexpected diffs is
expected.
- No change to `on:` triggers (still `schedule: */15 * * * *` only) or
`permissions:` — the new jobs-listing call is covered by the existing `actions:
read` grant.
## 1.3 Performance / Side-Effect Analysis
Extra cost is one additional paginated `GET .../jobs` call, but only when
the top-level run is still `queued` — the common `in_progress`/`completed` path
is untouched. Given the reported job count (~87 jobs, single page under the
100-per-page limit), this is a single extra request per affected PR per
15-minute tick, well within GitHub Actions' rate limits for a scheduled
maintenance job. The `details_url` fix is actually a net perf win: it should
stop the previously-guaranteed always-patch on every tick for every open PR,
replacing it with real no-ops once state converges.
## 1.4 Error Handling and Logging
No blocking issues found. Two low-severity items already covered in 1.1:
| # | Location | Problem | Risk | Improvement | Severity |
|---|----------|---------|------|-------------|----------|
| 1 | `update_build_status.yml` (`deriveCompletedConclusion`) | Job
`conclusion: 'stale'` is not explicitly mapped and falls through to `'success'`
| A stale job could be reported as a successful build for the derived
`queued`->`completed` fallback path only | Add an explicit `stale` branch (e.g.
map to `'stale'` or `'cancelled'`) | Low |
| 2 | `update_build_status.yml` | If all jobs are `completed` with zero
non-empty `conclusion` values, `fallbackConclusion` (often `null`) is patched
as `conclusion` while `status: 'completed'` | PATCH request would likely be
rejected by the Checks API (422); caught and logged by the outer try/catch, no
cascading failure | Guard with a final default (e.g. `'neutral'`) instead of
passing through a possibly-null fallback | Low |
Both are edge cases in a fallback path that only activates when GitHub
itself is already misreporting state, and both fail safely rather than crashing
the workflow.
# 2. Code Quality Assessment
## 2.1 Standards
Code style is consistent with the rest of the file. The comment explaining
the fallback exists (reusable-workflow queued-at-top-level quirk) matches the
"explain why, not what" bar. No stray debug output, no hardcoded PR/run IDs
from the incident report leaked into the code, no leftover scaffolding.
## 2.2 Test Coverage / Stability
This is the weakest part of the submission. There is no automated test
harness for this inline `actions/github-script` logic in the repository (a
pre-existing repo-wide gap, not one this PR introduces). Validation was done
via local Node.js simulation and a live reproduction against a fork workflow
run — neither the simulation script nor its output is included in the PR, so it
cannot be independently verified from the repository. Critically, this PR's own
CI run does not exercise this new logic at all, since `update_build_status.yml`
only triggers on `schedule`, never on `pull_request`/`push` — real-world
confirmation will only come from observing the next `*/15` cron run against a
genuinely stuck PR. This is inherent to the "CI fixing CI" nature of the change.
## 2.3 Documentation
No user-facing docs required (internal CI infrastructure, not a
connector/config/API change) — correctly out of scope. The PR description
explains the incident, root cause, and fix scope clearly and matches the actual
diff.
# 3. Architectural Soundness
## 3.1 Design cohesion
The fix is appropriately scoped to the layer that owns the bug: it patches
the status-derivation logic in the reconciliation cron job, not the underlying
`build_main.yml`/`backend.yml` job graph (which is the actual source-of-truth
CI). It does not attempt to "fix" GitHub's top-level `queued` staleness
upstream — correctly recognizes that's outside SeaTunnel's control and works
around it locally.
## 3.2 Coupling
`normalizeWorkflowRun` takes the fork's `{owner, repo, run_id}` as an
explicit parameter rather than reaching into outer closures more than
necessary, keeping the new functions independently testable in principle.
## 3.3 Failure isolation
As noted in 1.4, all new failure modes are caught either by a dedicated
try/catch around the new `normalizeWorkflowRun` call or the pre-existing outer
per-PR catch — a single bad job listing or malformed conclusion cannot take
down the batch or the whole scheduled run.
## 3.4 Extensibility
If GitHub later exposes similarly unreliable top-level state for other check
runs, the helper functions are generically written (not `Build`-check-specific)
and could be reused.
# 4. Issue Summary
| # | Severity | Location | Issue | Blocking? |
|---|----------|----------|-------|-----------|
| 1 | Low | `update_build_status.yml` (`deriveCompletedConclusion`) |
`stale` job conclusion unmapped, falls through to `success` | No |
| 2 | Low | `update_build_status.yml` | Possible `null` conclusion patched
when status is `completed` in a degenerate all-jobs-no-conclusion case | No
(fails safely) |
| 3 | Informational | PR description | Does not mention that switching
`details_url` to `html_url` also fixes a pre-existing always-patch bug — a
positive side effect worth documenting for posterity | No |
| 4 | Informational | Test coverage | No automated test harness for this
script (repo-wide pre-existing gap); this PR's own CI run does not exercise the
new code path | No |
**No blockers found.**
# 5. Merge Recommendation
### Conclusion: Ready to merge
This is a narrowly-scoped, well-isolated fix to an internal CI
reconciliation script. I traced the root cause independently (confirmed the
pre-existing `workflowRun.details_url` was always `undefined` against the real
GitHub Actions API response shape), verified the new script is syntactically
valid, and found no path by which this change could rename/remove a required
check, widen/narrow CI trigger coverage, or strand in-flight PRs. The two
low-severity edge cases (unmapped `stale` conclusion, theoretical
`null`-conclusion PATCH) are both in a fallback branch that only activates when
GitHub itself is already misreporting state, and both fail safely into per-PR
logged errors rather than crashing the scheduled job or affecting other PRs.
The one real gap is verification depth: this PR's own required checks never
exercise the new logic, so a green `Build`/`Notify test workflow`/`labeler` on
this PR provides no evidence about correctness of
`normalizeWorkflowRun`/`deriveCompletedConclusion`. Recommend the two
low-severity findings be addressed in a fast follow-up (both are trivial
one-line additions), and that after merge the next few `*/15` cron runs be
watched to confirm the derived-status patches land as expected in production.
Overall assessment: sound, minimal, low-risk fix for a real production CI
annoyance, with two non-blocking polish items and a test-coverage caveat that
is honestly disclosed rather than concealed.
(Note: since I authored this PR myself, this review is posted as a plain
comment rather than a formal approve/request-changes review — GitHub doesn't
allow self-approval, and I want the same level of independent scrutiny applied
here as to anyone else's PR.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
