diff --git a/install.sh b/install.sh index 0c00c5a4..a69fa8e7 100755 --- a/install.sh +++ b/install.sh @@ -71,25 +71,88 @@ CLAWBOX_HOME="/home/clawbox" # exits non-zero, and a machine-readable marker is left for the flash host. PROVISION_FAILURES=() PROVISION_STATUS_FILE="${CLAWBOX_PROVISION_STATUS_FILE:-/etc/clawbox/provision-status}" +# Identifies THIS run. Stamped into the marker and printed on stdout, so a +# reader holding both can tell whose verdict it is looking at. +PROVISION_RUN_ID="$(date -u +%Y%m%dT%H%M%SZ 2>/dev/null || echo unknown)-$$" +# Set when the marker channel could not be made to describe this run. The final +# verdict folds this in: a verdict we cannot publish is not a success. +PROVISION_STATUS_UNPUBLISHED=0 record_provision_failure() { PROVISION_FAILURES+=("$1") } +# ── The marker must never speak for a run other than this one ──────────────── +# The flash host reads $PROVISION_STATUS_FILE INSTEAD of parsing stdout, so the +# file has to satisfy two properties, neither of which "write it at the end and +# hope" provides: +# +# 1. Never stale. A run that cannot write the marker (read-only /etc, a file +# owned by another user, a full disk) used to leave the PREVIOUS run's +# STATUS=ok sitting there, and the flash host read it as this run's verdict +# — the same false-healthy result this whole block exists to prevent. So +# the marker is DELETED before provisioning starts: if the file exists at +# the end, this run wrote it. +# 2. Never half-written. Temp file + rename in the same directory, so a reader +# sees the whole old marker or the whole new one, and a truncated write +# cannot leave "STATUS=ok" with the rest of the record missing. +# +# When either cannot be guaranteed, that is itself a reason not to ship the box: +# the run says so on stdout and its verdict becomes "incomplete". Staying quiet +# is what produced the false "ok". + +# Drop any marker left behind by an earlier run. Called once, before the first +# provisioning step of a full install. +invalidate_provision_status() { + rm -f "$PROVISION_STATUS_FILE" 2>/dev/null || true + # `rm -f` reports success for an already-absent file and failure for one it + # could not remove, so test the outcome rather than its exit status. + if [ -e "$PROVISION_STATUS_FILE" ]; then + PROVISION_STATUS_UNPUBLISHED=1 + echo " WARNING: could not clear the previous provisioning marker" + echo " $PROVISION_STATUS_FILE — its contents describe an EARLIER" + echo " run and must not be read as this one's verdict." + return 1 + fi + return 0 +} + # Persist the final provisioning verdict where the flash host (or an operator, # or the next update) can read it without re-parsing install.sh's stdout. write_provision_status() { local status="$1"; shift - local dir + local dir tmp failed=0 dir="$(dirname "$PROVISION_STATUS_FILE")" + tmp="$PROVISION_STATUS_FILE.tmp.$$" mkdir -p "$dir" 2>/dev/null || true - { + if ! { echo "# Written by install.sh at the end of a full install. Machine-readable." + echo "# One marker per run: the previous one is removed before provisioning" + echo "# starts, so this file always describes the run named by RUN_ID." + echo "RUN_ID=$PROVISION_RUN_ID" echo "STATUS=$status" echo "FAILED_STEPS=$*" echo "TIMESTAMP=$(date -u +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" - } > "$PROVISION_STATUS_FILE" 2>/dev/null || true - chmod 644 "$PROVISION_STATUS_FILE" 2>/dev/null || true + } > "$tmp" 2>/dev/null; then + failed=1 + else + chmod 644 "$tmp" 2>/dev/null || true + # Rename last: until this succeeds the live path holds nothing (it was + # cleared at the start), never a partial record. + mv -f "$tmp" "$PROVISION_STATUS_FILE" 2>/dev/null || failed=1 + fi + # Either half failing means the same thing to the caller and wants the same + # answer, so there is one branch for both rather than two that must be kept + # saying the same thing. + if [ "$failed" -ne 0 ]; then + rm -f "$tmp" 2>/dev/null || true + PROVISION_STATUS_UNPUBLISHED=1 + echo " WARNING: could not publish $PROVISION_STATUS_FILE (status=$status)." + echo " This run has no marker. Use install.sh's exit code or the" + echo " [provision-status] line on stdout instead." + return 1 + fi + return 0 } # ── Edition (single-harness lock) ──────────────────────────────────────────── @@ -3241,6 +3304,12 @@ log() { echo "=== ClawBox Installer ===" +# Clear the previous run's verdict BEFORE provisioning anything. From here until +# the summary at the bottom there is deliberately no marker on disk, so a run +# that dies mid-way (or cannot write its own marker at the end) leaves the flash +# host with "no verdict" rather than with the last run's "ok". +invalidate_provision_status || true + log "Ensuring clawbox user exists..." step_ensure_user @@ -3382,7 +3451,9 @@ echo "" # success by the caller (the flash host's "Setup: N/N succeeded"). The marker # file carries the same verdict for a caller that reads a file instead of the # exit code, and the sentinel line ([provision-status] ...) for one that greps -# stdout. Keep all three in agreement. +# stdout. Keep all three in agreement — including when the marker cannot be +# written at all, in which case the other two must report "incomplete" rather +# than a success no reader of the file can see. FINAL_RC=0 if [ "${#PROVISION_FAILURES[@]}" -gt 0 ] || [ "${VALIDATE_RC:-0}" -ne 0 ]; then FINAL_RC=1 @@ -3400,11 +3471,29 @@ if [ "$FINAL_RC" -ne 0 ]; then echo " # Service validation FAILED (see the checks listed above)." fi echo " ############################################################" - write_provision_status incomplete "${PROVISION_FAILURES[*]:-}" + write_provision_status incomplete "${PROVISION_FAILURES[*]:-}" || true + # The sentinel lines below are a stdout contract with the flash host: keep the + # prefix and the verdict word byte-identical. The run id goes on its own line. echo "[provision-status] INCOMPLETE${PROVISION_FAILURES[*]:+ (${PROVISION_FAILURES[*]})}" + echo "[provision-run] $PROVISION_RUN_ID" else - write_provision_status ok "" - echo "[provision-status] OK" + write_provision_status ok "" || true + if [ "$PROVISION_STATUS_UNPUBLISHED" -ne 0 ]; then + # Every step passed, but the channel the flash host reads cannot be made to + # say so for THIS run. Reporting success here is how a stale marker gets + # read as a fresh verdict, so downgrade instead: an install whose result + # cannot be published is not an install anyone should ship. + FINAL_RC=1 + echo " ############################################################" + echo " # PROVISIONING INCOMPLETE — every step passed, but this run" + echo " # could not publish its verdict to $PROVISION_STATUS_FILE." + echo " # Do NOT ship this box as healthy; fix the path and re-run." + echo " ############################################################" + echo "[provision-status] INCOMPLETE (marker unwritable)" + else + echo "[provision-status] OK" + fi + echo "[provision-run] $PROVISION_RUN_ID" fi exit "$FINAL_RC" diff --git a/scripts/setup-hermes-dashboard-auth.sh b/scripts/setup-hermes-dashboard-auth.sh index 232bf6cc..9cb09996 100644 --- a/scripts/setup-hermes-dashboard-auth.sh +++ b/scripts/setup-hermes-dashboard-auth.sh @@ -287,8 +287,32 @@ PY done # Store the plaintext password for the proxy ONLY (clawbox-owned, 0600). - ( umask 077; printf '%s' "$pw" > "$PWFILE" ) - chmod 600 "$PWFILE" + # + # Same temp-file-and-rename as the config write above, for the same reason: + # `> "$PWFILE"` truncates in place, so the proxy — which re-reads this file on + # every session renewal — can observe an empty or partial password and answer + # 401, and a crash between the truncate and the write leaves it empty for good + # (classify_creds then calls that PW_MISSING). The temp file lives in the same + # directory so the rename is atomic, and is created 0600 by umask so the + # plaintext is never briefly world-readable. + local pwtmp="$PWFILE.tmp.$$" + if ! ( umask 077; printf '%s' "$pw" > "$pwtmp" ); then + rm -f "$pwtmp" 2>/dev/null || true + log "ERROR: failed to write $pwtmp" >&2 + return 1 + fi + # Checked separately from the write above: folding both into one subshell + # would let a successful chmod mask a partial printf. + if ! chmod 600 "$pwtmp"; then + rm -f "$pwtmp" 2>/dev/null || true + log "ERROR: failed to set mode 600 on $pwtmp" >&2 + return 1 + fi + if ! mv -f "$pwtmp" "$PWFILE"; then + rm -f "$pwtmp" 2>/dev/null || true + log "ERROR: failed to install $PWFILE" >&2 + return 1 + fi return 0 } @@ -385,6 +409,12 @@ while :; do "$CREDS_NOT_CONFIGURED"|"$CREDS_PW_MISSING") if [ "$attempt" -lt "$max_attempts" ]; then log "WARNING: the dashboard auth block / password file we just wrote to $HERMES_CONFIG is already gone — a concurrent process rewrote the file. The credentials generated were correct; the write was lost. Retrying under $CONFIG_LOCK (attempt $attempt/$max_attempts)." >&2 + # Back off before re-minting. This branch only runs when the lock did + # NOT hold us apart, i.e. exactly while the other writer is still inside + # its own read-modify-write window — retrying immediately loses the same + # race again, and all three attempts can finish before it lands its + # os.replace. Grows with the attempt so a slow writer still gets a turn. + sleep "$attempt" continue fi log "ERROR: after $attempt attempts our dashboard auth block did not survive in $HERMES_CONFIG — another process keeps rewriting the file and is not honouring $CONFIG_LOCK. This is a write/serialisation fault, NOT a credential mismatch: the password and hash were correct on every attempt." >&2 diff --git a/src/tests/unit/hermes-config-lock.test.ts b/src/tests/unit/hermes-config-lock.test.ts index 3ff21e82..0d95075b 100644 --- a/src/tests/unit/hermes-config-lock.test.ts +++ b/src/tests/unit/hermes-config-lock.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { spawnSync } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -43,11 +43,27 @@ describe("both writers share ONE lock file", () => { // its PyYAML reconcile AND holds it across the `hermes tools disable` CLI // call (which does its own wide load->save_config on the same file). expect(AUTH_SRC).toMatch(/acquire_config_lock[\s\S]*mint_credentials/); - const regTail = REGISTER_SRC.slice(REGISTER_SRC.indexOf("acquire_config_lock\n\nexport")); - expect(regTail).toContain("acquire_config_lock"); - expect(REGISTER_SRC.indexOf("acquire_config_lock\n\nexport")).toBeLessThan( - REGISTER_SRC.indexOf("tools disable browser"), - ); + + // Locate the registrar's CALL SITE with an anchored regex: a bare + // `acquire_config_lock` line at column 0. That cannot match the DEFINITION + // (`acquire_config_lock() {`) and does not depend on what happens to follow + // it, unlike the old "acquire_config_lock\n\nexport" formatting marker. + const call = REGISTER_SRC.search(/^acquire_config_lock[ \t]*$/m); + // The registrar's two writes of config.yaml: the PyYAML reconcile, and the + // Hermes CLI call that does its own load->save_config. The lock has to come + // before BOTH — "before the CLI call" alone was satisfied by taking it one + // line above, leaving the reconcile unprotected. + const reconcile = REGISTER_SRC.search(/^export CLAWBOX_MCP_HERMES_CONFIG=/m); + const cliCall = REGISTER_SRC.search(/^if "\$HERMES_BIN" tools disable browser/m); + // Every marker must have been FOUND before their order means anything: a + // `search` miss returns -1, and -1 < anything, so an ordering assertion over + // a moved marker passes while checking nothing. + expect(call, "register-mcp.sh: no top-level acquire_config_lock call").toBeGreaterThan(-1); + expect(reconcile, "register-mcp.sh: no PyYAML reconcile block").toBeGreaterThan(-1); + expect(cliCall, "register-mcp.sh: no `hermes tools disable browser` call").toBeGreaterThan(-1); + expect(call).toBeLessThan(reconcile); + expect(call).toBeLessThan(cliCall); + expect(AUTH_SRC).toContain("flock -w 120 9"); expect(REGISTER_SRC).toContain("flock -w 120 9"); }); @@ -78,10 +94,38 @@ describe.runIf(RUNNABLE)("the lock is really taken at runtime", () => { }); it.runIf(FLOCK)("waits for a held lock instead of racing through it", () => { - // Hold the shared lock for ~800ms in the background, then run the auth - // script. If it honours the lock it blocks until release (elapsed ≳ hold); - // if it ignored it, it would finish in well under 300ms. This is the mutual - // exclusion that stops the lost update. + // Timed in THIS process, not in the shell: `date +%s.%N` is a GNU coreutils + // extension, and on BSD/macOS date `%N` is emitted literally, so the old + // driver parsed "1770000000.N" and failed for a reason that had nothing to + // do with the lock. The suite gates only on platform !== "win32", so it runs + // there. And only the auth script's OWN duration is measured — timing the + // whole driver, including `wait` on the holder, would report ≈ the hold + // time whether or not the script honoured the lock. + const HOLD_S = 1.5; + + // Calibrate: what one UNCONTENDED run of this script costs on this machine. + // The assertion below is "honouring the lock adds most of the hold ON TOP of + // that", so it cannot be satisfied by a slow interpreter. Twice, keeping the + // faster: the first run in a fresh process pays cold-start costs (bash, + // python, page cache) that the contended run no longer pays, and a cold + // baseline would eat the margin and fail for the wrong reason. + const timeUncontendedRun = () => { + const baseRoot = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-lockbase-")); + const baseConfig = path.join(baseRoot, "hermes", "config.yaml"); + fs.mkdirSync(path.dirname(baseConfig), { recursive: true }); + fs.writeFileSync(baseConfig, "mcp_servers:\n clawbox:\n enabled: true\n"); + const baseStart = Date.now(); + const baseProc = spawnSync("bash", [AUTH], { + encoding: "utf-8", + timeout: 20000, + env: { ...process.env, CLAWBOX_ROOT: baseRoot, HERMES_CONFIG: baseConfig }, + }); + const ms = Date.now() - baseStart; + expect(baseProc.status, baseProc.stderr).toBe(0); + return ms; + }; + const uncontendedMs = Math.min(timeUncontendedRun(), timeUncontendedRun()); + const root = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-lockwait-")); const configPath = path.join(root, "hermes", "config.yaml"); fs.mkdirSync(path.dirname(configPath), { recursive: true }); @@ -90,25 +134,43 @@ describe.runIf(RUNNABLE)("the lock is really taken at runtime", () => { fs.writeFileSync(configPath, "mcp_servers:\n clawbox:\n enabled: true\n"); const lockFile = `${configPath}.lock`; - // One bash driver so the holder and the auth script actually overlap: launch - // a background holder that takes the lock for ~0.8s, wait 0.15s so it wins - // the lock first, then run the auth script and time how long it blocks. - const script = ` - set -e - LOCK="${lockFile}" - ( exec 9>"$LOCK"; flock 9; sleep 0.8 ) & - hold=$! - sleep 0.15 # ensure the holder has the lock first - start=$(date +%s.%N) - CLAWBOX_ROOT="${root}" HERMES_CONFIG="${configPath}" bash "${AUTH}" >/dev/null 2>&1 - end=$(date +%s.%N) - wait $hold - awk -v s="$start" -v e="$end" 'BEGIN{printf "%.3f", e - s}' - `; - const proc = spawnSync("bash", ["-c", script], { encoding: "utf-8", timeout: 20000 }); - const elapsed = parseFloat(proc.stdout.trim() || "0"); - // Held ~0.8s, started ~0.15s in, so the auth script should wait ≳0.5s. - expect(elapsed).toBeGreaterThan(0.5); + // Background holder: takes the shared lock and keeps it for HOLD_S. + const holder = spawn("bash", ["-c", `exec 9>"${lockFile}"; flock 9; sleep ${HOLD_S}`], { + stdio: "ignore", + }); + holder.on("error", () => {}); + try { + // Do not GUESS that the holder has the lock — prove it, by probing with a + // non-blocking flock until the probe is refused. A sleep-and-hope here is + // how this test would start measuring nothing on a loaded machine. + const deadline = Date.now() + 5000; + let held = false; + while (Date.now() < deadline) { + const probe = spawnSync("bash", ["-c", `exec 9>"${lockFile}"; flock -n 9`], { + encoding: "utf-8", + }); + if (probe.status !== 0) { + held = true; + break; + } + } + expect(held, "the background holder never took the lock").toBe(true); + + const start = Date.now(); + const proc = spawnSync("bash", [AUTH], { + encoding: "utf-8", + timeout: 20000, + env: { ...process.env, CLAWBOX_ROOT: root, HERMES_CONFIG: configPath }, + }); + const contendedMs = Date.now() - start; + expect(proc.status, proc.stderr).toBe(0); + // Held 1.5s and the script started while it was held, so honouring the + // lock costs ≳1.4s more than the uncontended run. Ignoring it would cost + // about the same as the uncontended run. 500ms separates those cleanly. + expect(contendedMs - uncontendedMs).toBeGreaterThan(500); + } finally { + holder.kill(); + } // And the foreign writer's key survived alongside the new dashboard block. const config = fs.readFileSync(configPath, "utf-8"); diff --git a/src/tests/unit/hermes-dashboard-auth-yaml.test.ts b/src/tests/unit/hermes-dashboard-auth-yaml.test.ts index f3704b2d..7287ded9 100644 --- a/src/tests/unit/hermes-dashboard-auth-yaml.test.ts +++ b/src/tests/unit/hermes-dashboard-auth-yaml.test.ts @@ -16,12 +16,15 @@ import path from "node:path"; */ const SCRIPT = path.join(process.cwd(), "scripts", "setup-hermes-dashboard-auth.sh"); +const SCRIPT_SRC = fs.readFileSync(SCRIPT, "utf-8"); // The script needs bash and a python3 with hashlib.scrypt. Both are present on // the device and on CI; skip rather than fail anywhere else. const RUNNABLE = process.platform !== "win32" && spawnSync("bash", ["-c", "command -v python3"], { encoding: "utf-8" }).status === 0; +// Permission-denied cases need a uid that permissions actually apply to. +const NON_ROOT = typeof process.getuid === "function" && process.getuid() !== 0; function makeRoot() { const root = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-dashauth-")); @@ -312,8 +315,15 @@ describe.runIf(RUNNABLE)("dashboard auth: honest failure classes", () => { }); }); -/** Full provision with extra env (used to inject a broken PATH). */ -function run2(root: string, configPath: string, extraEnv: NodeJS.ProcessEnv) { +/** + * Full provision with extra env (used to inject a broken PATH). + * + * `extraEnv` is a plain record, not NodeJS.ProcessEnv: callers pass only the + * variables they are overriding, and spreading process.env in at the call site + * to satisfy the wider type would let a stray CLAWBOX_ROOT in the developer's + * own environment silently override the root under test. + */ +function run2(root: string, configPath: string, extraEnv: Record) { const env: NodeJS.ProcessEnv = { ...process.env, CLAWBOX_ROOT: root, @@ -322,3 +332,138 @@ function run2(root: string, configPath: string, extraEnv: NodeJS.ProcessEnv) { }; return spawnSync("bash", [SCRIPT], { encoding: "utf-8", env }); } + +/** + * The plaintext password file has one reader that matters: the dashboard proxy, + * which re-reads it on every session renewal. `printf '%s' "$pw" > "$PWFILE"` + * truncates the file before it writes it, so that reader can observe an empty + * or partial password — which the proxy turns into the HTTP 401 that + * install.sh's validator now (correctly) treats as unhealthy — and a crash + * between the truncate and the write leaves it empty permanently, which + * classify_creds then reports as PW_MISSING. The config write already avoided + * all of that with a temp file and a rename; the password file now does too. + */ +describe.runIf(RUNNABLE)("the password file is replaced, never truncated in place", () => { + it("never targets the password file with a truncating redirect", () => { + // Pinned as source shape as well as behaviour: a redirect reintroduced here + // reopens a window no functional test can reliably catch, because it is a + // few microseconds wide. Comment lines are stripped first — the comment + // explaining the bug quotes the very redirect being banned. + const code = SCRIPT_SRC.split("\n") + .filter((l) => !l.trimStart().startsWith("#")) + .join("\n"); + expect(code).not.toMatch(/[^0-9&|]>\s*"\$PWFILE"/); + // Temp file in the SAME directory — a rename is only atomic within one + // filesystem, and a temp under /tmp would silently degrade to copy+unlink. + expect(SCRIPT_SRC).toMatch(/pwtmp="\$PWFILE\./); + expect(SCRIPT_SRC).toContain('mv -f "$pwtmp" "$PWFILE"'); + }); + + it("re-mints onto a NEW file, so no reader ever holds an empty one", () => { + const { root, configPath } = makeRoot(); + const pwPath = path.join(root, "data", ".hermes-dashboard-pw"); + expect(run(root, configPath, "clawbox").status).toBe(0); + const before = fs.statSync(pwPath); + const passwordBefore = fs.readFileSync(pwPath, "utf-8"); + + // Desync the pair so the next run re-mints (classify_creds -> MISMATCH). + // Node's write truncates in place, so the inode is unchanged going in — the + // comparison below is against the file the first provision created. + fs.writeFileSync(pwPath, "not-the-stored-password"); + expect(fs.statSync(pwPath).ino).toBe(before.ino); + + expect(run(root, configPath, "clawbox").status).toBe(0); + + const after = fs.statSync(pwPath); + // A DIFFERENT inode is the observable difference between truncating the + // same file and renaming a finished one over it: a reader that opened the + // old file keeps reading the old password, whole, until it closes it. + expect(after.ino).not.toBe(before.ino); + expect(fs.readFileSync(pwPath, "utf-8")).not.toBe(passwordBefore); + // Still 0600, and never briefly wider than that (umask 077 on the temp). + expect(after.mode & 0o777).toBe(0o600); + // And the temp file did not survive the rename. + expect(fs.readdirSync(path.dirname(pwPath))).toEqual([".hermes-dashboard-pw"]); + }); + + // A read-only directory does not stop root, and this suite may run as root on + // the device. Say so in the skip rather than letting the test pass vacuously. + it.runIf(NON_ROOT)("fails loudly when the replacement cannot be written", () => { + // The temp write is the first thing that can fail. It must surface as a + // write failure with the old password file left untouched — not as a + // truncated file, and not as a credential verdict. + const { root, configPath } = makeRoot(); + const dataDir = path.join(root, "data"); + fs.mkdirSync(dataDir, { recursive: true }); + fs.chmodSync(dataDir, 0o500); + try { + const proc = run(root, configPath, "clawbox"); + expect(proc.status).toBe(1); + expect(proc.stderr).toContain("failed to write"); + expect(`${proc.stdout}${proc.stderr}`).not.toMatch(/does not match|do not verify/); + // Nothing landed in data/ — no password, no orphaned temp. + expect(fs.readdirSync(dataDir)).toEqual([]); + } finally { + fs.chmodSync(dataDir, 0o700); + } + }); +}); + +/** + * A `python3` that runs the real program and then, if that program was the one + * writing config.yaml, erases the block it just wrote — the competing writer + * (register-mcp.sh, the Hermes CLI, /setup-api/hermes/*) landing an os.replace + * built from a snapshot taken before our write. Deterministic, so the retry + * path is exercised on every attempt instead of occasionally. + */ +function blockErasingPythonDir(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-clobber-")); + fs.writeFileSync( + path.join(dir, "python3"), + [ + `#!${REAL_PYTHON}`, + "import os, sys", + "src = sys.stdin.read()", + 'is_writer = "os.replace(tmp, cfg_path)" in src', + "code = 0", + "try:", + ' exec(compile(src, "", "exec"), {"__name__": "__main__"})', + "except SystemExit as exc:", + " code = exc.code if isinstance(exc.code, int) else (0 if exc.code is None else 1)", + "if is_writer and code == 0:", + ' with open(os.environ["CFG"], "w", encoding="utf-8") as fh:', + ' fh.write("mcp_servers:\\n clawbox:\\n enabled: true\\n")', + "sys.exit(code)", + "", + ].join("\n"), + { mode: 0o755 }, + ); + return dir; +} + +describe.runIf(RUNNABLE)("lost-write retries back off", () => { + it("waits between attempts instead of re-losing the same race", () => { + const { root, configPath } = makeRoot(); + const stubDir = blockErasingPythonDir(); + + const started = Date.now(); + const proc = run2(root, configPath, { + PATH: `${stubDir}${path.delimiter}${process.env.PATH}`, + }); + const elapsedMs = Date.now() - started; + + // 3 = the write kept being lost. NOT 2 (a real hash mismatch) and not 4 (an + // environment error): the credentials were correct on every attempt. + expect(proc.status, proc.stderr).toBe(3); + expect(proc.stderr).toMatch(/Retrying under/); + // Attempts 1 and 2 retry; attempt 3 gives up. Two retries, two backoffs. + expect(proc.stderr.match(/Retrying under/g)?.length).toBe(2); + + // The retry branch only runs while the competing writer is still inside its + // own read-modify-write window, so retrying immediately loses again — all + // three attempts used to finish in well under a second, before the other + // writer had landed anything. 1s then 2s of backoff puts a floor well above + // that, whatever this machine's scrypt costs. + expect(elapsedMs).toBeGreaterThan(2500); + }); +}); diff --git a/src/tests/unit/install-hermes-edition-step.test.ts b/src/tests/unit/install-hermes-edition-step.test.ts index b5acd1fe..a0309376 100644 --- a/src/tests/unit/install-hermes-edition-step.test.ts +++ b/src/tests/unit/install-hermes-edition-step.test.ts @@ -23,12 +23,30 @@ const HERMES_SETUP = readFileSync( "utf-8", ); -function extractShellFunction(name: string): string { +/** + * Slice one shell function out of install.sh. + * + * The end of the slice is the first line that is exactly `}` — a heuristic, and + * one that truncates silently the day an embedded program (an awk/jq/python + * heredoc) puts a `}` at column 0. A truncated slice makes every `not.toContain` + * over it pass for the wrong reason, so any caller making a negative assertion + * must pass `endsWith`: a fragment of the function's LAST statement, which the + * slice has to reach for the extraction to count as complete. + */ +function extractShellFunction(name: string, endsWith?: string): string { const start = INSTALL_SH.indexOf(`${name}() {`); if (start < 0) throw new Error(`${name} not found in install.sh`); const end = INSTALL_SH.indexOf("\n}", start); if (end < 0) throw new Error(`${name} has no closing brace`); - return INSTALL_SH.slice(start, end); + const body = INSTALL_SH.slice(start, end); + if (endsWith !== undefined && !body.includes(endsWith)) { + throw new Error( + `${name} was extracted TRUNCATED — the slice never reached ${JSON.stringify(endsWith)}. ` + + `A '}' at column 0 inside the function (an embedded heredoc?) ends the slice early, ` + + `and assertions over the short slice would silently stop testing.`, + ); + } + return body; } // `hermes_edition` being in DISPATCH_STEPS is already pinned by @@ -43,9 +61,11 @@ describe("hermes_edition is the updater's own step", () => { it("post_update no longer calls it — that would run provisioning twice", () => { // Two dashboard/proxy restarts per update, and the swallowed copy would // still be the one that ran first. - expect(extractShellFunction("step_post_update")).not.toMatch( - /^\s*step_hermes_edition\b/m, - ); + // Negative assertion → the slice has to cover the whole function, so name + // its last statement. + expect( + extractShellFunction("step_post_update", "step_update_smoke ||"), + ).not.toMatch(/^\s*step_hermes_edition\b/m); }); it("the full install still provisions directly, not via post_update", () => { @@ -185,7 +205,11 @@ describe("service validation checks the dashboard auth provider", () => { // browserless probe gets a 302/403, so a 401 must fail the check. Assert the // case pattern precisely — the word "401" still appears in the DESYNCED // message, and that is correct. - const fn = extractShellFunction("step_validate_services"); + // + // The negative assertion is only worth anything over the WHOLE function, so + // require the slice to reach the last statement: a `401` re-added past a + // truncation point would otherwise go unnoticed. + const fn = extractShellFunction("step_validate_services", "--step edition_foreign_teardown"); expect(fn).toContain("2*|3*|403) ;;"); expect(fn).not.toContain("2*|3*|401|403"); }); diff --git a/src/tests/unit/install-provision-status-marker.test.ts b/src/tests/unit/install-provision-status-marker.test.ts new file mode 100644 index 00000000..01388f08 --- /dev/null +++ b/src/tests/unit/install-provision-status-marker.test.ts @@ -0,0 +1,271 @@ +import { describe, expect, it } from "vitest"; +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +/** + * The provisioning marker is the channel the flash host reads INSTEAD of + * parsing install.sh's stdout. That makes one failure mode worse than no marker + * at all: a run that cannot write it leaves the PREVIOUS run's `STATUS=ok` + * sitting at the path, and the flash host reads a stale success as this run's + * verdict — the exact false-healthy result the surrounding change exists to + * remove. Every write in the original helper ended in `|| true` with stderr + * discarded, so that happened silently. + * + * Two mechanisms now prevent it, and both are pinned here: + * + * 1. The marker is DELETED before provisioning starts, so its presence at the + * end means this run wrote it. "No marker" is a possible outcome; "last + * run's marker" is not. + * 2. If the marker cannot be cleared or cannot be written, the run says so and + * its verdict becomes `incomplete` on every other channel — exit code and + * stdout sentinel — instead of an `ok` no reader of the file can see. + * + * These run the real shell out of install.sh rather than a copy of it: the + * helpers and the final-verdict block are extracted from the file and executed. + */ +const REPO = process.cwd(); +const INSTALL_SH = fs.readFileSync(path.join(REPO, "install.sh"), "utf-8"); + +const RUNNABLE = process.platform !== "win32"; + +/** + * Extract a shell function, closing brace included. Anchored at column 0 on + * both ends, and it throws rather than returning a short slice — a truncated + * function would either fail to parse or, worse, parse into something that + * quietly does less than the real one. + */ +function shellFunction(name: string): string { + const re = new RegExp(`^${name}\\(\\) \\{\\n[\\s\\S]*?^\\}$`, "m"); + const m = re.exec(INSTALL_SH); + if (!m) throw new Error(`${name}() not found in install.sh`); + return m[0]; +} + +/** + * The final-verdict block: everything from FINAL_RC through the exit. This is + * the part that has to refuse to say "ok" when the marker could not be + * published, so the test runs the real lines rather than restating them. + */ +function finalVerdictBlock(): string { + const start = INSTALL_SH.indexOf('FINAL_RC=0\nif [ "${#PROVISION_FAILURES[@]}"'); + const endMarker = 'exit "$FINAL_RC"'; + const end = INSTALL_SH.indexOf(endMarker, start); + if (start < 0 || end < 0) { + throw new Error("install.sh: could not locate the final-verdict block"); + } + return INSTALL_SH.slice(start, end + endMarker.length); +} + +/** + * Run the extracted helpers (and optionally the final-verdict block) against a + * chosen marker path, in a shell configured exactly like install.sh's. + */ +function runHarness(opts: { + statusFile: string; + body: string; + withVerdict?: boolean; + failures?: string[]; + validateRc?: number; +}) { + const failures = (opts.failures ?? []).map((f) => `"${f}"`).join(" "); + const script = [ + "set -euo pipefail", + 'PROJECT_DIR="/home/clawbox/clawbox"', + `PROVISION_STATUS_FILE="${opts.statusFile}"`, + 'PROVISION_RUN_ID="run-under-test"', + "PROVISION_STATUS_UNPUBLISHED=0", + `PROVISION_FAILURES=(${failures})`, + `VALIDATE_RC=${opts.validateRc ?? 0}`, + shellFunction("invalidate_provision_status"), + shellFunction("write_provision_status"), + opts.body, + opts.withVerdict ? finalVerdictBlock() : "", + 'echo "UNPUBLISHED=$PROVISION_STATUS_UNPUBLISHED"', + ].join("\n"); + const proc = spawnSync("bash", ["-c", script], { encoding: "utf-8", timeout: 20000 }); + return { ...proc, out: `${proc.stdout ?? ""}${proc.stderr ?? ""}` }; +} + +function tmpdir(tag: string): string { + return fs.mkdtempSync(path.join(os.tmpdir(), `clawbox-${tag}-`)); +} + +describe.runIf(RUNNABLE)("the provisioning marker never speaks for an earlier run", () => { + it("clears the previous run's marker before this run provisions anything", () => { + const dir = tmpdir("marker-clear"); + const statusFile = path.join(dir, "provision-status"); + fs.writeFileSync(statusFile, "STATUS=ok\nFAILED_STEPS=\n"); + + const proc = runHarness({ statusFile, body: "invalidate_provision_status" }); + + expect(proc.status, proc.out).toBe(0); + // Gone, not overwritten: from here until the summary there is deliberately + // no verdict on disk, so a run that dies mid-way leaves "no verdict". + expect(fs.existsSync(statusFile)).toBe(false); + expect(proc.out).toContain("UNPUBLISHED=0"); + }); + + it("reports a marker it could not clear, instead of leaving it to be read", () => { + // A path `rm -f` cannot remove for ANY uid (a directory, not a file) stands + // in for the read-only /etc or foreign-owned file seen in the field. + const dir = tmpdir("marker-stuck"); + const statusFile = path.join(dir, "provision-status"); + fs.mkdirSync(statusFile); + + const proc = runHarness({ + statusFile, + body: "invalidate_provision_status || true", + }); + + expect(proc.status, proc.out).toBe(0); + expect(proc.out).toContain("UNPUBLISHED=1"); + expect(proc.out).toContain("EARLIER"); + expect(proc.out).toContain(statusFile); + }); + + it("stamps the run id into the marker it writes", () => { + const dir = tmpdir("marker-stamp"); + const statusFile = path.join(dir, "provision-status"); + + const proc = runHarness({ + statusFile, + body: 'invalidate_provision_status\nwrite_provision_status incomplete "hermes_edition"', + }); + + expect(proc.status, proc.out).toBe(0); + const marker = fs.readFileSync(statusFile, "utf-8"); + expect(marker).toContain("RUN_ID=run-under-test"); + expect(marker).toContain("STATUS=incomplete"); + expect(marker).toContain("FAILED_STEPS=hermes_edition"); + expect(proc.out).toContain("UNPUBLISHED=0"); + }); + + it("renames a complete marker into place instead of truncating one", () => { + const dir = tmpdir("marker-atomic"); + const statusFile = path.join(dir, "provision-status"); + // An existing marker at the path, so the replacement is observable. + fs.writeFileSync(statusFile, "STATUS=ok\n"); + const before = fs.statSync(statusFile).ino; + + const proc = runHarness({ statusFile, body: 'write_provision_status ok ""' }); + + expect(proc.status, proc.out).toBe(0); + // A different inode means the record was written elsewhere and renamed over + // the old one. Writing in place would keep the inode and expose a truncated + // file — the same failure the password file had. + expect(fs.statSync(statusFile).ino).not.toBe(before); + // Nothing but the marker itself: a temp file left in the directory would + // mean the rename never happened. + expect(fs.readdirSync(dir)).toEqual(["provision-status"]); + const marker = fs.readFileSync(statusFile, "utf-8"); + // Every field of the record, not a prefix of it. + expect(marker).toMatch(/^RUN_ID=.+$/m); + expect(marker).toMatch(/^STATUS=ok$/m); + expect(marker).toMatch(/^FAILED_STEPS=$/m); + expect(marker).toMatch(/^TIMESTAMP=\d{4}-\d{2}-\d{2}T/m); + }); +}); + +describe.runIf(RUNNABLE)("a marker that cannot be written is not a healthy verdict", () => { + /** + * A marker path whose parent is a regular file: `mkdir -p` and the write both + * fail with ENOTDIR, for root as well as for anyone else, so this reproduces + * an unwritable /etc/clawbox without depending on the uid the suite runs as. + */ + function unwritableStatusFile(tag: string): string { + const dir = tmpdir(tag); + const blocker = path.join(dir, "clawbox"); + fs.writeFileSync(blocker, "not a directory\n"); + return path.join(blocker, "provision-status"); + } + + it("says so on stdout and returns non-zero rather than reporting success", () => { + const statusFile = unwritableStatusFile("marker-unwritable"); + + const proc = runHarness({ + statusFile, + body: 'write_provision_status ok "" || echo "RC=$?"', + }); + + expect(proc.status, proc.out).toBe(0); + expect(proc.out).toContain("RC=1"); + expect(proc.out).toContain("UNPUBLISHED=1"); + // Names the file, so the operator knows which channel stopped being true. + expect(proc.out).toContain(statusFile); + expect(proc.out).toMatch(/could not publish/); + expect(fs.existsSync(statusFile)).toBe(false); + }); + + it("downgrades an otherwise-green run to INCOMPLETE, and exits non-zero", () => { + // THE regression: every step passed, but the channel the flash host reads + // cannot be made to describe this run. Before the fix this printed + // "[provision-status] OK", exited 0, and left whatever the path already held + // — a previous run's STATUS=ok — as the flash host's answer. + const statusFile = unwritableStatusFile("marker-verdict"); + + const proc = runHarness({ + statusFile, + body: "", + withVerdict: true, + failures: [], + validateRc: 0, + }); + + expect(proc.status, proc.out).toBe(1); + expect(proc.out).toContain("[provision-status] INCOMPLETE"); + expect(proc.out).not.toContain("[provision-status] OK"); + expect(proc.out).toContain("Do NOT ship this box as healthy"); + }); + + it("still says OK, and exits 0, when the marker really was published", () => { + // The counterpart: the downgrade must be caused by the unwritable marker, + // not by the harness. Same green run, a usable path. + const dir = tmpdir("marker-verdict-ok"); + const statusFile = path.join(dir, "provision-status"); + + const proc = runHarness({ statusFile, body: "", withVerdict: true }); + + expect(proc.status, proc.out).toBe(0); + expect(proc.out).toContain("[provision-status] OK"); + expect(proc.out).toContain("[provision-run] run-under-test"); + expect(fs.readFileSync(statusFile, "utf-8")).toMatch(/^STATUS=ok$/m); + }); + + it("keeps INCOMPLETE for a run that failed a step, marker or no marker", () => { + const dir = tmpdir("marker-verdict-fail"); + const statusFile = path.join(dir, "provision-status"); + + const proc = runHarness({ + statusFile, + body: "", + withVerdict: true, + failures: ["hermes_edition"], + }); + + expect(proc.status, proc.out).toBe(1); + expect(proc.out).toContain("[provision-status] INCOMPLETE (hermes_edition)"); + expect(fs.readFileSync(statusFile, "utf-8")).toMatch(/^STATUS=incomplete$/m); + }); +}); + +describe("install.sh wires the marker into the full install", () => { + it("clears the previous verdict before the first provisioning step", () => { + // The guarantee in test 1 only holds if the full-install path actually calls + // it, and calls it BEFORE anything can fail. + const call = INSTALL_SH.search(/^invalidate_provision_status \|\| true$/m); + const firstStep = INSTALL_SH.search(/^log "Ensuring clawbox user exists\.\.\."$/m); + expect(call, "install.sh: no top-level invalidate_provision_status call").toBeGreaterThan(-1); + expect(firstStep, "install.sh: full-install path not found").toBeGreaterThan(-1); + expect(call).toBeLessThan(firstStep); + }); + + it("does not clear it in --step mode, which is not a provisioning run", () => { + // A single-step re-run must not destroy the last full install's verdict. + const dispatch = INSTALL_SH.indexOf('if [ "${1:-}" = "--step" ]; then'); + const call = INSTALL_SH.search(/^invalidate_provision_status \|\| true$/m); + expect(dispatch).toBeGreaterThan(-1); + expect(call).toBeGreaterThan(dispatch); + }); +});