-
Notifications
You must be signed in to change notification settings - Fork 412
fix(proxy): do not rewrite thread locality for a file-pin owner #1765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| schema: spec-driven | ||
| created: 2026-08-15 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| ## Purpose | ||
|
|
||
| Stop a live file pin from rewriting current-Codex thread locality. | ||
|
|
||
| ## Decision | ||
|
|
||
| The required owner bypasses the thread PROMPT_CACHE row the same way | ||
| it already bypasses the process-session soft row. | ||
|
|
||
| ## Example | ||
|
|
||
| Upload pins `file_xyz` to account A. Thread `t1` is already mapped to | ||
| account B. A Responses turn that references `file_xyz` goes to A; the | ||
| `t1` row remains B. The next unpinned `t1` turn still uses B. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| ## Context | ||
|
|
||
| `#1521` made file pins durable hard ownership and bypassed the | ||
| process-session soft row. `#1703` then made `thread_header` / | ||
| PROMPT_CACHE the current Codex soft mapping. The bypass was not | ||
| updated, so a required file owner still enters sticky persist and | ||
| upserts thread B→A. | ||
|
|
||
| ## Goals / Non-Goals | ||
|
|
||
| **Goals:** | ||
|
|
||
| - File-pinned routing stays on the pin account. | ||
| - An existing thread PROMPT_CACHE row is not rewritten. | ||
| - Process-session seed remains insert-if-absent. | ||
|
|
||
| **Non-Goals:** | ||
|
|
||
| - Changing 1011 file-pin reconnect. | ||
| - Weakening file-pin fail-closed or hard-owner conflict checks. | ||
| - Dashboard or settings changes. | ||
|
|
||
| ## Decisions | ||
|
|
||
| - Null the writable sticky key for both `session_header` and | ||
| `thread_header` in `preferred_owner_sticky_inputs`. Selection then | ||
| takes the unbound required-owner path. | ||
| - Keep `legacy_sticky_key` so a conflicting raw process-session owner | ||
| still fail-closes. | ||
| - Leave `sticky_seed_key` to the caller so a missing process | ||
| preference can still initialize without writing the thread row. | ||
|
|
||
| **Alternative considered:** persist the thread row onto the file | ||
| owner so later unpinned turns stay there. Rejected: the file pin is | ||
| hard only for this turn; thread locality is a separate soft mapping. | ||
|
|
||
| ## Risks / Trade-offs | ||
|
|
||
| - [Risk] A later unpinned turn on the same thread stays on the | ||
| pre-file account and cannot see the upload. → Mitigation: that is | ||
| the existing unpinned-file compatibility path; the pin still binds | ||
| any turn that references the file. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| ## Why | ||
|
|
||
| A live `input_file.file_id` pin is hard ownership. After thread-scoped | ||
| affinity, current Codex locality is the `thread_header` PROMPT_CACHE | ||
| row, but `preferred_owner_sticky_inputs` only bypasses | ||
| `session_header`. A file-pinned Responses turn therefore rewrites the | ||
| thread mapping to the upload account, so later unpinned turns follow | ||
| the file owner. | ||
|
|
||
| ## What Changes | ||
|
|
||
| - Treat `thread_header` as the current-Codex soft row that a resolved | ||
| file/response/bridge owner must bypass. | ||
| - Keep consulting the raw process-session compatibility row for hard | ||
| conflicts. | ||
| - Keep process-session seed insert-if-absent. Do not write or rebind | ||
| the thread row on the required-owner path. | ||
| - Keep explicit `turn_state` as hard ownership. | ||
|
|
||
| ## Capabilities | ||
|
|
||
| ### New Capabilities | ||
|
|
||
| - None. | ||
|
|
||
| ### Modified Capabilities | ||
|
|
||
| - `sticky-session-operations`: A resolved file-pin owner MUST be | ||
| selected without consulting or rewriting the thread-scoped soft | ||
| mapping. | ||
|
|
||
| ## Impact | ||
|
|
||
| - `app/modules/proxy/affinity.py` preferred-owner sticky inputs. | ||
| - Focused selection tests. | ||
| - No API, schema, setting, dashboard, or wire-format change. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: File-pin required owner does not rewrite thread locality | ||
|
|
||
| A resolved live `input_file.file_id` pin MUST be selected as the required owner without consulting or rewriting the current-Codex thread-scoped soft mapping. The process-session compatibility row MAY still be consulted as independent hard ownership. If that raw row conflicts with the pin account, the request MUST fail closed. A missing process-session preference MAY still initialize insert-if-absent. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Require the raw process-session lookup.
🤖 Prompt for AI Agents |
||
|
|
||
| #### Scenario: File-pinned request owner overrides thread locality | ||
|
|
||
| - **GIVEN** a request carries a `thread-id` whose bounded mapping points to account A | ||
| - **AND** its `input_file.file_id` is durably pinned to account B | ||
| - **WHEN** the request is routed | ||
| - **THEN** account B is treated as the required owner | ||
| - **AND** the thread mapping is neither consulted as an owner nor rewritten | ||
|
|
||
| #### Scenario: File pin still conflicts with a raw process-session owner | ||
|
|
||
| - **GIVEN** a raw process-session `codex_session` row points to account A | ||
| - **AND** a live file pin points to account B | ||
| - **WHEN** the request is routed | ||
| - **THEN** the service fails with `continuity_owner_conflict` before upstream dispatch | ||
| - **AND** neither the raw row nor the thread row is rewritten | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| ## 1. Implementation | ||
|
|
||
| - [x] 1.1 Bypass the writable `thread_header` sticky key in | ||
| `preferred_owner_sticky_inputs` the same way as `session_header`. | ||
|
|
||
| ## 2. Regression coverage | ||
|
|
||
| - [x] 2.1 Assert preferred-owner selection nulls the thread sticky key | ||
| and keeps the process seed / raw legacy key. | ||
| - [x] 2.2 Assert an existing thread row is not upserted when a file | ||
| pin is the required owner. | ||
| - [x] 2.3 Cover the same file-pin plus existing-thread case through | ||
| `/backend-api/codex/responses`, including the later unpinned | ||
| thread turn and process-seed sibling. | ||
|
|
||
| ## 3. Validation | ||
|
|
||
| - [x] 3.1 Run the focused selection tests. | ||
| - [x] 3.2 Run strict OpenSpec validation for this change. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,9 +24,11 @@ | |
| from app.core.auth.refresh import RefreshError | ||
| from app.core.clients.files import FileProxyError | ||
| from app.core.clients.proxy import ProxyResponseError | ||
| from app.db.models import FileAccountPin | ||
| from app.db.models import FileAccountPin, StickySessionKind | ||
| from app.db.session import SessionLocal | ||
| from app.modules.proxy.affinity import _codex_backend_identity, _codex_session_selection_key | ||
| from app.modules.proxy.file_pin_repository import FileAccountPinRepository | ||
| from app.modules.proxy.sticky_repository import StickySessionsRepository | ||
|
|
||
| pytestmark = pytest.mark.integration | ||
|
|
||
|
|
@@ -53,11 +55,12 @@ def _make_auth_json(account_id: str, email: str) -> dict: | |
| } | ||
|
|
||
|
|
||
| async def _import_account(async_client, account_id: str, email: str) -> None: | ||
| async def _import_account(async_client, account_id: str, email: str) -> str: | ||
| auth_json = _make_auth_json(account_id, email) | ||
| files = {"auth_json": ("auth.json", json.dumps(auth_json), "application/json")} | ||
| response = await async_client.post("/api/accounts/import", files=files) | ||
| assert response.status_code == 200 | ||
| return response.json()["accountId"] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
|
|
@@ -865,6 +868,112 @@ async def fake_stream( | |
| assert resolved is not None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_backend_responses_file_pin_does_not_rewrite_existing_thread_row( | ||
| async_client, | ||
| monkeypatch, | ||
| ): | ||
| from app.dependencies import get_proxy_service_for_app | ||
|
|
||
| thread_owner_chatgpt_id = "acc_file_pin_thread_owner" | ||
| file_owner_chatgpt_id = "acc_file_pin_file_owner" | ||
| thread_owner_id = await _import_account( | ||
| async_client, | ||
| thread_owner_chatgpt_id, | ||
| "file-pin-thread-owner@example.com", | ||
| ) | ||
| file_owner_id = await _import_account( | ||
| async_client, | ||
| file_owner_chatgpt_id, | ||
| "file-pin-file-owner@example.com", | ||
| ) | ||
| process_session = "file-pin-process" | ||
| thread_headers = {"session-id": process_session, "thread-id": "file-pin-thread"} | ||
| sibling_headers = {"session-id": process_session, "thread-id": "file-pin-sibling"} | ||
| thread_key = _codex_backend_identity(thread_headers).thread_selection_key | ||
| sibling_key = _codex_backend_identity(sibling_headers).thread_selection_key | ||
| process_key = _codex_session_selection_key(process_session) | ||
| assert thread_key is not None | ||
| assert sibling_key is not None | ||
|
|
||
| async with SessionLocal() as session: | ||
| await StickySessionsRepository(session).upsert( | ||
| thread_key, | ||
| thread_owner_id, | ||
| kind=StickySessionKind.PROMPT_CACHE, | ||
| ) | ||
|
|
||
| service = get_proxy_service_for_app(async_client._transport.app) | ||
| await service._pin_file_account("file_thread_locality", file_owner_id) | ||
| seen: list[str] = [] | ||
|
|
||
| async def fake_stream(payload, headers, access_token, account_id, **kwargs): | ||
| del payload, headers, access_token, kwargs | ||
| seen.append(account_id) | ||
| yield 'data: {"type":"response.completed","response":{"id":"resp_file_pin_thread"}}\n\n' | ||
|
|
||
| monkeypatch.setattr(proxy_module, "core_stream_responses", fake_stream) | ||
|
|
||
| pinned_response = await async_client.post( | ||
| "/backend-api/codex/responses", | ||
| headers=thread_headers, | ||
| json={ | ||
| "model": "gpt-5.2", | ||
| "instructions": "You are a helpful assistant.", | ||
| "input": [ | ||
| { | ||
| "role": "user", | ||
| "content": [ | ||
| {"type": "input_text", "text": "Read the file."}, | ||
| {"type": "input_file", "file_id": "file_thread_locality"}, | ||
| ], | ||
| } | ||
| ], | ||
| "stream": True, | ||
| }, | ||
| ) | ||
| assert pinned_response.status_code == 200 | ||
| assert seen == [file_owner_chatgpt_id] | ||
|
|
||
| async with SessionLocal() as session: | ||
| repo = StickySessionsRepository(session) | ||
| assert await repo.get_account_id(thread_key, kind=StickySessionKind.PROMPT_CACHE) == thread_owner_id | ||
| assert await repo.get_account_id(process_key, kind=StickySessionKind.CODEX_SESSION) == file_owner_id | ||
| assert await repo.get_account_id(sibling_key, kind=StickySessionKind.PROMPT_CACHE) is None | ||
|
Comment on lines
+938
to
+942
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win Assert that the thread row was not written. The current assertions pass if an erroneous upsert retains
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
|
|
||
| unpinned_response = await async_client.post( | ||
| "/backend-api/codex/responses", | ||
| headers=thread_headers, | ||
| json={ | ||
| "model": "gpt-5.2", | ||
| "instructions": "You are a helpful assistant.", | ||
| "input": "Continue without the file.", | ||
| "stream": True, | ||
| }, | ||
| ) | ||
| assert unpinned_response.status_code == 200 | ||
| assert seen == [file_owner_chatgpt_id, thread_owner_chatgpt_id] | ||
|
|
||
| sibling_response = await async_client.post( | ||
| "/backend-api/codex/responses", | ||
| headers=sibling_headers, | ||
| json={ | ||
| "model": "gpt-5.2", | ||
| "instructions": "You are a helpful assistant.", | ||
| "input": "Sibling thread without a file.", | ||
| "stream": True, | ||
| }, | ||
| ) | ||
| assert sibling_response.status_code == 200 | ||
| assert seen == [file_owner_chatgpt_id, thread_owner_chatgpt_id, file_owner_chatgpt_id] | ||
|
|
||
| async with SessionLocal() as session: | ||
| repo = StickySessionsRepository(session) | ||
| assert await repo.get_account_id(thread_key, kind=StickySessionKind.PROMPT_CACHE) == thread_owner_id | ||
| assert await repo.get_account_id(process_key, kind=StickySessionKind.CODEX_SESSION) == file_owner_id | ||
| assert await repo.get_account_id(sibling_key, kind=StickySessionKind.PROMPT_CACHE) == file_owner_id | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_derived_prompt_cache_key_does_not_block_file_id_pin(async_client): | ||
| """Regression: a ``prompt_cache_key`` that the proxy itself derived | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.