[WRONG BRANCH] fix(lab): supervise producers through child exit - #269
[WRONG BRANCH] fix(lab): supervise producers through child exit#269luvs01 wants to merge 1 commit into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughThe producer stores early result patches and defers promise resolution until the child process closes. Tests verify that continued child activity reaches timeout and prevents a later filesystem mutation. ChangesProducer supervision
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🟡 Moderate · up to The producer is intended to remain supervised until its child exits, but the regression test may assert too early and pass even if a late file mutation still occurs. Extend the wait beyond the fixture’s 2,500 ms deadline before treating the change as merge-ready. Sequence Diagram(s)sequenceDiagram
participant FabricTask
participant ProducerIsolate
participant ChildProcess
participant Filesystem
FabricTask->>ProducerIsolate: start isolated producer
ChildProcess->>ProducerIsolate: emit early result patch
ProducerIsolate->>ProducerIsolate: store receivedResult
ChildProcess->>FabricTask: continue activity
FabricTask->>ChildProcess: terminate after timeout
ChildProcess->>ProducerIsolate: emit close
ProducerIsolate->>FabricTask: return timeout outcome
ChildProcess->>Filesystem: attempt late mutation
FabricTask->>Filesystem: verify marker is absent
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@tests/lab-fabric-task.test.ts`:
- Around line 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 51bda4ae-424e-4cc6-b5c4-c2897bfb38a5
📒 Files selected for processing (2)
src/lab/fabric/producer-isolate.tstests/lab-fabric-task.test.ts
💤 Files with no reviewable changes (1)
- src/lab/fabric/producer-isolate.ts
| await Bun.sleep(750); | ||
| expect(existsSync(marker)).toBe(false); |
There was a problem hiding this comment.
🎯 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.
| 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26c6856d73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| receivedResult = message.patch; | ||
| finish(() => resolve({ patch: message.patch, lastActivityAt })); | ||
| return; |
There was a problem hiding this comment.
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 👍 / 👎.
1 participant
Motivation
resultprotocol line so it cannot continue running or mutate files after the parent has accepted a result.Description
runIsolatedFabricProduceruntil the child process actually closes instead of resolving immediately on a parsedresultprotocol line.resultinreceivedResultand let the childclosehandler perform the finalresolve/rejectdecision when appropriate.fabricEarlyResultPatchExecutorthat emits an earlyresult, stays alive, and attempts a late mutation, and add a testproducer result remains supervised until the child exitsto assert the parent remains authoritative and the late mutation cannot happen.Testing
env -u HTTP_PROXY -u HTTPS_PROXY -u ALL_PROXY -u NO_PROXY -u http_proxy -u https_proxy -u all_proxy -u no_proxy node_modules/.bin/bun test tests/lab-fabric-task.test.tsand the file-level tests passed (47 passed).env -u HTTP_PROXY -u HTTPS_PROXY -u ALL_PROXY -u NO_PROXY -u http_proxy -u https_proxy -u all_proxy -u no_proxy node_modules/.bin/bun test tests/lab-fabric-task.test.ts --test-name-pattern "producer result remains supervised"which passed.bun run typecheckandbun run privacy:scanwhich succeeded.bun run testwas executed but the end-to-end run encountered unrelated existing test failures/timeouts in other suites in this environment; the focused fabric producer tests described above were exercised and passed.Codex Task
Summary by CodeRabbit
Bug Fixes
Tests