refactor(web-integration): tighten CDP proxy ownership and split helpers#2374
refactor(web-integration): tighten CDP proxy ownership and split helpers#2374
Conversation
Deploying midscene with
|
| Latest commit: |
3243810
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1595ba2a.midscene.pages.dev |
| Branch Preview URL: | https://refactor-cdp-proxy-ownership.midscene.pages.dev |
![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: e35ff41345
ℹ️ 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".
| if (!Number.isFinite(pid)) { | ||
| sweepProxyMetadataFiles(); |
There was a problem hiding this comment.
killProxy() accepts any finite number from PROXY_PID_FILE, so 0, negative values, or a stale reused PID all pass validation and are later passed to process.kill(..., 'SIGTERM'). On Unix, 0/negative PIDs target process groups (and -1 can signal nearly every permitted process), so a corrupted or stale temp pid file during upstream switching can terminate unrelated processes instead of just the proxy. Tighten validation to a positive integer PID (and ideally confirm it is the proxy process) before signaling.
Useful? React with 👍 / 👎.
`killProxy()` previously sent SIGTERM and immediately unlinked PROXY_ENDPOINT_FILE / PROXY_PID_FILE / PROXY_UPSTREAM_FILE itself, racing with the proxy's own `cleanupIfOwned()` SIGTERM handler. The race was masked by a defensive "PID file missing → bail" branch added to `cleanupIfOwned()` in #2357. Move ownership of the proxy metadata files back to the proxy: - `killProxy()` now only signals the child (SIGTERM) and polls PROXY_PID_FILE for up to 2s, escalating to SIGKILL + a metadata sweep if the child is unresponsive. - The cross-command targetId file still belongs to mcp-tools and is cleared up front, regardless of the proxy's state. - `getProxyEndpoint()` now `await`s `killProxy()` so the next `spawnProxy()` cannot race the dying child's duplicate-proxy guard.
`mcp-tools-cdp.ts` had grown to handle six concerns at once: CDP URL resolution, proxy process lifecycle, proxy metadata IO, cross-command targetId storage, puppeteer agent construction and the MCP tool schema. Split the proxy-side and store-side concerns into dedicated modules so the file is just the tool class plus its puppeteer glue. - New `cdp-proxy-manager.ts` owns the proxy lifecycle and its on-disk metadata: `isProxyAlive`, `readProxyEndpoint`, `readProxyUpstream`, `killProxy`, `getProxyEndpoint`, plus the page-level URL resolver. All of these are now real module exports rather than file-private helpers behind a `__test__` namespace. - New `cdp-target-store.ts` owns the cross-command targetId persistence (`readSavedTargetId`, `saveTargetId`, `cleanupTargetIdFile`). - `mcp-tools-cdp.ts` keeps `WebCdpMidsceneTools`, the puppeteer `getTargetId()` glue and the target-discovery delay constant. Its external surface (`WebCdpMidsceneTools`) is unchanged. - Tests now import from `cdp-proxy-manager.js` directly; the `__test__` namespace is gone.
quanru
force-pushed
the
refactor/cdp-proxy-ownership
branch
from
e35ff41 to
3243810
Compare
2 participants
Summary
Architectural follow-up to #2357. Two refactors that surfaced during code review of #2357 but were too invasive to fold into that bug-fix PR:
killProxy()previously sent SIGTERM and immediately unlinkedPROXY_*_FILE, racing with the proxy's owncleanupIfOwned()SIGTERM handler. The race was masked by a defensive "PID file missing → bail" branch added insidecleanupIfOwned(). This PR makeskillProxy()only signal + poll for the child to exit (with a 2s grace + SIGKILL fallback), so the proxy is the sole writer/deleter of its own metadata files.mcp-tools-cdp.tsinto focused modules. It had grown to handle six concerns: CDP URL resolution, proxy lifecycle, proxy metadata IO, cross-command targetId persistence, puppeteer agent construction, and MCP tool schema. Extract:cdp-proxy-manager.ts— proxy lifecycle + metadata IO + URL resolution.cdp-target-store.ts— cross-command targetId persistence.mcp-tools-cdp.tskeeps onlyWebCdpMidsceneToolsand the puppeteergetTargetId()glue.External surface (
WebCdpMidsceneTools) is unchanged.Dependencies
This PR is stacked on top of #2357. The diff currently includes #2357's commits because that PR has not merged yet. After #2357 lands I will rebase this branch onto
mainso the diff cleans up to just the two refactor commits below.The refactor commits to review are:
0f08a80refactor(web-integration): let CDP proxy own its metadata cleanupe35ff41refactor(web-integration): split CDP-mode helpers out of mcp-tools-cdpTest plan
npx rslib build(packages/web-integration)npx vitest run tests/unit-test/cdp-proxy.test.ts— all 12 tests pass, including the newgetProxyEndpoint replaces a live proxy when upstream changesintegration test introduced in fix(web-integration,shared): CDP CLI tab reuse, upstream switch, hang, diagnostics & MCP error formatting #2357 that now also exercises the awaitedkillProxy()lifecyclepnpm run lint🤖 Generated with Claude Code