fix(runners): guard SEGMENT against set -u in start-runner.sh - #5233
fix(runners): guard SEGMENT against set -u in start-runner.sh#5233jensenbox wants to merge 1 commit into
Conversation
`start-runner.sh` assigns `SEGMENT` only inside the X-Ray branch (line 191) but
consumes it unconditionally at line 263 and in the `error_handler` trap (line
98). With `enable_ssm_on_runners`-style tracing disabled — the default — the
branch never runs and `SEGMENT` is never assigned.
That is harmless while user-data runs without `set -u`. It becomes fatal as soon
as anything enables `-u` earlier in the same shell, and user-data is one
concatenated script: `${post_install}` is spliced inline at
`modules/runners/templates/user-data.sh:63`. A consumer whose `post_install`
hook opens with `set -euo pipefail` therefore aborts user-data here — after the
runner has registered with GitHub, but before its listener starts.
The failure mode is unusually quiet: the runner appears healthy in the GitHub
UI (it registered and holds an agent ID), but never claims a job, so scale-down
reaps it as idle and the scaler launches an identically-broken replacement. We
lost every job on one runner pool for ~19h before tracing it to:
√ Runner successfully added
Tagging instance with GitHub runner agent ID: 49534
/var/lib/cloud/instance/scripts/part-001: line 703: SEGMENT: unbound variable
ERROR: runner-start-failed with exit code 1 occurred on 1
FAILED Failed to start cloud-final.service - Cloud-init: Final Stage.
Use `"$${SEGMENT:-}"` at both unguarded call sites. This restores the script's
existing intent rather than changing behaviour — both helpers already open with
local SEGMENT_DOC="$1"
if [ -z "$SEGMENT_DOC" ]; then
echo "No segment doc provided"
return
fi
so they were written to tolerate an absent segment; the call sites simply never
expressed it in a `-u`-safe way. Line 192's `echo "$SEGMENT"` is deliberately
left alone: it sits inside the branch that assigns the variable.
Note the `$$` — this template is itself rendered through `templatefile()`, so a
bare `${SEGMENT:-}` is parsed as a Terraform interpolation and fails the plan
with "Template interpolation doesn't expect a colon at this location". Existing
precedent in the same file: `$${extra_flags}` and `$${config}`.
Verified by rendering the template with `templatefile()` and running `bash -n`
over the output.
|
Still applies cleanly to current Adding real-world impact in case it helps prioritise, since this looks like a trivial two-character change on the surface. When On our fleet that cost roughly 19 hours of deploy outage before we found it — jobs sat queued against runners that looked healthy in the GitHub UI. We are carrying this patch in a fork to stay unblocked, but it belongs upstream: anyone hitting it has no realistic path to diagnosing it from the logs they would normally check. Happy to rebase or adjust the form ( |
Problem
modules/runners/templates/start-runner.shassignsSEGMENTonly inside the X-Ray branch (line 191) but consumes it unconditionally at line 263 and in theerror_handlertrap (line 98). With tracing disabled — the default — the branch never executes andSEGMENTis never assigned.That is harmless while user-data runs without
set -u. It becomes fatal the moment anything enables-uearlier in the same shell, and user-data is one concatenated script:${post_install}is spliced inline atmodules/runners/templates/user-data.sh:63. So a consumer whosepost_installhook opens withset -euo pipefail— a perfectly ordinary thing for a hook to do — silently arms this latent bug for the module's own script.Why it is worth fixing rather than documenting
The failure mode is unusually quiet. User-data dies after the runner registers with GitHub but before its listener starts, so:
From the outside it looks like a capacity or scaling problem, not a script error. We lost every job on one runner pool for ~19 hours before finding it in the instance console:
Fix
"$${SEGMENT:-}"at both unguarded call sites.This restores the script's existing intent rather than changing behaviour. Both helpers already begin with:
so they were written to tolerate an absent segment doc — the call sites simply never expressed it in a
-u-safe way. With tracing enabled nothing changes; with tracing disabled the helpers now hit the early return they already had.Line 192's
echo "$SEGMENT"is deliberately untouched: it sits inside the branch that assigns the variable.Note the doubled
$— this template is itself rendered throughtemplatefile(), so a bare${SEGMENT:-}is parsed as a Terraform interpolation and fails the plan with "Extra characters after interpolation expression; Template interpolation doesn't expect a colon at this location". Existing precedent in the same file:$${extra_flags}and$${config}(lines 257, 269).Verification
templatefile()and confirmed the output contains"${SEGMENT:-}"at both sites.bash -nover the rendered script.SEGMENT: unbound variablenow reach a running listener, claim jobs, and complete them.Scope
Two characters at two call sites. No behaviour change for tracing-enabled consumers.