test(server): settle three contention flakes that killed canary verifies (#13186)
## 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
This commit is contained in:
parent
d1ba17eeca
commit
2585ed0550
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
Loading…
Reference in New Issue