test(daemon): stabilize singleton fixture readiness and timing - #348
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| process.on('message',stop); | ||
| const server=net.createServer(s=>s.on('data',chunk=>{const r=JSON.parse(chunk);s.end(JSON.stringify({id:r.id,ok:true,result:r.method==='status'?{pid:process.pid,socketPath:${JSON.stringify(socket)},protocolVersion:1,servers:[]}:true}));if(r.method==='stop')setTimeout(stop,250);})); | ||
| server.listen(${JSON.stringify(socket)},()=>fs.writeFileSync(${JSON.stringify(metadata)},JSON.stringify({pid:process.pid,socketPath:${JSON.stringify(socket)}}),{mode:384}));`, | ||
| server.listen(${JSON.stringify(socket)},()=>{fs.writeFileSync(${JSON.stringify(metadata)},JSON.stringify({pid:process.pid,socketPath:${JSON.stringify(socket)}}),{mode:384});process.send('ready');});`, |
| process.on('message',stop); | ||
| const server=net.createServer(s=>s.on('data',chunk=>{const r=JSON.parse(chunk);s.end(JSON.stringify({id:r.id,ok:true,result:r.method==='status'?{pid:process.pid,socketPath:${JSON.stringify(socket)},protocolVersion:1,servers:[]}:true}));if(r.method==='stop')setTimeout(stop,250);})); | ||
| server.listen(${JSON.stringify(socket)},()=>fs.writeFileSync(${JSON.stringify(metadata)},JSON.stringify({pid:process.pid,socketPath:${JSON.stringify(socket)}}),{mode:384}));`, | ||
| server.listen(${JSON.stringify(socket)},()=>{fs.writeFileSync(${JSON.stringify(metadata)},JSON.stringify({pid:process.pid,socketPath:${JSON.stringify(socket)}}),{mode:384});process.send('ready');});`, |
|
Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 3:47 PM ET / 19:47 UTC. ClawSweeper reviewWhat this changesThe PR scales relay test timeouts on Windows and waits for an explicit child-process readiness message before inspecting migration metadata. Merge readiness✅ Ready for maintainer review The PR remains useful: current main and v0.13.9 retain both fixture weaknesses. The focused test changes introduce no blocking defect, and collaborator-authored work is protected from automatic closure. Priority: P3 Review scores
Verification
How this fits togetherMCPorter's daemon shares retained MCP server connections across clients. These integration fixtures exercise authenticated Chrome relay reuse and safe retirement of older daemon processes. flowchart TD
A[Test runner] --> B[Platform timing budget]
B --> C[Chrome relay fixture]
A --> D[Legacy daemon child]
D --> E[Write metadata and signal ready]
E --> F[Migration assertions]
C --> G[Connection reuse assertions]
Before mergeNone. Agent review detailsSecurityNone. Review metricsNone. Technical reviewBest possible solution: Keep readiness tied to completed fixture initialization and use the existing platform budget for process-heavy test deadlines. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the metadata race: main polls file existence before reading JSON while a separate process writes it. The Windows timeout failure is reported in the PR; neither failure was executed during this read-only review. Is this the best way to solve the issue? Yes. Explicit readiness removes the initialization race, and the existing platform helper supplies narrowly scoped timeout headroom without weakening daemon assertions. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 9957bd1e233c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Maintainer validation on The local suite reproduced a migration-fixture empty JSON read before this repair; readiness now comes from IPC sent after the metadata write, rather than file existence. The relay test uses the existing Windows timeout scale without changing production limits. CI: https://github.com/openclaw/mcporter/actions/runs/33988089802. The first attempt passed the repaired fixtures but hit an unrelated Windows process-ownership observation failure in |
|
Reviewed CodeQL alerts 22 and 23 on the edited fixture line. They flag the existing |
2 participants
The singleton daemon fixtures have two readiness/timing races: the Chrome-relay case can exceed the global ten-second timeout on Windows, and the migration fixture can observe a metadata file before the child finishes writing its JSON.
Use the existing platform-scaled test budget for the relay file and wait for the migration child's bounded IPC readiness message after its metadata write. Production behavior and timeouts are unchanged.
Validation:
pnpm check; fullpnpm test(217 files, 1,861 tests passed); focused real relay fixture cases; built CLI list/call against a synthetic loopback MCP server. Independent local autoreview through P2 is clean. The original Windows failure is https://github.com/openclaw/mcporter/actions/runs/33986588630; the migration race was reproduced locally as an empty JSON read.