Repository navigation
Conversation
… smoke files The readiness probe used urlopen, which honors http_proxy. Without a loopback entry in no_proxy it asked the proxy: an answering proxy reported a server ready when nothing listened, and an unreachable one made the launcher stop a healthy server. It now uses a proxy-free opener. A second launch on a free port bound fine and replaced a running server's session.json, which #308's failed-bind guard cannot see. The launcher now refuses while the session's pid is alive and its loopback port answers. The .PHONY test planted stray files in backend/ and deleted them, destroying any real backend/local-api. It now runs make from tmp_path. The make helper drops an inherited PORT and MAKEFLAGS, so a supported PORT override no longer fails the default-port test, and the empty-port test passes --session-dir. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A port from _free_port() is released before the launcher picks its own, so the dead proxy could in rare cases land on the server and let the old probe pass. A bound, never-listening socket refuses connections and keeps the port. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
|
@codex review |
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. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 832314ca0c
ℹ️ 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".
| live = _live_session(session_path) | ||
| if live is not None: |
There was a problem hiding this comment.
Serialize session claims before binding
When two launcher processes start concurrently with the same --session-dir, both can read the absent/stale session.json before either writes it, then bind separate ports and overwrite the file in sequence. The final session can point to one server while the other is also running with different credentials, so clients may still be directed to a mismatched server despite this guard. Claim the session directory atomically (for example with a lock or exclusive marker) before allowing either process to bind and publish credentials.
Useful? React with 👍 / 👎.
Hold an exclusive flock from the live-session check through server shutdown so concurrent launches cannot replace credentials. Add deterministic race and dead-owner regression tests. Co-Authored-By: Claude Code <noreply@anthropic.com>
Zephyrus task Z6. This fixes the open review threads on #281 (
obv/burst11-bs-local-api-serve). It is stacked on #308, which writessession.jsononly after the port is bound, and its base is #308's branch.What changed
The launcher reports ready only when its own listener answers.
_wait_healthusedurlopen, which honorshttp_proxy. When a proxy was exported andno_proxyhad no loopback entry, the/v1/healthzprobe went to the proxy. A proxy that answers made the launcher print "ready" while nothing was listening. A proxy that can't reach loopback made it stop a healthy server once the health timeout ran out. The probe now uses an opener with no proxy handler (local_capture_api.py:190).A second launch no longer replaces a live server's session. #308 keeps
session.jsonwhen a second launch fails to bind. A second launch on a different port (PORT=8123 make local-apiwhile port 8000 is serving) binds fine, though, and it still overwrote the file, so clients read tokens for the wrong server. Before binding, the launcher now exits 1 if the existingsession.jsonnames a pid that is alive and a loopbackbaseUrlport that accepts a TCP connection (_live_session, the check inmain). It checks both because a crashed server's pid can be reused and its port can be taken by another process. A file left behind by a server that has exited is still replaced. The docs and the Makefile comment now describe this.The smoke tests no longer touch the developer's files. The
.PHONYtest wrotebackend/local-apiandbackend/local-api-smoke, truncating any real file with either name, and then deleted both. It now runsmake -f backend/Makefile -I backendfromtmp_pathand creates the stray files there (test). The empty---porttest now passes--session-dir, so a regression there can't write the realbackend/runs/local-api/session.json. Every httpx client in the file now setstrust_env=False, so the developer's proxy can't sit between the tests and the launcher either.A supported
PORToverride no longer fails the smoke suite (the P2 thread)._make_ndrops an inheritedPORT, and alsoMAKEFLAGS, which is wheremake local-api-smoke PORT=8123passes the port. It does this before checking the default (helper).The thread "Second launch loses working credentials" is now fixed in both cases: #308 covers the same port, and this PR covers a different port.
Verification
All commands ran from
backend/afteruv sync --locked --extra api. The launcher tests need uvicorn, which theapiextra provides;make local-api-smokeinstalls it the same way. pytest-xdist isn't installed, so the runs don't use-n 2.backend/withgit archiveand copied the new test file in. Result: 4 failed, 16 passed.test_second_launch_refuses_a_live_sessionfailed because the second launch kept running.test_live_session_needs_a_live_pid_and_an_answering_portfailed because the base launcher has no_live_session.test_readiness_needs_the_listener_not_a_shell_proxyfailed because_wait_healthreturned True with nothing listening.test_launcher_reports_ready_under_an_unreachable_proxyfailed with "failed its health check within 20s; stopping".OMP_NUM_THREADS=1 uv run pytest -q tests/test_local_capture_api.pygives 20 passed. At 220ccbe, the same file also gave 20 passed withhttp_proxy=http://127.0.0.1:9 no_proxy= PORT=8123exported in the shell that ran pytest. The only change after 220ccbe is in the tests: they keep their closed ports reserved.PORT=8123 uv run pytest -k documented_defaultgives 1 failed.backend/local-apicontaining "keep me", the base stray-file test passed and deleted the file. With this PR, the file survives the test..PHONYcoverage. I removedlocal-apifrom.PHONYin a copy of the Makefile and ranmake -n local-apifrom a temp directory. It printed "up to date", so the moved test still catches a missing.PHONYentry.uv run ruff check .passes.uv run ruff format --check .flags one file,server/orchestrator/tests/test_greptile_followup.py. This PR doesn't touch it, and it is unchanged from 9bddc66.OMP_NUM_THREADS=1 uv run pytest -qgave 1528 passed, 16 skipped, 5 failed, 14 errors in 37 minutes. None of these are in the launcher test file, and this PR changes no file those tests import. I reran the 19 against 9bddc66'sbackend/:tests/test_capture_cold_restart.pyareImportError: cannot import name 'load_receipt' from 'scripts.replay_capture', and they error the same way on 9bddc66.server/eval/tests/test_gates.py('FAIL' == 'KNOWN_FAIL'andassert 1 == 0) fail the same way on 9bddc66.tests/test_capture_real_downstream.py::test_dropping_the_published_result_fails_the_modefailed only in the full run. At 220ccbe it passes on its own and with the rest of its file (12 passed), so the failure depends on load or test order. The test never starts the launcher.Risk
The launcher now opens
session-dir/.session.lockwithO_CREAT|O_NOFOLLOWand holds an exclusivefcntl.flockfrom the live-session check through server shutdown. A competing launch waits for the owner to publish its session, then reports the live pid and URL instead of binding or replacing credentials. If the owner dies, the kernel releases the lock and the next launch keeps the existing stale-session behavior.The new race test failed on 832314c with 2 failed and passes on ca7e89c with 2 passed. The local launcher suite was 20 passed on the base and is 22 passed after the fix. The full backend suite was 1528 passed, 16 skipped, 5 failed, 14 errors in the prior recorded run and is 1523 passed, 16 skipped, 12 failed, 14 errors on ca7e89c. The 14 cold-restart errors are the known missing
load_receiptimport; the additional full-suite failures are outside the launcher tests and need separate diagnosis.Checklist
🤖 Generated with Claude Code