fix(server): make the wake-claim lease test deterministic (#12331)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The server test suite checks question response delivery and wake claims > - One test used wall-clock time and could start a second delivery under load > - The second delivery reused one promise resolver and could hang for 15 seconds > - This pull request uses the injected clock and one resolver for each wakeup > - The benefit is a deterministic test that fails at once if a second wakeup occurs ## Linked Issues or Issue Description **What happened?** The wake-claim lease test slept for 70 milliseconds before it ran the pending sweep. Under load, the lease could look stale during that interval. The sweep then started a second delivery. The second delivery reused one promise resolver, so the test hung until the 15-second suite timeout. **Expected behavior** The test must control the time used by the service. One wakeup must use one resolver. An unexpected second wakeup must fail at once. **Steps to reproduce** 1. Run the question response delivery test under CPU load. 2. Let the test sleep before the pending sweep. 3. Observe that a second delivery can start and the test can reach the 15-second timeout. **Paperclip version or commit** Commit `0dd735e53a5cc9f6d3395834b826dfb0b1da2ea9`. **Deployment mode** Built from source with the server test suite. **Installation method** Built from source. **Agent adapter(s) involved** Not adapter-specific (core test issue). **Database mode** Not database-related. **Additional context** The change affects one test file. It does not change production source code. ## What Changed - Drive the test with the service's injected clock. - Give each wakeup call its own promise resolver. - Assert that lease renewal advances the last attempt time. - Assert that the wakeup runs one time and the attempt count stays at 1. ## Verification - Run `pnpm exec vitest run server/src/services/__tests__/question-response-delivery.test.ts`. - The changed file reports 29 passing tests. - Run the changed file 25 times, including 5 runs under CPU load. ## Risks Low risk. The change affects one test file and test setup only. It does not change production behavior. ## Model Used OpenAI GPT-5, exact runtime model ID supplied by the Paperclip agent environment, tool use and code review assistance. ## 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 - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
parent
bdd8f1bedb
commit
666f5a6e69
|
|
@ -551,23 +551,56 @@ describeEmbeddedPostgres("question response delivery", () => {
|
|||
|
||||
it("keeps a long wake claim leased while the side effect is active", async () => {
|
||||
const seeded = await seed({ sourceStatus: "running" });
|
||||
let releaseWake!: (value: null) => void;
|
||||
const wakeup = vi.fn(() => new Promise<null>((resolve) => {
|
||||
releaseWake = resolve;
|
||||
}));
|
||||
// Each call gets its own resolver. A second, unexpected call fails at
|
||||
// once instead of sharing one resolver with the first call and hanging.
|
||||
const wakeResolvers: Array<(value: null) => void> = [];
|
||||
const wakeup = vi.fn(() => {
|
||||
if (wakeResolvers.length > 0) {
|
||||
throw new Error(
|
||||
"wakeup was invoked a second time for the same claim; a re-entrant delivery must fail at once, not hang",
|
||||
);
|
||||
}
|
||||
return new Promise<null>((resolve) => {
|
||||
wakeResolvers.push(resolve);
|
||||
});
|
||||
});
|
||||
|
||||
// The renewal timer and the sweep both read time from this injected
|
||||
// clock. The test moves the clock by hand, so the assertions below do
|
||||
// not depend on the speed of a database round trip or a timer callback.
|
||||
let clock = new Date("2026-01-01T00:00:00.000Z");
|
||||
const service = questionResponseDeliveryService(db, {
|
||||
heartbeat: { wakeup } as never,
|
||||
steer: vi.fn(),
|
||||
now: () => clock,
|
||||
claimStaleMs: 40,
|
||||
claimRefreshMs: 5,
|
||||
});
|
||||
|
||||
const deliveryPromise = service.deliver(seeded.interaction.id);
|
||||
await vi.waitFor(() => expect(wakeup).toHaveBeenCalledTimes(1));
|
||||
await new Promise((resolve) => setTimeout(resolve, 70));
|
||||
const [claimedRow] = await db.select().from(issueQuestionResponseDeliveries)
|
||||
.where(eq(issueQuestionResponseDeliveries.interactionId, seeded.interaction.id));
|
||||
const claimedAt = claimedRow!.lastAttemptAt!.getTime();
|
||||
|
||||
// Move the clock past claimStaleMs, then wait for a real renewal tick to
|
||||
// pick it up. This proves the renewal ran. It does not only assert that
|
||||
// a fixed real-time sleep was long enough.
|
||||
clock = new Date(clock.getTime() + 1000);
|
||||
await vi.waitFor(async () => {
|
||||
const [row] = await db.select().from(issueQuestionResponseDeliveries)
|
||||
.where(eq(issueQuestionResponseDeliveries.interactionId, seeded.interaction.id));
|
||||
expect(row?.lastAttemptAt?.getTime()).toBeGreaterThan(claimedAt);
|
||||
});
|
||||
|
||||
await expect(service.sweepPending()).resolves.toMatchObject({ scanned: 0 });
|
||||
releaseWake(null);
|
||||
wakeResolvers[0]!(null);
|
||||
await expect(deliveryPromise).resolves.toBeNull();
|
||||
|
||||
expect(wakeup).toHaveBeenCalledTimes(1);
|
||||
const [delivery] = await db.select().from(issueQuestionResponseDeliveries)
|
||||
.where(eq(issueQuestionResponseDeliveries.interactionId, seeded.interaction.id));
|
||||
expect(delivery).toMatchObject({ status: "pending", attemptCount: 1 });
|
||||
});
|
||||
|
||||
it("fences a stale worker after a newer claim generation takes ownership", async () => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue