Skip to content

[WRONG BRANCH] fix(lab): supervise producers through child exit #269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
luvs01 wants to merge 1 commit into
base: main
Choose a base branch
from
+49 −2
Draft

[WRONG BRANCH] fix(lab): supervise producers through child exit #269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/lab/fabric/producer-isolate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ export async function runIsolatedFabricProducer(request: IsolateRequest): Promis
if (message.type === "result") {
if (settled) return;
receivedResult = message.patch;
finish(() => resolve({ patch: message.patch, lastActivityAt }));
return;
Comment on lines 116 to 117

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Resolve trailing results from the close handler

When a child writes valid result JSON without a final newline and then exits, the close handler parses the buffered result, but this branch now only stores it; the subsequent if (receivedResult) return exits without calling finish. Because the child has already closed, the timers cannot trigger another close event, so the Lab run hangs indefinitely even beyond its total timeout. Resolve the buffered result after parsing it or route it through the common close decision.

AGENTS.md reference: src/AGENTS.md:L17-L17

Useful? React with 👍 / 👎.

}
if (message.type === "error") {
Expand Down
50 changes: 49 additions & 1 deletion tests/lab-fabric-task.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,38 @@ export async function execute(_input: FabricPatchExecutorInput): Promise<Synthet
});
}

function fabricEarlyResultPatchExecutor(home: string): { executor: TrustedFabricPatchExecutor; marker: string } {
const dir = join(home, "fabric-executors");
mkdirSync(dir, { recursive: true });
const modulePath = join(dir, "early-result-patch.ts");
const marker = join(home, "late-child-mutation.txt");
writeFileSync(modulePath, `
import { writeFileSync } from "node:fs";
import type { FabricPatchExecutorInput, SyntheticPatchV1 } from "${repoImport("src/lab/fabric/types")}";
import { SYNTHETIC_AFTER_UTF8, SYNTHETIC_VALUE_PATH } from "${repoImport("src/lab/fabric/constants")}";

const patch: SyntheticPatchV1 = {
schemaVersion: 1,
operations: [{ op: "replace", path: SYNTHETIC_VALUE_PATH, contentUtf8: SYNTHETIC_AFTER_UTF8 }],
};

export async function execute(input: FabricPatchExecutorInput): Promise<SyntheticPatchV1> {
process.stdout.write(JSON.stringify({ type: "result", patch }) + "\\n");
const deadline = Date.now() + ${FAST_FABRIC_ISOLATION.totalTimeoutMs + 500};
while (Date.now() < deadline) {
input.reportActivity();
await Bun.sleep(100);
}
writeFileSync(${JSON.stringify(marker)}, "late\\n");
return patch;
}
`);
return {
executor: createHostIssuedFabricPatchExecutor(modulePath, async () => correctSyntheticPatch()),
marker,
};
}

function fabricTraversalPatchExecutor(home: string): TrustedFabricPatchExecutor {
const dir = join(home, "fabric-executors");
mkdirSync(dir, { recursive: true });
Expand Down Expand Up @@ -561,6 +593,22 @@ describe("CL-07 task effectiveness producer", () => {
expect(result.outcome.failure?.code).toBe("inactivity_timeout");
}, 20_000);

test("producer result remains supervised until the child exits", async () => {
const home = tempHome();
process.env.OPENCODEX_HOME = home;
const { executor, marker } = fabricEarlyResultPatchExecutor(home);
const result = await runFabricSyntheticPatchTaskForRoute({
routeContext: fabricMockRoute(),
destination: await fabricDestination(home),
patchExecutor: executor,
configDir: home,
});
expect(result.outcome.outcome).not.toBe("pass");
expect(result.outcome.failure?.code).toBe("timeout");
await Bun.sleep(750);
expect(existsSync(marker)).toBe(false);
Comment on lines +608 to +609

@coderabbitai coderabbitai Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Wait past the fixture’s late-write deadline.

Line 165 sets the child deadline to 2,500 ms, and Line 170 writes the marker only after that deadline. If the producer resolves on the early result, the task can return before the child reaches Line 170. Lines 608-609 then wait only 750 ms, so the test can pass even though the child writes the marker later. Wait beyond the fixture deadline before asserting that the marker does not exist.

Suggested fix 
-    await Bun.sleep(750);
+    await Bun.sleep(FAST_FABRIC_ISOLATION.totalTimeoutMs + 750);
     expect(existsSync(marker)).toBe(false);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await Bun.sleep(750);
expect(existsSync(marker)).toBe(false);
await Bun.sleep(FAST_FABRIC_ISOLATION.totalTimeoutMs + 750);
expect(existsSync(marker)).toBe(false);
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/lab-fabric-task.test.ts` around lines 608 - 609, Increase the delay
before the marker assertion in the relevant test so it exceeds the fixture child
deadline and late-write timing defined by the test setup. Keep the existing
expect(existsSync(marker)).toBe(false) assertion unchanged, ensuring the test
observes whether the child writes after the producer resolves.

}, 20_000);

test("activity resets inactivity deadline within total budget", async () => {
const home = tempHome();
process.env.OPENCODEX_HOME = home;
Expand Down Expand Up @@ -1081,4 +1129,4 @@ describe("CL-07 task effectiveness producer", () => {
expect(text.includes("system prompt")).toBe(false);
expect(text.includes(CREDENTIAL_CANARY)).toBe(false);
});
});
});
Loading