From 8f34d0f726b237de4e1081b508fe27d51fe2b596 Mon Sep 17 00:00:00 2001 From: Harold Kim Date: Tue, 28 Jul 2026 19:17:54 +0000 Subject: [PATCH] test(e2e): narrow signoff-policy retry to run-lock 409 conflicts only The retry loop re-issued the request on any 409 while the issue was still assigned to the acting agent, which could mask genuine lifecycle rejections (paused project, unresolved blockers, or a non-participant's "checked out by another agent") behind a transient-looking retry. Match on the response body instead of the bare status: only "Issue run ownership conflict" (a lost PATCH run-lock race) and "Issue checkout conflict" (a lost checkout race) are retried under the winning run id. Every other 409, and the missing-comment 400, is returned untouched so the negative paths keep failing for the right reason. Co-authored-by: Paperclip --- tests/e2e/signoff-policy.spec.ts | 60 +++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/tests/e2e/signoff-policy.spec.ts b/tests/e2e/signoff-policy.spec.ts index 1b43c73ca2b..291856eb712 100644 --- a/tests/e2e/signoff-policy.spec.ts +++ b/tests/e2e/signoff-policy.spec.ts @@ -75,6 +75,39 @@ async function getIssueRunLockState(board: APIRequestContext, issueId: string): }; } +/** + * A lost run-lock race surfaces as one of two 409 conflicts, depending on which + * endpoint lost: a PATCH under a stale run id answers "Issue run ownership + * conflict", while a checkout that lost the lock answers "Issue checkout + * conflict". Only these two are transient and safe to retry under the winning + * run id. + * + * Every other 409 is a genuine outcome the suite's negative paths must observe + * unretried — a non-participant's "Issue is checked out by another agent" + * rejection, a paused-project refusal, or a lifecycle block. (The missing + * comment case is a 400, so it never enters a retry path.) Matching on the + * error message, rather than the bare 409 status, keeps those failing for the + * right reason. + */ +const RUN_LOCK_CONFLICT_ERRORS = new Set([ + "Issue run ownership conflict", + "Issue checkout conflict", +]); + +async function isRunLockConflictResponse( + res: Awaited>, +): Promise { + if (res.status() !== 409) return false; + let body: unknown; + try { + body = await res.json(); + } catch { + return false; + } + const error = (body as { error?: unknown } | null)?.error; + return typeof error === "string" && RUN_LOCK_CONFLICT_ERRORS.has(error); +} + async function retryAgentPatchWithCurrentLockOnConflict( board: APIRequestContext, agent: AgentAuth, @@ -82,7 +115,7 @@ async function retryAgentPatchWithCurrentLockOnConflict( failedRes: Awaited>, patchData: Record, ) { - if (failedRes.status() !== 409) return failedRes; + if (!(await isRunLockConflictResponse(failedRes))) return failedRes; const issueRunLock = await getIssueRunLockState(board, issueId); if (issueRunLock.assigneeAgentId !== agent.agentId) return failedRes; @@ -100,17 +133,20 @@ async function retryAgentPatchWithCurrentLockOnConflict( * PATCH an issue as an agent, using a freshly invoked heartbeat run. * * Invoking a heartbeat starts a background run that races this PATCH for the - * issue's run-lock: the background run may check the issue out (flipping it to - * `in_progress` under its own run id) a moment before — or after — this PATCH - * lands, and the server answers the loser with a 409 ("Issue is checked out by - * another agent"). With `retries: 0` / `workers: 1` a single transient 409 - * fails the whole shard, so we retry a run-lock 409 under the issue's *current* - * lock, bounded by escalating backoff to cover the race window. + * issue's run-lock: the background run may check the issue out (flipping its + * checkout run id under its own id) a moment before — or after — this PATCH + * lands, and the server answers the loser with a run-ownership 409 ("Issue run + * ownership conflict"). With `retries: 0` / `workers: 1` a single transient 409 + * fails the whole shard, so we retry that specific conflict under the issue's + * *current* lock, bounded by escalating backoff to cover the race window. * * The retry is intentionally narrow so the suite's negative paths keep failing * for the right reason: - * - It only re-PATCHes while the issue is still assigned to the acting agent, - * so a non-participant's genuine 409/403 rejection is returned untouched. + * - It only retries the run-lock conflict itself (see + * `isRunLockConflictResponse`); any other 409 — a non-participant's + * "checked out by another agent" rejection, a lifecycle block — is returned + * untouched for the caller to assert on. + * - It only re-PATCHes while the issue is still assigned to the acting agent. * - It re-PATCHes under the winning run id (or the invoked run id once the * background run has released its lock), so a real validation error such as * the missing-comment 400 surfaces instead of a masking transient 409. @@ -134,11 +170,11 @@ async function agentPatch( }); let res = await patchWith(runId); - for (let attempt = 1; attempt < maxAttempts && res.status() === 409; attempt++) { + for (let attempt = 1; attempt < maxAttempts && (await isRunLockConflictResponse(res)); attempt++) { await new Promise((resolve) => setTimeout(resolve, Math.min(maxBackoffMs, backoffMs * 2 ** (attempt - 1)))); const issueRunLock = await getIssueRunLockState(board, issueId); - // A 409 on an issue no longer assigned to us is a genuine rejection, not a - // run-lock race — leave it for the caller to assert on. + // A run-lock conflict on an issue no longer assigned to us is a genuine + // rejection, not a race — leave it for the caller to assert on. if (issueRunLock.assigneeAgentId !== agent.agentId) break; const retryRunId = issueRunLock.checkoutRunId ?? issueRunLock.executionRunId ?? runId; res = await patchWith(retryRunId);