Order the Windows signing jobs instead of grouping them - #200
Merged
Merged
Conversation
The concurrency group added an hour ago made things worse, not better. Only one job per group may be pending: when the CLI matrix's three legs and the Windows app all queued on `certum-simplysign`, each new pending leg cancelled the one before it, and the v1.8.0 run lost "CLI (linux)" and "CLI (windows)" outright. The documentation's "up to 100 pending" describes `queue: max`, not the default. So the group now covers only jobs that are not matrices — the Windows app and the manual signing check — where at most one can ever be pending. The race it was meant to fix is between the Windows app and the CLI's windows leg, and that is now ordering: `cli` lists `windows` in `needs`, with an `always()` guard so a failed Windows app still lets the CLI build and sign rather than skipping it. The cost is that the linux and macOS legs start after the app job, which the group was charging them anyway. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a regression I introduced in #199.
What went wrong
Only one job per concurrency group may be pending. When a newer job wants the group, the one already waiting is cancelled.
cancel-in-progress: falsegoverns the running job, not the queue. (The "up to 100 pending" in the docs describesqueue: max, which is a different setting.)cliandauditare matrix jobs, so putting one group on them enrolled every leg. In the v1.8.0 run,CLI (linux)andCLI (windows)were cancelled the second they started:I had raised this hazard when proposing the fix and then talked myself out of it on a docs answer that was about a different setting. That was the error.
What this does instead
clilistswindowsinneeds, guarded withalways() && needs.check.result == 'success'so a failed Windows app still lets the CLI build and sign instead of skipping it.Verification
YAML parses and the three jobs carry what they should. Not proven against a real release, and deliberately so: each attempt is a Certum login, and the account locks on repeated failure. See the note below before the next attempt.
Unrelated: the v1.8.0 signing attempt failed differently
The run after the re-tag failed at a new place —
Certificate *** did not appear in Cert:\CurrentUser\My within 180 seconds, i.e. no rejection modal at all, the certificate simply never arrived. That is not the TOTP race and not this change. Given how many logins that account saw today, I would not spend another one before someone checks the account's standing in SimplySign.Co-authored-by: Claude Opus 5 noreply@anthropic.com