fix(cli): probe endpoint liveness in-process so the standalone resolve does not fork execPath - #5418
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
리뷰 · 우선순위 61 / 80데스크톱이 런타임을 켜기 전에, 같이 들어 있는 그 질문이 막혀 있었다. 서버가 있는지 보려면 작은 프로그램을 하나 더 띄우는데, 실행 파일이 이 PR은 그 검사를 같은 프로세스 안에서 한다. src/server/proxy-liveness.ts tests/server/proxy-liveness.test.ts - 연결 거절이 진짜 소켓이 아니다. PR 본문 - 데스크톱 셸의 spawn, stale-record, late-external-proxy는 댓글로 올리겠다고 했는데, 그 댓글은 아직 없다. 고친 자리가 그 셸의 Resolving 단계다. 메인테이너의 판단이 필요한 지점
너의 추천 고친 방향은 맞다. 단독 실행 파일에서 이 댓글은 grok-bot이 작성했습니다 |
|
Re: review (grok-bot, 5756336766) — addressed in 7a68e92:
Desktop shell e2e on Linux (release build,
|
| # | Scenario | Result |
|---|---|---|
| A | source proxy already on 10177 + runtime record → app | attach: no sidecar, Usage renders, proxy survives app exit |
| B | nothing running | sidecar ocx start --port 10100, Usage renders; window close (no tray on this desktop) stops the sidecar |
| C | stale runtime-port.json → 10177, nothing listening |
absence proven, sidecar started on configured 10100, Usage renders (before this PR: stuck at Resolving, exit-1 resolve) |
| D | app first, source ocx start ~800 ms later on 10100 |
app owns 10100 and is Ready; the late CLI refuses with Proxy already running (PID …, port 10100) — single owner |
| E | stale 10177 record, app first, source ocx start --port 10177 2.5 s later |
app Ready on 10100 (owns it); the late CLI refuses on the shared spend ledger; nothing on 10177 |
| F | second launch | exits 0, existing window focused, one process, one sidecar |
Not covered here: Windows (the reporter is testing that separately) and the tray Quit menu (Plasma tray not reachable in this desktop; the no-tray window-close exit path was exercised instead).
|
CI note: the 3 failures on the last run ( |
…e does not fork execPath Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…e AggregateError Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…unner timeout) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
47a1a2a to
5416388
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5416388172
ℹ️ 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 (record.code === "ECONNREFUSED" || record.code === "ConnectionRefused") return true; | ||
| if (typeof record.code === "string" && record.code.endsWith("ECONNREFUSED")) return true; | ||
| if (Array.isArray(record.errors) && record.errors.length > 0) { |
There was a problem hiding this comment.
Check aggregate members before accepting wrapper refusal
When a configured hostname resolves to multiple addresses, node:net's autoSelectFamily can produce an AggregateError whose top-level code is copied from the first child (for example ECONNREFUSED) even though a later child is ETIMEDOUT or ENETUNREACH. These lines therefore return true before examining errors, causing probeEndpointLiveness to report dead and potentially authorize a duplicate start or shared uninstall teardown despite the mixed result. Inspect a nonempty errors array before accepting the wrapper's code; the added test currently misses this because its synthetic aggregate has no top-level code.
Useful? React with 👍 / 👎.
| probeEndpointLiveness, | ||
| START_OWNERSHIP_LIVENESS, | ||
| type EndpointLiveness, | ||
| type LiveProxy, | ||
| } from "../server/proxy-liveness"; | ||
| import { endpointsToProve, everyEndpointProvenDownAsync, type ProbeEndpoint } from "./uninstall-plan"; |
There was a problem hiding this comment.
Synchronize the owned resolve contract document
This switches ocx resolve to probeEndpointLiveness and everyEndpointProvenDownAsync, but structure/runtime.md:55 still states that the contract uses the updater's probeProxyLiveness and synchronous everyEndpointProvenDown. Update that owned structure document in this change so maintainers do not rely on an obsolete description of this launch-safety path.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Summary
Since #5384 the desktop shell asks the bundled CLI
ocx resolve --jsonbefore it may start a runtime. In the compiled standalone sidecar that command always exited 1:Root cause:
runResolveproved absence withsrc/update/proxy-liveness-probe.mjs, which runsspawnSync(process.execPath, ["-e", script, ...]). Inside a Bun standalone binaryprocess.execPathisocx, so the "probe child" was anotherocxinvocation that exited 1 →"unknown"→ resolve refuses → the packaged app fails at the Resolving phase and never spawns a sidecar whenever no proxy is already running (attaching to an existing proxy still worked). Confirmed withstraceon the Linux release build; the same latent bug sat inocx stop(abandoned-teardown proof) andocx uninstall.Change:
src/server/proxy-liveness.ts: newprobeEndpointLiveness(endpoint, io): Promise<"live"|"dead"|"unknown">— in-process viadirectLocalHttpFetch, same tri-state rules as the .mjs (only ECONNREFUSED or a clean 200 that is not ours ⇒dead; timeouts/resets/non-200/unreadable body ⇒unknown). Loopback endpoints are dialled on every host fromloopbackProbeHosts(127.0.0.1 and[::1]):liveif any answers as ours,deadonly if all are dead.isConnectionRefusedmoved here (re-exported fromcli/status-probes.ts) and now also looks insideAggregateError.errors(whatautoSelectFamilysurfaces).src/cli/uninstall-plan.ts:everyEndpointProvenDownAsync(sync variant kept).src/cli/resolve.ts,src/cli/index.ts(handleStop,handleUninstall): use the in-process probe.ResolveIo.probeEndpointmay now return a Promise..mjsprobe is unchanged: the plain-Node updater launcher (src/update/index.ts,job.ts) still needs a sync probe and thereexecPathreally is Node.Verification
bun run typecheck,bun run structure:checkbun test tests/cli/cli-resolve.test.ts tests/cli/cli-resolve-subprocess.test.ts tests/cli/uninstall.test.ts tests/server/proxy-liveness.test.ts tests/providers/xai/grok-lifecycle.test.ts— all pass (incl. a real closed-socket refusal case)bun run test:changed(serial) — 17528 pass / 3 fail intests/codex-integrationClaude discovery tests that pass in isolation and are untouched by this diffOPENCODEX_HOME→ exit 0"status":"absent-proven"(was exit 1); live source proxy on 10177 →"status":"live"with its pidChecklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Link to Devin session: https://app.devin.ai/sessions/ef425ed6b27b44a6acdeeeb8fade5c06
Open in Devin Desktop: https://app.devin.ai/desktop/session/ef425ed6b27b44a6acdeeeb8fade5c06?variant=devin
Requested by: @lidge-jun