[matrei]

Summary

This is a follow-up to #16673. It contains the review points that were still open when it was merged: @codeconsole's change request and the second review round.

Publish chain after a cancel

The problem. publishGradle needed only buildGradle. Since #16673, a newer push cancels the run in flight, and the publish jobs run on always(). So on any push run, a cancel more than a few minutes in still published part of the superseded commit:

• Run with tests: the Gradle plugins were published, but not the core. 8.0.x run 39638916842 shows this. Its publishGradle published at 17:13, and a newer push cancelled the run at 17:26, before its tests had finished.
• Run without tests (every push to 7.x): the plugins, core and docs were published, but not Forge, which was still building. @codeconsole found this case.

The fix. publishGradle now also waits for buildForge and validateDependencies, with the same explicit success checks. publish now also waits for buildRerunTasks, the only test job it did not need, so the core is no longer published while that job is still running, or after it failed (@codeconsole found this in review). publish and everything after it already wait for publishGradle, so:

• A cancel before the builds finish publishes nothing of that commit.
• A commit whose Forge build or dependency validation failed is no longer published.
• A cancel after the builds still lets the chain publish what it can:
  • on a run without tests, the whole chain;
  • on a run whose tests are still running, only the Gradle plugins;
  • on a run whose tests have all passed, the whole chain.

The comments in gradle.yml and the CI policy section of AGENTS.md now describe this, instead of saying that a cancelled commit is never published.

The cost is that the publishes start later:

• Run without tests (every push to 7.x): every publish moves back, because publish and everything after it wait for publishGradle. In 7.0.x run 39615263956, publishGradle started at 8.5 minutes and buildForge ended at 14.2, so the plugins, core, Forge and docs would each have been published about 5 to 6 minutes later.
• Run with tests: the plugin snapshot waits for the Forge build. In 8.0.x run 39644767954 it would have started at 56 minutes instead of 40. The core publish waits for the slowest test job, which can now be buildRerunTasks. In 39644767954, publish started at 236.7 minutes and buildRerunTasks ended at 260.3, so the core snapshot would have been published 24 minutes later.

Docs

• AGENTS.md:
  • The snapshot publish is no longer listed among the jobs that run on pull requests. It runs on a push, and on a manual run except for the docs.
  • The CI policy section names force-cancel (gh api -X POST repos/apache/grails-core/actions/runs/<run id>/force-cancel) for a publish that a normal cancel cannot stop. The upload's retry step lets a stuck publish keep running for hours.
• CONTRIBUTING.md: no longer says that every push to a branch cancels the run in flight. Only CI and the Groovy joint build do; for drafts, the relevant case is a push to a pull request, which every workflow cancels.
• RELEASE.md: notes that a merge to the release branch cancels the manual test run, because both share a concurrency group.

Merging up

• The needs line of publish conflicts on 8.0.x, 8.1.x and 9.0.x, which also list hibernate7Functional and neo4jFunctional there. The resolution keeps both sides and adds casSecurityConfig, the CAS test job that 155a845 added to 8.0.x without adding it to publish, together with its success || skipped check.
• Those branches also have validateBomProperties, validateGithubBranch and buildLogic. They go into the needs of publishGradle with a success check, and into the list of build jobs in the comment at the top of gradle.yml and in AGENTS.md.
• With that resolution, the job-graph check passes on all three branches, every job is upstream of publish, and actionlint is clean.

Not changed here, discussed at the weekly

These are policy questions rather than bugs, so they were left out of this PR and discussed at the weekly:

1. [skip tests] on pull requests. Since Gate CI through one policy: draft and no-ci pull requests, tests by branch and marker #16673, a [skip tests] head commit skips the tests on a pull request into 8.0.x and above too, and on a fork pull request the author picks that commit (@codeconsole). Outcome: a follow-up pull request makes the marker count on a pull request only when its author is OWNER, MEMBER or COLLABORATOR.
2. run ci on drafts. On a draft, run ci also runs the full test matrix, so a draft into an older release branch or a side branch can't get a build-only run. Outcome: kept as it is. Before Gate CI through one policy: draft and no-ci pull requests, tests by branch and marker #16673, every draft ran the full matrix.

Verification

• Job-graph check:

  • Every workflow parses, and in each one every needs resolves.
  • Every job that reads a needs result lists that job, there is no cycle, and every always() job checks the result of every job it needs.
  • Every job of gradle.yml other than policy and the publish jobs is upstream of publish, and every test job is in its needs.
  • The same checks pass on the result of merging this into 8.0.x, 8.1.x and 9.0.x, with the resolution under "Merging up".

• actionlint 1.7.7: clean on the changed workflows. Its shellcheck notes are all in scripts this change doesn't touch.

• ./gradlew rat: passes.

• Publish chain under cancels. I modelled gradle.yml's job graph with GitHub's documented rules and cancelled it at every minute. The timings are rounded from the 7.0.x merge push of Gate CI through one policy: draft and no-ci pull requests, tests by branch and marker #16673: builds end at 4, 5 and 11 minutes, and tests at 61.

  Run	Cancel arrives	Before (publishGradle needs buildGradle)	After (it needs every build)
  without tests	before buildGradle ends	nothing	nothing
  without tests	after buildGradle, before buildForge ends	plugins, core, docs	nothing
  without tests	after every build	everything	everything
  with tests	before buildGradle ends	nothing	nothing
  with tests	after buildGradle, before buildForge ends	plugins	nothing
  with tests	after the builds, tests running	plugins	plugins
  with tests	after every test passed	everything	everything

  Before the second commit, a cancel while buildRerunTasks was the only test job still running published everything. In 8.0.x run 39644767954 that window lasted 24 minutes.

• First real run. The publish jobs only run on a push in apache, so the merge push to 7.0.x is the first real run of this change. On that run, publishGradle should start after buildForge, and publish, publishForge, verifyWrapper and docs should follow as before.

[jamesfredley]

Approving. publishGradle now waits for buildForge and validateDependencies and requires each result to be success, so a cancel or failure before those builds finish publishes nothing. A cancel after the builds still lets the documented chain publish. The docs match the workflow.

[jdaugherty]

@matrei thank you for following up on this.

• I agree with the publish changes / the changes you've made.

As for the discussion topics:

• A merge of the PR creates it's own commit, so skip tests doesn't skip unless someone intentionally adds [skip tests] to the merge commit. Since we control the behavior on merge, I don't think any change is required here.
• I don't think I understand the point about drafts: It was my understanding we only publish from X.X.X branches. By default, the drafts won't run. If you want to run normal CI as they did before this change, we would add run ci. What am I missing?

[codeconsole]

Approving. This is the fix I asked for on #16673: publishGradle now needs buildForge and validateDependencies and checks that each of them succeeded. publish and everything after it need publishGradle, so a cancel or a failure before the builds finish publishes nothing. I checked the job graph against two real runs:

• 7.0.x run 39615263956 (the merge of #16673, no tests): buildGradle ended about 6.2 minutes in, validateDependencies at 7.8 and buildForge at 14.2. publishGradle had already published by then (8.5 to 11.8). With this change, a cancel any time before 14.2 publishes nothing.
• 8.0.x run 39638916842: publishGradle ran from 17:10:46 to 17:13:11, and the cancel came at about 17:26 while every Build Grails Forge leg was still running. With this change, publishGradle would have been skipped.

The PR also merges into 8.0.x, 8.1.x and 9.0.x without conflicts. Both CI failures here are unrelated: the Forge code style job got a 429 from repo.grails.org, and the Release Drafter was cancelled by its per-base-branch concurrency group. The RELEASE.md and CONTRIBUTING.md wording matches the concurrency settings of every workflow.

I left two inline comments; neither blocks:

• One test job isn't in publish's needs, so the new "only publishGradle" sentence is wrong during a window that shows up in real runs.
• When this merges up, the same reasoning applies to three more 8.0.x jobs.

A note on the cost section: on a run without tests, the core, Forge and docs publishes move back by the same amount as the plugin snapshot, because publish waits for publishGradle. In 39615263956 that would have been about 5 minutes.

On discussion point 1, for the weekly: @jdaugherty, a merge commit does get its own message, but this repository also allows squash and rebase merges. The squash title setting is COMMIT_OR_PR_TITLE, so squashing a single-commit PR reuses that commit's title. A [skip tests] head commit then carries into the push unless the person merging edits the title. A rebase merge keeps the commit's message as it is. The other half of the point is the PR run itself. On a fork PR into 8.0.x or above, a [skip tests] head commit means the PR's tests never ran, which is easy to miss when every check is green.

[codeconsole]

buildRerunTasks is a test job, but it isn't in publish's needs. While it is the only test job still running, this sentence (and the same one at AGENTS.md:258) doesn't hold. Its result is never checked either, so the core is published even if it fails.

The window shows up in real runs. In 8.0.x run 39644767954:

• publish started 236.7 minutes in, right after the last Build Grails-Core leg finished.
• Build Grails-Core Rerunning all Tasks kept running until 260.3.

A cancel during those 24 minutes publishes the whole chain, and a failure there wouldn't have stopped it either. On 8.0.x, casSecurityConfig is missing from publish's needs in the same way: it was added in 155a845, after ee2b481 made the test jobs gate the publish.

I'd add buildRerunTasks to publish's needs here, with the same success || skipped check, and add casSecurityConfig in the merge-up. In 39644767954 that would have delayed the core snapshot by those 24 minutes. If that's too slow, narrow the wording to "a cancelled test job that publish needs" instead.

[matrei]

Good catch, thanks. I added buildRerunTasks to the needs and the checks of publish in dbf938c. I'd rather take the 24 minutes than publish a core whose --rerun-tasks build failed: it is the only core test run whose results can't come from the build cache.

The same commit adds a line to the publish comment and a rule to AGENTS.md: every test job goes into the needs of publish, and every job that builds or validates without tests goes into those of publishGradle. That should catch the next casSecurityConfig.

[codeconsole]

This merges into 8.0.x and 9.0.x without conflicts, but both branches also have validateBomProperties, buildLogic and validateGithubBranch, and none of them is in these needs. In 39644767954 all three finished well before buildForge (at 7.3, 15.3 and 7.0 minutes, against 55.9), so the cancel behaviour still holds there. The problems are elsewhere:

• A commit whose BOM property validation failed would still be published.
• On those branches, "every build job (buildGradle, buildForge and validateDependencies)" in the comment at the top would no longer be true.

It's worth adding the three jobs to needs and to the check in the merge-up commit.

[matrei]

Agreed. With buildRerunTasks added, the merge-up now conflicts on the needs line of publish on 8.0.x and above, because those branches also list hibernate7Functional and neo4jFunctional there. So the merge-up needs a hand anyway. I checked this resolution on 8.0.x, 8.1.x and 9.0.x:

publishGradle:
  needs: [ buildGradle, buildForge, validateDependencies, validateBomProperties, validateGithubBranch, buildLogic ]
publish:
  needs: [ publishGradle, build, buildRerunTasks, functional, security, securityConfig, casSecurityConfig, redis, hibernate5Functional, hibernate7Functional, mongodbFunctional, neo4jFunctional ]

It also needs the matching result checks (success for the new build jobs, success || skipped for casSecurityConfig), and the three build jobs added to the list in the comment at the top and in AGENTS.md. With that, the job-graph check passes on all three branches, every job is upstream of publish, and actionlint is clean. I can do the 8.0.x merge-up of this PR myself, or open a PR against 8.0.x right after it, whichever suits whoever merges up.

[matrei]

Thanks both.

@codeconsole, you're right about the cost: on a run without tests, everything after publishGradle moves back by the same amount. I've corrected the cost section of the description with the numbers from 39615263956 and 39644767954, and added a "Merging up" section.

@jdaugherty, the drafts point isn't about publishing, sorry for the confusion. On a draft, run ci does two things: it lets the draft run at all, and it asks for the tests. So a draft into 7.0.x, or into a side branch, gets either no CI or the full test matrix. It never gets the build-only run it gets once it's marked ready. Before #16673 every draft ran the full matrix, so nothing is lost compared with that. If that's how you want it, I'm fine leaving it.

On [skip tests], @codeconsole's reply covers what I meant. A squash merge of a single-commit PR, or a rebase merge, keeps the commit's title. And on a fork PR into 8.0.x, the PR's own tests never run. Happy to leave that for the weekly.

[jdaugherty]

@matrei Let's discuss these 2 items in the weekly and then merge what's been done already? Do you agree?

[jamesfredley]

A cancelled run no longer publishes, and the CI policy text matches that. 7.0.x is the right line.

[matrei]

@jdaugherty, agreed. I've merged 7.0.x in to resolve the conflict with #16779 in CONTRIBUTING.md, and recorded the weekly's outcome in the description. The [skip tests] change will follow in its own pull request once this is merged. From my side this is ready to merge.

[testlens-app]

🚨 Jobs failed without executing tests 🚨

Failed Jobs without Test Failures

❌ Build Health / Forge Projects
❌ Licensing - RAT Report / rat-audit

🏷️ Commit: 93b8c0e
▶️ Tests: 0 executed
⚪️ Checks: 32/32 completed

Rerun Controls

Click the checkbox to trigger a rerun:

• Rerun jobs

---

Learn more about TestLens at testlens.app/docs.