From 6008482aabed04bcccc08e62e7d9c3cc96733700 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sun, 2 Aug 2026 12:11:35 -0700 Subject: [PATCH] fix(decisions): remove clock race from sweep-expiry tests (#10701) 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. > - The decisions service lets agents propose decisions with a TTL, and a sweep expires them. > - Four sweep tests create decisions that expire 5 ms in the future, then race the service's own clock read. > - On a loaded CI runner more than 5 ms routinely elapse before validation, so `create` itself rejects the decision and the test fails on unrelated PRs. > - This pull request makes the expiry deterministic: create with a comfortable future TTL, then move `expiresAt` into the past directly in the store. > - The benefit is that the decisions suite stops failing intermittently and stops blocking unrelated PRs. ## Linked Issues or Issue Description No open issue exists; the defect is described here following the bug template. **What happened?** `decisions-service.test.ts` fails intermittently in CI with `expiresAt must be within 30 days` in `bounds expiration work to the configured batch size` and `falls back to the default sweep batch size for invalid configuration`. The failure hits unrelated PRs — for example the `PR` workflow runs for #10699 failed three times on this suite while the same suite passes locally. **Steps to reproduce** 1. Run `pnpm vitest run src/__tests__/decisions-service.test.ts` on a machine under load (or add a ~10 ms delay inside `decisionService.create` before the expiry validation). 2. The test builds `expiresAt: new Date(Date.now() + 5)`; by the time `create` validates, `expiresAt.getTime() <= Date.now()` is true. 3. `create` throws `expiresAt must be within 30 days` (the past-expiry branch of the validator) and the test fails before the sweep runs. **Expected behavior** The sweep tests exercise expiry deterministically and never depend on fewer than 5 ms elapsing between two clock reads in different modules. **Paperclip version** master (`717684ad8f`); the tests landed with the decisions desk workflow in #10672. **Deployment mode** Not deployment-specific — CI and local test runs. ## What Changed - `server/src/__tests__/decisions-service.test.ts`: added two helpers — `nearFutureExpiry()` (a 60 s TTL that passes validation with a wide margin) and `expireDecisionNow(id)` (moves the stored `expiresAt` into the past). The four affected tests create decisions with the future TTL, force-expire them through the store, and drop the 10 ms sleeps. The sweep observes the same expired state as before with no scheduler-timing dependence. ## Verification - `cd server && pnpm vitest run src/__tests__/decisions-service.test.ts` — five consecutive local runs, 31/31 passing each. - No production code changed; the diff is test-only. ## Risks - Low risk: test-only change. The force-expire helper writes the store directly, which is the same technique other TTL suites use to avoid sleeping through real time. ## Model Used Claude Fable 5 (`claude-fable-5`, Anthropic) via Claude Code — extended thinking, tool use; diagnosis, fix, and verification runs. ## 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 --- .../src/__tests__/decisions-service.test.ts | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/server/src/__tests__/decisions-service.test.ts b/server/src/__tests__/decisions-service.test.ts index 002e900fe4..c356c7d09d 100644 --- a/server/src/__tests__/decisions-service.test.ts +++ b/server/src/__tests__/decisions-service.test.ts @@ -88,6 +88,16 @@ describePg("decisionService", () => { ...extra, }); + // Make an existing decision TTL-expired for the next sweep. Creating a + // decision that is already expired is impossible (create rejects a past + // expiresAt), and creating one that expires a few milliseconds later races + // the service's own clock read — under CI load the create itself can fail + // with "expiresAt must be within 30 days". Create with a comfortable future + // expiry instead, then move expiresAt into the past directly in the store. + const nearFutureExpiry = () => new Date(Date.now() + 60_000); + const expireDecisionNow = (id: string) => + db.update(decisions).set({ expiresAt: new Date(Date.now() - 1_000) }).where(eq(decisions.id, id)); + it("returns the existing decision for concurrent idempotent creates", async () => { const input = { companyId, actor: agentActor(), agentId, runId, title: "Same?", body: "Body", idempotencyKey: "concurrent-create", @@ -501,27 +511,29 @@ describePg("decisionService", () => { it("bounds expiration work to the configured batch size", async () => { process.env.PAPERCLIP_DECISIONS_SWEEP_BATCH_SIZE = "1"; - await createCommentDecision("lenient", { idempotencyKey: "batch-1", expiresAt: new Date(Date.now() + 5) }); - await createCommentDecision("lenient", { idempotencyKey: "batch-2", expiresAt: new Date(Date.now() + 5) }); - await new Promise((resolve) => setTimeout(resolve, 10)); + const first = await createCommentDecision("lenient", { idempotencyKey: "batch-1", expiresAt: nearFutureExpiry() }); + const second = await createCommentDecision("lenient", { idempotencyKey: "batch-2", expiresAt: nearFutureExpiry() }); + await expireDecisionNow(first.id); + await expireDecisionNow(second.id); expect((await service().sweepExpired()).expired).toBe(1); expect((await service().sweepExpired()).expired).toBe(1); }); it("falls back to the default sweep batch size for invalid configuration", async () => { process.env.PAPERCLIP_DECISIONS_SWEEP_BATCH_SIZE = "not-a-number"; - await createCommentDecision("lenient", { idempotencyKey: "invalid-batch-1", expiresAt: new Date(Date.now() + 5) }); - await createCommentDecision("lenient", { idempotencyKey: "invalid-batch-2", expiresAt: new Date(Date.now() + 5) }); - await new Promise((resolve) => setTimeout(resolve, 10)); + const first = await createCommentDecision("lenient", { idempotencyKey: "invalid-batch-1", expiresAt: nearFutureExpiry() }); + const second = await createCommentDecision("lenient", { idempotencyKey: "invalid-batch-2", expiresAt: nearFutureExpiry() }); + await expireDecisionNow(first.id); + await expireDecisionNow(second.id); await expect(service().sweepExpired()).resolves.toMatchObject({ expired: 2 }); }); it("expires TTL and target-gone decisions and wakes the origin agent", async () => { - const ttl = await createCommentDecision("lenient", { expiresAt: new Date(Date.now() + 5) }); + const ttl = await createCommentDecision("lenient", { expiresAt: nearFutureExpiry() }); const gone = await createCommentDecision("strict", { idempotencyKey: "gone" }); await db.update(issues).set({ status: "cancelled" }).where(eq(issues.id, targetIssueId)); - await new Promise((resolve) => setTimeout(resolve, 10)); + await expireDecisionNow(ttl.id); expect((await service().sweepExpired()).expired).toBe(2); const rows = await db.select().from(decisions); expect(rows.find((row) => row.id === ttl.id)?.metadata).toMatchObject({ expiredReason: "ttl" }); @@ -542,14 +554,14 @@ describePg("decisionService", () => { companyId, actor: agentActor(), agentId, runId, ruleKey: "routing.assign", title: "Assign again?", body: "Body", options: [{ id: "assign", label: "Assign", effects: [] }, { id: "skip", label: "Skip", effects: [] }], }); - await service().create({ + const stale = await service().create({ companyId, actor: agentActor(), agentId, runId, ruleKey: "cleanup.stale", title: "Clean up?", body: "Body", - options: [{ id: "clean", label: "Clean", effects: [] }], expiresAt: new Date(Date.now() + 5), + options: [{ id: "clean", label: "Clean", effects: [] }], expiresAt: nearFutureExpiry(), }); await service().decide({ id: accepted.id, optionId: "assign", decidedByUserId, userActor: boardActor() }); await service().decide({ id: acceptedAgain.id, optionId: "assign", decidedByUserId, userActor: boardActor() }); await service().dismiss(rejected.id, decidedByUserId, boardActor(), "Not this time"); - await new Promise((resolve) => setTimeout(resolve, 10)); + await expireDecisionNow(stale.id); await service().sweepExpired(); const stats = await service().stats(companyId, { originAgentId: agentId });