Skip to content

fix(cli): probe endpoint liveness in-process so the standalone resolve does not fork execPath - #5418

Merged
lidge-jun merged 4 commits into
devfrom
devin/1789970465-resolve-probe-standalone
Sep 21, 2026
Merged

lidge-jun merged 4 commits into
devfrom
devin/1789970465-resolve-probe-standalone

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Since #5384 the desktop shell asks the bundled CLI ocx resolve --json before it may start a runtime. In the compiled standalone sidecar that command always exited 1:

resolve: liveness is unknown (a probe timed out or a listener withheld /healthz); refusing to treat unknown as absent.

Root cause: runResolve proved absence with src/update/proxy-liveness-probe.mjs, which runs spawnSync(process.execPath, ["-e", script, ...]). Inside a Bun standalone binary process.execPath is ocx, so the "probe child" was another ocx invocation 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 with strace on the Linux release build; the same latent bug sat in ocx stop (abandoned-teardown proof) and ocx uninstall.

Change:

  • src/server/proxy-liveness.ts: new probeEndpointLiveness(endpoint, io): Promise<"live"|"dead"|"unknown"> — in-process via directLocalHttpFetch, 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 from loopbackProbeHosts (127.0.0.1 and [::1]): live if any answers as ours, dead only if all are dead. isConnectionRefused moved here (re-exported from cli/status-probes.ts) and now also looks inside AggregateError.errors (what autoSelectFamily surfaces).
  • 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.probeEndpoint may now return a Promise.
  • The .mjs probe is unchanged: the plain-Node updater launcher (src/update/index.ts, job.ts) still needs a sync probe and there execPath really is Node.

Verification

  • bun run typecheck, bun run structure:check
  • bun 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 in tests/codex-integration Claude discovery tests that pass in isolation and are untouched by this diff
  • Rebuilt the standalone sidecar and ran it directly on Linux: empty isolated OPENCODEX_HOME → exit 0 "status":"absent-proven" (was exit 1); live source proxy on 10177 → "status":"live" with its pid
  • Desktop shell e2e on Linux (release build) against this branch — attach / spawn / stale record / late external proxy ×2 / single instance all pass; table and screenshot in the PR comment.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

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

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ed770f0b-f5d9-4d68-9b23-55ca977a7b3d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the bug Something isn't working label Sep 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions

github-actions Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ 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.

0/4 boxes ticked.

Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked.

@github-actions
github-actions Bot marked this pull request as draft September 21, 2026 06:25
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 61 / 80

데스크톱이 런타임을 켜기 전에, 같이 들어 있는 ocx에게 resolve로 물어본다. 이 집에 서버가 이미 있으면 그걸 쓰고, 정말 없으면 새로 켠다.

그 질문이 막혀 있었다. 서버가 있는지 보려면 작은 프로그램을 하나 더 띄우는데, 실행 파일이 process.execPath다. 개발 중에는 이게 node라서 검사 코드가 돈다. 묶어서 만든 단독 실행 파일에서는 process.execPathocx 자신이다. 검사가 또 다른 ocx가 되고, 그 프로세스는 바로 끝난다. 그 결과는 "없다"가 아니라 "모르겠다"다. resolve는 모르겠으면 새 서버를 허락하지 않는다. 그래서 떠 있는 프록시가 없을 때 앱이 Resolving에서 멈춘다. 이미 떠 있는 프록시에 붙을 때는 이 검사를 타지 않아서 괜찮았다.

이 PR은 그 검사를 같은 프로세스 안에서 한다. probeEndpointLiveness/healthz를 직접 연다. 연결이 거절되거나, 200인데 우리 서버가 아니면 "없다". 시간 초과, 연결이 끊김, 200이 아닌 응답, 본문을 못 읽으면 "모르겠다". ocx stop이 남이 남긴 정리를 끝낼 때와 ocx uninstall이 정말 꺼졌는지 볼 때도 같은 함수를 쓴다. 업데이터가 쓰는 src/update/proxy-liveness-probe.mjs는 그대로다. 그 경로는 동기이고, 거기서는 execPath가 정말 node다.

src/server/proxy-liveness.ts probeEndpointLiveness - 주소를 probeHostname으로 고친다. ::127.0.0.1이 된다. 남겨 둔 .mjs::::1로 연다. 윈도우에서 ::로 연 서버는 127.0.0.1에 대답하지 않는 경우가 있다. 그때 새 검사는 "없다"고 하고, resolve는 없음을 증명했다고 본다. 두 번째 런타임을 켤 수 있다. 업데이터는 같은 주소에 아직 "살아 있다"고 할 수 있다. 본문의 리눅스 확인은 빈 홈이라 주소가 127.0.0.1이고, 이 경우를 안 본다.

tests/server/proxy-liveness.test.ts - 연결 거절이 진짜 소켓이 아니다. { code: "ECONNREFUSED" }를 던질 뿐이다. 실제 경로 directLocalHttpFetchautoSelectFamily: true로 붙는다. 거절 코드가 AggregateErrorerrors에만 있으면 isConnectionRefused는 못 본다. 그 함수는 .cause만 따라간다. 그러면 테스트는 통과한 채로 다시 "모르겠다"가 되고, 이번처럼 시작이 막힌다.

PR 본문 - 데스크톱 셸의 spawn, stale-record, late-external-proxy는 댓글로 올리겠다고 했는데, 그 댓글은 아직 없다. 고친 자리가 그 셸의 Resolving 단계다.

메인테이너의 판단이 필요한 지점

.mjs를 남기는 이유는 있다. 업데이터 런처는 비동기로 바꿀 수 없고, 거기서는 node가 맞다. 남긴다면 "없다"의 기준만 같아야 한다. ::::1로 볼지 127.0.0.1로 볼지다. everyEndpointProvenDownAsync는 후보를 한꺼번에 전부 친다. 예전 동기 함수는 하나라도 "없다"가 아니면 바로 멈췄다. 판정은 같고, 느린 포트가 있으면 실패할 때도 그 시간만큼 늦어진다. 이 PR은 아직 초안이고 준비 체크는 0/4다.

너의 추천

고친 방향은 맞다. 단독 실행 파일에서 execPath로 자식을 띄우지 않는 것이 이 버그의 수정이다. 리눅스에서 빈 홈이 absent-proven으로 나온 기록은 보고된 증상과 맞다. 머지 전에 :: 다이얼을 .mjs와 맞추고, 거절 테스트를 실제 소켓 에러로 하나 두면 좋겠다. 데스크톱 세 경우가 댓글에 오기 전에는 패키지 앱 수정으로 닫지 않는 쪽을 추천한다.

이 댓글은 grok-bot이 작성했습니다

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Re: review (grok-bot, 5756336766) — addressed in 7a68e92:

  • :: / localhost dial: probeEndpointLiveness now walks loopbackProbeHosts(hostname) (127.0.0.1 then [::1], or the reverse for an explicit ::1): live as soon as one family identifies our proxy, dead only when every candidate is refused or foreign, otherwise unknown. At least as strict as the .mjs's single ::1 dial, so an IPv6-only listener can no longer be "proven" absent.
  • isConnectionRefused now also descends into AggregateError.errors (bounded by the same depth), which is what autoSelectFamily: true surfaces.
  • The refusal test binds and closes a real net.createServer port and runs the probe with the default directLocalHttpFetch, plus explicit AggregateError cases.

Desktop shell e2e on Linux (release build, --no-bundle, this branch, isolated OPENCODEX_HOME)

# 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

Usage after spawn

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).

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

CI note: the 3 failures on the last run (release asset verification ×2 — extra OpenCodex-2.61.0-linux-amd64.deb.sig — and desktop install identity > the owner values are the ones the record accepts) reproduce on origin/dev head ac3df52cc with a plain bun test tests/clients/desktop-install-identity.test.ts tests/ci-workflows/release-desktop-scripts.test.ts, so they are preexisting on base and not touched by this diff. Merged origin/dev into the branch (47a1a2a) so the PR sits on the latest dev; expect the same 3 to stay red until they are fixed on dev.

lidge-jun and others added 4 commits September 21, 2026 17:24
…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>
@lidge-jun
lidge-jun force-pushed the devin/1789970465-resolve-probe-standalone branch from 47a1a2a to 5416388 Compare September 21, 2026 08:25
@lidge-jun lidge-jun closed this Sep 21, 2026
@lidge-jun lidge-jun reopened this Sep 21, 2026
@lidge-jun
lidge-jun marked this pull request as ready for review September 21, 2026 09:23
@lidge-jun
lidge-jun merged commit 52acf81 into dev Sep 21, 2026
90 of 94 checks passed
@lidge-jun
lidge-jun deleted the devin/1789970465-resolve-probe-standalone branch September 21, 2026 09:23

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment on lines +168 to +170
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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

Comment thread src/cli/resolve.ts
Comment on lines +42 to +47
probeEndpointLiveness,
START_OWNERSHIP_LIVENESS,
type EndpointLiveness,
type LiveProxy,
} from "../server/proxy-liveness";
import { endpointsToProve, everyEndpointProvenDownAsync, type ProbeEndpoint } from "./uninstall-plan";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-21T09:27:11.065414Z 5416388 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant