Skip to content

Hold staging for the whole deploy-and-verify sequence, not just the deploy - #381

Merged
Babissimo merged 1 commit into
mainfrom
fix/staging-deploy-verify-atomic
Sep 14, 2026
Merged

Babissimo merged 1 commit into
mainfrom
fix/staging-deploy-verify-atomic

Conversation

@Babissimo

Copy link
Copy Markdown
Contributor

Closes ClickUp 123zgec26td.

The race

deploy-staging was the only job in the staging-deploy concurrency group.
staging-smoke-tests and e2e-staging carried no concurrency at all, so the
deploy was serialised while everything that verifies the deploy was not. A
following run's deploy was free to land between one run's smoke tests and its
E2E, restarting the box underneath a suite that was testing it.

Three consecutive merges failed that way on 2026-09-10 (#348, #351, #352), and
four runs inside three minutes produced the same on 2026-08-21 (#223). None
failed for a fault in the change.

Adding group: staging-deploy to the two verification jobs does not close it:
concurrency is acquired and released per job, so a deploy can still slot in
between one run's smoke tests and its E2E. The environment has to stay held
across the whole sequence.

The change

The three jobs move to .github/workflows/staging-deploy-verify.yml, and
ci.yml calls it from one Staging job that carries the group. A calling job
is not complete until every job in the called workflow has finished, so the
group is held from the first byte of the deploy to the last E2E assertion.
jobs.<job_id>.concurrency is
supported on a job that calls a reusable workflow,
so this is documented behaviour rather than an inference.

The step bodies move unchanged — verified byte-identical against origin/main
(124 / 35 / 27 lines). Only the job headers differ.

Two things the move forced:

  • env does not cross a workflow_call boundary, and with: on a calling job
    cannot read the env context either. APP_DIR is therefore re-exported as a
    changes output rather than restated in the new file, so the deploy
    directory keeps one definition. The re-export refuses an empty value: cd ""
    succeeds into $HOME, where every one of the deploy's pre-flights passes
    before git fetch fails in a directory nothing is served from.
  • playwright-image joins the staging job's prerequisites, so an unpublished
    image fails the run before the droplet is touched rather than after a deploy
    and a smoke suite have already landed on it. The cost is that a registry blip
    now blocks the staging deploy instead of only the E2E.

What this does not fix, deliberately

  • A cancelled Staging job during a burst is expected. At most one run sits
    pending per concurrency group, so a third merge arriving while one run holds
    staging and another is queued cancels the queued one. main is linear, so
    the run that replaces it deploys a superset — but the cancelled commit's own
    run never reaches production, and the last merge in a burst has no successor
    to carry it. Documented in ONBOARDING rather than changed; queue: max would
    serialise every merge's full sequence instead, which is a worse trade.
  • The group is now held for longer. The sum of the called jobs' caps is 95
    minutes, against 30 before. A hung E2E used to block nothing and now blocks
    staging. That is the cost of holding the environment, and it is the point.

Verification

  • actionlint 1.7.12 clean on both files, including its shellcheck pass. I
    confirmed the lint is not a no-op by breaking the input name and watching it
    report the workflow-call contract error, then restoring.
  • Structural checks over the job graph, the call contract (inputs, secrets) and
    needs.*.outputs.* references.
  • backend/.venv/bin/pre-commit run --all-files passes.
  • Branch protection on main lists no required status checks, so renaming the
    check contexts breaks nothing.

The one thing no pre-merge check can prove is the runtime semantic itself: the
staging chain is gated on push to main, so this PR's own run does not execute
the called workflow. The first burst of merges after this lands is what
confirms it, and the ticket records what to look for.

🤖 Generated with Claude Code

@claude

This comment has been minimized.

…eploy

`deploy-staging` was the only job in the `staging-deploy` concurrency group.
`staging-smoke-tests` and `e2e-staging` carried no concurrency at all, so the
deploy was serialised while everything that verifies the deploy was not: a
following run's deploy was free to land between one run's smoke tests and its
E2E, restarting the box underneath a suite that was testing it.

Three consecutive merges failed that way on 2026-09-10 (#348, #351, #352), and
four runs within three minutes produced the same thing on 2026-08-21 (#223).
None of them failed for a fault in the change. The failure does not present as
a flake either: a test hitting a half-swapped application gets a specific,
plausible assertion failure, which reads as a content regression and costs
someone real time before they conclude otherwise. It also blocks releases, as a
red staging E2E skips `deploy-production`.

Adding the group to the two verification jobs does not close it. Concurrency is
acquired and released per job, so a deploy can still slot in between one run's
smoke tests and its E2E. The environment has to stay held across the sequence.

So the three jobs move to `staging-deploy-verify.yml` and ci.yml calls it from
a single `Staging` job that carries the group. A calling job is not complete
until every job in the called workflow has finished, so the group is held from
the first byte of the deploy to the last E2E assertion. The step bodies move
unchanged; only the job headers differ.

Two things the move forced:

- `env` does not cross a `workflow_call` boundary and `with:` cannot read the
  `env` context, so APP_DIR is re-exported as a `changes` output rather than
  restated in the new file, keeping one definition of the deploy directory.
- `playwright-image` joins the staging job's prerequisites. An unpublished
  image now fails the run before the droplet is touched rather than after a
  deploy and a smoke suite have landed on it.

Two comments that named the old job IDs move with them: the DNS-tolerance
notes in frontend/e2e/dashboard.spec.ts and deploy/staging-smoke-test.sh both
explained themselves by their job being a direct `needs:` of deploy-production,
which is now indirect through the calling job.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the fix/staging-deploy-verify-atomic branch from 827841e to 3b9f691 Compare September 14, 2026 16:19
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 8m 36s —— View job


Review of PR #381

  • Read CLAUDE.md / ONBOARDING.md for repo-specific rules
  • Review the diff (ci.yml, new staging-deploy-verify.yml, ONBOARDING.md, docs/architecture.md, and the two comment updates)
  • Check the reusable-workflow mechanics (concurrency, env/with boundary, needs.*.outputs.*, secrets/inputs contract)
  • Verify step bodies are byte-identical to origin/main as claimed
  • Check whether previously-flagged stale comments were fixed
  • Post review feedback

Note: actionlint still isn't installed in this sandbox, so I couldn't independently execute the lint (same limitation as the prior pass on this PR).

What's new since the last review

The two stale comments I flagged previously are fixed in this update:

  • deploy/staging-smoke-test.sh:106 now says the WARN-tolerance logic "runs inside the staging job that deploy-production needs (ci.yml calls staging-deploy-verify.yml)" instead of naming the retired staging-smoke-tests job ID.
  • frontend/e2e/dashboard.spec.ts:104-106 similarly now describes the E2E suite as running "inside the staging job that deploy-production needs" instead of naming e2e-staging.

Both accurately reflect the new structure.

Mechanics re-verified

  • deploy, smoke-tests, and e2e in staging-deploy-verify.yml remain byte-identical to the old job bodies, only job headers and the inputs.playwright_image reference differ.
  • The staging job's with:/secrets: block matches the reusable workflow's declared inputs/secrets exactly.
  • deploy-production's needs: [staging] correctly inherits the same skip/failure propagation the old [staging-smoke-tests, e2e-staging] list gave it.
  • The APP_DIR re-export still guards against an empty value before it can reach cd "".

One residual risk (not a bug, already acknowledged by the author)

The entire fix hinges on GitHub holding the staging-deploy concurrency group for the full duration of the called reusable workflow — i.e., that a job's concurrency: doesn't release once the uses: call is dispatched, but only once every job inside staging-deploy-verify.yml finishes. This is documented GitHub behavior (linked in the PR body) and is architecturally sound, but as the PR description itself says, it's "the one thing no pre-merge check can prove" — it can only be confirmed by watching the first burst of merges after this lands. Worth keeping an eye on per the ClickUp ticket; not a blocker.

Nothing else stood out. The concurrency-holding mechanism, the secret/input contract, and the moved step bodies all check out.

@Babissimo
Babissimo merged commit f2d5f57 into main Sep 14, 2026
14 checks passed
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.

1 participant