Skip to content

test(sim): deterministic proxy turn-lifecycle simulation harness - #1647

Open
Komzpa wants to merge 14 commits into
mainfrom
deterministic-sim-harness
Open

test(sim): deterministic proxy turn-lifecycle simulation harness#1647
Komzpa wants to merge 14 commits into
mainfrom
deterministic-sim-harness

Conversation

@Komzpa

@Komzpa Komzpa commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Adds a virtual-clock + controlled-scheduler simulation harness so the proxy turn lifecycle can be tested deterministically instead of via wall-clock sleeps and production flakes. This is the implementation-level leg of the codex-lb verification effort (alongside the TLA+ model #1621 and the timeout-invariant linter #1622).

Production unchanged: new app/core/clock.py (Clock/Scheduler protocols with RealClock/RealScheduler as the defaults + tolerant scheduler_for/clock_for accessors). Clock threaded through ProxyService/LoadBalancer/retry-circuit; scheduler through work-admission/bridge/websocket sleep+wait_for+task-spawn sites, so a simulation owns every task a turn spawns.

Tests:

  • 3 audit-listed timing tests converted to virtual time (work-admission wait, streaming-timeout hardening, http-bridge cancel/drain) — same behavioral assertions, no real sleeps.
  • A schedule-exploring property test: 200 seeded schedules interleaving admission-wait / upstream-terminal / downstream-cancel / retry as concurrent virtual tasks, asserting exactly one terminal outcome per request and exactly-once release of the response-create / api-key / account leases, on real product release paths.
  • A canary that runs the property checker against a double-release-on-cancel toy and asserts it FAILS — plus 3 more planted mutants the checker rejects while accepting the correct implementation.

Runs in ~2s wall (33 tests), 3 consecutive green. openspec change add-deterministic-proxy-simulation validates strictly. The 2 pre-existing test_v1_responses_http_bridge_reconnects* failures also fail on plain origin/main (baseline, not introduced here).

@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: b90968755e

ℹ️ 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 app/modules/proxy/service.py
Comment thread tests/simulation/test_proxy_turn_lifecycle_property.py
Comment thread tests/simulation/virtual_time.py Outdated
@Komzpa Komzpa added 🤖 codex: needs work [@codex review] raised an issue and removed 🤖 codex: needs work [@codex review] raised an issue labels Aug 6, 2026
@Komzpa

Komzpa commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator 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: 7413f657fc

ℹ️ 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 app/modules/proxy/service.py
Comment thread app/modules/proxy/_service/http_bridge/mixin.py
Comment thread app/modules/proxy/api.py
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 8, 2026
@Soju06

Soju06 commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Thanks for the fast turnaround on the first review round — all three of those fixes landed cleanly. The 08-08 re-review raised 3 new P2 threads that are still open and are the only remaining merge blockers (CI is green and the branch is mergeable):

  1. app/modules/proxy/service.py:991 — clock-domain mismatch in thread-goal budgeting. I verified this at the current head: thread_goal_request computes deadline = self._clock.monotonic() + budget, but _call_goal (~line 1046) and the freshness-check loop (~line 1521) still call the module-level _remaining_budget_seconds(deadline), which subtracts host time.monotonic(). With a VirtualClock starting near zero, remaining budget is immediately <= 0 and the path raises budget-exhausted before any upstream call. The 69b0c50 fix only switched _ensure_fresh_with_budget to self._remaining_budget_seconds; the outer consumers of this deadline need the same treatment (or the module-level helper could take the clock explicitly so mixed usage can't compile-slip back in).

  2. app/modules/proxy/_service/http_bridge/mixin.py:1964 — the scheduler-owned bridge reader spawns its receive/wakeup children with bare asyncio.create_task (upstream_events.py:812, :822) and the cancellation-resistant receive creates an unowned deferred drain (helpers.py:1721). These escape VirtualScheduler._tasks, so cancel_owned_tasks() can report quiescence while a receive/drain task is still live — exactly the false-quiescence failure mode the harness exists to prevent. Routing these through the same scheduler seam keeps the ownership invariant airtight.

  3. app/modules/proxy/api.py:5660 — only the initial capacity-probe timeout is virtualized; the signal-discovery branch (lines 5747-5750) and post-ready branch (lines 5727-5730) still derive deadlines from asyncio.get_running_loop().time() and block in real asyncio.wait(..., timeout=...), so deterministic tests can still stall up to two wall-clock seconds. All bounded probe branches should go through the injected scheduler.

Since #1 undermines the harness's own core promise (virtual-clock runs of the thread-goal path fail spuriously) and #2 can mask the very races the property test explores, these are worth fixing before merge. Given how quickly the first round was addressed, this looks close — happy to re-review once pushed.

@Komzpa Komzpa added the needs rebase Needs rebase or conflict repair against current main label Aug 10, 2026
Komzpa added 12 commits August 14, 2026 21:07
Extends the deterministic-simulation scheduler seam to the remaining anchored
HTTP-bridge and websocket lifecycle owners so a simulation can own every task
spawned by a proxy turn. Declares the clock and scheduler collaborators on the
HTTP-bridge and websocket service protocols. Real clock/scheduler stay the
defaults, so production behavior is unchanged.
Drives the idle-timeout and recovery-wait cancel/drain scenarios through the
virtual scheduler instead of real asyncio timeouts, keeping the same
behavioral assertions about queue revocation and completed-delivery dominance.
…aths

The seeded schedule checker now dispatches each lifecycle event as a concurrent
task with its own virtual wake-up deadline, so events sharing a deadline
interleave at the await points inside the production release helpers instead of
running in a fixed order. Release ownership goes through
_release_websocket_response_create_ownership_for_cleanup and
ProxyService._release_websocket_request_state_reservation, and the admission
wait contends for a permit on a real WorkAdmissionController, which adds a
liveness arm: a permit that is never handed back fails the checker.

The canary plants the double-release-on-cancel bug in the cancel path itself.
Local mutation checks confirm the checker also rejects a lost terminal claim, a
dropped API-key reservation release, and a permit that is never released.
Drops the retry-circuit cooldown stub that had been added to the untouched
real-clock completed-delivery test - it changes nothing there - and documents
why the virtual-time idle-timeout scenario still needs it. Records the extra
seam and verification tasks in the OpenSpec change.
Bridge and websocket lifecycle behavior lives in mixins whose methods the unit
suite also drives with partial service doubles. Reading self._scheduler
directly made four http-bridge idle-lease tests hang, so the seam now goes
through scheduler_for()/clock_for(), which fall back to the real asyncio
scheduler and real clock. Production wiring is unchanged.

Collapses the admission-wait-timeout getattr in _get_work_admission onto one
line so service.py stays inside the proxy architecture ratchet.
@Komzpa
Komzpa force-pushed the deterministic-sim-harness branch from 7413f65 to 3dccb14 Compare August 14, 2026 17:40
@Komzpa Komzpa removed the needs rebase Needs rebase or conflict repair against current main label Aug 14, 2026
@Komzpa Komzpa added 🤖 codex: needs work [@codex review] raised an issue and removed 🤖 codex: needs work [@codex review] raised an issue labels Aug 14, 2026
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 15, 2026
@Komzpa Komzpa added the needs rebase Needs rebase or conflict repair against current main label Aug 15, 2026
@Soju06

Soju06 commented Aug 16, 2026

Copy link
Copy Markdown
Owner

The three 08-08 P2s all have real fixes on this head (injected clock through thread-goal budgeting, scheduler-owned bridge reader children, virtualized capacity-probe branches) — thanks. But the current head cannot be reviewed for merge:

  1. Conflicts with main in 10 files — the scheduler/clock seams run through http_bridge/*, websocket/*, api.py, load_balancer.py, service.py, which have all moved since 08-11 (fix(proxy): fence successor bridge claims against the retiring predecessor #1751, fix(proxy): sweep idle bridge sessions without request traffic #1747, fix(proxy): scope backend Codex affinity by thread identity #1703, fix(http-bridge): preserve goal-restart recovery across reconnects #1680, fix(proxy): durably recover hard HTTP bridge operations #1657, ...). A rebase is required.
  2. Architecture budget breachscripts/check_proxy_architecture.py fails: http_bridge/mixin.py has 2446 lines; limit is 2436 (this also fails test_check_proxy_architecture).
  3. Unit regressions — the seam changes break existing production-path tests: two test_proxy_http_bridge eventless-timeout tests and six test_proxy_utils select-account/capacity tests now raise ProxyResponseError 400/502.

Structural suggestion: this PR keeps getting invalidated because the production seam edits sit in the highest-churn files in the repo. Consider splitting it — land tests/simulation/ plus only the minimal seam hooks in one small PR, and stage the broader task-spawning virtualization separately. As-is, each week of main churn resets the review.

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

Labels

needs rebase Needs rebase or conflict repair against current main

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants