From 2585ed0550c2596d14853d16855824971d1c856d Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 10 Sep 2026 17:24:10 -0700 Subject: [PATCH] test(server): settle three contention flakes that killed canary verifies (#13186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Every master push publishes a canary through release-verify; the staging fleet and the nightly/beta/stable chain start from those canaries > - The server suite grew substantially on 2026-09-10 and now runs under real contention in CI, where three tests assert timing properties that only hold on an idle machine > - Each of the three failed a release-verify canary run that day (runs 34497348802 and 34517515849), and together with the shard timeouts (#13185) they kept any canary from publishing after 18:50 UTC > - This pull request makes the three assertions contention-tolerant without weakening the invariants they prove > - The benefit is a canary lane whose verdicts reflect the code, not the load on the runner ## Linked Issues or Issue Description **What happened?** Three server tests failed release-verify canary runs on 2026-09-10 under CI load: 1. `chat-channels.integration.test.ts › returns a retryable webhook failure when the delivery insert fails before durable receipt` — the duplicate-redelivery request drew the retryable 503 instead of an immediate 200 (run 34517515849). 2. `chat-channels.integration.test.ts › returns ephemeral guidance for exact Slack controls in channels without creating tasks or actions` — a `provider_effect` row was read before its async settlement reached `processed` (run 34497348802). 3. `runner-connection-eval-fixtures.test.ts › resets paired attempts…` — the fixture's `TRUNCATE companies CASCADE` was chosen as a deadlock victim (40P01) against the helper app's own background sweeps (run 34497348802). **Expected behavior** Verify runs fail only for real regressions. A momentary-contention 503 on a duplicate redelivery, an in-flight settlement row, and a deadlock-victim reset are all recoverable states the code handles by design. **Steps to reproduce** Run the three tests under a loaded 3-shard release-verify split; the timing assertions flake. Under `pr-trusted`'s lighter shards they usually pass, which is why the PRs that introduced them were green. **Paperclip version or commit** `master` at `d1ba17eec`. ## What Changed - The duplicate-redelivery assertion retries on 503 the way Slack itself would (bounded, 250 ms apart), then asserts the 200 and the unchanged dedup invariants: duplicate count increments, still exactly one issue. - The channel-controls settlement read is wrapped in a bounded `vi.waitFor`, the same pattern the file's durable-receipt paths already use. - The runner eval fixture retries its TRUNCATE on Postgres error 40P01, bounded at five attempts, and rethrows anything else. ## Verification - All three run green locally: the two chat tests via `-t` filters, the eval fixtures file in full (6 tests). - Each change is assertion-shape only; no product code is touched. - Observation for a follow-up, not this PR: the chat integration file costs ~15 s transform + ~28 s import per vitest worker before any test executes — splitting it would give back real shard time. ## Risks - Low risk: the retries and waits are bounded, so a genuine regression (permanent 503, settlement that never lands, persistent deadlock) still fails within the same timeouts as before. ## Model Used - Claude (Anthropic), model ID `claude-fable-5` (Claude Fable 5), extended thinking, tool use via Claude Code CLI. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- .../chat-channels.integration.test.ts | 51 +++++++++++++------ .../__tests__/helpers/runner-api-server.ts | 20 +++++++- 2 files changed, 55 insertions(+), 16 deletions(-) diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 71e5e2f2eb..b565ed645d 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -20225,7 +20225,21 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { "@maya prove durable receipt", ); expect(JSON.stringify(timingEvents)).not.toContain(signature); - const acceptedRedelivery = await observedRequest(true); + // A duplicate redelivery can momentarily contend with the first + // delivery's settlement and draw the retryable 503 — that is the + // webhook contract (Slack re-sends, the dedup path keeps it + // idempotent), not a defect. Retry the way the provider would + // instead of asserting an accidental no-contention property; this + // exact assertion drew a 503 under CI shard load on 2026-09-10. + let acceptedRedelivery = await observedRequest(true); + for ( + let attempt = 0; + acceptedRedelivery.status === 503 && attempt < 20; + attempt += 1 + ) { + await new Promise((resolve) => setTimeout(resolve, 250)); + acceptedRedelivery = await observedRequest(true); + } expect(acceptedRedelivery.status).toBe(200); await vi.waitFor(async () => { const [delivery] = await db @@ -48459,21 +48473,28 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { .from(issues) .where(eq(issues.companyId, fixture.companyId)), ).toHaveLength(0); - expect( - await db - .select() - .from(chatActions) - .where( - and( - eq(chatActions.endpointId, endpoint.id), - eq(chatActions.kind, "provider_effect"), + // The ephemeral post is observable before its provider_effect row + // settles, so under suite load the third row can still be + // mid-settlement when the mock resolves (drew a not-yet-processed row + // in CI on 2026-09-10). Wait for the bookkeeping, bounded, like the + // durable-receipt paths above do. + await vi.waitFor(async () => { + expect( + await db + .select() + .from(chatActions) + .where( + and( + eq(chatActions.endpointId, endpoint.id), + eq(chatActions.kind, "provider_effect"), + ), ), - ), - ).toEqual([ - expect.objectContaining({ kind: "provider_effect", status: "processed" }), - expect.objectContaining({ kind: "provider_effect", status: "processed" }), - expect.objectContaining({ kind: "provider_effect", status: "processed" }), - ]); + ).toEqual([ + expect.objectContaining({ kind: "provider_effect", status: "processed" }), + expect.objectContaining({ kind: "provider_effect", status: "processed" }), + expect.objectContaining({ kind: "provider_effect", status: "processed" }), + ]); + }); }); it("keeps Telegram start and unknown commands as terse guidance without creating work", async () => { diff --git a/server/src/__tests__/helpers/runner-api-server.ts b/server/src/__tests__/helpers/runner-api-server.ts index f0eee5cad2..a071b36fe6 100644 --- a/server/src/__tests__/helpers/runner-api-server.ts +++ b/server/src/__tests__/helpers/runner-api-server.ts @@ -46,7 +46,25 @@ export async function startRunnerApiTestServer() { if (options.connectionScenario !== undefined && !CONNECTION_SCENARIOS.includes(options.connectionScenario)) throw new Error(`Unknown connection eval scenario: ${String(options.connectionScenario)}`); // This DB is created inside this helper, never supplied by a caller. Paid // paired runs reset it between attempts so modeled IDs and data match. - if (options.reset) await db.execute(sql`TRUNCATE companies CASCADE`); + // The helper's own app runs background sweeps against this DB, and one + // can hold row locks when the reset fires; Postgres then picks a + // deadlock victim (observed against TRUNCATE in CI on 2026-09-10). The + // loser's transaction rolls back the moment it is chosen, so a short + // bounded retry makes the reset deterministic instead of flaky. + if (options.reset) { + for (let attempt = 0; ; attempt += 1) { + try { + await db.execute(sql`TRUNCATE companies CASCADE`); + break; + } catch (error) { + const code = + (error as { code?: string }).code ?? + (error as { cause?: { code?: string } }).cause?.code; + if (attempt >= 4 || code !== "40P01") throw error; + await new Promise((resolve) => setTimeout(resolve, 100)); + } + } + } const id = (key: string) => { if (!options.reset) return randomUUID(); const hex = createHash("sha256").update(`runner-api-fixture:${key}`).digest("hex");