From 333abdd2c26ed68ab79b33fbfe082c1355c095ae Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Thu, 3 Sep 2026 17:55:12 -0700 Subject: [PATCH] test(plugin-worker): remove the wall-clock race from the duplex buffered-replay tests (#12799) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The plugin worker manager runs agent plugin workers through duplex channels. > - The duplex buffered-replay tests check data that arrives before a listener attaches. > - The tests used a fixed 60 ms sleep as the barrier for worker output. > - Worker startup and output latency can exceed that delay under load. > - This pull request uses a worker exit frame as a deterministic barrier. > - The benefit is stable test results without a product code change. ## Linked Issues or Issue Description **What happened?** The duplex buffered-replay tests used a fixed 60 ms sleep before they attached a data listener. Under load, worker output could arrive after the sleep. The tests then saw a partial buffer and failed. **Expected behavior** The tests must wait until the worker sends all three data frames before they inspect the pre-bind buffer. **Steps to reproduce** 1. Run npx vitest run src/__tests__/plugin-worker-manager-duplex.test.ts in the server package. 2. Add a 200 ms or 800 ms delay to the worker fixture emit path. 3. Repeat the test run and observe the old fixed-sleep barrier fail intermittently. **Paperclip version or commit** b773f0f2e29f628fa09442e8a3f1431c418d1ee7 **Deployment mode** Built from source. This change affects tests only. ## What Changed - Replace the fixed sleep in both buffered-replay tests with an exit-frame barrier. - Write the three data frames and the exit frame in one worker output write. - Wait for the session to settle before the tests attach listeners. - Keep the non-batch buffer-then-drain path and the throwing-listener behavior. - Remove the retry wrapper from the first test because the drain runs synchronously. ## Verification - Run npx vitest run src/__tests__/plugin-worker-manager-duplex.test.ts in the server package. - The full file passes 35 of 35 tests. - Run the full file 15 times. All 15 runs pass. - Test the new barrier with 200 ms and 800 ms worker-output delays. Both tests pass. - The server type check still reports 71 pre-existing errors in native-runtime and paperclip-runner. No new error appears in the changed test file. - Search GitHub for duplicate or related public issues and pull requests. No duplicate open item exists. - Check ROADMAP.md. This test-only fix does not duplicate planned core work. ## Risks - This change affects test synchronization only. - The test could become invalid if the worker stops sending the exit frame. The session wait then fails instead of hiding the problem behind a clock delay. - No product code, database schema, or runtime behavior changes. ## Model Used OpenAI GPT-5, exact model ID gpt-5, API model with code execution and tool use. The model used a 1M-token context window. No extended reasoning mode was specified. ## 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 described the issue in-PR following the bug report template - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] My branch name describes the change 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 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 --- .../plugin-worker-manager-duplex.test.ts | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/server/src/__tests__/plugin-worker-manager-duplex.test.ts b/server/src/__tests__/plugin-worker-manager-duplex.test.ts index d0c28781d3..45084757ca 100644 --- a/server/src/__tests__/plugin-worker-manager-duplex.test.ts +++ b/server/src/__tests__/plugin-worker-manager-duplex.test.ts @@ -130,17 +130,22 @@ describe("plugin worker manager duplex channel route", () => { const session = await handle.openDuplexChannel( duplexOpenInput({ data: [{ chunk: "one" }, { chunk: "two" }, { chunk: "three" }], + exitCode: 0, }), ); - // Wait so the three data notifications arrive and buffer before a listener - // attaches. The drain then delivers them in order. - await new Promise((resolve) => setTimeout(resolve, 60)); + // The worker writes the three data notifications and the exit in one + // stdout write. The host reads worker stdout line by line, so it buffers + // all three data frames before it reads the exit. The exit settles the + // wait, so the wait is a deterministic barrier: once it resolves, the + // host holds all three frames and no listener has attached yet. This + // barrier replaces a fixed sleep, so the test does not race the + // subprocess start or the stdio latency. + await session.wait(); const chunks: string[] = []; // The session streams raw `Uint8Array` chunks. Decode each one back to // text, so the assertion below compares the plain-text payload the // fixture directive scripted. session.onData((chunk) => chunks.push(new TextDecoder().decode(chunk))); - await vi.waitFor(() => expect(chunks.length).toBe(3)); expect(chunks).toEqual(["one", "two", "three"]); await session.close(); } finally { @@ -213,11 +218,17 @@ describe("plugin worker manager duplex channel route", () => { const session = await handle.openDuplexChannel( duplexOpenInput({ data: [{ chunk: "one" }, { chunk: "boom" }, { chunk: "three" }], + exitCode: 0, }), ); - // Wait so the three data notifications arrive and buffer before a listener - // attaches. The drain then delivers them in order. - await new Promise((resolve) => setTimeout(resolve, 60)); + // The worker writes the three data notifications and the exit in one + // stdout write. The host reads worker stdout line by line, so it buffers + // all three data frames before it reads the exit. The exit settles the + // wait, so the wait is a deterministic barrier for "the host holds every + // pre-bind frame and no listener has attached". This barrier replaces a + // fixed sleep, so the test does not race the subprocess start or the + // stdio latency. + await session.wait(); const chunks: string[] = []; // The listener throws on one buffered chunk. The manager catches the throw // inside the drain, so it does not escape `onData` and every buffered chunk