From bc26b7738fbc942fe9f64940d4613a21c827b16e Mon Sep 17 00:00:00 2001 From: freeqaz Date: Fri, 23 Oct 2026 15:15:22 +0000 Subject: [PATCH 1/2] fix(test): accept a losing owner that retires the replaced token Two owners race one maintenance pass over a family whose rejected token is still current. The winner claims, issues and commits the replacement, and that commit moves the rejected token into retirement custody. A losing pass that reaches its retirement step after the commit revokes that token and reports `retired`; one that reaches it earlier reports `idle` and the next pass retires it. Both are correct, but the test only accepted `idle`. Add a shared `raceMaintenance` helper that asserts exactly one owner commits and returns the losers, use it in both prepared GitHub race tests, and in the token-service case accept `idle` or `retired` while counting the rejected token's revocations: one only when the loser retired it, exactly one retirement by the next pass, and none replayed by later passes. --- tests/helpers/github-metadata-exchange.mjs | 11 ++++++++ ...ostgres-prepared-github-lifecycle.test.mjs | 11 +++++--- .../postgres-prepared-token-service.test.ts | 26 ++++++++++++++----- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/tests/helpers/github-metadata-exchange.mjs b/tests/helpers/github-metadata-exchange.mjs index 8d65e3399f..c21e0c7e82 100644 --- a/tests/helpers/github-metadata-exchange.mjs +++ b/tests/helpers/github-metadata-exchange.mjs @@ -75,3 +75,14 @@ export async function dispatchMetadata( } return protectedResponse; } + +/** Runs one maintenance pass on every owner at once over the same durable family. Exactly one + * owner claims the due family and commits; returns the other owners' results, sorted. A loser + * finds no due family, so its pass reports only its own retirement step: `idle`, or `retired` + * when it ran after the winner's commit moved the superseded token into retirement custody. */ +export async function raceMaintenance(owners, signal) { + const results = await Promise.all(owners.map((owner) => owner.maintain(signal))); + const committed = results.filter((result) => result === "committed"); + assert.equal(committed.length, 1, `exactly one owner must commit: ${results.join(", ")}`); + return results.filter((result) => result !== "committed").sort(); +} diff --git a/tests/integration/postgres-prepared-github-lifecycle.test.mjs b/tests/integration/postgres-prepared-github-lifecycle.test.mjs index 4fecf5cfd5..99941a4909 100644 --- a/tests/integration/postgres-prepared-github-lifecycle.test.mjs +++ b/tests/integration/postgres-prepared-github-lifecycle.test.mjs @@ -6,7 +6,11 @@ import { join } from "node:path"; import test from "node:test"; import pg from "pg"; import { setTimeout as delay } from "node:timers/promises"; -import { captureMetadata, dispatchMetadata } from "../helpers/github-metadata-exchange.mjs"; +import { + captureMetadata, + dispatchMetadata, + raceMaintenance, +} from "../helpers/github-metadata-exchange.mjs"; import { postgresEgressSendReceipts } from "../../packages/occ/src/state/egress-send-receipts.ts"; import { bindRuntimeSource } from "../helpers/postgres-protected-scenarios.mjs"; import { postgresApp } from "../helpers/postgres-egress-app.mjs"; @@ -53,9 +57,10 @@ test( // Two workers race over a real family. Only the durable claim holder reaches the issuer. const competitor = createOwner(); await competitor.connect(f.pool); - const results = await Promise.all([lifecycle.maintain(signal), competitor.maintain(signal)]); + // The family is cold, so the loser has nothing to retire. + const losers = await raceMaintenance([lifecycle, competitor], signal); await competitor.close(); - assert.deepEqual(results.sort(), ["committed", "idle"]); + assert.deepEqual(losers, ["idle"]); const old = await lifecycle.readWarm(source, readWindow()); assert.equal(old.ownerEpoch, 1); assert.deepEqual(gh.issuesOfTokens[0].permissions, { metadata: "read" }); diff --git a/tests/integration/postgres-prepared-token-service.test.ts b/tests/integration/postgres-prepared-token-service.test.ts index a08d9d9e84..cd954501d7 100644 --- a/tests/integration/postgres-prepared-token-service.test.ts +++ b/tests/integration/postgres-prepared-token-service.test.ts @@ -38,7 +38,11 @@ import { import { bindRuntimeSource } from "../helpers/postgres-protected-scenarios.mjs"; import { postgresApp } from "../helpers/postgres-egress-app.mjs"; import { requiresPostgres } from "../helpers/postgres-database.mjs"; -import { captureMetadata, dispatchMetadata } from "../helpers/github-metadata-exchange.mjs"; +import { + captureMetadata, + dispatchMetadata, + raceMaintenance, +} from "../helpers/github-metadata-exchange.mjs"; import { staticEgressFixture } from "../helpers/static-egress.mjs"; import { temporaryDirectory } from "../fixtures/repository-credentials/process.mjs"; import { @@ -632,11 +636,19 @@ test( // Advance only the polling schedule, not token validity. Separate owners must coalesce // against the same durable family even though the rejected token is still fresh. await due(); - const results = await Promise.all([ - restarted.maintain(t.signal), - competitor.maintain(t.signal), - ]); - assert.deepEqual(results.sort(), ["committed", "idle"]); + // Revocations of the rejected token: the test's own revocation of `late`, then retirement. + const [{ index: rejected }] = gh.issuesOfTokens; + const revocations = () => + gh.trace.filter( + ({ method, target, tokenIndex }) => + method === "DELETE" && target === "/installation/token" && tokenIndex === rejected, + ).length; + assert.equal(revocations(), 1); + // The commit moves the rejected token into retirement custody. A loser whose pass reaches + // its retirement step after that commit retires it (`retired`); otherwise the next pass does. + const [loser] = await raceMaintenance([restarted, competitor], t.signal); + assert(loser === "idle" || loser === "retired", `unexpected losing pass: ${loser}`); + assert.equal(revocations(), loser === "retired" ? 2 : 1); const replacement = await restarted.readWarm(source, window()); assert(replacement); assert.notEqual(replacement.materialVersion, old.materialVersion); @@ -646,6 +658,7 @@ test( assert.equal(await restartedReceipts.reject(late.sender, late, late.material), "fenced"); await due(); assert.equal(await restarted.maintain(t.signal), "ready"); + assert.equal(revocations(), 2, "exactly one pass retires the rejected token"); assert.equal( (await restarted.readWarm(source, window()))?.materialVersion, replacement.materialVersion, @@ -672,6 +685,7 @@ test( assert.equal(await competitor.maintain(t.signal), "blocked"); assert.equal(gh.issuesOfTokens.length, 3, "rejection cannot replay uncertain issuance"); assert.equal(await competitor.readWarm(source, window()), undefined); + assert.equal(revocations(), 2, "later passes never replay the rejected token's retirement"); assert.deepEqual(gh.errors, []); }, ); From a3ecb7963d2e81ef7c8d32cca18f8d0da0291169 Mon Sep 17 00:00:00 2001 From: freeqaz Date: Fri, 23 Oct 2026 15:27:12 +0000 Subject: [PATCH 2/2] test: keep the race helper comment generic and check the replacement is never retired --- tests/helpers/github-metadata-exchange.mjs | 5 +++-- tests/integration/postgres-prepared-token-service.test.ts | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/helpers/github-metadata-exchange.mjs b/tests/helpers/github-metadata-exchange.mjs index c21e0c7e82..8deb05e826 100644 --- a/tests/helpers/github-metadata-exchange.mjs +++ b/tests/helpers/github-metadata-exchange.mjs @@ -3,6 +3,7 @@ import { get } from "node:https"; // The test plays the enrolled supervisor. OCE owns the genuine signed installation // exchange, private response acceptance and material/dispatch/response decisions. +// `raceMaintenance` is the prepared GitHub owner race these tests share. export function captureMetadata(f, changed = {}) { const captured = f.capture(); const [route] = f.installation.plan.routes; @@ -78,8 +79,8 @@ export async function dispatchMetadata( /** Runs one maintenance pass on every owner at once over the same durable family. Exactly one * owner claims the due family and commits; returns the other owners' results, sorted. A loser - * finds no due family, so its pass reports only its own retirement step: `idle`, or `retired` - * when it ran after the winner's commit moved the superseded token into retirement custody. */ + * claims nothing, so it reports only its own retirement step: `idle`, or `retired` when a + * superseded token was already in retirement custody by then. */ export async function raceMaintenance(owners, signal) { const results = await Promise.all(owners.map((owner) => owner.maintain(signal))); const committed = results.filter((result) => result === "committed"); diff --git a/tests/integration/postgres-prepared-token-service.test.ts b/tests/integration/postgres-prepared-token-service.test.ts index cd954501d7..d3ad5710e2 100644 --- a/tests/integration/postgres-prepared-token-service.test.ts +++ b/tests/integration/postgres-prepared-token-service.test.ts @@ -638,10 +638,10 @@ test( await due(); // Revocations of the rejected token: the test's own revocation of `late`, then retirement. const [{ index: rejected }] = gh.issuesOfTokens; - const revocations = () => + const revocations = (index = rejected) => gh.trace.filter( ({ method, target, tokenIndex }) => - method === "DELETE" && target === "/installation/token" && tokenIndex === rejected, + method === "DELETE" && target === "/installation/token" && tokenIndex === index, ).length; assert.equal(revocations(), 1); // The commit moves the rejected token into retirement custody. A loser whose pass reaches @@ -675,6 +675,7 @@ test( ).length, 2, ); + assert.equal(revocations(gh.issuesOfTokens[1]!.index), 0, "never retire the replacement"); const next = await dispatched(true); gh.disconnectAfterMutation("POST", `/app/installations/${fixtureInstallationId}/access_tokens`); await due();