fix(decisions): remove clock race from sweep-expiry tests (#10701)
## 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
This commit is contained in:
parent
2ffebd4836
commit
6008482aab
|
|
@ -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 });
|
||||
|
|
|
|||
Loading…
Reference in New Issue