Skip to content

fix(local-api): probe readiness directly, keep live sessions, isolate smoke files - #320

Draft
SamGu-NRX wants to merge 3 commits into
obv/burst11-bs-local-api-servefrom
gpt-burst/1022z-bs-281-rest
Draft

SamGu-NRX wants to merge 3 commits into
obv/burst11-bs-local-api-servefrom
gpt-burst/1022z-bs-281-rest

Conversation

@SamGu-NRX

@SamGu-NRX SamGu-NRX commented Oct 22, 2026 •

Copy link
Copy Markdown
Owner

Zephyrus task Z6. This fixes the open review threads on #281 (obv/burst11-bs-local-api-serve). It is stacked on #308, which writes session.json only 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_health used urlopen, which honors http_proxy. When a proxy was exported and no_proxy had no loopback entry, the /v1/healthz probe 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.json when a second launch fails to bind. A second launch on a different port (PORT=8123 make local-api while 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 existing session.json names a pid that is alive and a loopback baseUrl port that accepts a TCP connection (_live_session, the check in main). 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 .PHONY test wrote backend/local-api and backend/local-api-smoke, truncating any real file with either name, and then deleted both. It now runs make -f backend/Makefile -I backend from tmp_path and creates the stray files there (test). The empty---port test now passes --session-dir, so a regression there can't write the real backend/runs/local-api/session.json. Every httpx client in the file now sets trust_env=False, so the developer's proxy can't sit between the tests and the launcher either.

A supported PORT override no longer fails the smoke suite (the P2 thread). _make_n drops an inherited PORT, and also MAKEFLAGS, which is where make local-api-smoke PORT=8123 passes 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/ after uv sync --locked --extra api. The launcher tests need uvicorn, which the api extra provides; make local-api-smoke installs it the same way. pytest-xdist isn't installed, so the runs don't use -n 2.

  • Fail first. I ran this PR's test file against the base launcher. To do that, I extracted 9bddc66's backend/ with git archive and copied the new test file in. Result: 4 failed, 16 passed.
    • test_second_launch_refuses_a_live_session failed because the second launch kept running.
    • test_live_session_needs_a_live_pid_and_an_answering_port failed because the base launcher has no _live_session.
    • test_readiness_needs_the_listener_not_a_shell_proxy failed because _wait_health returned True with nothing listening.
    • test_launcher_reports_ready_under_an_unreachable_proxy failed with "failed its health check within 20s; stopping".
  • After the fix. At 832314c, OMP_NUM_THREADS=1 uv run pytest -q tests/test_local_capture_api.py gives 20 passed. At 220ccbe, the same file also gave 20 passed with http_proxy=http://127.0.0.1:9 no_proxy= PORT=8123 exported in the shell that ran pytest. The only change after 220ccbe is in the tests: they keep their closed ports reserved.
  • The two test-only fixes can't fail first as permanent tests, so I reproduced both bugs with the base test file:
    • PORT=8123 uv run pytest -k documented_default gives 1 failed.
    • With backend/local-api containing "keep me", the base stray-file test passed and deleted the file. With this PR, the file survives the test.
  • .PHONY coverage. I removed local-api from .PHONY in a copy of the Makefile and ran make -n local-api from a temp directory. It printed "up to date", so the moved test still catches a missing .PHONY entry.
  • Lint. 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.
  • Full backend suite. At 220ccbe, OMP_NUM_THREADS=1 uv run pytest -q gave 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's backend/:
    • The 14 errors in tests/test_capture_cold_restart.py are ImportError: cannot import name 'load_receipt' from 'scripts.replay_capture', and they error the same way on 9bddc66.
    • The 4 failures in server/eval/tests/test_gates.py ('FAIL' == 'KNOWN_FAIL' and assert 1 == 0) fail the same way on 9bddc66.
    • tests/test_capture_real_downstream.py::test_dropping_the_published_result_fails_the_mode failed 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.
    • I don't have a full-suite count for 9bddc66 itself. Five other suites were running on this machine at the time, and I stopped that run after about 116 of the 1563 tests.

Risk

The launcher now opens session-dir/.session.lock with O_CREAT|O_NOFOLLOW and holds an exclusive fcntl.flock from 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_receipt import; the additional full-suite failures are outside the launcher tests and need separate diagnosis.

Checklist

  • The scope is focused; related changes belong together
  • I explained what changed and why
  • N/A: before/after images. Nothing visible changes.
  • N/A: video. No motion or interaction changes.

🤖 Generated with Claude Code

SamGu-NRX and others added 2 commits October 22, 2026 12:25
… 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>
@coderabbitai

coderabbitai Bot commented Oct 22, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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 size:M 30-99 effective changed lines. label Oct 22, 2026
@SamGu-NRX
SamGu-NRX changed the base branch from gpt-burst/1022w2-pr281-session-publish to obv/burst11-bs-local-api-serve October 22, 2026 18:09
@SamGu-NRX

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 22, 2026 •

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-10-23T11:10:58.621320Z 832314c Manual request
ℹ️ 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.

@SamGu-NRX

Copy link
Copy Markdown
Owner Author

@codex review

@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: 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".

Comment thread backend/scripts/local_capture_api.py Outdated
Comment on lines +314 to +315
live = _live_session(session_path)
if live is not None:

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 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>
@github-actions github-actions Bot added size:L 100-499 effective changed lines. and removed size:M 30-99 effective changed lines. labels Oct 23, 2026

This branch has not been deployed

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

Labels

size:L 100-499 effective changed lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant