fix(ci) #5701: coverage-report waits for the HA suite, and refuses to publish a partial merge - #5705
Conversation
… publish a partial merge `coverage-report` downloaded `ha-integration-coverage-reports` but listed only [unit-tests, integration-tests, slow-unit-tests] in `needs`. A download only sees artifacts already uploaded, so the job started ~14 minutes before `ha-integration-tests` finished - it is the longest job in the workflow at ~55 minutes - the download failed, `continue-on-error: true` swallowed it, and the merge went to Codecov without the ha-raft module. Codecov read the module as uncovered and reported 1,110 covered lines disappearing on a PR that had touched 29 lines in `engine/.../index/hash/`. It was a race, not a constant: on runs where the other three jobs happened to finish late the artifact was there. Run 30693081526 downloaded it (HA finished 11s before), runs 30691165476 and 30691056907 did not. So the check flipped between correct and wrong on identical config, and the incorrect runs also became the base later pull requests were compared against. - add `ha-integration-tests` to `coverage-report.needs`, and relax the job condition to `!cancelled()` so a suite with failing tests still publishes what JaCoCo wrote - replace the bare `find` with `collect-coverage-reports.sh`, which names the suites it expects, fails when one contributed no report, and writes a per-suite table to the step summary. The Codacy and Codecov steps lose their `if: success() || failure()` so they are skipped on that failure: a partial upload is worse than no upload, because it poisons the base too - add `check-workflow-artifact-deps.py`, which asserts that every `download-artifact` step's producer is in the consumer's transitive `needs` closure, over every workflow. It reports this bug on the unfixed file and nothing else across the other 17 workflows. Wired into `setup` Related: #5702 (the HA suite's own flakiness), which this fix is independent of - the suite runs with --fail-never, so its coverage artifact uploads whether or not tests pass.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 100 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Tick the box to add this pull request to the merge queue (same as
|
…tifact-deps D213 on two multi-line docstrings and D403 on one summary. D213 is also the convention the repository's own Python already follows by a wide margin (342 docstrings to 105).
Review: fix(ci) #5701 - coverage-report waits for the HA suiteThorough, well-diagnosed PR. The root cause (a A few observations, all minor - nothing blocking. Correctness / robustness
Behavior worth confirming is intended
Nits
TestsCoverage is excellent: both directions (must-fail / must-not-fail), transitive needs, matrix-vs-pattern, opaque selectors, unselective downloads, cross-run, the two collector refusal cases, and the output-shape contract. The mutation-check note in the description (revert the Overall: correct fix, strong verification, good docs. The items above are hardening suggestions, not objections. |
Codacy has both D212 and D213 enabled, and they are mutually exclusive - satisfying D213 made it report D212 on the same two docstrings. Neither rule applies to a one-line summary, so the explanation moves to a comment above the function and nothing is lost.
|
Review: fix(ci) #5701 - coverage-report waits for the HA suite Thorough, well-reasoned fix. The root-cause analysis (download-artifact has no happens-before with the uploader outside of Correctness / behavior notes
Minor
Trade-off The ~10-minute-later Codecov/Codacy signal is called out honestly in the description and is the unavoidable cost of a complete merge. Agreed it is the right call - a true-but-late signal beats a fast-but-fabricated one. Nice work. The generalization into an enforced invariant plus regression fixtures is the part that keeps the whole class of #5701 from recurring, and it is done carefully. LGTM. Note: I reviewed by reading the full diff; I could not execute the scripts in this environment, so the self-test pass/fail counts are taken from the PR description rather than independently re-run. |
…porting a suite that never ran Code review follow-ups on #5705. Bidirectional glob matching over-matched: `build-${{ matrix.os }}` collapsed to `build-*`, which covers an unrelated `build-logs` uploaded by another job, so consuming that literal was reported as needing the matrix job too. A false violation is expensive here - this check gates every other job - so rather than note the tradeoff, the matrix is now resolved outright: `matrix.os` expands to exactly {ubuntu, macos} from the base matrix and from `include` entries, which is the shape most of this repository's matrices use. The glob remains only as the fallback for a matrix built at run time (`fromJSON`), where over-matching is the safe direction. - `run-id: ${{ github.run_id }}` names the current run, so that step is an ordinary consumer. It was being skipped as a cross-run read, which silently disabled the check for it - `coverage-report` now skips when a suite was *skipped* rather than failing on its absent artifact. A skipped suite means build-and-package failed and nothing ran, so the second red check added nothing; the collector's failure now means only what it says, a suite that should have uploaded and did not - pin pyyaml==6.0.3, for the reason the actions in that job are pinned to a SHA Five new fixtures: the prefix-collision false positive and its true-positive twin, an include-only matrix, a run-time matrix falling back to the glob, and a current-run `run-id`. 20 checks.
|
Thanks - all six addressed in d02d6b4. Taking them in order: 1. Bidirectional glob over-match. Valid, and fixed properly rather than documented. The The glob survives only as the fallback for a matrix that cannot be read statically - 2. 3. Hard gate on 4. if: >-
${{ !cancelled()
&& needs.unit-tests.result != 'skipped'
&& ... }}which keeps the collector's failure meaning only what it says - a suite that should have uploaded 5. Unpinned pyyaml. Valid, pinned to 6. Agreed, no change. Suite is at 20 checks, all green, and |
Review: coverage-report ordering fixReviewed the diff, the two new scripts, the self-test, and the reworked Verified
Minor observations (non-blocking)
Overall: solid root-cause fix, well-generalized, well-tested. The only substantive item is the reusable-workflow blind spot (1), and even that is currently benign. LGTM. |
Review: coverage-report waits for the HA suiteOverall this is a high-quality fix. The root-cause analysis (race between A few notes, mostly minor. Nothing here is blocking. Design / correctness
Nits
Style / conventionsMatches the repo: no em dashes, SHA-pinned actions preserved, pyyaml pinned for the same reason, docstrings reworked to satisfy pydocstyle (D212/D213) rather than suppressed. Good. Verification noteI reviewed the full final diff (all four commits, including the matrix-resolution rework). I could not execute Nice work - a genuinely hard class of bug (green on incomplete input, race-dependent) turned into a standing invariant with a regression test. |
…upload Both reviews flagged the same blind spot: a job that delegates to a reusable workflow (`jobs.<id>.uses:`) has no steps to read, so an upload inside it is invisible and a consumer of that artifact would be reported as downloading something "no job in this workflow uploads" - a spurious hard failure, in a check that gates every other job. Documented in the header as asked, and made safe rather than only documented: a workflow containing such a job stops reporting the missing-producer violation, since the producer may be one of the steps the check cannot see. Ordering violations it can still see are still reported, which the second new fixture pins. - give PyYAML its own step in `setup`. The self-test invokes the Python checker, so it depended on a `pip install` buried in the preceding step's script - reordering them would have broken it - reflow a comment that wrapped as "build- / and-package"
|
Both rounds addressed in d7572ef. The one point both reviews raised independently was the Reusable workflows (R3.1, R4.2). Both asked for a one-line note. Documented, and also made PyYAML step coupling (R3.2). Valid - the self-test invoked the checker, which needed a Comment reflow (R3.4). Fixed. Warning-only rollout (R4.1). Declining, with reasoning. The original issue's complaint is that All-or-nothing skip / hand-synced HA infra failure reds coverage-report (R4.3). Correctly identified as the one real behavior Suite is at 22 checks, all green. |
Review: fix(ci) #5701 - coverage-report waits for the HA suiteThorough diagnosis and a fix that treats the class of bug rather than just the instance. The write-up (race table across three real runs, mutation check) makes the root cause easy to trust. Overall this is high-quality work; comments below are mostly confirmations and a couple of things worth stating explicitly. What's right
Things worth stating / questions
Trade-offThe stated ~10-minute latency cost (coverage now waits on the 55-min HA job) is real and unavoidable given the ordering requirement - correctly called out. No objection. Nice work - the fix, the regression fixtures, and the general invariant together are exactly the right shape for this bug. |
…uppresses The suppression added for the reusable-workflow case was silent, and a typo'd artifact name in a file that also calls a reusable workflow looks exactly like an artifact that workflow uploads. The benefit of the doubt still goes to the workflow - this check gates every other job - but the finding is now printed with the reason it was not raised. Silently discarding it is how #5701 stayed invisible in the first place. `find` in collect-coverage-reports.sh now writes to a file instead of straight into the read loop. A process substitution is not part of the pipeline, so `pipefail` never saw its exit status and an unreadable directory read as an empty one - the exact distinction the script exists to draw. Two fixtures: the suppressed finding is reported, and an unreadable suite directory fails. 24 checks.
|
Addressed in 7ef8284. 1. Is a skipped 2. Informational, exit 0. Silently discarding a finding is how #5701 stayed invisible in the first 3. 4. Python + pyyaml on the critical path. Acknowledged. Pinned, quiet, ~1s. 24 checks, all green. |
Review: fix(ci) #5701 - coverage-report waits for the HA suiteThorough, well-reasoned fix. The root cause (a A few observations, none blocking: 1. New hard-failure path can newly block PRs (worth stating in the PR body)Previously a cancelled/absent HA artifact was swallowed and 2. All-or-nothing skip couples to an assumption about why a suite is skippedThe 3.
|
The skip condition reads any one skipped suite as "none of them ran", which is true only because all four hang off build-and-package. Give one its own path filter or condition and the other three would still have coverage worth publishing, and the condition would drop it. Noted where the next editor will be standing. Also name the two halves of the (violations, notes) tuples distinctly in main().
|
Addressed in 3e5e38f plus a PR-body edit. 1. New hard-failure path, worth stating explicitly. Agreed - added a "Behavior change worth 2. All-or-nothing skip couples to why a suite is skipped. Good catch, and the assumption was 3. 4. The linter gates the whole build. Conscious of it - it is why the matrix resolution went in Minor: renamed the two comprehension variables in 24 checks, all green. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 100 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Review: fix(ci) #5701 Reviewed the full diff, cross-checked the workflow wiring against the two scripts, and read the checker logic case by case. This is a careful, well-reasoned fix and the root-cause analysis (a Correctness checks that pass
Minor observations (non-blocking)
Test coverage Trade-offs, well stated No blocking concerns. LGTM. Note: this review covers CI config/scripts only; there is no Java to compile or unit-test here. |
… publish a partial merge (#5705) * fix(ci) #5701: coverage-report waits for the HA suite, and refuses to publish a partial merge `coverage-report` downloaded `ha-integration-coverage-reports` but listed only [unit-tests, integration-tests, slow-unit-tests] in `needs`. A download only sees artifacts already uploaded, so the job started ~14 minutes before `ha-integration-tests` finished - it is the longest job in the workflow at ~55 minutes - the download failed, `continue-on-error: true` swallowed it, and the merge went to Codecov without the ha-raft module. Codecov read the module as uncovered and reported 1,110 covered lines disappearing on a PR that had touched 29 lines in `engine/.../index/hash/`. It was a race, not a constant: on runs where the other three jobs happened to finish late the artifact was there. Run 30693081526 downloaded it (HA finished 11s before), runs 30691165476 and 30691056907 did not. So the check flipped between correct and wrong on identical config, and the incorrect runs also became the base later pull requests were compared against. - add `ha-integration-tests` to `coverage-report.needs`, and relax the job condition to `!cancelled()` so a suite with failing tests still publishes what JaCoCo wrote - replace the bare `find` with `collect-coverage-reports.sh`, which names the suites it expects, fails when one contributed no report, and writes a per-suite table to the step summary. The Codacy and Codecov steps lose their `if: success() || failure()` so they are skipped on that failure: a partial upload is worse than no upload, because it poisons the base too - add `check-workflow-artifact-deps.py`, which asserts that every `download-artifact` step's producer is in the consumer's transitive `needs` closure, over every workflow. It reports this bug on the unfixed file and nothing else across the other 17 workflows. Wired into `setup` Related: #5702 (the HA suite's own flakiness), which this fix is independent of - the suite runs with --fail-never, so its coverage artifact uploads whether or not tests pass. * fix(ci) #5701: satisfy Codacy's pydocstyle rules in check-workflow-artifact-deps D213 on two multi-line docstrings and D403 on one summary. D213 is also the convention the repository's own Python already follows by a wide margin (342 docstrings to 105). * fix(ci) #5701: keep the docstrings single-line, prose moves to comments Codacy has both D212 and D213 enabled, and they are mutually exclusive - satisfying D213 made it report D212 on the same two docstrings. Neither rule applies to a one-line summary, so the explanation moves to a comment above the function and nothing is lost. * fix(ci) #5701: resolve the matrix instead of globbing it, and stop reporting a suite that never ran Code review follow-ups on #5705. Bidirectional glob matching over-matched: `build-${{ matrix.os }}` collapsed to `build-*`, which covers an unrelated `build-logs` uploaded by another job, so consuming that literal was reported as needing the matrix job too. A false violation is expensive here - this check gates every other job - so rather than note the tradeoff, the matrix is now resolved outright: `matrix.os` expands to exactly {ubuntu, macos} from the base matrix and from `include` entries, which is the shape most of this repository's matrices use. The glob remains only as the fallback for a matrix built at run time (`fromJSON`), where over-matching is the safe direction. - `run-id: ${{ github.run_id }}` names the current run, so that step is an ordinary consumer. It was being skipped as a cross-run read, which silently disabled the check for it - `coverage-report` now skips when a suite was *skipped* rather than failing on its absent artifact. A skipped suite means build-and-package failed and nothing ran, so the second red check added nothing; the collector's failure now means only what it says, a suite that should have uploaded and did not - pin pyyaml==6.0.3, for the reason the actions in that job are pinned to a SHA Five new fixtures: the prefix-collision false positive and its true-positive twin, an include-only matrix, a run-time matrix falling back to the glob, and a current-run `run-id`. 20 checks. * fix(ci) #5701: do not hard-fail on artifacts a reusable workflow may upload Both reviews flagged the same blind spot: a job that delegates to a reusable workflow (`jobs.<id>.uses:`) has no steps to read, so an upload inside it is invisible and a consumer of that artifact would be reported as downloading something "no job in this workflow uploads" - a spurious hard failure, in a check that gates every other job. Documented in the header as asked, and made safe rather than only documented: a workflow containing such a job stops reporting the missing-producer violation, since the producer may be one of the steps the check cannot see. Ordering violations it can still see are still reported, which the second new fixture pins. - give PyYAML its own step in `setup`. The self-test invokes the Python checker, so it depended on a `pip install` buried in the preceding step's script - reordering them would have broken it - reflow a comment that wrapped as "build- / and-package" * fix(ci) #5701: report the findings the reusable-workflow blind spot suppresses The suppression added for the reusable-workflow case was silent, and a typo'd artifact name in a file that also calls a reusable workflow looks exactly like an artifact that workflow uploads. The benefit of the doubt still goes to the workflow - this check gates every other job - but the finding is now printed with the reason it was not raised. Silently discarding it is how #5701 stayed invisible in the first place. `find` in collect-coverage-reports.sh now writes to a file instead of straight into the read loop. A process substitution is not part of the pipeline, so `pipefail` never saw its exit status and an unreadable directory read as an empty one - the exact distinction the script exists to draw. Two fixtures: the suppressed finding is reported, and an unreadable suite directory fails. 24 checks. * fix(ci) #5701: write down the "all four suites skip together" assumption The skip condition reads any one skipped suite as "none of them ran", which is true only because all four hang off build-and-package. Give one its own path filter or condition and the other three would still have coverage worth publishing, and the condition would drop it. Noted where the next editor will be standing. Also name the two halves of the (violations, notes) tuples distinctly in main(). (cherry picked from commit 0a13d82)
1 participant
Fixes #5701.
The bug
coverage-reportdownloadsha-integration-coverage-reportsbut itsneedslisted only[unit-tests, integration-tests, slow-unit-tests].actions/download-artifactreads the artifactstore of the current run and has no happens-before relationship with the uploader - that ordering
exists only through
needs.ha-integration-teststakes ~55 minutes, by far the longest job inthe workflow, so
coverage-reportusually started well before it finished.The download then failed,
continue-on-error: trueswallowed the failure, and the merged reportwent to Codecov with the whole
ha-raftmodule absent. Codecov does not read an absent module as"unknown" - it reads it as uncovered, which is how a 29-line change to
engine/src/main/java/com/arcadedb/index/hash/producedHits 101236 -> 100126 (-1110)on #5685.Confirmed on the real runs
Artifact not found for name: ha-integration-coverage-reportsSo it was a race, not a constant. That is the part that made it hard to see: identical config
produced a complete report on a slow run and an incomplete one on a fast run, and the incomplete
runs also became the base every later pull request was compared against - one bad run yields a
phantom drop and then a phantom recovery on whoever comes next.
What changed
1. The ordering.
ha-integration-testsadded tocoverage-report.needs. The job conditionbecomes
!cancelled()rather thansuccess() || failure(), so a suite that reported failing testsstill publishes what JaCoCo wrote (the suites run with
--fail-never), while a cancelled run - theone case where an artifact is genuinely absent rather than merely late - still skips.
2. No more silent partial merges. The issue notes that
continue-on-error: truehides exactlythis class of bug, and suggests failing or at least logging. This goes with failing.
.github/scripts/collect-coverage-reports.shreplaces the barefind . -path **/jacoco*.xml -printf '%p,': it takes the suites it expects as<suite>=<dir>pairs, verifies each contributed at least one report, writes a per-suite table to the step summary,
and exits non-zero naming the missing suites. The Codacy and Codecov steps lose their
if: success() || failure()so they are skipped when it fails. A partial upload is worse than noupload, and a red
coverage-reportnaming the missing suite is a far better signal than a greenone reporting a fabricated regression.
3. The generalisation - suggested in place of just adding the
needsentry. Adding one entryfixes today's instance and leaves the class open: nothing about a
download-artifactstep tellsyou which job must precede it, so the next one is equally invisible.
.github/scripts/check-workflow-artifact-deps.pymakes it an enforced invariant - for everydownload step it resolves the producing job and asserts it is in the consumer's transitive
needsclosure, across every workflow. It handles transitive
needs, matrix-interpolated artifact namesmatched as globs against
pattern:consumers, selector-less downloads (which depend on everyproducer), and cross-run downloads (which this run's ordering cannot constrain). Run against the
unfixed workflow it reports this bug and nothing else; across the other 17 workflows it is clean.
Wired into the
setupjob.Verification
.github/scripts/tests/test-ci-scripts.sh(also wired intosetup) - 15 checks, both directions,against fixtures rather than the live workflows:
Mutation-checked: reverting only the
needsline and re-running turnsthe repository's own workflows are orderedred with the exact #5701 diagnostic, and nothing elsechanges. The two
collect-coverage-reports.shrefusal cases are the #5701 shape directly - the HAartifact never downloaded, and the HA job ran but wrote no report.
Trade-off worth stating
coverage-reportnow waits for the slowest job, so the Codecov and Codacy checks land roughly 10minutes later than they do today. That is the cost of the signal being true; there is no ordering
that produces a complete merge sooner.
On #5702
Independent of it.
ha-integration-testsruns with--fail-neverand uploads withif: success() || failure(), so its coverage artifact exists whether or not tests pass. The issuenotes this fix will surface partial HA coverage from partially-failed runs - it will, and that is
the correct behaviour: coverage genuinely lost because tests did not run is a real signal, unlike a
module vanishing entirely. Only a cancelled HA job produces no artifact, and that now fails loudly
by name instead of silently.
Behavior change worth knowing about
coverage-reportcan now go red where it previously went green. Two paths:which
--fail-neveralready covers - leaves its artifact missing and the collector fails thejob by name. Given ha-integration-tests fails on main ~75% of the time, with a different test each run #5702, the HA suite is the likely one.
skippedskipscoverage-reportentirely rather than publishing three offour.
Both are deliberate. The alternative to (1) is publishing a merge without
ha-raft, which is#5701 itself; a red check naming the suite is the honest version of the same event. If it proves
noisy, the remedy is fixing #5702 rather than re-enabling silent partial merges.
Neither can block a merge today:
mainhas no branch protection (GET /branches/main/protectionreturns 404), no rulesets, and Mergify's only rule targets Dependabot. No status check is required.