From 64d87b2ec21c2bc763e6c6c74ceb1f25a18d8591 Mon Sep 17 00:00:00 2001 From: Muhammad Atif Ali Date: Thu, 24 Sep 2026 18:14:54 +0500 Subject: [PATCH 1/3] feat(agent-relay-claude-code): drain the runner on workspace stop 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 runner was therefore killed outright: its session was never released server-side, the post-session hook never ran, and in-flight commits were lost. Add a run_on_stop script that relays the signal to the runner and waits for the supervisor to record its exit. coder-utils has no stop hook, so it is a standalone coder_script beside it. That half only works if the platform grants the time to use it, which a module cannot arrange because it owns no compute resource. Export shutdown_grace_seconds for the template to wire into docker_container.destroy_grace_seconds or a pod's termination_grace_period_seconds. Measured, not guessed: the runner prints its budget at startup, 80s bare, 110s with --push-outcome-on-release and 170s with --drain-wait-sec 60, and the local adds the 20s in-flight release and the 5s the agent spends on SSH first. Also expose drain_wait_sec and push_outcome_on_release, each omitting its flag at the neutral value so the paired env var keeps working, and client_label, defaulting to / so a runner is identifiable in the Anthropic console without the template setting a hostname. Fix two doc errors found on the way: the lifecycle section listed a runner command line missing --base-dir and --exec-path, and exit_if_unused_min's description cited dispatch_deadline, which is the Cursor pools' setting. Claude Code pools use the work order's issue time plus 300s. --- .../modules/agent-relay-claude-code/README.md | 106 ++++++++++++- .../agent-relay-claude-code/main.test.ts | 91 ++++++++++- .../modules/agent-relay-claude-code/main.tf | 94 +++++++++-- .../agent-relay-claude-code/main.tftest.hcl | 148 +++++++++++++++++- .../agent-relay-claude-code/start.sh.tftpl | 13 ++ .../agent-relay-claude-code/stop.sh.tftpl | 66 ++++++++ 6 files changed, 503 insertions(+), 15 deletions(-) create mode 100644 registry/coder/modules/agent-relay-claude-code/stop.sh.tftpl diff --git a/registry/coder/modules/agent-relay-claude-code/README.md b/registry/coder/modules/agent-relay-claude-code/README.md index 535ea2af0..58a01d28e 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,54 @@ 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: + +| `drain_wait_sec` | `push_outcome_on_release` | `shutdown_grace_seconds` | +| ---------------- | ------------------------- | ------------------------ | +| `0` (default) | `true` (default) | 135 | +| `0` | `false` | 105 | +| `60` | `true` | 195 | + +`push_outcome_on_release` is on by default: it 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. 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. Set it to `false` if that is not acceptable. + +`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. 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..66d43098c 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"); + // On by default, so it lands in this argv too. + expect(args).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,78 @@ 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 drain wait to the runner when set", async () => { + const { id, scripts } = await setup({ drain_wait_sec: "30" }); + 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"); + }); }); diff --git a/registry/coder/modules/agent-relay-claude-code/main.tf b/registry/coder/modules/agent-relay-claude-code/main.tf index 7e9f86d01..f002ee343 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" { + type = number + default = 0 + 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." + + 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" { + type = bool + default = true + 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." +} + +variable "client_label" { + type = string + default = "" + 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." +} + 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..6f497ca81 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,163 @@ 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 == 135 + error_message = "the default shutdown budget is 105s plus 30s for the outcome push" + } + + # 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=130") + 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 = "the outcome push is on by default and the drain wait is off" + } +} + +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 == 195 && strcontains(local.stop_script, "budget=190") + error_message = "the drain wait must extend both the advertised budget and the script's own" + } +} + +run "push_outcome_disabled" { + command = plan + + variables { + push_outcome_on_release = false + } + + # Leaving the flag off is what keeps SELF_HOSTED_RUNNER_PUSH_OUTCOME_ON_RELEASE + # usable: a flag the module emits always beats the paired env var. + assert { + condition = !strcontains(local.start_script, "--push-outcome-on-release") + error_message = "push_outcome_on_release false must leave the flag off for the env escape hatch" + } + + assert { + condition = output.shutdown_grace_seconds == 105 + error_message = "dropping the outcome push drops its 30s from the budget" + } +} + +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" + } } 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..488a6f385 --- /dev/null +++ b/registry/coder/modules/agent-relay-claude-code/stop.sh.tftpl @@ -0,0 +1,66 @@ +#!/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. +runner_alive() { + [ -n "$${1:-}" ] && kill -0 "$1" 2>/dev/null && + tr '\0' ' ' <"/proc/$1/cmdline" 2>/dev/null | grep -qF -- "$(basename "${cli_binary}")" +} + +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 + +SECONDS=0 +while runner_alive "$pid" && [ "$SECONDS" -lt "$budget" ]; do + sleep 0.2 +done + +if runner_alive "$pid"; then + echo "Runner did not exit within $${budget}s; leaving it to the platform." >&2 +else + echo "Runner exited after $${SECONDS}s. State: $(cat "$state_file" 2>/dev/null)" +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 From f5503383c334a78808a0531de4bb403faddb98f7 Mon Sep 17 00:00:00 2001 From: Muhammad Atif Ali Date: Fri, 25 Sep 2026 19:24:42 +0500 Subject: [PATCH 2/3] fix(agent-relay-claude-code): address review feedback Wait for the supervisor to record the exit, not just for the runner to go. The supervisor is a separate process, so /proc/ can disappear before it resumes from `wait` and writes "done ". Returning in that gap left the state file saying "working" with a dead pid, which the status script reports as orphaned -- a clean drain graded as a failure. Match the runner's argv in the pid guard rather than the bare basename. A basename alone matches any command line merely containing it, which is exactly the recycled-pid case the guard exists for. Order the new variables description -> type -> default -> validation, and replace the budget table in the README with prose, both per AGENTS.md. Document that the budget only counts what the module passes: a template using SELF_HOSTED_RUNNER_DRAIN_WAIT_MS or the push-outcome env var instead of the inputs makes the runner spend time the number does not know about. --- .../modules/agent-relay-claude-code/README.md | 15 +++++++---- .../modules/agent-relay-claude-code/main.tf | 6 ++--- .../agent-relay-claude-code/main.tftest.hcl | 19 ++++++++++++++ .../agent-relay-claude-code/stop.sh.tftpl | 26 +++++++++++++++---- 4 files changed, 53 insertions(+), 13 deletions(-) diff --git a/registry/coder/modules/agent-relay-claude-code/README.md b/registry/coder/modules/agent-relay-claude-code/README.md index 58a01d28e..456269644 100644 --- a/registry/coder/modules/agent-relay-claude-code/README.md +++ b/registry/coder/modules/agent-relay-claude-code/README.md @@ -180,11 +180,9 @@ 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: -| `drain_wait_sec` | `push_outcome_on_release` | `shutdown_grace_seconds` | -| ---------------- | ------------------------- | ------------------------ | -| `0` (default) | `true` (default) | 135 | -| `0` | `false` | 105 | -| `60` | `true` | 195 | +It comes to 135 seconds by default. Turning the outcome push off drops +its 30 seconds, and every second of `drain_wait_sec` is added on top -- +so `drain_wait_sec = 60` makes it 195. `push_outcome_on_release` is on by default: it pushes the session's outcome branch to `origin` before the branch is deleted, so commits survive an @@ -196,3 +194,10 @@ leave branches behind. Set it to `false` if that is not acceptable. `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.tf b/registry/coder/modules/agent-relay-claude-code/main.tf index f002ee343..60d2394f0 100644 --- a/registry/coder/modules/agent-relay-claude-code/main.tf +++ b/registry/coder/modules/agent-relay-claude-code/main.tf @@ -91,9 +91,9 @@ 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 - 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." validation { condition = var.drain_wait_sec >= 0 && floor(var.drain_wait_sec) == var.drain_wait_sec @@ -102,15 +102,15 @@ variable "drain_wait_sec" { } 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 = true - 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." } 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 = "" - 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." } variable "serving_log_pattern" { 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 6f497ca81..92f0d2abd 100644 --- a/registry/coder/modules/agent-relay-claude-code/main.tftest.hcl +++ b/registry/coder/modules/agent-relay-claude-code/main.tftest.hcl @@ -404,4 +404,23 @@ run "stop_script_guards_against_a_recycled_pid" { 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/stop.sh.tftpl b/registry/coder/modules/agent-relay-claude-code/stop.sh.tftpl index 488a6f385..f2835df5a 100644 --- a/registry/coder/modules/agent-relay-claude-code/stop.sh.tftpl +++ b/registry/coder/modules/agent-relay-claude-code/stop.sh.tftpl @@ -20,10 +20,13 @@ state_file="${state_file}" 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. +# 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}")" + tr '\0' ' ' <"/proc/$1/cmdline" 2>/dev/null | + grep -qF -- "$(basename "${cli_binary}") self-hosted-runner" } if [ ! -f "$state_file" ]; then @@ -49,15 +52,28 @@ echo "Draining Claude Code runner $pid (up to $${budget}s)..." # 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 runner_alive "$pid" && [ "$SECONDS" -lt "$budget" ]; do +while ! recorded && [ "$SECONDS" -lt "$budget" ]; do sleep 0.2 done -if runner_alive "$pid"; then +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 exited after $${SECONDS}s. State: $(cat "$state_file" 2>/dev/null)" + 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 From a3de94b919043ff996f0778982fa72299354d975 Mon Sep 17 00:00:00 2001 From: Muhammad Atif Ali Date: Sat, 26 Sep 2026 09:35:21 +0500 Subject: [PATCH 3/3] fix(agent-relay-claude-code): default push_outcome_on_release to false Adding the input with a true default changed behaviour for every template upgrading from 0.1.1: they would start pushing outcome branches for incomplete, idle-released and failed sessions without asking, and that path needs git auth in the workspace. AGENTS.md asks new variables to keep backward compatibility, so it is off unless a template opts in. The default shutdown budget drops to 105s accordingly; turning the push on puts its 30s back. --- .../modules/agent-relay-claude-code/README.md | 23 +++++++++------- .../agent-relay-claude-code/main.test.ts | 12 ++++++--- .../modules/agent-relay-claude-code/main.tf | 2 +- .../agent-relay-claude-code/main.tftest.hcl | 27 +++++++++---------- 4 files changed, 35 insertions(+), 29 deletions(-) diff --git a/registry/coder/modules/agent-relay-claude-code/README.md b/registry/coder/modules/agent-relay-claude-code/README.md index 456269644..4fe13edb5 100644 --- a/registry/coder/modules/agent-relay-claude-code/README.md +++ b/registry/coder/modules/agent-relay-claude-code/README.md @@ -180,16 +180,19 @@ 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 135 seconds by default. Turning the outcome push off drops -its 30 seconds, and every second of `drain_wait_sec` is added on top -- -so `drain_wait_sec = 60` makes it 195. - -`push_outcome_on_release` is on by default: it 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. 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. Set it to `false` if that is not acceptable. +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 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 66d43098c..9eb268df2 100644 --- a/registry/coder/modules/agent-relay-claude-code/main.test.ts +++ b/registry/coder/modules/agent-relay-claude-code/main.test.ts @@ -266,8 +266,8 @@ 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"); - // On by default, so it lands in this argv too. - expect(args).toContain("--push-outcome-on-release"); + // 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"); @@ -437,8 +437,11 @@ describe("agent-relay-claude-code", () => { expect(stop.stdout).toContain("already gone"); }); - it("passes the drain wait to the runner when set", async () => { - const { id, scripts } = await setup({ drain_wait_sec: "30" }); + 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+$/); @@ -447,5 +450,6 @@ describe("agent-relay-claude-code", () => { .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 60d2394f0..6138640db 100644 --- a/registry/coder/modules/agent-relay-claude-code/main.tf +++ b/registry/coder/modules/agent-relay-claude-code/main.tf @@ -104,7 +104,7 @@ variable "drain_wait_sec" { 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 = true + default = false } variable "client_label" { 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 92f0d2abd..62637b9e9 100644 --- a/registry/coder/modules/agent-relay-claude-code/main.tftest.hcl +++ b/registry/coder/modules/agent-relay-claude-code/main.tftest.hcl @@ -306,20 +306,20 @@ run "graceful_shutdown_defaults" { # 105 baseline + 30 for the outcome push, which is on by default. assert { - condition = output.shutdown_grace_seconds == 135 - error_message = "the default shutdown budget is 105s plus 30s for the outcome push" + 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=130") + 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 = "the outcome push is on by default and the drain wait is off" + 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" } } @@ -338,28 +338,27 @@ run "drain_wait_enabled" { # 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 == 195 && strcontains(local.stop_script, "budget=190") + 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_disabled" { +run "push_outcome_enabled" { command = plan variables { - push_outcome_on_release = false + push_outcome_on_release = true } - # Leaving the flag off is what keeps SELF_HOSTED_RUNNER_PUSH_OUTCOME_ON_RELEASE - # usable: a flag the module emits always beats the paired env var. assert { - condition = !strcontains(local.start_script, "--push-outcome-on-release") - error_message = "push_outcome_on_release false must leave the flag off for the env escape hatch" + 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 == 105 - error_message = "dropping the outcome push drops its 30s from the budget" + 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" } }