From eaf415664ca94e5294325d695b562a54c262ad52 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sat, 5 Sep 2026 21:19:03 +0000 Subject: [PATCH 1/3] test(deploy): pin the two retirement loops' messaging asymmetry (BLO-32122) Ally's remaining non-blocking suggestion from #1671, deferred there on that review's own recommendation because it needs a harness change rather than a new assertion. `:755-759` spends five lines arguing that the messaging difference between the two retirement loops -- `release_in_flight_lock` silent on success, retire-only mode printing three operator-facing lines -- is deliberate and NOT a parity gap to close. Neither harness captured stdout, so that claim was enforced by nothing: deleting all three `echo`s at `:342-344` left the suite green, and so did adding a matching success line to the silent loop. Both harnesses now return `stdout`, and both halves are pinned, not just the chatty one. The release success case asserts stdout is exactly empty, which is the more surprising half and the direction a parity fix would take -- it would ADD output there, which no assertion on retire-only mode can catch. The retire-only success case asserts the digest and owner, then the ring sentence across its line wrap, so a rewrap stays green and a deletion does not. The shell script is unmodified; the asymmetry it documents is correct. Mutation-verified at this head, each confirmed as a real edit (non-empty diff, `bash -n` clean) and the script restored clean after each: delete all three `:342-344` echoes -> retire-only success test fails delete only `:342` (digest+owner) -> retire-only success test fails delete only `:343-344` (ring) -> retire-only success test fails add a success echo at `:714` -> release success test fails The four BLO-32109 pacing mutations still fail their own exhaustion tests (retire-only `:374` -> `sleep 1` and -> `sleep 0`; release `:761` -> `sleep "$attempt"` and -> `sleep 9`), so this did not weaken them. Unmutated suite 64/64. Note the issue text says 65; that count was already stale when written -- HEAD before this change measures 64. Co-Authored-By: Claude --- scripts/approve-paperclip-api-digest.test.js | 48 ++++++++++++++++++-- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/scripts/approve-paperclip-api-digest.test.js b/scripts/approve-paperclip-api-digest.test.js index 4d406938ecf2..5ebc9b457e1f 100644 --- a/scripts/approve-paperclip-api-digest.test.js +++ b/scripts/approve-paperclip-api-digest.test.js @@ -1721,7 +1721,12 @@ release_in_flight_lock`; const sleeps = existsSync(sleepLog) ? readFileSync(sleepLog, "utf8").split("\n").filter(Boolean) : []; - return { status: r.status, stderr: r.stderr, writes, sleeps }; + // stdout as well as stderr: the two loops' MESSAGING asymmetry -- this one + // silent on success, retire-only mode chatty -- is the other half of what + // `:755-759` defends at length against a parity fix, and until both harnesses + // captured stdout it was asserted by nothing. Both directions of that fix are + // now failures: adding a success `echo` here, or deleting retire-only's. + return { status: r.status, stdout: r.stdout, stderr: r.stderr, writes, sleeps }; } test("a non-retriable retirement write bails once and reports the cause", () => { @@ -1773,7 +1778,7 @@ test("a conflicting retirement write is still retried to exhaustion", () => { ); }); -test("a retirement write that succeeds returns at the first attempt without sleeping", () => { +test("a retirement write that succeeds returns at the first attempt, silently and without sleeping", () => { // The control for the two pacing assertions: they pin what an EXHAUSTED loop // spends, and would still pass if the sleep had migrated above the success // check and started charging every caller. A succeeding retirement is the @@ -1782,6 +1787,20 @@ test("a retirement write that succeeds returns at the first attempt without slee assert.equal(r.writes, 1, "a succeeding write must not be repeated"); assert.equal(r.status, 0, "a retired lock must report success"); assert.deepEqual(r.sleeps, [], "the success path must not sleep"); + // The SILENT half of the messaging asymmetry, and the more surprising one -- + // a parity fix reading retire-only mode's three chatty success lines would + // "correct" this loop by ADDING output, which is the direction the retire-only + // assertion below cannot catch. `:755-759` argues this silence is required, + // not merely tolerated: the caller (`cleanup_on_exit`) prints the operator + // guidance itself, and this loop only ever runs after a deploy has already + // failed, so a success line here would bury the failure the cleanup exists to + // report. Asserted as exact emptiness rather than a `doesNotMatch` on today's + // wording, so any new success line fails regardless of how it is phrased. + assert.equal( + r.stdout, + "", + "this loop must stay silent on success -- its caller prints the guidance, and a success line here would bury the failure the cleanup is running after", + ); }); test("a retirement write with no stderr still explains itself", () => { @@ -1895,7 +1914,10 @@ ${retireRegion}`; const sleeps = existsSync(sleepLog) ? readFileSync(sleepLog, "utf8").split("\n").filter(Boolean) : []; - return { status: r.status, stderr: r.stderr, writes, sleeps }; + // stdout captured for the reason spelled out in `runReleaseWrite`: this is the + // chatty half of that messaging asymmetry, and the success lines asserted + // below are what the release harness's empty-stdout assertion is the mirror of. + return { status: r.status, stdout: r.stdout, stderr: r.stderr, writes, sleeps }; } test("retire-only mode's non-retriable write bails once and reports the cause", () => { @@ -1948,7 +1970,7 @@ test("retire-only mode's conflicting write is still retried to exhaustion", () = ); }); -test("retire-only mode's succeeding write exits 0 at the first attempt without sleeping", () => { +test("retire-only mode's succeeding write exits 0 at the first attempt, telling the operator what it retired", () => { // Same control as the release path's. Retire-only mode `exit 0`s where the // release loop `return 0`s, so this also pins that the success exit survives // the slice -- a mode that retired the lock and then exited non-zero would @@ -1957,6 +1979,24 @@ test("retire-only mode's succeeding write exits 0 at the first attempt without s assert.equal(r.writes, 1, "a succeeding write must not be repeated"); assert.equal(r.status, 0, "a retired lock must exit 0"); assert.deepEqual(r.sleeps, [], "the success path must not spend the backoff"); + // The CHATTY half, and the output this mode exists to produce: an operator + // ran it by hand and the three lines at `:342-344` are the only report they + // get. Until the harness captured stdout, silencing all three "for parity + // with `release_in_flight_lock`" left the suite green. + assert.match( + r.stdout, + /Retired the in-flight approval lock on sha256:cafe \(owner owner-9\)/, + "the operator must be told WHICH lock was retired, by digest and owner", + ); + // The consequence, not just the act: this is what tells the operator they can + // re-run a corrected plan without editing the ring by hand. Matched across the + // line wrap so a rewrap stays green and a deletion does not -- the two lines + // are one sentence and neither is separately meaningful. + assert.match( + r.stdout, + /The ring still lists that digest, so a corrected plan or a rollback is\s+admitted without an out-of-band edit\./, + "the operator must be told the retirement left the digest admissible, or they will assume they have to re-approve it", + ); }); test("retire-only mode's write with no stderr still explains itself", () => { From b7564e466a6618f6cc0360cb3a9022a302463d6a Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sat, 5 Sep 2026 21:37:24 +0000 Subject: [PATCH 2/3] test(deploy): pin the release loop's exhaustion silence too (BLO-32122) `:757-758` forbids a chatty success *or exhaustion* on `release_in_flight_lock`, because its caller (`cleanup_on_exit`) prints the operator guidance and this loop only ever runs after a deploy has already failed. The parent commit pinned the success half; the exhaustion half was still enforced by nothing -- Ally's M5 mutation (an `echo` before the trailing `return 1`) stayed green. Asserts exact emptiness on the exhaustion path, for the same reason the success assertion does: any new line fails regardless of wording. The harness change that makes this a one-liner is already in the parent. Mutation-verified, script restored clean after each: - chatty exhaustion on the release path (M5): now fails, exactly 1 test -- the exhaustion test. Was green before this commit. - chatty success on the release path (M1): still fails, still exactly 1 test -- the success test. Precision not blunted by the new assertion. - silence retire-only's three success lines (M2): still fails, exactly 1 test. 64/64 green unmutated. `scripts/approve-paperclip-api-digest.sh` is not modified; the messaging asymmetry is correct and deliberate. Co-Authored-By: Claude --- scripts/approve-paperclip-api-digest.test.js | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/scripts/approve-paperclip-api-digest.test.js b/scripts/approve-paperclip-api-digest.test.js index 5ebc9b457e1f..a2f1a6c5000d 100644 --- a/scripts/approve-paperclip-api-digest.test.js +++ b/scripts/approve-paperclip-api-digest.test.js @@ -1722,10 +1722,11 @@ release_in_flight_lock`; ? readFileSync(sleepLog, "utf8").split("\n").filter(Boolean) : []; // stdout as well as stderr: the two loops' MESSAGING asymmetry -- this one - // silent on success, retire-only mode chatty -- is the other half of what - // `:755-759` defends at length against a parity fix, and until both harnesses - // captured stdout it was asserted by nothing. Both directions of that fix are - // now failures: adding a success `echo` here, or deleting retire-only's. + // silent on both success and exhaustion, retire-only mode chatty -- is the + // other half of what `:755-759` defends at length against a parity fix, and + // until both harnesses captured stdout it was asserted by nothing. Every + // direction of that fix is now a failure: adding an `echo` here on either + // path, or deleting retire-only's. return { status: r.status, stdout: r.stdout, stderr: r.stderr, writes, sleeps }; } @@ -1776,6 +1777,17 @@ test("a conflicting retirement write is still retried to exhaustion", () => { /cannot retire the in-flight lock on/, "the non-retriable message must not fire on a retriable conflict", ); + // The EXHAUSTION half of the silence `:757-758` requires, which the success + // test's assertion cannot reach: that comment forbids a chatty success *or + // exhaustion*, and exhaustion is where the temptation is stronger, because + // the loop has just given up and so has something to report. It reports it on + // stderr or not at all -- the operator guidance belongs to `cleanup_on_exit`, + // and a stdout line here would bury the deploy failure that ran the cleanup. + assert.equal( + r.stdout, + "", + "an exhausted retirement must stay silent on stdout too -- its caller prints the guidance, and a line here would bury the failure the cleanup is running after", + ); }); test("a retirement write that succeeds returns at the first attempt, silently and without sleeping", () => { From a96e258bd3f84cc257ab139145b6b356bc761878 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sun, 6 Sep 2026 02:00:08 +0000 Subject: [PATCH 3/3] test(deploy): repoint the stale line citations and pin them mechanically (BLO-32122) Ally's review of #1674 at b7564e46 found all four line citations this PR adds pointing at the wrong lines, plus three pre-existing ones already broken on master. Cause: BLO-31842 inserted four lines into the shell script's header comment, shifting every cited line below it by +4 in one commit. Repoints all seven, not just the four in this PR's diff -- they live in one file, and fixing four while leaving three known-stale would be arbitrary. Each new target verified by CONTENT against the pre-rebase script (6dbfa475e), not by assuming a uniform +4: old :82 -> :86, :342-344 -> :346-348, :747-756 -> :751-760, :751-753 -> :755-757, :755-759 -> :759-763, :757-758 -> :761-762. The worst of them, :342-344, landed a reader on the "nothing to retire" branch's `exit 0` rather than the three success echoes the assertion beside it is about -- so the comment read as if it pinned a different code path. Also takes Ally's Suggestion, because the repoint alone leaves the convention exactly as fragile as it was: adds LINE_CITATIONS, pinning each citation to a phrase its range must still contain, plus a completeness check so a NEW citation cannot be added unpinned. Content-anchored rather than arithmetic, so a rewrap or a reworded neighbour stays green while real drift fails. scripts/approve-paperclip-api-digest.sh is unmodified. Co-Authored-By: Claude --- scripts/approve-paperclip-api-digest.test.js | 139 ++++++++++++++++++- 1 file changed, 132 insertions(+), 7 deletions(-) diff --git a/scripts/approve-paperclip-api-digest.test.js b/scripts/approve-paperclip-api-digest.test.js index a2f1a6c5000d..06ad4cc38e39 100644 --- a/scripts/approve-paperclip-api-digest.test.js +++ b/scripts/approve-paperclip-api-digest.test.js @@ -1656,7 +1656,7 @@ function runReleaseWrite({ stderrText, attempts = 3, writeSucceeds = false }) { ); // Heredoc body and terminator sit at column 0 deliberately; indenting them // here would make the generated bash unparseable rather than failing loudly. - // Flags match the shipping script's `set -euo pipefail` (`:82`) so what runs + // Flags match the shipping script's `set -euo pipefail` (`:86`) so what runs // here runs under production's error handling; all three cases behave // identically either way, so the fidelity is free. It is NOT extra mutation // coverage for the trailing-sleep guard below: bash exempts the left side of @@ -1723,7 +1723,7 @@ release_in_flight_lock`; : []; // stdout as well as stderr: the two loops' MESSAGING asymmetry -- this one // silent on both success and exhaustion, retire-only mode chatty -- is the - // other half of what `:755-759` defends at length against a parity fix, and + // other half of what `:759-763` defends at length against a parity fix, and // until both harnesses captured stdout it was asserted by nothing. Every // direction of that fix is now a failure: adding an `echo` here on either // path, or deleting retire-only's. @@ -1761,7 +1761,7 @@ test("a conflicting retirement write is still retried to exhaustion", () => { }); assert.equal(r.writes, 3, "a conflict must consume every attempt, not bail on the first"); assert.equal(r.status, 1, "exhausting the attempts still reports failure"); - // FLAT, and asserted by argument rather than by count: `:747-756` spends ten + // FLAT, and asserted by argument rather than by count: `:751-760` spends ten // lines defending this flatness against the "helpful parity fix" that would // spell it `sleep "$attempt"` like retire-only mode's, and until the stub // recorded its argument that defence was enforced by nothing. ["1","1"] is @@ -1777,7 +1777,7 @@ test("a conflicting retirement write is still retried to exhaustion", () => { /cannot retire the in-flight lock on/, "the non-retriable message must not fire on a retriable conflict", ); - // The EXHAUSTION half of the silence `:757-758` requires, which the success + // The EXHAUSTION half of the silence `:761-762` requires, which the success // test's assertion cannot reach: that comment forbids a chatty success *or // exhaustion*, and exhaustion is where the temptation is stronger, because // the loop has just given up and so has something to report. It reports it on @@ -1802,7 +1802,7 @@ test("a retirement write that succeeds returns at the first attempt, silently an // The SILENT half of the messaging asymmetry, and the more surprising one -- // a parity fix reading retire-only mode's three chatty success lines would // "correct" this loop by ADDING output, which is the direction the retire-only - // assertion below cannot catch. `:755-759` argues this silence is required, + // assertion below cannot catch. `:759-763` argues this silence is required, // not merely tolerated: the caller (`cleanup_on_exit`) prints the operator // guidance itself, and this loop only ever runs after a deploy has already // failed, so a success line here would bury the failure the cleanup exists to @@ -1963,7 +1963,7 @@ test("retire-only mode's conflicting write is still retried to exhaustion", () = // arguments rather than the attempt count is what makes the two loops' // pacing a tested difference instead of a commented one: with a discarding // stub, spelling this `sleep 1` -- or `sleep 0` -- left the suite green. - // 3s total, the number `:751-753` names as the cost retire-only mode can + // 3s total, the number `:755-757` names as the cost retire-only mode can // afford because it has an operator at a terminal and no grace-period clock. assert.deepEqual( r.sleeps, @@ -1992,7 +1992,7 @@ test("retire-only mode's succeeding write exits 0 at the first attempt, telling assert.equal(r.status, 0, "a retired lock must exit 0"); assert.deepEqual(r.sleeps, [], "the success path must not spend the backoff"); // The CHATTY half, and the output this mode exists to produce: an operator - // ran it by hand and the three lines at `:342-344` are the only report they + // ran it by hand and the three lines at `:346-348` are the only report they // get. Until the harness captured stdout, silencing all three "for parity // with `release_in_flight_lock`" left the suite green. assert.match( @@ -2030,3 +2030,128 @@ test("retire-only mode's write with no stderr still explains itself", () => { "the message must not end at the colon it promises to expand on", ); }); + +// The `:NNN` citations sprinkled through the comments above name lines in the +// shipping script, and until now nothing checked them. That is not hypothetical +// drift: when the BLO-31842 ReplicaSet work inserted four lines into the +// script's header comment, all seven citations in this file went stale by +// exactly +4 in one commit, silently. The worst of them then pointed a reader +// at the "nothing to retire" branch's `exit 0` instead of the three success +// lines the assertion beside it is actually about -- so the comment read as if +// it were pinning a completely different code path. +// +// Every OTHER cross-file reference here is already mechanical -- the function +// body by name via extractShellFunction(), the numeric defaults and limits by +// pattern via shellDefault()/shellLimit() -- which left the line citations as +// the one convention in this suite held together by nothing but care. This +// stack's whole thesis is that its comments are checkable, so they are checked. +// +// Pinned by the range's FIRST LINE, matched against that one line rather than +// searched for across the range. Searching a range is what a first attempt at +// this did, and it is too weak twice over: a citation ten lines wide still +// "contains" its anchor after a four-line shift, so it tolerates exactly the +// drift this exists to catch; and a spanning `[\s\S]*` pattern happily matches +// an earlier occurrence elsewhere in the script and then reports a confidently +// wrong line number -- the very defect being fixed. An exact first-line match +// admits neither. `contains` additionally holds the rest of the range to the +// content the citing comment claims is there. +const LINE_CITATIONS = [ + { + cite: "86", + startsWith: /^set -euo pipefail$/, + claim: "the shipping script's error-handling flags, mirrored by the harness", + }, + { + cite: "346-348", + startsWith: /^\s*echo "Retired the in-flight approval lock on /, + contains: /ring still lists that digest[\s\S]*without an out-of-band edit/, + claim: "retire-only mode's three operator-facing success lines", + }, + { + cite: "751-760", + startsWith: /^\s*# Flat, where retire-only mode backs off linearly/, + claim: "the ten lines defending the release loop's flat backoff", + }, + { + cite: "755-757", + startsWith: /^\s*# `trap 'exit 143' TERM`, so the runner's grace period/, + contains: /2s of total sleep beats 3s/, + claim: "the 3s figure retire-only mode can afford and the release loop cannot", + }, + { + cite: "759-763", + startsWith: /^\s*# Their MESSAGING differences are deliberate too/, + claim: "the messaging-asymmetry defence against a parity fix", + }, + { + cite: "761-762", + startsWith: /^\s*# guidance that retire-only mode prints itself, so a chatty success or/, + contains: /exhaustion here would bury/, + claim: "the clause forbidding a chatty success OR exhaustion", + }, +]; + +const scriptLines = script.split("\n"); + +function citedRange(cite) { + const [start, end = start] = cite.split("-").map(Number); + return scriptLines.slice(start - 1, end).join("\n"); +} + +test("every line citation in this file still points at the content it claims", () => { + // Reports EVERY drifted citation in one run, rather than failing on the + // first. Drift arrives all at once -- BLO-31842 shifted all seven of these by + // +4 in a single commit, changing nothing about them -- so failing one at a + // time would turn one mechanical repair into seven fix-and-rerun cycles. + const problems = []; + for (const { cite, startsWith, contains, claim } of LINE_CITATIONS) { + const start = Number(cite.split("-")[0]); + if (!startsWith.test(scriptLines[start - 1] ?? "")) { + // Report WHERE it moved to. Every matching line is listed rather than + // just the first: if the anchor is no longer unique, that ambiguity is + // itself the thing to fix, and silently naming one of several would be + // the same confidently-wrong answer this test exists to prevent. + const at = scriptLines + .map((line, i) => (startsWith.test(line) ? i + 1 : 0)) + .filter(Boolean); + problems.push( + at.length === 0 + ? ` :${cite} claims to point at ${claim}, which is no longer anywhere in the script -- it is dangling, so drop it or repoint it at whatever replaced that code` + : ` :${cite} no longer points at ${claim} -- that content now starts at :${at.join(" or :")}`, + ); + continue; + } + if (contains && !contains.test(citedRange(cite))) { + problems.push( + ` :${cite} starts in the right place, but the rest of the range no longer holds ${claim} -- the citing comment describes content that is not there any more`, + ); + } + } + assert.deepEqual( + problems, + [], + `line citations have drifted from the script:\n${problems.join("\n")}\nUpdate the comments that cite them, and this table, to match.`, + ); +}); + +test("no line citation can be added to this file without being pinned", () => { + // The completeness half, and the one that makes the pinning durable: without + // it a NEW citation is unpinned by default and the convention rots again from + // the next comment onward. Scans this file's own source, so the two sets are + // derived from the same place a reader looks. + const testSource = readFileSync(fileURLToPath(import.meta.url), "utf8"); + const found = new Set( + [...testSource.matchAll(/`:(\d+(?:-\d+)?)`/g)].map((m) => m[1]), + ); + const pinned = new Set(LINE_CITATIONS.map((c) => c.cite)); + assert.deepEqual( + [...found].filter((c) => !pinned.has(c)).sort(), + [], + "a `:NNN` citation was added without a LINE_CITATIONS entry -- add one naming the phrase it points at", + ); + assert.deepEqual( + [...pinned].filter((c) => !found.has(c)).sort(), + [], + "a LINE_CITATIONS entry no longer matches any citation in the file -- remove it rather than leaving it pinning nothing", + ); +});