diff --git a/registry/coder/modules/agent-relay-claude-code/README.md b/registry/coder/modules/agent-relay-claude-code/README.md index 535ea2af0..4fe13edb5 100644 --- a/registry/coder/modules/agent-relay-claude-code/README.md +++ b/registry/coder/modules/agent-relay-claude-code/README.md @@ -16,7 +16,7 @@ on each build and runs the Claude Code self-hosted runner. ```tf module "claude_code_runner" { source = "registry.coder.com/coder/agent-relay-claude-code/coder" - version = "0.1.1" + version = "0.2.0" agent_id = coder_agent.main.id # Downloads the Claude Code CLI at start when it is not in the image. Bake @@ -36,6 +36,13 @@ resource "coder_agent" "main" { } ``` +Each runner registers with a label the Anthropic console shows beside it, +defaulting to `/` so it is identifiable whether or not the +template sets a hostname. `client_label` overrides it. The label is display +only: it never affects authorization, and it cannot steer which sessions a +runner is assigned — routing is per environment, and one pool is one +environment. + The `agent_relay_status` metadata block is required. It has to live on the `coder_agent`, which the module cannot declare; the relay reads it to decide when to reap the workspace. @@ -47,6 +54,9 @@ when to reap the workspace. image for the fastest start. `cli_binary` overrides the path. - Builds must finish inside Agent Relay's 300s spawn budget: pre-pulled images, no persistent volumes. +- The compute resource must give the workspace time to shut down. Wire + `shutdown_grace_seconds` into it; without that the runner is killed + mid-session. See [Graceful shutdown](#graceful-shutdown). ## Parameters @@ -70,9 +80,50 @@ runner. Everything lands under `$HOME/.coder-modules/coder/agent-relay-claude-co | `supervise.sh` | the detached supervisor that owns the runner | | `wrapper.sh` | session wrapper that forces `bypassPermissions` | +The stop step is a plain `coder_script` rather than a coder-utils step, so its +output goes to the agent's own script log, not to `logs/` above. + +## Environment + +The module always passes `--capacity`, `--base-dir`, `--exec-path` and +`--client-label`, passes `--exit-if-unused-min` and +`--push-outcome-on-release` unless you turn them off, passes +`--drain-wait-sec` when you set it, and sets +`SELF_HOSTED_RUNNER_ENVIRONMENT_SECRET` and +`SELF_HOSTED_RUNNER_LOCK_TO_ACCOUNT` from the parameters the relay stamps. +Anything else the CLI accepts can be set by the template, because the +supervisor inherits the agent's environment: + +```tf +resource "coder_env" "hooks_dir" { + agent_id = coder_agent.main.id + name = "SELF_HOSTED_RUNNER_HOOKS_DIR" + value = "/etc/claude-hooks" +} +``` + +Three things to know before relying on that: + +- **A flag beats its paired environment variable.** Setting + `SELF_HOSTED_RUNNER_BASE_DIR` against the module's own `--base-dir` does + nothing; use the `base_dir` input instead. The same applies to every flag + the module emits, including the optional ones once you enable them — so + `SELF_HOSTED_RUNNER_CLIENT_LABEL` has no effect and `client_label` is the + only way to change the label. +- **Duration environment variables are milliseconds**, while the CLI flags + they pair with are seconds or minutes, and the names do not always mirror: + `--exit-if-unused-min` pairs with `SELF_HOSTED_RUNNER_IDLE_SHUTDOWN_MS`. +- **Not every flag has an environment variable.** `--capacity` is one, and + it is reserved anyway: Agent Relay's one-workspace-per-session model + depends on it being 1. + +Run `claude self-hosted-runner --help` for what your CLI actually accepts; +the pairings above are the CLI's contract, not this module's, and move with +it. + ## Runner lifecycle -The start step launches `claude self-hosted-runner --capacity 1 --exit-if-unused-min 10` detached and exits, +The start step launches `claude self-hosted-runner` detached and exits, so the agent reaches `ready` immediately. The runner is wrapped so every session runs with `--permission-mode bypassPermissions`; there is no terminal attached, so an approval prompt would hang it. When the runner exits, Agent @@ -94,3 +145,62 @@ rather than reporting `working` forever. | `failed ` | runner could not start, e.g. `runner-agent-missing` when the CLI is absent | Renaming the `agent_relay_status` key breaks reaping. + +## Graceful shutdown + +The start step detaches the supervisor with `setsid`, so it lives in its own +session and never receives the SIGTERM the container's init gets on shutdown. +The module therefore registers a stop script that relays the signal to the +runner and waits for the supervisor to record `done `. It sends SIGTERM +only, never escalates, and never writes the state file. + +That half only works if the platform gives the workspace time to use it, which +the module cannot arrange: it owns no compute resource. Wire the exported +budget into the one the template owns. + +```tf +resource "docker_container" "workspace" { + # ... + destroy_grace_seconds = module.claude_code_runner.shutdown_grace_seconds +} +``` + +On Kubernetes the equivalent is the pod spec's +`termination_grace_period_seconds`. + +**Skip it and the drain never happens.** The Docker provider destroys the +container with a zero stop timeout unless `destroy_grace_seconds` is set, so +the container is killed before the stop script can finish: the session is +never released server-side, the post-session hook never runs, and in-flight +commits are lost. Kubernetes defaults to 30s, which is below the runner's own +budget. The value is a ceiling rather than a fixed wait, so a workspace whose +runner has already finished still stops immediately. + +The budget is the runner's advertised 80s to stop the Claude process and run +the post-session hook, plus 20s for a session release already in flight, plus +the 5s the agent spends shutting down SSH first: + +It comes to 105 seconds by default. Turning on the outcome push adds its 30 +seconds, and every second of `drain_wait_sec` is added on top — so both +together at `drain_wait_sec = 60` make it 195. + +`push_outcome_on_release` pushes the session's outcome branch to `origin` +before the branch is deleted, so commits survive an ephemeral workspace and a +resumed session continues from them. Without it, work an incomplete session had +already committed dies with the container. + +It is off by default, so upgrading changes nothing until you ask for it. Turn +it on deliberately: it fires on every runner-initiated incomplete end, not only +a drain — idle-release and failed sessions push too — so the workspace needs +git auth and those sessions leave branches behind. + +`drain_wait_sec` is off by default. It buys "the current turn may finish", at +a second of grace period per second of wait, which is the most expensive part +of the budget. + +The budget only counts what the module passes. If you reach for the +environment escape hatch instead -- `SELF_HOSTED_RUNNER_DRAIN_WAIT_MS`, or +`SELF_HOSTED_RUNNER_PUSH_OUTCOME_ON_RELEASE` with the input left off -- the +runner will spend time this number does not know about, and the platform can +kill it mid-drain. Use the inputs, or add the difference to the grace period +yourself. diff --git a/registry/coder/modules/agent-relay-claude-code/main.test.ts b/registry/coder/modules/agent-relay-claude-code/main.test.ts index 371ec46d9..9eb268df2 100644 --- a/registry/coder/modules/agent-relay-claude-code/main.test.ts +++ b/registry/coder/modules/agent-relay-claude-code/main.test.ts @@ -8,6 +8,7 @@ import { } from "bun:test"; import { execContainer, + findResourceInstance, readFileContainer, removeContainer, runContainer, @@ -65,7 +66,7 @@ const setup = async (vars: Record = {}) => { return { id, scripts, statusScript }; }; -type Scripts = { install: string; start: string }; +type Scripts = { install: string; start: string; stop: string }; // coder-utils owns the coder_script resources; find ours by display name. const collectScripts = (state: TerraformState): Scripts => { @@ -84,7 +85,9 @@ const collectScripts = (state: TerraformState): Scripts => { `expected install and start scripts, found ${Object.keys(byDisplayName)}`, ); } - return { install, start }; + // Ours, not coder-utils', so it is addressable by resource name. + const stop = findResourceInstance(state, "coder_script", "stop").script; + return { install, start, stop }; }; const stubBinary = async (id: string, path: string, body: string) => { @@ -123,6 +126,11 @@ const runScripts = async (id: string, scripts: Scripts, env: string[]) => { const runDispatched = (id: string, scripts: Scripts) => runScripts(id, scripts, DISPATCH_ENV); +// The agent runs the stop step on workspace shutdown, with no dispatch +// env in scope: the supervisor already holds the credential. +const runStop = (id: string, scripts: Scripts) => + execContainer(id, ["bash", "-c", scripts.stop]); + const readState = async (id: string) => (await readFileContainer(id, STATE_FILE)).trim(); @@ -258,6 +266,11 @@ describe("agent-relay-claude-code", () => { // The CLI's own default is /workspace, which the agent user cannot // create; the module points it at a directory it made. expect(args[args.indexOf("--base-dir") + 1]).toBe("/root/workspace"); + // Both optional runner behaviours are off unless asked for. + expect(args).not.toContain("--push-outcome-on-release"); + // A base64 decode that silently produced nothing would otherwise look + // like a runner that simply registered with an empty label. + expect(args[args.indexOf("--client-label") + 1]).toBe("default/default"); const baseDir = await execContainer(id, ["test", "-d", "/root/workspace"]); expect(baseDir.exitCode).toBe(0); @@ -361,4 +374,82 @@ describe("agent-relay-claude-code", () => { const pwned = await execContainer(id, ["test", "-e", "/tmp/PWNED"]); expect(pwned.exitCode).not.toBe(0); }); + // The supervisor is setsid'd into its own session, so the SIGTERM the + // container's init receives never reaches the runner. Without the stop + // step the runner is killed outright and the session is never released. + it("relays SIGTERM to the detached runner and lets the supervisor record the exit", async () => { + const { id, scripts } = await setup(); + // `& wait` is load-bearing: bash defers a trap until a foreground + // command returns, so a foreground sleep would hang this for 300s. + await stubClaude(id, "trap 'sleep 2; exit 7' TERM\nsleep 300 & wait"); + await runDispatched(id, scripts); + await waitForState(id, /^working \d+$/); + + const startedAt = Date.now(); + const stop = await runStop(id, scripts); + const elapsedMs = Date.now() - startedAt; + + expect(stop.exitCode).toBe(0); + expect(stop.stdout).toContain("Draining Claude Code runner"); + // The runner's own exit code, recorded by the supervisor's wait. + expect(await waitForState(id, /^done 7$/)).toBe("done 7"); + // Proves we waited for the drain rather than firing and forgetting. + expect(elapsedMs).toBeGreaterThanOrEqual(2000); + }); + + it("no-ops when the workspace was never dispatched", async () => { + const { id, scripts } = await setup(); + await stubClaude(id, "sleep 300"); + // No credential: the start step records idle and exits. + await runScripts(id, scripts, []); + expect(await readState(id)).toBe("idle"); + + const stop = await runStop(id, scripts); + expect(stop.exitCode).toBe(0); + expect(stop.stdout).toContain("Nothing to drain"); + expect(await readState(id)).toBe("idle"); + }); + + // Three states with no live runner behind them. They share a container: + // each only writes the state file and runs the stop step, and a stale + // pid is safe to signal only because the container has its own PID + // namespace. + it("no-ops when there is no runner to drain", async () => { + const { id, scripts } = await setup(); + + let stop = await runStop(id, scripts); + expect(stop.exitCode).toBe(0); + expect(stop.stdout).toContain("No runner state"); + + await execContainer(id, ["mkdir", "-p", MODULE_DIR]); + await writeFileContainer(id, STATE_FILE, "done 3\n", { user: "root" }); + stop = await runStop(id, scripts); + expect(stop.exitCode).toBe(0); + expect(stop.stdout).toContain("Nothing to drain"); + // A terminal state the reaper already grades must survive untouched. + expect(await readState(id)).toBe("done 3"); + + await writeFileContainer(id, STATE_FILE, "working 999999\n", { + user: "root", + }); + stop = await runStop(id, scripts); + expect(stop.exitCode).toBe(0); + expect(stop.stdout).toContain("already gone"); + }); + + it("passes the optional runner flags when set", async () => { + const { id, scripts } = await setup({ + drain_wait_sec: "30", + push_outcome_on_release: "true", + }); + await stubClaude(id, 'printf "%s\\n" "$@" >/tmp/claude-args\nsleep 300'); + await runDispatched(id, scripts); + await waitForState(id, /^working \d+$/); + + const args = (await readFileContainer(id, "/tmp/claude-args")) + .trim() + .split("\n"); + expect(args[args.indexOf("--drain-wait-sec") + 1]).toBe("30"); + expect(args).toContain("--push-outcome-on-release"); + }); }); diff --git a/registry/coder/modules/agent-relay-claude-code/main.tf b/registry/coder/modules/agent-relay-claude-code/main.tf index 7e9f86d01..6138640db 100644 --- a/registry/coder/modules/agent-relay-claude-code/main.tf +++ b/registry/coder/modules/agent-relay-claude-code/main.tf @@ -82,7 +82,7 @@ variable "base_dir" { variable "exit_if_unused_min" { type = number default = 10 - description = "Minutes the runner waits for a session before exiting on its own (the CLI's --exit-if-unused-min). A dispatched workspace that never receives its session would otherwise report working forever and never be reaped. 0 disables the bound. Keep it above Agent Relay's dispatch deadline so a slow claim is not cut short." + description = "Minutes the runner waits for a session before exiting on its own (the CLI's --exit-if-unused-min). A dispatched workspace that never receives its session would otherwise report working forever and never be reaped. 0 disables the bound. Keep it above Agent Relay's spawn budget, which for Claude Code pools is the work order's issue time plus 300s (dispatch_deadline is the Cursor pools' setting and does not apply here)." validation { condition = var.exit_if_unused_min >= 0 && floor(var.exit_if_unused_min) == var.exit_if_unused_min @@ -90,12 +90,39 @@ variable "exit_if_unused_min" { } } +variable "drain_wait_sec" { + description = "Seconds the runner waits for the session's in-flight turn and background tasks to finish before stopping the session process, once it is asked to shut down (the CLI's --drain-wait-sec). 0 leaves the flag off, so a template can set SELF_HOSTED_RUNNER_DRAIN_WAIT_MS itself; note that one is milliseconds. Every second here is added to shutdown_grace_seconds, which the template must honor." + type = number + default = 0 + + validation { + condition = var.drain_wait_sec >= 0 && floor(var.drain_wait_sec) == var.drain_wait_sec + error_message = "drain_wait_sec must be a whole number of seconds, 0 to disable." + } +} + +variable "push_outcome_on_release" { + description = "Push the session's outcome branch to origin before deleting it when the runner ends a session it could not complete (the CLI's --push-outcome-on-release), so commits survive an ephemeral workspace being torn down and a resumed session continues from them. Fires on every runner-initiated incomplete end, which includes idle-release and failed sessions, so it needs git auth in the workspace and it creates branches for those too. Adds 30s to shutdown_grace_seconds. false leaves the flag off, so a template can set SELF_HOSTED_RUNNER_PUSH_OUTCOME_ON_RELEASE itself." + type = bool + default = false +} + +variable "client_label" { + description = "Label the runner registers with, shown beside it in the Anthropic console (the CLI's --client-label). Empty uses /, so a runner is identifiable without the template having to set a hostname. The label is display only: it is never used for authorization or routing, and it cannot steer which sessions this runner is assigned." + type = string + default = "" +} + variable "serving_log_pattern" { type = string default = "Picked up session" description = "Runner log substring that means a session was claimed. Only distinguishes the agent_relay_status values working and serving; a stale pattern degrades to working and affects nothing else." } +data "coder_workspace" "me" {} + +data "coder_workspace_owner" "me" {} + data "coder_parameter" "agent_relay_session_id" { name = "agent_relay_session_id" display_name = "Agent Relay session" @@ -204,18 +231,48 @@ locals { # tree so one directory holds everything a debugger needs. module_directory = "$HOME/.coder-modules/coder/agent-relay-claude-code" + # coder-utils prefixes its own steps with this; the stop step is a + # plain coder_script, so it joins the same naming by hand. + display_name_prefix = "Claude Code runner" + icon = "/emojis/1f916.png" + + client_label = var.client_label != "" ? var.client_label : "${data.coder_workspace_owner.me.name}/${data.coder_workspace.me.name}" + install_script = templatefile("${path.module}/install.sh.tftpl", { cli_binary = var.cli_binary install_cli = var.install_cli }) start_script = templatefile("${path.module}/start.sh.tftpl", { - cli_binary = var.cli_binary - install_cli = var.install_cli - state_file = var.state_file - log_file = var.log_file - base_dir = var.base_dir - exit_if_unused_min = var.exit_if_unused_min + cli_binary = var.cli_binary + state_file = var.state_file + log_file = var.log_file + base_dir = var.base_dir + exit_if_unused_min = var.exit_if_unused_min + drain_wait_sec = var.drain_wait_sec + push_outcome_on_release = var.push_outcome_on_release + # Free-form text: base64 so a label with quotes or spaces can never + # become shell in the supervisor script. + client_label = base64encode(local.client_label) + }) + + # What the runner itself needs once it is signalled: 80s to stop the + # Claude process and run the post-session hook, plus up to 20s for a + # session release already in flight. The two optional behaviors extend + # it by exactly what their flags document. This is what the stop script + # waits for, because it starts counting when it runs. + runner_budget = 100 + var.drain_wait_sec + (var.push_outcome_on_release ? 30 : 0) + + # What the platform must grant, counted from the signal: the runner's + # budget plus the 5s the agent spends shutting down SSH before it runs + # stop scripts at all. Larger than runner_budget by exactly that gap, + # so the script's own wait always ends first. + shutdown_grace_seconds = local.runner_budget + 5 + + stop_script = templatefile("${path.module}/stop.sh.tftpl", { + cli_binary = var.cli_binary + state_file = var.state_file + drain_timeout_s = local.runner_budget }) } @@ -225,12 +282,26 @@ module "coder_utils" { agent_id = var.agent_id module_directory = local.module_directory - display_name_prefix = "Claude Code runner" - icon = "/emojis/1f916.png" + display_name_prefix = local.display_name_prefix + icon = local.icon install_script = local.install_script start_script = local.start_script } +# coder-utils runs install and start steps only, so the stop step is a +# plain coder_script beside it. It is deliberately outside the +# `coder exp sync` ordering the module's other scripts take part in: +# nothing runs after it. +resource "coder_script" "stop" { + agent_id = var.agent_id + display_name = "${local.display_name_prefix}: Stop Script" + icon = local.icon + run_on_start = false + run_on_stop = true + start_blocks_login = false + script = local.stop_script +} + # The coder provider has no standalone agent-metadata resource: the # metadata block belongs to coder_agent, which the template owns. The # template must add the block below; this output renders its script so @@ -249,6 +320,11 @@ output "scripts" { value = module.coder_utils.scripts } +output "shutdown_grace_seconds" { + description = "Seconds the platform must give the workspace to shut down for the runner to drain instead of being killed. Wire it into the compute resource the template owns: docker_container.destroy_grace_seconds, or a pod spec's termination_grace_period_seconds. A module owns no compute resource and cannot set this itself. The value is a ceiling, not a fixed wait: a workspace whose runner has already finished stops immediately." + value = local.shutdown_grace_seconds +} + output "dispatched" { description = "Whether this workspace was spawned by Agent Relay (credential set) or manually (empty)." value = data.coder_parameter.agent_relay_credential.value != "" diff --git a/registry/coder/modules/agent-relay-claude-code/main.tftest.hcl b/registry/coder/modules/agent-relay-claude-code/main.tftest.hcl index a80e84c9d..62637b9e9 100644 --- a/registry/coder/modules/agent-relay-claude-code/main.tftest.hcl +++ b/registry/coder/modules/agent-relay-claude-code/main.tftest.hcl @@ -245,19 +245,181 @@ run "overridden_paths" { condition = strcontains(output.status_metadata_script, base64encode("custom pattern")) && !strcontains(output.status_metadata_script, "custom pattern") && can(regex("/var/log/relay.log", output.status_metadata_script)) error_message = "the status script must use the configured log file and carry the pattern base64-encoded only" } + + # The stop script reads the pid out of the same file the supervisor + # writes it to, so an override must reach both. + assert { + condition = strcontains(local.stop_script, "/var/run/relay/state") + error_message = "the stop script must read the configured state file" + } } -run "serving_log_pattern_is_data" { +run "untrusted_text_is_data" { command = plan variables { serving_log_pattern = "x\"; touch /tmp/PWNED; \"" + client_label = "y\"; touch /tmp/PWNED; \"" } - # Free-form text never lands in the script as shell; it is decoded into + # Free-form text never lands in a script as shell; it is decoded into # a variable and matched as a fixed string. assert { condition = !strcontains(output.status_metadata_script, "PWNED") && strcontains(output.status_metadata_script, "grep -qF -- \"$serving_log_pattern\"") error_message = "serving_log_pattern must be base64-encoded and matched with grep -F" } + + assert { + condition = !strcontains(local.start_script, "PWNED") && strcontains(local.start_script, base64encode("y\"; touch /tmp/PWNED; \"")) + error_message = "client_label must cross into the supervisor base64-encoded" + } +} + +run "graceful_shutdown_defaults" { + command = plan + + # The supervisor runs under setsid, so the agent's own SIGTERM never + # reaches the runner. The stop script is the only thing that relays it. + assert { + condition = coder_script.stop.run_on_stop == true && coder_script.stop.run_on_start == false + error_message = "the stop script must run on stop and never on start" + } + + assert { + condition = coder_script.stop.start_blocks_login == false + error_message = "the stop script must never block login" + } + + # SIGTERM starts the runner's own drain. Escalating would defeat it, + # and the platform SIGKILLs soon enough on its own. + assert { + condition = strcontains(local.stop_script, "kill -TERM") && !strcontains(local.stop_script, "kill -9") && !strcontains(local.stop_script, "-KILL") + error_message = "the stop script must send SIGTERM only and never escalate" + } + + # supervise.sh is the sole writer of terminal state; a second writer + # would race the "done " line the reaper grades. + assert { + condition = !strcontains(local.stop_script, "state_file.tmp") + error_message = "the stop script must not write the state file" + } + + # 105 baseline + 30 for the outcome push, which is on by default. + assert { + condition = output.shutdown_grace_seconds == 105 + error_message = "the default shutdown budget is the runner's 100s plus the agent's own shutdown" + } + + # One number: the script's own wait must never outlive the grace the + # template was asked to grant. + assert { + condition = strcontains(local.stop_script, "budget=100") + error_message = "the stop script must wait the runner budget, which is shutdown_grace_seconds minus the agent's own shutdown" + } + + assert { + condition = !strcontains(local.start_script, "--push-outcome-on-release") && !strcontains(local.start_script, "--drain-wait-sec") + error_message = "both optional runner behaviours are off by default" + } +} + +run "drain_wait_enabled" { + command = plan + + variables { + drain_wait_sec = 60 + } + + assert { + condition = strcontains(local.start_script, "--drain-wait-sec 60") + error_message = "drain_wait_sec must reach the runner as a flag" + } + + # Every second the runner may spend draining is a second the platform + # must grant on top of the baseline. + assert { + condition = output.shutdown_grace_seconds == 165 && strcontains(local.stop_script, "budget=160") + error_message = "the drain wait must extend both the advertised budget and the script's own" + } +} + +run "push_outcome_enabled" { + command = plan + + variables { + push_outcome_on_release = true + } + + assert { + condition = strcontains(local.start_script, "--push-outcome-on-release") + error_message = "push_outcome_on_release must reach the runner as a flag" + } + + # Pushing the outcome branch is 30s of extra shutdown work. + assert { + condition = output.shutdown_grace_seconds == 135 && strcontains(local.stop_script, "budget=130") + error_message = "the outcome push adds its 30s to both the budget and the script's wait" + } +} + +run "drain_wait_rejects_fraction" { + command = plan + + variables { + drain_wait_sec = 2.5 + } + + expect_failures = [var.drain_wait_sec] +} + +run "client_label_defaults_to_owner_and_workspace" { + command = plan + + # Display only: the console shows it beside the runner, and it never + # steers which sessions this runner is assigned. + assert { + condition = strcontains(local.start_script, "--client-label \"\\$client_label\"") + error_message = "the runner must register with a client label" + } + + assert { + condition = strcontains(local.start_script, base64encode("${data.coder_workspace_owner.me.name}/${data.coder_workspace.me.name}")) + error_message = "the default label identifies the workspace without the template setting a hostname" + } +} + +run "stop_script_guards_against_a_recycled_pid" { + command = plan + + # This is the one caller that sends a signal, so a pid the supervisor + # recorded and the OS has since handed to something else must not be + # SIGTERMed. + assert { + condition = strcontains(local.stop_script, "runner_alive") && strcontains(local.stop_script, "cmdline") + error_message = "the stop script must confirm the pid is still our runner before signalling it" + } + + assert { + condition = !strcontains(local.stop_script, "kill -0 \"$pid\"") + error_message = "a bare kill -0 would trust a recycled pid" + } + + # A bare basename matches any command line containing it, which is the + # case the guard exists for. + assert { + condition = strcontains(local.stop_script, "self-hosted-runner\"") + error_message = "the pid guard must match the runner's argv, not just the binary name" + } +} + +run "stop_script_waits_for_the_recorded_exit" { + command = plan + + # The supervisor is a separate process, so the runner's pid can vanish + # before "done " is written. Returning in that gap leaves the + # state saying working with a dead pid, which reads as orphaned. + assert { + condition = strcontains(local.stop_script, "recorded") && strcontains(local.stop_script, "while ! recorded") + error_message = "the stop script must wait for the supervisor to record the exit, not just for the runner to go" + } } diff --git a/registry/coder/modules/agent-relay-claude-code/start.sh.tftpl b/registry/coder/modules/agent-relay-claude-code/start.sh.tftpl index 27fa61015..13dda0558 100644 --- a/registry/coder/modules/agent-relay-claude-code/start.sh.tftpl +++ b/registry/coder/modules/agent-relay-claude-code/start.sh.tftpl @@ -76,11 +76,17 @@ chmod +x "$wrapper" # The supervisor outlives this script: it owns the runner process and is # the only writer of terminal state. Written to disk rather than inlined # so setsid gets a clean argv. +# +# The client label is free-form text a human wrote, so it crosses into the +# supervisor base64-encoded and is decoded into a variable there rather +# than rendered as a command word. supervisor="$state_dir/supervise.sh" cat >"$supervisor" <"$state_file.tmp" mv "$state_file.tmp" "$state_file" @@ -93,6 +99,13 @@ ${cli_binary} self-hosted-runner \\ %{ if exit_if_unused_min > 0 ~} --exit-if-unused-min ${exit_if_unused_min} \\ %{ endif ~} +%{ if drain_wait_sec > 0 ~} + --drain-wait-sec ${drain_wait_sec} \\ +%{ endif ~} +%{ if push_outcome_on_release ~} + --push-outcome-on-release \\ +%{ endif ~} + --client-label "\$client_label" \\ --exec-path "$wrapper" >"$log_file" 2>&1 & runner_pid=\$! write_state "working \$runner_pid" diff --git a/registry/coder/modules/agent-relay-claude-code/stop.sh.tftpl b/registry/coder/modules/agent-relay-claude-code/stop.sh.tftpl new file mode 100644 index 000000000..f2835df5a --- /dev/null +++ b/registry/coder/modules/agent-relay-claude-code/stop.sh.tftpl @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# Stop step, run by the agent when the workspace shuts down. +# +# The start step launches the supervisor with setsid, so it lives in its +# own session and never receives the SIGTERM the container's init gets. +# Without this script the runner is killed outright: the session is never +# released server-side, the post-session hook never runs, and in-flight +# commits are lost. So relay the signal to the runner explicitly and wait +# for the supervisor to record the exit. +# +# This script is only half the fix. The agent runs it with whatever time +# the platform grants, which on Docker is nothing at all unless the +# template sets destroy_grace_seconds. See the module README. +set -euo pipefail + +state_file="${state_file}" +# The runner's own shutdown budget. The platform is asked for this plus +# the time the agent spends before running stop scripts, so this wait +# always ends before the platform's grace period does. +budget=${drain_timeout_s} + +# True when the pid is alive and is still our runner. kill -0 alone would +# trust a recycled pid, and this is the caller that sends the signal, so +# match the argv the supervisor launches rather than the bare basename: +# that would accept any command line merely containing it. +runner_alive() { + [ -n "$${1:-}" ] && kill -0 "$1" 2>/dev/null && + tr '\0' ' ' <"/proc/$1/cmdline" 2>/dev/null | + grep -qF -- "$(basename "${cli_binary}") self-hosted-runner" +} + +if [ ! -f "$state_file" ]; then + echo "No runner state at $state_file; nothing to drain." + exit 0 +fi + +# "working " is the only state with a live runner behind it. Every +# other value is terminal, or a workspace the relay never dispatched. +read -r state pid <"$state_file" || true +if [ "$${state:-}" != "working" ]; then + echo "Runner is not running (state: $${state:-empty}). Nothing to drain." + exit 0 +fi + +if ! runner_alive "$${pid:-}"; then + echo "Runner $${pid:-unknown} is already gone." + exit 0 +fi + +echo "Draining Claude Code runner $pid (up to $${budget}s)..." +# SIGTERM only. The runner's own drain starts here; escalating would +# defeat the point, and the platform SIGKILLs us soon enough anyway. +kill -TERM "$pid" 2>/dev/null || true + +# Wait for the supervisor to record the exit, not just for the runner to +# go. The supervisor is a separate process: /proc/ can disappear +# before it resumes from `wait` and writes "done ". Returning in +# that gap leaves the state file saying "working" with a dead pid, which +# the status script reports as orphaned -- a clean drain graded as a +# failure. +recorded() { + read -r s _ <"$state_file" 2>/dev/null || return 1 + [ "$${s:-working}" != working ] +} + +SECONDS=0 +while ! recorded && [ "$SECONDS" -lt "$budget" ]; do + sleep 0.2 +done + +if recorded; then + echo "Runner exited after $${SECONDS}s. State: $(cat "$state_file" 2>/dev/null)" +elif runner_alive "$pid"; then + echo "Runner did not exit within $${budget}s; leaving it to the platform." >&2 +else + echo "Runner is gone but the supervisor never recorded it within $${budget}s." >&2 +fi + +# Always succeed. The supervisor is the only writer of terminal state, and +# a nonzero stop script only buys a shutdown_error lifecycle on a workspace +# that is about to disappear. +exit 0