Skip to content

fix(proxy): never inject a cross-account previous_response_id anchor - #1274

Open
aididhaiqal wants to merge 3 commits into
Soju06:mainfrom
aididhaiqal:pr/anchor-owner-guard
Open

fix(proxy): never inject a cross-account previous_response_id anchor#1274
aididhaiqal wants to merge 3 commits into
Soju06:mainfrom
aididhaiqal:pr/anchor-owner-guard

Conversation

@aididhaiqal

Copy link
Copy Markdown

Summary

The HTTP-bridge "compact anchor" continuity optimization injects previous_response_id = session.last_completed_response_id and trims the already-stored history prefix so a follow-up turn only carries the new items. A previous_response_id is account-scoped upstream — only the account that created a response can resume it — but the injection had no ownership check.

When a Codex session fails over to a different account (the durable owner account became unavailable, or a durable record is restored onto a session bound to another account), the anchor points at a response the serving account never created. Upstream accepts the WebSocket response.create but never emits response.created, and because the history was trimmed away there is no fallback. The per-bridge response_create_gate (Semaphore(1)) stays held:

  • the holder's client eventually reports stream disconnected before completion: idle timeout waiting for SSE;
  • later requests on the same session return codex-lb is temporarily overloaded during http_bridge_response_create_gate.

Observed live: sessions fanned across several accounts repeatedly wedged on the same anchor even though the accounts had quota — freeing the gate doesn't help because the session re-injects the same cross-account anchor and re-wedges.

Fix: track the account that owns last_completed_response_id (last_completed_response_account_id), set in lockstep at both setters — the real response.completed path records the session's current account, the durable-restore path records the durable owner account — and gate the session-level anchor injection on owner == serving account. On mismatch, fall through to a full-history resend (correct output, slightly more tokens), never a cross-account previous_response_id.

This is codex-faithful: the real Codex CLI never carries a previous_response_id across a connection/account boundary, so declining to inject a foreign anchor matches upstream behavior more closely, not less.

OpenSpec

openspec/changes/fix-cross-account-previous-response-anchor/sticky-session-operations delta: compact previous_response_id anchors are account-scoped; codex-lb MUST NOT inject an anchor whose owning account differs from the serving account, and MUST resend the full history instead. openspec validate --specs green.

Tests

  • test_stream_via_http_bridge_injects_session_anchor_when_account_owns_it — anchor still injected when the serving account owns it (no regression to the optimization).
  • test_stream_via_http_bridge_skips_session_anchor_after_cross_account_failover — anchor skipped and full history resent on cross-account failover; no wedge.
  • Full tests/unit/test_proxy_http_bridge.py (267) + tests/integration/test_http_responses_bridge.py (89) green.

@aididhaiqal

Copy link
Copy Markdown
Author

@codex review

1 similar comment
@Soju06

Soju06 commented Jul 16, 2026

Copy link
Copy Markdown
Owner

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 91b0b3aabe

ℹ️ 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".

@Soju06

Soju06 commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Codex review at the current head is clean, but after syncing with main the required Type check (ty) job fails:

error[invalid-argument-type]: Argument to function _fingerprint_input_items is incorrect

_fingerprint_input_items's signature moved on main, so this looks like a semantic conflict with a recent merge rather than a problem in your original diff. Could you rebase and adapt the call site? The Contributors attribution failure is being handled repo-side in #1365 — nothing needed from you there.

Soju06 added a commit that referenced this pull request Jul 16, 2026
…#1365)

Unblocks the Contributors attribution gate for #1312 and #1274.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@Komzpa
Komzpa force-pushed the pr/anchor-owner-guard branch from 5f2bee5 to 5f85df3 Compare July 17, 2026 19:18
@Komzpa

Komzpa commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 5f85df3042

ℹ️ 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".

@Komzpa Komzpa added 🤖 codex: ok [@codex review] says no issues found. needs rebase Needs rebase or conflict repair against current main and removed 🤖 codex: ok [@codex review] says no issues found. labels Jul 17, 2026
@Soju06

Soju06 commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Re-triaged against current main: still very much wanted. Main has no equivalent guard — last_completed_response_account_id doesn't exist there, and the session-level anchor injection in app/modules/proxy/_service/http_bridge/streaming.py still injects session.last_completed_response_id without any ownership check — and neither #1479 nor #1492 (including 5f422af, which fixed reservation sticky-key matching, a different surface) covers this failure mode. Unfortunately the branch conflicts again: the injection block moved with the recent proxy work, so the 07-17 rebase went stale. The good news is the conflict is confined to streaming.pysupport.py, upstream_events.py, and the tests still merge clean — so it should be a small round. Could you rebase once more? Codex was clean and CI fully green at 5f85df3, so after the rebase I'll re-run @codex review, approve CI, and take it from there. This is fix-class, so it's exempt from the current beta-soak train. Apologies for the second rebase round-trip — that one's on our merge cadence, not on you.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

@Soju06

Soju06 commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Ping @aididhaiqal — the 07-27 rebase ask still stands, and the conflict remains confined to app/modules/proxy/_service/http_bridge/streaming.py, so it should be a small round. Since this is fix-class (exempt from the beta-soak train), Codex was clean, and CI was fully green at 5f85df3, we'd like to land it soon: if the rebase isn't up within the next couple of days we're happy to carry it over the line ourselves with a maintainer rebase, keeping your commits and attribution intact. Just say the word either way.

aididhaiqal and others added 3 commits July 30, 2026 20:03
The HTTP-bridge compact-anchor optimization injects
`previous_response_id = session.last_completed_response_id` and trims the stored
history prefix. A `previous_response_id` is account-scoped upstream: only the
account that created a response can resume it. The injection had no ownership
check, so when a Codex session failed over to a different account (durable owner
unavailable, or a durable record restored onto a session bound to another
account) the anchor pointed at a response the serving account never created.
Upstream accepted the `response.create` but never emitted `response.created`, and
with the history trimmed away there was no fallback — the per-bridge
`response_create_gate` (`Semaphore(1)`) wedged. The holder's client saw "idle
timeout waiting for SSE"; queued requests saw "temporarily overloaded".

Track the owning account of `last_completed_response_id`
(`last_completed_response_account_id`), set in lockstep at both setters — the real
`response.completed` path records the session's current account, the durable
restore records the durable owner account — and gate the session-level anchor
injection on owner == serving account. On mismatch, fall through to a
full-history resend (correct output, more tokens), never a cross-account anchor.

Regression tests: anchor injected when same-account; anchor skipped + full
history resent after cross-account failover. OpenSpec:
fix-cross-account-previous-response-anchor (sticky-session-operations).
@aididhaiqal
aididhaiqal force-pushed the pr/anchor-owner-guard branch from 5f85df3 to 9b93a15 Compare July 30, 2026 12:24
@aididhaiqal

Copy link
Copy Markdown
Author

Hey, sorry wasnt around. Should be clear now

@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: 9b93a15b62

ℹ️ 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 +7273 to +7274
@pytest.mark.asyncio
async def test_stream_via_http_bridge_skips_session_anchor_after_cross_account_failover(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Exercise the failover and response-create gate in the regression test

This regression test does not reproduce the production failure it claims to cover: _run_session_anchor_owner_stream returns a session that is already bound to acc-2, mocks _get_or_create_http_bridge_session and _submit_http_bridge_request, and preloads the event queue with None. Consequently, it never performs an account failover, sends a frame upstream, waits for response.created, or acquires/releases the response-create gate, so it would still pass if the real bridge path remained wedged. Add bridge-level coverage that completes an anchor on one account, reconnects to another, and verifies the full-history request completes without blocking the gate.

AGENTS.md reference: AGENTS.md:L125-L128

Useful? React with 👍 / 👎.

@Komzpa Komzpa added 🤖 codex: needs work [@codex review] raised an issue and removed needs rebase Needs rebase or conflict repair against current main labels Jul 30, 2026
@Soju06

Soju06 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Rebase landed clean — CI fully green on 9b93a15. Two items left:

  1. Codex's P1 on the regression test stands: _run_session_anchor_owner_stream pre-binds the session to acc-2 and stubs _get_or_create_http_bridge_session/_submit_http_bridge_request, so it never exercises the failover or the response-create gate — it would still pass if the real bridge path stayed wedged. Please add bridge-level coverage: complete a turn on account A, fail over to account B, and assert the follow-up goes upstream as a full-history resend (no previous_response_id) and settles.
  2. fix(http-bridge): stabilize silent and clean-close recovery #1394 merged since your push and reworked http_bridge/streaming.py, so the branch conflicts again. Should be mechanical: the last_completed_response_id setters on main are still the same four sites your guard pairs with last_completed_response_account_id (mixin.py:2393, streaming.py:1887/1988, upstream_events.py:1808) — no new setters landed.

Fix-class, so still soak-exempt; with those two done this is ready to land.

@Komzpa Komzpa added the needs rebase Needs rebase or conflict repair against current main label Aug 4, 2026
@Komzpa

Komzpa commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Rebased onto current main and finished the two open review items in #1638, with your three commits carried over verbatim (authorship intact) — the only conflict was mechanical, in mixin.py against main's newer try/abort structure. While rebasing I found and closed a second unguarded post-binding injection site (the owner-forward recovery rebind), and replaced the stubbed test with a real-bridge integration test that reproduces the wedge when the guard is removed. Your diagnosis was exactly right — this is the root cause of the wedge family we spent today mitigating from the consumer side. Happy to fold #1638 back into this branch instead if you prefer landing it under your own PR.

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

Labels

🤖 codex: needs work [@codex review] raised an issue needs rebase Needs rebase or conflict repair against current main

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants