Skip to content

fix: correct sed delimiter in GH_REPO_URL extraction (#63)#64

Merged
kurok merged 1 commit into
mainfrom
fix/issue-63-gh-repo-url-sed-delimiter
Jul 6, 2026
Merged

fix: correct sed delimiter in GH_REPO_URL extraction (#63)#64
kurok merged 1 commit into
mainfrom
fix/issue-63-gh-repo-url-sed-delimiter

Conversation

@kurok

@kurok kurok commented Jul 6, 2026

Copy link
Copy Markdown

Summary

Fixes #63.

buildReusableUserData()'s per-boot registerScript extracted GH_REPO_URL with sed using # as the delimiter, while the GH_TOKEN and GH_LABEL extraction lines directly above it correctly use /. Because the sed pattern ends in the end-of-line anchor $ immediately followed by the # delimiter, and the whole sed argument sits inside a double-quoted shell string, bash expands $# (the special "number of positional arguments" parameter, always 0 for this script) before sed ever sees it. That corrupts the sed script into s#^GH_REPO_URL='\(.*\)'0\1#p, which errors with unterminated 's' command.

Since this is a plain GH_REPO_URL=$(...) assignment under set -euo pipefail (no || true guard), the sed failure aborts the whole registerScript at the configuring phone-home step — before config.sh is ever invoked. This is a 100% reproducible failure for every reuse: stop warm-pool launch.

Fix

Swap the delimiter to ,, which isn't a bash special-parameter suffix when preceded by $:

-'GH_REPO_URL=$(printf \'%s\\n\' "$UD" | sed -n "s#^GH_REPO_URL=\'\\(.*\\)\'$#\\1#p" | head -n1)',
+'GH_REPO_URL=$(printf \'%s\\n\' "$UD" | sed -n "s,^GH_REPO_URL=\'\\(.*\\)\'$,\\1,p" | head -n1)',

Verified against both BSD sed (macOS) and GNU sed semantics — this mirrors the exact root cause and fix from the issue writeup.

Test plan

  • Added a regression test in tests/warmpool.test.js that extracts the actual GH_REPO_URL=$(... sed ...) line out of the real generated user-data (not a reimplementation) and executes it in bash with a controlled fake IMDS user-data value.
  • Verified the new test fails against the old #-delimiter line (sed: unescaped newline inside substitute pattern, command exits non-zero under set -e).
  • Verified the new test passes against the fix (extracts https://github.com/foo/bar correctly).
  • Full suite: npm test — 227/227 passing (226 pre-existing + 1 new).
  • npm run lint — clean.
  • npm run package — rebuilt dist/index.js; confirmed the rebuild is deterministic (byte-identical across two consecutive runs) and the diff is scoped to the single fixed line.

buildReusableUserData()'s per-boot registerScript extracted GH_REPO_URL
with sed using "#" as the delimiter, while GH_TOKEN and GH_LABEL used
"/". Because the pattern ends in the sed end-of-line anchor "$"
immediately followed by "#", and the whole sed argument sits inside a
double-quoted shell string, bash expands "$#" (number of positional
arguments, always 0 for this script) before sed ever runs. That
corrupts the sed script and makes it error out with "unterminated 's'
command". Since the assignment isn't guarded with `|| true` and the
script runs under `set -euo pipefail`, the failure aborted the whole
registerScript at the "configuring" phone-home step, so every
`reuse: stop` warm-pool launch failed registration before config.sh
was ever invoked.

Switch the delimiter to "," which isn't a bash special-parameter
suffix, matching the fix verified in the issue against both BSD and
GNU sed.

Adds a regression test that extracts the real GH_REPO_URL sed line out
of the generated user-data and executes it in bash, so it fails
against the old "#" delimiter and passes with the fix.

Fixes #63

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
@kurok kurok merged commit 301e1c1 into main Jul 6, 2026
5 checks passed
@kurok kurok deleted the fix/issue-63-gh-repo-url-sed-delimiter branch July 6, 2026 15:07
kurok added a commit to namecheap/terraform-provider-namecheap that referenced this pull request Jul 6, 2026
The previous re-pin (aa10310) got us the #62 phone-home fix, which then
revealed the real blocker: buildReusableUserData()'s GH_REPO_URL
extraction used '#' as its sed delimiter, but '$#' inside the
double-quoted sed script is bash's positional-argument-count special
parameter, which silently corrupts the pattern before sed ever sees it
(confirmed via bash -x tracing and reproduced against both BSD and GNU
sed). Every reuse:stop registration failed at the configuring step as a
result. Fixed upstream in namecheap/ec2-github-runner#64 (fixes #63),
merged as 301e1c1, v4.0.0 moved to match. Re-pinning this PR's 4 usages
to that commit.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
dm1tr1yvovk pushed a commit to namecheap/terraform-provider-namecheap that referenced this pull request Jul 8, 2026
…job hygiene sweeps (#281)

* ci: warm-pool EC2 runner lifecycle with cross-repo EIP mutex and per-job hygiene sweeps

Reuses the self-hosted acceptance-test EC2 instance across pushes (stop/start warm pool) instead of terminating it every run, adds a nightly-drain/leak-reaper workflow, and replaces the ec2-github-runner action's permissive EIP association with a test-and-set mutex (scripts/eip-mutex.sh) plus explicit pre/post disk hygiene sweeps (scripts/hygiene-sweep.sh) so no code, secrets, or scratch files survive on the reused disk. Closes #279.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* docs: correct cross-repo hazard note in SECURITY_COMPLIANCE.md

Remove the inaccurate 'a companion issue is being filed' claim and
TBD-companion-issue placeholder — filing that issue against a
different repository was out of scope for this change and was not
done; the hazard and the remediation it needs are still documented,
just without a false pointer.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: keep sandbox EIP attached across warm-pool cycles, restore boot-time connectivity

A live acceptance run failed to reach the internet during instance
bootstrap. CloudTrail showed the launched instance never had a public
IP assigned, and the VPC has no NAT gateway, so the atomic
associate-address test-and-set added by the prior EIP-mutex commit
(deferred until after "Start EC2 runner" returned) left the instance
with no route out during its own runner registration.

Restoring eip-allocation-id on the action's "Start EC2 runner" step
fixes cold launches, but surfaced a second, architectural gap: the
action's warm-restart path (reuse: stop) never re-associates the EIP
itself, so a warm-started instance needs the EIP already attached at
boot or it hits the exact same connectivity failure on restart.
Keeping the EIP permanently attached only per-stop and re-acquiring
per-start does not fit that path.

The confirmed design instead treats the EIP as attached to the
warm-pool instance for the whole day: start-runner now runs
`wait-until-free` as a precondition gate before the cold-launch
association (so it doesn't race a still-live holder), and `verify`
as a hard-fail post-check that the instance actually ended up owning
it, on both cold and warm paths. There is no explicit release step;
the EIP is only freed as a side effect of cleanup-ec2-runners.yml
terminating the pool instance (normally the nightly full drain, or
occasionally the leak-reaper pass if the instance ages out between
drains). This weakens cross-repo EIP fairness during the day in
exchange for warm-restart connectivity -- SECURITY_COMPLIANCE.md's
cross-repo hazard note is updated to disclose that wait/verify can
only detect a steal after the fact, not prevent one, pending
namecheap/mcp-server-namecheap adopting the same discipline.

Also fixes a timeout bypass in wait-until-free's reap branch (the
stopped/foreign-holder path looped via `continue` without ever
checking EIP_MUTEX_TIMEOUT_SECONDS), guards that same reap call
against a set -e abort if the foreign owner reclaims the instance
mid-race, suppresses cmd_reap_if_stopped's status stdout when it's
called in-process so wait-until-free's exit-code-only contract holds,
and corrects header comments that overstated the retry loops as
"backoff" (they are flat, fixed-interval retries) and
SECURITY_COMPLIANCE.md's claim that EIP release "only" happens via
the nightly drain.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: re-pin ec2-github-runner to the fixed v4.0.0 commit

The v4.0.0 tag was still pointing at c5db9f0 (the pre-#62 commit) when
this PR pinned it, so today's acceptance_test run hit the exact silent
5-minute registration timeout #62 already fixed upstream: cloud-init's
own console output completes normally at ~41s uptime and never resumes,
because the RUNNER_DOWNLOAD warm-pool path lacks phone-home diagnostics
on that commit.

v4.0.0 has since been moved to 032802a9 (the #62 merge commit, current
tip of main, green on lint/unit/verify-dist/pinned-runner-checksum).
Re-pinning this PR's 4 usages to that commit so CI actually exercises
the fix.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: temporarily enable ec2-github-runner debug logging

Re-pinning to the phone-home fix in the previous commit didn't change
the observed symptom: still a silent 5-minute timeout with the EC2
console output going dark at ~41s uptime. wait_for_runner_poll's
bootstrap-tag debug line (src/wait.js) only emits when the action's own
debug input is true, so flip it on for this diagnostic run to see
whether the phone-home self-tag is advancing (and where it's stuck) or
never updating at all (which would point at a permissions gap on the
instance's own ec2:CreateTags rather than a registration hang).

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: re-pin ec2-github-runner to include the #64 sed-delimiter fix

The previous re-pin (aa10310) got us the #62 phone-home fix, which then
revealed the real blocker: buildReusableUserData()'s GH_REPO_URL
extraction used '#' as its sed delimiter, but '$#' inside the
double-quoted sed script is bash's positional-argument-count special
parameter, which silently corrupts the pattern before sed ever sees it
(confirmed via bash -x tracing and reproduced against both BSD and GNU
sed). Every reuse:stop registration failed at the configuring step as a
result. Fixed upstream in namecheap/ec2-github-runner#64 (fixes #63),
merged as 301e1c1, v4.0.0 moved to match. Re-pinning this PR's 4 usages
to that commit.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: pre-create /opt/ci with runner ownership before the runner starts

First live run past the EC2 registration fixes hit a new failure:
scripts/hygiene-sweep.sh's 'mkdir -p /opt/ci/jobs/current' fails with
Permission denied. /opt is root-owned (mode 755) on the AMI, and every
step in the acceptance_test job (including this one) runs as the
unprivileged 'runner' user that config.sh/run.sh register and run under
-- it has no write access to create a new directory directly under /opt.

Use the action's pre-runner-script input (runs as root, once, on the
cold-launch bootstrap only) to create /opt/ci and chown it to runner
before the runner ever starts. /opt/ci then persists with the right
ownership across every subsequent warm stop/start cycle on the same
root volume, since reuse: stop never re-runs cloud-init/user-data on
restart.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: trigger warm-restart verification run

Empty commit to force a push event and confirm the warm-pool instance
stopped after the last successful run (i-04aa4d095d2c664df, cycles=1)
gets warm-started rather than cold-launched.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: re-pin ec2-github-runner to include the #65 error-detail diagnostics

The warm-restart path (unlike the already-verified cold-launch path)
still fails at the configuring step with zero detail beyond the step
name. Since buildReuseUserData() regenerates the identical bootstrap
script buildUserData() uses (already confirmed sed-fix-correct), the
warm-restart-specific failure cause is otherwise invisible without live
instance access. namecheap/ec2-github-runner#65 adds a captured stderr
snippet to the failed:<step> phone-home tag (now readable via this
repo's existing debug:true diagnostic flag), merged as 453ef2e, v4.0.0
moved to match. Re-pinning this PR's 4 usages so the next warm-restart
run surfaces the real config.sh error instead of another opaque
'bootstrap failed during configuring'.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: trigger warm-restart verification run (with error-detail diagnostics)

Empty commit to force a push event and confirm whether the warm-pool
instance stopped after the last successful run (i-0aa9ab735974cd1ef)
warm-starts cleanly now, or -- if it still fails at configuring --
whether #65's new error-detail snippet finally reveals the real
config.sh failure reason.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: re-pin ec2-github-runner to the #66 warm-start user-data fix

Bumps both the start-runner and stop-runner pins from 453ef2e (#65) to
65fbe4f (#66), which fixes the reuse: stop warm-restart registration
failure. warmStartInstance() was handing ModifyInstanceAttribute an
already-base64'd string for the user-data blob; the SDK base64-encodes a
blob on the wire, so it double-encoded and IMDS served the base64 TEXT of
the bootstrap script. The per-boot register step then read an empty
GH_REPO_URL and config.sh aborted with "Invalid configuration provided for
url" on every warm start (cold launches were unaffected). This also clears
the cascaded stop-runner failure, which only fired because
cleanup-on-start-failure terminated the instance the stop step then tried
to stop.

This push cold-launches (the pool is empty after the last failed run
terminated its instance); a follow-up push exercises the warm restart that
was broken.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: trigger warm-restart verification run (#66 user-data fix)

Empty commit to exercise the reuse: stop warm restart against the stopped
pool instance seeded by the previous push, now that ec2-github-runner is
pinned to 65fbe4f (#66). The only variable changed vs. the last failing
run is the action SHA, so a green start-runner here confirms the
double-base64 user-data fix.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: drop temporary debug diagnostic; warm-restart verification run

Removes the TEMPORARY `debug: "true"` added while investigating the
warm-restart registration timeout. Root cause is fixed in ec2-github-runner
#66 (now pinned): warmStartInstance double-base64-encoded the
ModifyInstanceAttribute user-data blob, so IMDS served base64 text and the
per-boot register step read an empty GH_REPO_URL. The failure-detail
phone-home tag (#65) remains on regardless of this flag, so warm-start
failures still surface their step + config.sh stderr without it.

This push also serves as the warm-restart verification: it reuses the
stopped pool instance seeded by the cold run two pushes back.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: tidy warm-pool runner comments after the #66 fix

Post-investigation cleanup of leftover narrative in the acceptance-runner
job now that the reuse: stop warm restart is fixed and verified:

- Rewrite the eip-allocation-id comment to state why the association exists
  (outbound connectivity during bootstrap on cold + warm, and the
  whitelisted sandbox IP the Namecheap API requires) instead of narrating
  that it was "removed by mistake" earlier in this PR.
- Collapse the ec2-github-runner pin provenance comment from a running list
  of intermediate fixes into a concise "post-v4.0.0 main ... (#62/#64/#65/#66)"
  note on both the start and stop pins.

No functional change: the pinned action SHA (65fbe4f) and every input are
unchanged. This push also re-exercises the full push pipeline (warm-reusing
the pool instance seeded by the previous run).

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* docs: point the cross-repo EIP hazard note at the dedicated-EIP plan

The "tracked outside this repository" line is now stale: the fix is tracked
concretely under the "Dedicated per-repo sandbox EIP" milestone
(terraform-provider-namecheap#282 + mcp-server-namecheap#16), and the chosen
resolution is a dedicated per-repo EIP that removes the contention at the
source (letting scripts/eip-mutex.sh + the wait/verify steps + the
DisassociateAddress grant be deleted), rather than mcp-server-namecheap
adopting the same wait/verify discipline. Documentation-only.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: retire the cross-repo EIP mutex; dedicate the sandbox EIP to this repo

Implements #282. eipalloc-1796f61b was shared with mcp-server-namecheap's
acceptance workflow, which could reassociate ("steal") it mid-run; this repo
defended with a cross-repo lock (scripts/eip-mutex.sh: wait-until-free
reaping + verify), an ec2:DisassociateAddress grant, and a large mutex
section in SECURITY_COMPLIANCE.md. mcp-server-namecheap's "Acceptance (EC2)"
workflow has now been disabled (pending its own dedicated EIP, mcp#16), so
this repo is the sole user of eipalloc-1796f61b and the whole mutex can go.

- Delete scripts/eip-mutex.sh.
- start-runner: drop the "Wait for sandbox EIP to become free" step and the
  now-unused actions/checkout that fed it. Replace "Verify EIP ownership"
  (eip-mutex.sh verify) with a tiny "Resolve sandbox EIP public IP" step that
  reads the allocation's public IP via describe-addresses and publishes it as
  the public-ip output. acceptance_test's credential-free "Verify sandbox EIP"
  gate is unchanged and remains the real assertion that the runner holds the
  whitelisted IP before make testacc runs.
- Keep eip-allocation-id (still associates the dedicated EIP) and the
  within-repo ahmadnassri/action-workflow-queue (one EIP still can't serve two
  concurrent runs of this repo).
- Docs: SECURITY_COMPLIANCE.md loses the "EIP mutex" and "Cross-repo hazard"
  bullets and the DisassociateAddress prerequisite, replaced by a dedicated-EIP
  description; cleanup-ec2-runners.yml comment updated. IAM now needs
  AssociateAddress + DescribeInstances + DescribeAddresses; DisassociateAddress
  is no longer required (an AWS admin can remove it from the CI role).

Part of the "Dedicated per-repo sandbox EIP" milestone (#282 + mcp#16).

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: address review — repo-var EIP, v4.0.1 pins, accurate compliance docs

Resolves the review feedback on #281:

- Source SANDBOX_EIP_ALLOCATION_ID from `vars.SANDBOX_EIP_ALLOCATION_ID`
  (repo variable now set to eipalloc-1796f61b) instead of a literal, so other
  repos can adopt this workflow with their own dedicated EIP without editing
  YAML; fix the env comment that falsely claimed cleanup-ec2-runners.yml
  mirrors the literal (it only mentions it in prose).
- Pin all four ec2-github-runner `uses:` sites (ci start+stop, cleanup x2) to
  the same SHA (65fbe4f) under a conforming `# v4.0.1 @ 2026-07-08` comment.
  cleanup-ec2-runners.yml was still on 453ef2e (v4.0.0, missing the #66
  warm-start fix) — now consistent with ci.yml and tagged v4.0.1.
- SECURITY_COMPLIANCE.md: stop claiming mcp-server-namecheap "moved to its own
  dedicated EIP" (its acceptance workflow is only disabled as an interim
  stopgap; #282 + mcp#16 still open) and complete the IAM prerequisite list
  (warm-pool StopInstances/StartInstances/ModifyInstanceAttribute + diagnostics
  GetConsoleOutput/DescribeTags), which the prior text understated.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: explicitly (re)associate the sandbox EIP on every runner start

The "Verify sandbox EIP" gate failed on a warm restart: the runner came up on
an auto-assigned IP (18.144.83.110) instead of the whitelisted EIP
(54.215.19.157). ec2-github-runner associates the EIP only on a cold launch
(via eip-allocation-id, for bootstrap connectivity); on a warm restart it never
re-associates and assumes the EIP is still attached from a prior run. A pooled
instance can lose that association while stopped, stranding the runner on a
non-whitelisted IP that the Namecheap sandbox API rejects.

Stop depending on that persistence: start-runner now runs
`aws ec2 associate-address --allow-reassociation` against the started instance
on every run — a no-op reattach after a cold launch, the actual fix after a
warm one — then resolves the public IP for the acceptance gate. Needs only
ec2:AssociateAddress + ec2:DescribeAddresses, both already required.
SECURITY_COMPLIANCE.md updated to describe the explicit re-association.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: pre-attach the sandbox EIP before warm start instead of after

c4ede4c associated the EIP *after* mode:start, which changed the public IP of
the already-registered runner and severed its GitHub connection — the
acceptance job then hung for ~19 min and was cancelled. Wrong approach.

Attach the EIP to the stopped warm-pool instance *before* the action starts it
(new "Pre-attach sandbox EIP" step, filter mirroring the action's own
findStoppedPoolInstance selection: managed+repository+pool tags, stopped,
matching instance-type). The instance then boots with the whitelisted IP and
the runner registers on it — no mid-session IP change. On a cold launch there's
no stopped instance to match, so the step is a no-op and eip-allocation-id
associates during bootstrap as before. The post-start step goes back to
resolve-only (publish the public IP for the acceptance gate; never
re-associate a running instance).

Uses only ec2:DescribeInstances + ec2:AssociateAddress (already required).
SECURITY_COMPLIANCE.md updated to describe the pre-attach.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

* ci: rename/re-comment the queue step for the warm-pool model

Non-blocking review nit: the workflow-queue step was still named "Wait for
older acceptance runs to release the EIP" and described the old
ephemeral/terminate lifecycle ("holds the IP until its stop-runner job
terminates the host"). Under warm pools the EIP isn't released per run and
stop-runner stops (not terminates) the instance. Rename to "Wait for older
acceptance runs to finish (single sandbox EIP)" and rewrite the comment to
describe the current model: FIFO serialization so only one run holds the
single sandbox EIP / warm-pool instance at a time. Comment-only; no behavior
change.

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>

---------

Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
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.

buildReusableUserData(): $# collides with sed delimiter in GH_REPO_URL extraction, breaks all reuse:stop registrations

1 participant