fix: correct sed delimiter in GH_REPO_URL extraction (#63)#64
Merged
Conversation
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
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>
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
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
Fixes #63.
buildReusableUserData()'s per-bootregisterScriptextractedGH_REPO_URLwithsedusing#as the delimiter, while theGH_TOKENandGH_LABELextraction 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, always0for this script) before sed ever sees it. That corrupts the sed script intos#^GH_REPO_URL='\(.*\)'0\1#p, which errors withunterminated 's' command.Since this is a plain
GH_REPO_URL=$(...)assignment underset -euo pipefail(no|| trueguard), the sed failure aborts the wholeregisterScriptat theconfiguringphone-home step — beforeconfig.shis ever invoked. This is a 100% reproducible failure for everyreuse: stopwarm-pool launch.Fix
Swap the delimiter to
,, which isn't a bash special-parameter suffix when preceded by$:Verified against both BSD sed (macOS) and GNU sed semantics — this mirrors the exact root cause and fix from the issue writeup.
Test plan
tests/warmpool.test.jsthat extracts the actualGH_REPO_URL=$(... sed ...)line out of the real generated user-data (not a reimplementation) and executes it inbashwith a controlled fake IMDS user-data value.#-delimiter line (sed: unescaped newline inside substitute pattern, command exits non-zero underset -e).https://github.com/foo/barcorrectly).npm test— 227/227 passing (226 pre-existing + 1 new).npm run lint— clean.npm run package— rebuiltdist/index.js; confirmed the rebuild is deterministic (byte-identical across two consecutive runs) and the diff is scoped to the single fixed line.