Skip to content

fix(ci): stop the CLA workflow cancelling itself on its own instructions - #148

Merged
L4XB merged 1 commit into
mainfrom
fix/cla-concurrency
Sep 15, 2026
Merged

L4XB merged 1 commit into
mainfrom
fix/cla-concurrency

Conversation

@L4XB

@L4XB L4XB commented Sep 15, 2026

Copy link
Copy Markdown
Member

Summary

cancel-in-progress: false on the CLA workflow, so it stops cancelling itself on
the sequence CONTRIBUTING.md asks every contributor to perform.

Seen on #144, and on every pull request before it:

Canceling since a higher priority waiting request for cla-144 exists
CLA acceptance verified for @L4XB on pull request #144.
##[error]The operation was canceled.

The run had already verified the acceptance and published the status. It was
cancelled during cleanup by the next event in the same concurrency group, and
GitHub reports a cancelled run as a failed check.

Behavior and compatibility

What was happening

  1. pull_request_target: opened starts run A.
  2. The author posts the acceptance sentence — because CONTRIBUTING.md tells
    them to — which fires issue_comment: created and queues run B.
  3. B enters group cla-<number>, cancel-in-progress: true kills A.
  4. A is reported as a failed check named Publish CLA status, sitting next to
    the green CLA / acceptance it produced moments earlier.

The required check was never in danger: CLA / acceptance is the required
context and it was green, and Publish CLA status is not required, so nothing
was ever blocked. Nothing on the pull-request page says that. A first-time
contributor sees a red X next to the word CLA immediately after doing exactly
what they were asked.

Why queueing is correct here, not just quieter

Cancelling would be the right choice if a superseded run's work were wasted.
It is not, and it was never safe:

  • check_cla.py reads the head SHA from GET /pulls/{n} at the start of each
    run, not from the event payload, and posts the status against that commit.
    Two runs therefore write per-commit statuses that cannot clobber each other
    wrongly.
  • With the group serialized, the run that finishes last is the one that read the
    newest comments and the newest head. Convergence, not a race.
  • Cancelling, by contrast, left a real window: a run could publish pending, be
    killed before publishing the verdict, and leave a pull request pending on a
    check that nothing would re-trigger until the next event.

issue-claims.yml already reaches the same conclusion for its own reason ("Two
people claiming the same issue in the same second must not both win"). This
brings the second workflow that reacts to comments into line with it.

Cost

One job of about eight seconds per comment event on a pull request, instead of
collapsing them. They run one at a time, so a chatty pull request queues rather
than fans out.

Residual case, stated rather than hidden

With cancel-in-progress: false, GitHub still cancels a pending run when a
newer one queues behind an already-running one. That needs three events in quick
succession — open, comment, comment — and cancels a run that never started
rather than one mid-flight. The common sequence this issue is about, open then
accept, produces no cancellation at all. Eliminating the remaining case would
mean not triggering on ordinary comments, which would also stop noticing a
deleted acceptance; that trade is not worth making here.

Not changed

The verifier itself, the status context, the acceptance sentence, the reminder,
the permissions, the checkout of the protected default branch, and the triggers.
Rollback is reverting one line.

Validation

$ uv run pytest -q tests/test_repository_automation.py   # 74 passed  (72 before)
$ uv run pytest -q                                       # 357 passed
$ uv run ruff check src tests .github/scripts            # All checks passed!
$ uv run ruff format --check src tests .github/scripts   # 24 files already formatted
$ uv run mypy src/sixsentences                           # no issues found in 40 source files
$ python -c "import yaml; yaml.safe_load(open('.github/workflows/cla.yml'))"
  concurrency: {'group': 'cla-…', 'cancel-in-progress': False}

Two new tests, one per acceptance criterion that can be asserted locally:

  • test_the_cla_workflow_queues_its_runs_instead_of_cancelling_them pins the
    setting and the reason, in the same shape as the existing claim-workflow test.
  • test_repeated_cla_runs_repeat_the_result_without_repeating_the_reminder
    plays the real serialized sequence against a stubbed API — open (reminder
    posted), the reminder's own event (no second reminder), acceptance (success),
    and one more run (still success, still one reminder). This is the criterion
    about idempotency and the one about not reminding twice.

The criterion about a synchronize mid-run is covered by construction and by
the existing test_cla_status_is_bound_to_the_exact_pull_request_head: the
status is always posted against the head SHA read from the API during that run,
so the queued synchronize run ends with the newest commit carrying a verdict.

The end-to-end criterion — "opening a pull request and posting the CLA sentence
immediately afterwards leaves no failed check" — can only be observed on a real
pull request. This pull request is that test: it was opened and the
acceptance sentence posted immediately afterwards. Note that the workflow that
runs for this pull request is the one on main, so the red X is expected here
and should be gone on the next pull request after the merge. I will confirm it
on the following one.

  • Engine checks pass, or the engine is unaffected.
  • API, migration, worker, and web-contract checks pass, or they are unaffected. — unaffected
  • Web type-check, tests, and production build pass, or the web app is unaffected. — unaffected
  • Browser-extension contracts and a deployment-bound build pass, or the extension is unaffected. — unaffected
  • macOS Companion boundary check, locked resolution, tests, and release build pass on the pinned Xcode toolchains—or the Companion is unaffected. — unaffected
  • Self-hosting tests and container builds pass, or deployment is unaffected. — unaffected
  • User-facing behavior has a focused test or the omission is explained.

Review boundaries

  • Security and privacy effects were reviewed. The workflow keeps
    pull_request_target reading only the protected default branch, still
    checks out no contribution ref, still executes no contributor-controlled
    code, and keeps the same permissions: block. Concurrency changes when a
    run executes, never what it is allowed to do. Serializing removes a
    cancellation window in which a pull request could be left permanently
    pending on a required status.
  • New network calls and processors are operator-configurable, fail closed, and document data egress, cost, retention, and failure behavior—or none were added. — none
  • Dependencies and bundled assets are justified, locked, and redistribution-compatible—or none were added. — none
  • Native-client changes include explicit origin, local-retention/deletion, permission, signing, update, and binary-distribution implications. — none
  • Research-method assumptions, limitations, and provenance remain visible—or no research-facing behavior changed. — none
  • Accessibility and keyboard behavior were reviewed for UI changes—or no UI changed. — no UI
  • Browser permissions, capture bounds, pairing callbacks, extension storage, and generated host access were reviewed—or the extension is unaffected. — unaffected

Source-release hygiene

  • No secret, private key, production configuration, customer/participant data, user upload, database dump, log, private prompt, or non-redistributable research content is included.
  • The change belongs in the community stack; payment, subscription, commercial-plan, hosted-administration, and marketing-site code remains separate.
  • Public behavior and limitations are documented — in the workflow comment and above, including the residual case.
  • CHANGELOG.md is updated for user-visible changes, or the omission is explained above: this changes when a repository workflow runs. It does not change the engine, the API, the web client or any released artifact, so there is nothing for a user of a release to read.
  • Every commit carries my own matching DCO Signed-off-by trailer.
  • I have read CLA.md and posted its exact acceptance sentence as a standalone pull-request comment.
  • I have read and will follow the Code of Conduct.

Visual evidence

No UI change.

Closes #146

@L4XB

L4XB commented Sep 15, 2026

Copy link
Copy Markdown
Member Author

I have read and agree to the SixSentences CLA v1.0.

Every pull request showed a failed check named "Publish CLA status" beside the
green "CLA / acceptance" it had just produced:

    Canceling since a higher priority waiting request for cla-144 exists
    CLA acceptance verified for @L4XB on pull request #144.
    ##[error]The operation was canceled.

The run had already done its work. It was cancelled during cleanup, and GitHub
reports a cancelled run as a failed check.

Two events seconds apart share the concurrency group: opening the pull request
fires `pull_request_target`, and posting the acceptance sentence fires
`issue_comment`. That is not an unlucky ordering — CONTRIBUTING.md instructs the
author to post that sentence on the pull request they just opened, so the
comment reliably lands mid-run. The workflow cancelled itself on the one
sequence the documentation asks every contributor to perform, and showed a red X
next to the word CLA to the people least equipped to read it.

The runs queue now, as issue-claims.yml already does for the same reason. They
are idempotent: each reads the head commit and the comments live and publishes a
status for that exact commit, so serialized runs converge rather than fight, and
the reminder stays behind its marker check. A test plays the real sequence —
open, remind, remind again, accept, re-run — and asserts one reminder and a
stable verdict.

Queueing is also the safer of the two. Cancelling left a window where a
half-finished run could publish a stale verdict; serialized runs mean the one
that finishes last is the one that saw the newest state.

Closes #146

Signed-off-by: L4XB <L4XB@users.noreply.github.com>
@L4XB
L4XB force-pushed the fix/cla-concurrency branch from 3797e83 to b54eb22 Compare September 15, 2026 11:04
@L4XB

L4XB commented Sep 15, 2026

Copy link
Copy Markdown
Member Author

Self-review

Recorded under the founding-maintainer exception in GOVERNANCE.md. Two files,
both read in full.

.github/workflows/cla.yml — one value changes, cancel-in-progress: true
false, plus the comment explaining it. Checked:

  • YAML shape. Parsed with yaml.safe_load; concurrency resolves to
    {'group': 'cla-…', 'cancel-in-progress': False} and the single job is still
    acceptance-status. A comment inside a mapping cannot change the structure,
    but the file gates a required check, so it was parsed rather than assumed.
  • The group expression is untouched. It still falls back to github.run_id
    for an event carrying neither a pull request nor an issue, which keeps
    unrelated runs out of one another's queue.
  • Queue depth. With cancel-in-progress: false GitHub runs one and queues
    one; a third event arriving while one is already pending cancels the pending
    one. That needs three events in quick succession and cancels a run that never
    started. Stated in the pull-request description rather than left for someone
    to discover.
  • Cost. One ~8 second job per comment event instead of collapsing them,
    serialized. On a pull request with thirty comments that is thirty short runs,
    one at a time.
  • Nothing else moved. permissions:, the pull_request_target checkout of
    the protected default branch, persist-credentials: false, the triggers and
    the --remind invocation are all unchanged, so the security properties the
    existing tests assert are untouched — and those tests still pass.

tests/test_repository_automation.py — two tests added, none modified.

  • test_the_cla_workflow_queues_its_runs_instead_of_cancelling_them mirrors the
    existing test_the_claim_workflow_never_puts_a_comment_into_a_shell in shape.
    It asserts the absence of cancel-in-progress: true as well as the presence
    of false, so a job-level concurrency block reintroducing cancellation fails
    the test rather than slipping past a substring check.
  • test_repeated_cla_runs_repeat_the_result_without_repeating_the_reminder
    plays the sequence serialization actually produces: open → reminder, the
    reminder's own event → no second reminder, acceptance → success, one more run
    → still success. The stub for _post_guidance appends to the same list
    _comments returns, so guidance_is_needed is exercised through main
    rather than called directly. main returning 0 on a missing acceptance is
    deliberate in the script — the status carries the verdict, not the exit code —
    and the test asserts 0 throughout so that contract is not quietly changed
    either.

What this cannot prove locally

The end-to-end criterion. The workflow that ran for this pull request is the
one on main, so the red Publish CLA status here is expected. Confirming it
is gone belongs on the next pull request opened after this merges, and I will
check it there rather than claim it now.

Checks

Every required context is green.

Merging.

@L4XB
L4XB merged commit c259a0a into main Sep 15, 2026
23 checks passed
@L4XB
L4XB deleted the fix/cla-concurrency branch September 15, 2026 11:25
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.

ci: the CLA workflow cancels itself on the sequence CONTRIBUTING tells authors to follow

1 participant