From 19231513a9b3b80c90999b0c06224edea6bf692b Mon Sep 17 00:00:00 2001 From: garrytan Date: Fri, 30 Oct 2026 17:06:42 +0000 Subject: [PATCH] fix: pre-landing review fixes (TTL checks only on create, leftover line via UBI_LEFT_FILE, pregate TTL from its deadline) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/TESTING_INTERNALS.md | 5 +++-- scripts/ubicloud/pregate-remote.sh | 9 +++++---- scripts/ubicloud/ubi-runner.sh | 29 ++++++++++++++++++++-------- test/ubicloud-pregate-remote.test.ts | 2 +- test/ubicloud-runner.test.ts | 14 ++++++++++---- ubicloud/SKILL.md | 1 + ubicloud/SKILL.md.tmpl | 1 + 7 files changed, 42 insertions(+), 19 deletions(-) diff --git a/docs/TESTING_INTERNALS.md b/docs/TESTING_INTERNALS.md index 2f196ca8d..5c3cd0052 100644 --- a/docs/TESTING_INTERNALS.md +++ b/docs/TESTING_INTERNALS.md @@ -990,7 +990,8 @@ recorded there before the create call and forgotten only after a confirmed destroy, so a killed runner leaves a name to clean up. `run` and `verify` end with `UBICLOUD: left=<0|1|unknown> owner= vm= others=`, checked against the project's VM list after teardown; anything but `left=0` turns a -passing command into exit 1. No owner-age sweep runs by default (the TTL reaper +passing command into exit 1. pregate-remote.sh reads that line from the file +named by `UBI_LEFT_FILE`, never from the command's output. No owner-age sweep runs by default (the TTL reaper below covers every owner); set `UBI_GC_HOURS` to a positive number (or run `gc HOURS`; `gc 0` is a no-op) to sweep only this owner's VMs older than that. For other commands, use the runner @@ -999,7 +1000,7 @@ or its `up` / `ssh` / `sync` / `pull` / `down` steps (`--help` lists them). The runner is also the `/ubicloud` skill's engine (`bin/gstack-ubi` execs it). A generated name ends in `-t` (`UBI_TTL_HOURS`, 1-999, default 3; pregate-remote.sh -uses `-t1`), the creator's own statement of how long the VM may live: `reap` +uses its stop deadline plus 15 minutes, rounded up: `-t1` today), the creator's own statement of how long the VM may live: `reap` destroys every VM in the project whose TTL ended more than ten minutes ago, whichever owner made it, and every `ubirun-*` name without a TTL (older runner copies) once it is `UBI_LEGACY_HOURS` old (default 4); every `up` runs it diff --git a/scripts/ubicloud/pregate-remote.sh b/scripts/ubicloud/pregate-remote.sh index 8e153684f..d15e1732f 100755 --- a/scripts/ubicloud/pregate-remote.sh +++ b/scripts/ubicloud/pregate-remote.sh @@ -216,11 +216,11 @@ while :; do attempt=$(( attempt + 1 )) : >"$log" : >"$stream" - rm -f "$log.pid" "$log.timedout" + rm -f "$log.pid" "$log.timedout" "$log.left" t0=$(date +%s) - vm_name="ubirun-$(bash "$here/ubi-runner.sh" owner)-$(date +%s)-$(head -c4 /dev/urandom | od -An -tx1 | tr -d ' \n')-t1" + vm_name="ubirun-$(bash "$here/ubi-runner.sh" owner)-$(date +%s)-$(head -c4 /dev/urandom | od -An -tx1 | tr -d ' \n')-t$(( (stop_after + 900 + 3599) / 3600 ))" say "VM $vm_name; if this run is killed before its teardown line, destroy it with: bash scripts/ubicloud/ubi-runner.sh down $vm_name" - python3 -c 'import os, sys; os.setsid(); os.execvp(sys.argv[1], sys.argv[1:])' \ + UBI_LEFT_FILE="$log.left" python3 -c 'import os, sys; os.setsid(); os.execvp(sys.argv[1], sys.argv[1:])' \ bash -c "$vm_session" pregate-remote-vm "$stop_after" "$log" "$stream" "$here/ubi-runner.sh" "${args[@]}" -n "$vm_name" & vmrun=$! tail -n +1 -f "$stream" & @@ -254,7 +254,8 @@ span() { if [ -n "$1" ] && [ -n "$2" ]; then echo "$(( $2 - $1 ))s"; else echo " say "phases: create $(span "$t0" "$t_ready"), setup $(span "$t_setup" "$t_run"), run $(span "$t_run" "$t_exit"), teardown $(span "$t_exit" "$t_end"); total $(( t_end - started ))s (target 600s)" say "log: $log" -left=$(sed -n 's/^[0-9]* UBICLOUD: left=\([^ ]*\) .*/\1/p' "$log" | tail -1) +left="" +[ ! -f "$log.left" ] || left=$(sed -n 's/^UBICLOUD: left=\([^ ]*\) .*/\1/p' "$log.left" | tail -1) if [ "$left" != 0 ]; then stop 4 PREGATE_REMOTE_INFRA "VM cleanup not confirmed (UBICLOUD: left=${left:-missing}); no result reported" \ "VM: $vm_name; destroy it with: bash scripts/ubicloud/ubi-runner.sh down $vm_name" "log: $log" "spent=$spent ($billed billed min on $size)" diff --git a/scripts/ubicloud/ubi-runner.sh b/scripts/ubicloud/ubi-runner.sh index 2b2a0a498..0936b9fab 100755 --- a/scripts/ubicloud/ubi-runner.sh +++ b/scripts/ubicloud/ubi-runner.sh @@ -37,7 +37,7 @@ # that `down`; further signals are ignored until it finishes. A create refused # for vCPU quota prints the project's usage per owner. # -# Stale-VM sweeps are off by default. With UBI_GC_HOURS set to a positive +# The owner-age sweep is off by default (the TTL reap below always runs). With UBI_GC_HOURS set to a positive # number, every `up` first destroys this owner's VMs older than that; `gc HOURS` # runs the same sweep on demand. Neither ever touches another owner's VMs. # @@ -54,7 +54,9 @@ # UBICLOUD: left=<0|1|unknown> owner= vm= others= # after teardown: left counts this invocation's own VM, checked against the # project's VM list, and others counts this owner's other VMs (information only). -# A left other than 0 turns a passing command into exit 1. +# A left other than 0 turns a passing command into exit 1; `run --keep` reports +# its live VM as left=1 kept=1 and keeps its exit status. With UBI_LEFT_FILE set, +# the line is also appended to that file, out of reach of the command's output. # # Quota preflight: when the project's vCPU ceiling is known (UBI_QUOTA_VCPUS, or # the maximum a refusal printed, cached in $STATE_ROOT/quota-max), `up` adds the @@ -107,8 +109,6 @@ for bin in curl python3 ssh ssh-keygen tar; do done UBICLOUD_API_KEY="${UBICLOUD_API_KEY:-${UBICLOUD_API_TOKEN:-}}" [[ "$GC_HOURS" =~ ^[0-9]+$ ]] || die "UBI_GC_HOURS must be a whole number of hours (unset or 0 disables the sweep)" -[[ "$TTL_HOURS" =~ ^[1-9][0-9]{0,2}$ ]] || die "UBI_TTL_HOURS must be a whole number of hours from 1 to 999 with no leading zero (every VM carries a TTL; set it above your longest run)" -[[ "$LEGACY_HOURS" =~ ^[1-9][0-9]{0,3}$ ]] || die "UBI_LEGACY_HOURS must be a whole number of hours from 1 to 9999 with no leading zero (how old a TTL-less ubirun-* VM must be before reap destroys it)" for v in IDLE_TIMEOUT RUN_TIMEOUT HEARTBEAT QUOTA_WAIT; do [[ "${!v}" =~ ^[0-9]+$ ]] || die "UBI_$v must be a whole number of seconds (0 disables it)" done @@ -133,9 +133,19 @@ owner_id() { } OWNER=$(owner_id) -# require_owner: up, run and verify create VMs, and a VM must name its owner. +# require_owner: up, run and verify create VMs, and a VM must name its owner +# and carry a TTL. Only creates check UBI_TTL_HOURS, so an old value never +# blocks down, reap or list. require_owner() { [ -n "${UBI_OWNER:-}" ] || die "UBI_OWNER is not set: every VM must carry its owner; export UBI_OWNER= (for example gsta164) and rerun (UBI_OWNER_REQUIRED)" + [[ "$TTL_HOURS" =~ ^[1-9][0-9]{0,2}$ ]] || die "UBI_TTL_HOURS must be a whole number of hours from 1 to 999 with no leading zero (every VM carries a TTL; set it above your longest run)" +} + +# leftover FIELDS...: print the UBICLOUD: leftover line, and append it to +# UBI_LEFT_FILE when set, so a caller can read it apart from the command's output. +leftover() { + echo "UBICLOUD: $*" >&2 + [ -z "${UBI_LEFT_FILE:-}" ] || echo "UBICLOUD: $*" >>"$UBI_LEFT_FILE" } # api OUT ARGS...: send one Ubicloud CLI command, write the body to OUT, and @@ -312,6 +322,7 @@ vm_rows() { # (nobody declared a lifetime for it). Names outside the prefix are never touched. cmd_reap() { local now rows loc name owner expires created n=0 failed=0 + [[ "$LEGACY_HOURS" =~ ^[1-9][0-9]{0,3}$ ]] || die "UBI_LEGACY_HOURS must be a whole number of hours from 1 to 9999 with no leading zero (how old a TTL-less ubirun-* VM must be before reap destroys it)" now=$(date +%s) rows=$(vm_rows) || { log "reap: could not list the project's VMs; nothing destroyed"; return 1; } while read -r loc name owner expires created; do @@ -752,7 +763,7 @@ run_teardown() { else left=unknown others=unknown fi - echo "UBICLOUD: left=$left owner=$OWNER vm=$RUN_VM others=$others" >&2 + leftover "left=$left owner=$OWNER vm=$RUN_VM others=$others" [ "$left" = 0 ] && return 0 log "VM cleanup not confirmed for $RUN_VM (left=$left); destroy it with: $0 down $RUN_VM" [ "$rc" -ne 0 ] || exit 1 @@ -794,12 +805,14 @@ cmd_run() { cmd_up "${up_args[@]}" >/dev/null if [[ "$name" =~ -t([0-9]+)$ ]]; then log "$name carries a ${BASH_REMATCH[1]}h TTL (any later run of this script reaps it); if this process dies first: $0 down $name" - else + elif [[ "$name" =~ ^$PREFIX-[a-z][a-z0-9]{0,11}-[0-9]{10}- ]]; then log "$name has no TTL (reap destroys it once it is ${LEGACY_HOURS}h old); if this process dies before its teardown line, destroy it with: $0 down $name" + else + log "$name has no TTL and no reapable name, so nothing reaps it; if this process dies before its teardown line, destroy it with: $0 down $name" fi if [ "$keep" = 1 ]; then trap - EXIT - echo "UBICLOUD: left=1 owner=$OWNER vm=$name kept=1" >&2 + leftover "left=1 owner=$OWNER vm=$name kept=1" log "--keep: leaving $name running; destroy with: $0 down $name" fi diff --git a/test/ubicloud-pregate-remote.test.ts b/test/ubicloud-pregate-remote.test.ts index 361b3d25b..852170405 100644 --- a/test/ubicloud-pregate-remote.test.ts +++ b/test/ubicloud-pregate-remote.test.ts @@ -40,7 +40,7 @@ const UBI_RUNNER_STUB = `#!/usr/bin/env bash # Every teardown is appended to $STUB_CALLS.down. set -uo pipefail [ "$1" != owner ] || { echo stub; exit 0; } -left() { [ "\${STUB_LEFT:-0}" = none ] || echo "UBICLOUD: left=\${STUB_LEFT:-0} owner=stub vm=$name others=0" >&2; } +left() { [ "\${STUB_LEFT:-0}" = none ] || echo "UBICLOUD: left=\${STUB_LEFT:-0} owner=stub vm=$name others=0" | tee -a "\${UBI_LEFT_FILE:-/dev/null}" >&2; } [ "$1" = run ] || exit 2 shift printf '%s\\n' "$*" >>"$STUB_CALLS" diff --git a/test/ubicloud-runner.test.ts b/test/ubicloud-runner.test.ts index a0d7c96e6..ed44aa8cb 100644 --- a/test/ubicloud-runner.test.ts +++ b/test/ubicloud-runner.test.ts @@ -484,11 +484,16 @@ describe('ubi-runner run, verify and quota against a fake API and a fake ssh', ( test('run: ends with UBICLOUD: left=0 for its own VM; a same-owner sibling is counted as information and does not fail the run', () => { const f = fakeVm({ listed: 'eu-central-h1 ubirun-thrd1-9999999999-sib1-t3\n' }); try { - const r = f.run(['run', '--src', f.root, '--', 'true'], { UBI_REAP: '0' }); + const leftFile = join(f.root, 'left'); + const r = f.run(['run', '--src', f.root, '--', 'true'], { UBI_REAP: '0', UBI_LEFT_FILE: leftFile }); const err = r.stderr.toString(); expect(r.exitCode, err).toBe(0); expect(err).toMatch(/UBICLOUD: left=0 owner=thrd1 vm=ubirun-thrd1-\d{10}-[0-9a-f]{8}-t3 others=1\n/); expect(f.listed()).toBe('eu-central-h1 ubirun-thrd1-9999999999-sib1-t3\n'); + // Value: protects=UBI_LEFT_FILE carries the same leftover line pregate-remote.sh parses, apart from the command's output; fails_when=the file is not written or its format drifts from pregate-remote.sh's parser; why_new=the pregate stub hand-copies the format; seam=none; no_claim=real VM teardown (fake API and ssh) + expect(readFileSync(leftFile, 'utf8')).toMatch(/^UBICLOUD: left=0 owner=thrd1 vm=ubirun-thrd1-\d{10}-[0-9a-f]{8}-t3 others=1\n$/); + const parser = read('scripts/ubicloud/pregate-remote.sh').match(/left=\$\(sed -n '([^']+)' "\$log\.left"/)![1]; + expect(Bun.spawnSync(['sed', '-n', parser, leftFile]).stdout.toString()).toBe('0\n'); // Value: protects=run --keep reports its live VM as UBICLOUD: left=1 ... kept=1 and never a clean left=0; fails_when=the kept line is dropped or --keep still prints the teardown line; why_new=no test runs run --keep; seam=none; no_claim=real VM lifetime (fake API and ssh) const kept = f.run(['run', '--src', f.root, '--keep', '--', 'true'], { UBI_REAP: '0' }); const keptErr = kept.stderr.toString(); @@ -731,13 +736,14 @@ describe('ubi-runner run, verify and quota against a fake API and a fake ssh', ( expect(listFails.exitCode).toBe(1); expect(listFails.stderr.toString()).toContain("reap: could not list the project's VMs; nothing destroyed"); for (const ttl of ['0', '08', '1000', 'abc']) { - const r = run(f, ['owner'], { UBI_TTL_HOURS: ttl }); + const r = run(f, ['up'], { UBI_TTL_HOURS: ttl }); expect(r.exitCode).toBe(1); expect(r.stderr.toString()).toContain('UBI_TTL_HOURS must be a whole number of hours from 1 to 999'); } - expect(run(f, ['owner'], { UBI_TTL_HOURS: '999' }).exitCode).toBe(0); + // Value: protects=an old UBI_TTL_HOURS=0 or bad UBI_LEGACY_HOURS never blocks commands that create nothing (down, list, owner); fails_when=the TTL checks move back to startup; why_new=the checks were global and blocked cleanup; seam=none; no_claim=real down against the API (owner stands in for every read-only command) + expect(run(f, ['owner'], { UBI_TTL_HOURS: '0', UBI_LEGACY_HOURS: '0' }).exitCode).toBe(0); for (const legacy of ['0', '04', 'x']) { - const r = run(f, ['owner'], { UBI_LEGACY_HOURS: legacy }); + const r = run(f, ['reap'], { UBI_LEGACY_HOURS: legacy }); expect(r.exitCode).toBe(1); expect(r.stderr.toString()).toContain('UBI_LEGACY_HOURS must be a whole number of hours from 1 to 9999'); } diff --git a/ubicloud/SKILL.md b/ubicloud/SKILL.md index b7dbd6c87..2dffeca09 100644 --- a/ubicloud/SKILL.md +++ b/ubicloud/SKILL.md @@ -246,6 +246,7 @@ and for browser tests `xvfb` plus it with every result: `left=0` means this run's VM is confirmed gone. `left=1` or `left=unknown` turns a passing command into exit 1; run the printed `down` command now. `others` counts this owner's other VMs, for information. + `run --keep` prints `left=1 ... kept=1` instead and keeps its exit status. - Exit 124 with `watchdog: printed nothing for s` (or `ran s`) means the remote hung or exceeded its budget; the last 40 lines follow. Diagnose from them before rerunning; a hang is a failure, not infrastructure. diff --git a/ubicloud/SKILL.md.tmpl b/ubicloud/SKILL.md.tmpl index db9be819d..feff69cfd 100644 --- a/ubicloud/SKILL.md.tmpl +++ b/ubicloud/SKILL.md.tmpl @@ -172,6 +172,7 @@ and for browser tests `xvfb` plus it with every result: `left=0` means this run's VM is confirmed gone. `left=1` or `left=unknown` turns a passing command into exit 1; run the printed `down` command now. `others` counts this owner's other VMs, for information. + `run --keep` prints `left=1 ... kept=1` instead and keeps its exit status. - Exit 124 with `watchdog: printed nothing for s` (or `ran s`) means the remote hung or exceeded its budget; the last 40 lines follow. Diagnose from them before rerunning; a hang is a failure, not infrastructure.