Skip to content

fix(ci): repair the invalid service-refresh workflow, add actionlint - #1140

Merged
jfrench9 merged 1 commit into
mainfrom
bugfix/service-refresh-invalid-workflow
Aug 11, 2026
Merged

fix(ci): repair the invalid service-refresh workflow, add actionlint#1140
jfrench9 merged 1 commit into
mainfrom
bugfix/service-refresh-invalid-workflow

Conversation

@jfrench9

Copy link
Copy Markdown
Member

Summary

service-refresh.yml on main is an invalid workflow file, and has been since the first commit of #1139. Since prod.yml and staging.yml call it via workflow_call, and an invalid callee fails the caller, deploys are broken until this lands.

Both errors came from #1139. This fixes them and adds actionlint so the class cannot recur silently — the guard #1139 added for exactly this purpose caught neither.

Changes

The two errors.github/workflows/service-refresh.yml

  1. A comment inside the Compute refresh job timeout run: block contained the literal ${{ }} delimiters, while explaining that arithmetic isn't allowed inside them. The expression parser scans the entire run: body and has no concept of a shell comment, so an empty expression there invalidates the whole file. The comment about the bug was the bug.
  2. timeout-minutes read a job output directly. Job outputs are always strings and timeout-minutes must be a number, so it needs fromJSON. The || 45 fallback didn't help — it yields a string too — and was unnecessary, since that job only runs when the collect job set the output.

Both are now covered by a comment saying why, including an explicit "do not write the delimiters in this block, not even in a comment."

Why it wasn't caughttests/infrastructure/test_workflow_expressions.py, deleted

That test was added in #1139 to prevent this exact class of bug. It was a regex hunting for arithmetic operators, so it matched the empty ${{ }}, found no operators inside, and passed. The type error was never in its reach at all. It's deleted rather than extended: keeping a weaker duplicate of a real linter is the same duplicated-fact drift this series has been fixing.

The replacement — actionlint

  • Added as a uv dev dependency (actionlint-py), so uv run actionlint works locally and in CI the same way cfn-lint does — no separate install step, no CI-only tool.
  • just lint-actions added and wired into both test-code and test-all, so it runs wherever the rest of the gate runs. just lint-actions-shell for the noisy shellcheck-enabled variant.
  • New CI step in test.yml alongside the CloudFormation lint.
  • Verified it fails on both errors above and exits 0 once fixed — a guard that only ever passes is worth nothing, which is the lesson from the one it replaces.

The baseline.github/actionlint.yaml

19 findings predate the linter. Fixing them isn't in scope for an urgent repair, so each is ignored with a stated reason: deliberately string-typed workflow_call toggles (service-refresh accepts both forms on purpose), org-level ACTIONS_TOKEN, type: keys on composite action inputs, the intentional empty tier choice in graph-asg-refresh.yml, and required-inputs-with-defaults. The file says explicitly that adding to that list is a decision needing the same justification, not a way to silence a new finding.

Confirmed by diffing normalized actionlint output between main and this branch: exactly the two errors disappear, nothing new appears (21 → 19).

shellcheck integration is off in the gate — dozens of pre-existing info-level SC2086 quoting notes across unrelated workflows would bury the errors that actually invalidate a file. Worth its own pass later.

Breaking Changes

None. No application code, schema, or API surface. One new dev-only dependency.

Testing

  • just test-code — passed (ruff, format, basedpyright, cf-lint, and the new actionlint step).
  • uv run actionlint -shellcheck= — exit 0 across all workflows and composite actions.
  • Negative test: reintroduced both errors, confirmed actionlint reports them and exits 1; restored, confirmed exit 0.
  • uv lock --check — clean.
  • tests/infrastructure/ — 43 passed (the 21 test_userdata_refresh.py cases from refactor(graph): move container refresh onto the instance #1139 still pass; 22 CloudFormation).

Not verified: nothing here can be confirmed until main is fixed, since the proof is GitHub accepting the file. After merge, the absence of a new startup-failure run on the merge commit is the signal.

service-refresh.yml has been an invalid workflow file since #1139's first
commit. GitHub records that as a failed run against each pushed commit even
though the workflow has no push trigger, which is the only place it showed
up — four such runs, easy to read as unrelated noise. Because prod.yml and
staging.yml call this workflow via workflow_call, an invalid file fails the
caller, so deploys were broken too.

Two errors, both introduced by #1139:

1. A comment inside the "Compute refresh job timeout" run: block contained
   the literal expression delimiters while explaining that arithmetic is not
   allowed in them. The expression parser scans the entire run: body and has
   no concept of a shell comment, so an empty expression there invalidates
   the file. The comment about the bug was the bug.

2. timeout-minutes read a job output directly. Job outputs are always
   strings and timeout-minutes must be a number, so it needs fromJSON. The
   `|| 45` fallback did not help — it yields a string too — and was
   unnecessary, since the job only runs when the collect job set the output.

The guard added in #1139 caught neither: it was a regex looking for
arithmetic, so it matched the empty expression, found no operators, and
passed. A type error was never in its reach at all. That approach was the
wrong tool, and it is deleted rather than extended — keeping a weaker
duplicate of a real linter is the same duplicated-fact drift this series has
been fixing.

Replaced with actionlint, added as a uv dev dependency so `uv run actionlint`
works locally and in CI exactly like cfn-lint, wired into `just lint-actions`
and both `test-code` and `test-all`, and run as a CI step. Verified it fails
on both errors above and passes once fixed.

.github/actionlint.yaml baselines 19 findings that predate the linter, each
with a stated reason: deliberately string-typed workflow_call toggles,
org-level ACTIONS_TOKEN, `type:` keys on composite action inputs, the
intentional empty tier choice, and required-with-default inputs. The file
says explicitly that adding to that list is a decision, not a way to silence
a new finding. shellcheck integration is off in the gate because dozens of
pre-existing info-level quoting notes would bury the errors that actually
invalidate a file; `just lint-actions-shell` still shows them.
@jfrench9
jfrench9 merged commit 688371b into main Aug 11, 2026
7 checks passed
@jfrench9
jfrench9 deleted the bugfix/service-refresh-invalid-workflow branch August 11, 2026 06:12
jfrench9 added a commit that referenced this pull request Aug 11, 2026
Completes the fleet-refresh spec. The graph container refresh no longer
enumerates instances into a GitHub Actions matrix with a runner job each; it
issues one tag-targeted SSM command per node-type group. That removes a hard
256-job matrix ceiling — `databases_per_instance: 1` makes customer count
equal instance count — and stops paying runner-minutes to sleep through a
30-minute busy-wait that now happens on the instance.

- Phase 3: replicas are tagged LadybugRole=replica alongside the existing
  NodeType=shared_replica. The value must not be shared_replica, which would
  match graph_volume_monitor's volume-expansion filter and pull the replica
  fleet into a loop it is deliberately excluded from.
- Phase 4: bin/lambda/graph_container_refresh.py (start/status), declared in
  graph-infra.yaml with SendCommand scoped by resource tag, plus an
  EventBridge rule on SSM invocation status-change filtered to
  Failed/TimedOut into InfraAlertTopic. Failures now page whether or not
  anyone is watching the run, which the old design never did at any fleet
  size.
- Phase 5: service-refresh.yml's collector and matrix collapse into one job;
  .github/actions/refresh-graph-containers is deleted; graph-maintenance.yml's
  serial per-instance docker-cleanup loop becomes one tag-targeted command.

Three deviations from the spec, each for a reason:

Targeting is per node-type group rather than one expression. SSM ANDs across
target keys and ORs within one, so "writers by LadybugRole OR replicas by
NodeType" cannot be a single command. Splitting them also answers the spec's
open question — a replica restart is far less customer-visible than a
writer's and wants its own rate control. The Lambda matches replicas on
NodeType, not the new LadybugRole=replica, so the fan-out works during that
tag's boot-time rollout instead of silently selecting nothing.

Retiring the composite action removed the code that translated exit 3/4 into
a warning, and SSM records every non-zero exit as Failed — so the first draft
of the collapse silently reintroduced the #1140 regression where an
un-cycled fleet fails the deploy. The Lambda's status now buckets those two
codes as skipped and reports failed separately; the workflow fails only on
failed. The exit-code contract test moved to follow it.

FORCE_RESTART is new. secrets-rotation.yml refreshes containers so they
re-read rotated credentials, but rotation does not change the image, so the
digest-skip made that step a fleet-wide no-op. Credentials come from Secrets
Manager and are cached in-process behind a 1-hour TTL, so the restart is the
work; without the override rotation accomplishes nothing until the TTL
lapses. Also corrects secrets_manager's docstring, which claimed an
lru_cache on the read path that does not exist — the false claim implies
rotation never self-heals without a restart.

Adds 19 Lambda tests covering targeting, command construction, rate-control
defaults, and every branch of the skip classification, plus two more on the
shell side for FORCE_RESTART.
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