refactor(graph): move container refresh onto the instance - #1139
Merged
Conversation
Adds bin/userdata/common/refresh-graph-container.sh as the single
instance-side refresh entry point — "bring yourself to the current image,
safely" — and reduces the composite action to dispatching one SSM command
to it and reporting the result. The GHA matrix is unchanged; what moved is
where the per-instance logic runs, which makes a refresh runnable and
debuggable by hand instead of only via a workflow dispatch.
Writing down what a refresh actually needs surfaced six live defects, all
of the same shape — a fact duplicated in two places and drifted:
- The `aws ssm wait command-executed` waiter is delay:5 x maxAttempts:20 =
100s, shorter than a pull + restart. On expiry it reported failure for a
refresh that was still succeeding, and the natural re-run cycled the
container twice. Replaced with a bounded poll whose budget derives from
max-wait-minutes.
- The collector read WriterTier with a describe-tags call per instance,
inside the loop, though describe-instances had already returned it.
- graph-maintenance.yml queried LadybugRole=shared-writer, a value nothing
sets (all tiers are LadybugRole=writer, distinguished by WriterTier), so
it matched nothing on every run. Inert — shared writers were already
covered by the query above it.
- graph_volume_monitor filtered LadybugRole on shared_master/shared_replica,
which are NODE_TYPE values. Also inert, but a trap: tagging replicas
shared_replica would have silently pulled them into the volume-expansion
loop discover_replica_instance_ids exists to keep them out of.
- A refreshed shared replica was not the same container as a launched one.
ladybug-replica.sh exported nine mount/profile vars at boot and persisted
none, so a refresh fell back to defaults and came back with no Lance or
staging mount while LANCE_INDEX_PATH still pointed into the missing one.
Silent: the database path survived only because its derivation is
relative. Now persisted, and GRAPH_ENV_SCHEMA makes an incomplete
environment a refusal rather than a silent default.
- refresh-graph's timeout-minutes was 15 while max_wait_minutes defaulted
to 30, so a busy instance could never be waited out. Now derived.
- The refresh pulled ${ECR_URI}:${environment} rather than the ECR_IMAGE_TAG
the instance recorded. Usually identical, but the shared replicas can be
pinned to a build tag during a storage-format-breaking engine upgrade
precisely so a boot cannot pull a new engine early — a refresh in that
window defeated the pin. The instance knows its own pin.
Also: CONTAINER_NAME is now derived in exactly one place, queried via
run-graph-container.sh --print-container-name; the digest-skip turns an
unchanged image into a no-op pull instead of a customer-visible restart;
and the now-dead graph_health_check_timeout plumbing is removed from
service-refresh.yml, prod.yml and staging.yml.
Because /etc/environment is written at boot, un-cycled instances will skip
with a GHA warning (exit 3, REFRESH_RESULT=skipped-stale-env) rather than
refresh or fail, until ASG churn or a one-shot backfill carries the fix.
Validated against 20 stubbed cases covering every fail-open branch of the
busy-counter wait, the digest-skip matrix, failure propagation, and the
pinned tag.
Pre-ship review of the previous commit found a blocker and two more copies of the container-name mapping. The blocker: common userdata scripts are downloaded from S3 in userdata, i.e. only at boot, so package-scripts uploading refresh-graph-container.sh does not put it on a running instance. The next deploy would have run the new action against instances with no script, bash would have returned 127, and the action would have failed every graph instance. The dispatched command now carries an explicit -x presence guard that exits 4, and the action maps that to the same warning-and-skip as the exit-3 stale environment case. The guard is an explicit test rather than an inference from 127 on purpose: a missing docker inside the script exits 127 too, and that is a real failure that must not be downgraded to a warning. The container-name mapping had four derivations, not the two the previous commit claimed to have collapsed. graph-health-check.sh and graph-lifecycle.sh each had their own copy; both now ask run-graph-container.sh --print-container-name. A sweep for the literal graph-api-shared is how to check it stays that way. Converting graph-health-check.sh also showed the earlier mount-persistence fix was worth more than stated: that script restarts a dead container by calling run-graph-container.sh directly, from a 5-minute cron, and both health-check wrappers source the same /etc/environment. A crashed replica was being auto-restarted without its Lance and staging mounts, unattended — the service refresh was just the path someone would eventually notice. Persisting the variables fixed both paths at once, because both read the same file. Adds tests/infrastructure/test_userdata_refresh.py: 21 cases driving the real script as a bash process against stubbed aws/docker binaries, with no test hooks added to the production script. It covers every fail-open branch of the busy-counter wait (which is a coordination signal, not a guard, so each "proceed anyway" path is deliberate and a future tightening of one would look like a bug fix), the digest-skip matrix, failure propagation, and the pinned tag. It also pins the exit-code contract between the script and the composite action, which have no other coupling — if the codes drift apart a stale instance silently goes from a warning back to a red deploy.
Member
Author
|
@claude can you review |
|
Claude finished @jfrench9's task in 2m 17s —— View job ReviewReviewed the diff against Verified correct:
Worth a glance, not a blocker:
Test coverage ( Nothing here blocks merge. |
The @claude review on #1139 flagged the derived `timeout-minutes` as relying on a newer expression feature. It is worse than that: GitHub Actions expressions have no arithmetic operators at all. The documented operator set is grouping, index, property dereference, `!`, the four comparisons, `==`, `!=`, `&&` and `||` — so the `+` in timeout-minutes: ${{ fromJSON(inputs.max_wait_minutes || '30') + 15 }} is a parse error rather than a sum. Because service-refresh.yml is a reusable workflow called by both prod.yml and staging.yml, the first thing it would have broken is a deploy — and it would have broken the API, Dagster and worker refresh jobs alongside the graph one, none of which this PR otherwise touches. The derivation itself was the right idea (a static 15 was shorter than the 30-minute default busy-wait, so a busy instance could never be waited out), so it moves rather than reverts: collect-graph-instances computes the value in a run step, validates it is a positive integer, and exposes it as a job output that refresh-graph reads. Same no-drift property, only supported syntax. Adds tests/infrastructure/test_workflow_expressions.py to keep this class of bug from recurring silently. There is no actionlint in CI, so a malformed expression is invisible until the workflow is triggered. The guard scans every ${{ }} in .github/workflows and .github/actions for arithmetic, after stripping the two constructs that legitimately contain those characters: single-quoted literals ('us-east-1') and object-filter wildcards (needs.*.result — which the first version of this test flagged as multiplication). Verified it fails on the exact expression above and passes on the replacement.
jfrench9
added a commit
that referenced
this pull request
Aug 11, 2026
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.
This was referenced Aug 11, 2026
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
1 participant
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.
Summary
Moves the per-instance graph container refresh off the GitHub Actions runner and onto the instance, as a single script (
refresh-graph-container.sh) an operator can run by hand over SSM. This is phases 1–2 ofspecs/data-plane/fleet-refresh-control-plane.md; the GHA matrix is unchanged, so this is a prerequisite for the later tag-targeted fan-out rather than the fan-out itself.Writing down what a refresh actually needs surfaced six live defects in the existing path, all of the same shape — a fact duplicated in two places and drifted apart. Two of them (a defeated image-tag pin, and replicas losing volume mounts on restart) were silent and would not have shown up as failures.
Changes
New instance-side entry point —
bin/userdata/common/refresh-graph-container.sh/etc/environment, waits out in-flight destructive ops, pulls before stopping, no-ops when the image digest is unchanged, delegates the swap torun-graph-container.sh, prunes.instance_busyis a coordination signal, not a guard (ref/data-plane.md§65) — and the script says so, because a future edit tightening one would look like a bug fix.bin/userdata/common/*.sh, so no upload change was needed.Six defects fixed
aws ssm wait command-executedisdelay:5 × maxAttempts:20= 100s, shorter than a pull plus restart. On expiry it reported failure for a refresh that was still succeeding, and re-running the deploy cycled the container a second time. Replaced with a bounded poll whose budget derives frommax-wait-minutes.WriterTierwith adescribe-tagscall per instance, inside the loop, thoughdescribe-instanceshad already returned it. Now one--query.graph-maintenance.ymlqueriedLadybugRole=shared-writer— a value nothing sets (all writer tiers areLadybugRole=writer, distinguished byWriterTier), so it matched nothing on every run. Inert; shared writers were already covered by the query above it.graph_volume_monitorfilteredLadybugRoleonshared_master/shared_replica, which areNODE_TYPEvalues. Also inert, but a trap: it would have silently pulled replicas into the volume-expansion loop thatdiscover_replica_instance_idsexists to keep them out of.ladybug-replica.shexported nine mount/profile variables at boot and persisted none to/etc/environment, so any restart fell back to defaults and the container came back with no Lance and no staging mount — whileLANCE_INDEX_PATHstill pointed into the missing one. The database path survived only because its derivation is relative. This affected the 5-minute health-check cron too, not just refreshes, so a crashed replica was being auto-restarted degraded and unattended.${ECR_URI}:${environment}rather than theECR_IMAGE_TAGthe instance recorded. Usually identical — but shared replicas can be pinned to a build tag during a storage-format-breaking engine upgrade, specifically so a boot cannot pull a new engine before the new-formatsec.lbugis published (deploy-graph.yml:57). A refresh in that window defeated the pin during the upgrade it protects.Also:
refresh-graph'stimeout-minuteswas 15 whilemax_wait_minutesdefaulted to 30, so a busy instance could never be waited out — now derived from the same input, as are the SSMexecutionTimeoutand the poll budget.Container-name mapping consolidated — worth a close look
CONTAINER_NAMEhad four derivations:run-graph-container.sh, the composite action,graph-health-check.sh, andgraph-lifecycle.sh. The first is now the sole owner and answers--print-container-name; the other three ask it.grep -rn 'graph-api-shared' bin/ .github/is the cheap invariant.Un-cycled instances skip rather than fail — worth a close look
Common userdata scripts are downloaded from S3 at boot, so uploading a new script does not place it on a running instance. Two distinct outcomes, both a GHA
::warning::with the container left alone:4skipped-no-script3/etc/environmentpredates the completeness contract (GRAPH_ENV_SCHEMA)skipped-stale-envAny other non-zero exit stays a hard failure. Exit 4 comes from an explicit
-xtest in the dispatched command, deliberately not inferred from bash's 127 — a missingdockerinside the script also exits 127, and that is a real failure that must not be downgraded to a warning.Consequence for review: until the graph fleet cycles (or
/etc/environmentis backfilled with a one-shot SSM command), a graph refresh will reportskipped-*on every instance and change nothing. That is the intended transitional behavior, not a regression — but it means the instance-side path is unproven on a real instance until then, which is why this PR stops at phase 2 and does not retire the existing GHA path.Composite action reduced to a trigger —
refresh-graph-containersIts inlined busy-wait, pull/stop sequence, separate health-check step, and separate prune step are gone. Its
aws-account-idandhealth-check-timeoutinputs are removed, along with the now-deadgraph_health_check_timeoutplumbing inservice-refresh.yml;prod.ymlandstaging.ymlare updated in the same commit so the reusable-workflow call stays valid.Breaking Changes
None.
No application code, GraphQL schema, operations envelope, or REST shape is touched — the diff is CI/CD workflows, EC2 userdata scripts, one Lambda's tag filter, and a test. No SDK regen is implied for either client.
The removed
graph_health_check_timeoutinput is an internal reusable-workflow parameter, not a published surface; both in-repo callers are updated in the same commit.Testing
just test-all— passed (unit suite, dbt, ruff, format, basedpyright, cf-lint).just test-code— re-run after the formatter touched the new test file; passed.tests/infrastructure/test_userdata_refresh.py— new, 21 cases, passing. Drives the real script as a bash process against stubbedaws/dockerbinaries, with no test hooks added to the production script. Covers every fail-open branch of the busy-counter wait, the digest-skip matrix (including that a digest match on a stopped container still restarts), failure propagation, and the pinned tag. It also pins the exit-code contract between the script and the composite action, which have no other coupling — if those drift, a stale instance silently goes from a warning back to a red deploy.aws --queryexpressions validated against fixtures viajmespath; the SSM--parametersshorthand validated against the realawsclishorthand parser (the embedded=in the command string parses correctly).Not verified on a real instance. The script has never run on an EC2 host — that requires a staging deploy plus one hand-run SSM invocation, which is the spec's own phase-2 acceptance criterion and the next step after this merges.