Skip to content

fix(proxy): persist file ownership across replicas - #1521

Open
mastertyko wants to merge 4 commits into
Soju06:mainfrom
mastertyko:fix/persist-file-account-pins
Open

fix(proxy): persist file ownership across replicas#1521
mastertyko wants to merge 4 commits into
Soju06:mainfrom
mastertyko:fix/persist-file-account-pins

Conversation

@mastertyko

Copy link
Copy Markdown
Contributor

Summary

Persist live file_id -> account_id ownership pins in the shared database so file finalization and Responses input_file requests remain bound to the uploading account across replicas.

The durable owner decision uses database-authoritative time, does not trust a process-local cache, and fails closed before account selection or upstream dispatch when ownership cannot be established safely.

Type of change

  • fix: — bug fix (no behavior change beyond the bug)
  • feat: — new user-facing feature or capability
  • refactor: — internal refactor (no behavior change, no API change)
  • docs: — documentation only
  • chore: / ci: / build: — tooling, CI, packaging
  • test: — test-only change
  • Breaking change

Linked issue: None — no matching issue or pull request in the bounded upstream search.

OpenSpec

  • This PR includes / updates an OpenSpec change
  • Not applicable — bug fix that matches the existing spec
  • Not applicable — docs / CI / chore only
  • This PR touches a codex-faithful path and preserves upstream-equivalent request behavior

Change directory: openspec/changes/persist-file-account-pins/

Changes

  • Add a reversible Alembic migration and focused repository for durable file-account pins, including idempotent same-owner renewal, conflict rejection, expired-ID reclaim, indexed cleanup, and database-authoritative expiry decisions.
  • Resolve finalize, Responses, compact, and HTTP-bridge file ownership from the shared database on every hard owner decision; forwarded metadata only corroborates the receiver's fresh lookup.
  • Keep reservation cleanup single-owned across local and cross-replica stream/compact handoff, including cancellation, definitive rejection, and ambiguous dispatch paths.
  • Add targeted repository, cross-replica, fail-closed, migration, concurrency, and lifecycle regressions together with the scoped OpenSpec contract.

Simplicity

  • The fix works with zero new configuration.
  • No new required manual setup step is introduced; the schema change uses the existing automatic migration path.
  • New setting(s) and why each can't be a default: None.
  • No README section, .env.example entry, dashboard navigation item, dependency, or public API surface is added.

Test plan

  • Focused repository and proxy product-seam pytest selection after the final candidate change: 34 passed.
  • Real PostgreSQL verification: parent → head → downgrade → head and drift checks; asyncpg/DB-time SQL; two-connection same-owner, conflicting-owner, reclaim, and rollback-waiter races; indexed 100k-row cleanup; controlled database outage fail-closed behavior and same-engine recovery: passed.
  • Changed-Python Ruff check/format, ty, and architecture checks: passed.
  • openspec validate persist-file-account-pins --strict: passed.
  • openspec validate --specs --strict: 48/48 specs passed.
  • /opsx:verify persist-file-account-pins: 11/11 tasks, 3/3 requirements, 35/35 scenarios, zero findings.
  • Independent candidate review: no P1/P2 blocker.
  • Full local repository CI was not completed. GitHub required CI, GitGuardian, and current-head review remain the authoritative publication gates.

Screenshots / output

Not applicable: this is a backend routing/persistence change and does not change dashboard rendering.

Verified behavior includes:

  • a second replica finalizes and routes an input file through the durable uploading account;
  • an unavailable ownership store returns the stable fail-closed owner error before account selection, retry, failover, or upstream invocation;
  • recovery restores durable owner resolution without using stale replica-local state.

Checklist

  • Title is in Conventional Commits format.
  • Bounded upstream issue and PR searches found no matching report.
  • Added or updated tests covering the change.
  • Ran the relevant focused local checks; the intentionally incomplete full local gate is stated above.
  • OpenSpec strict validation passes and /opsx:verify is clean.
  • Simplicity gates reviewed: P1–P5.
  • CHANGELOG is not edited by hand.

@github-actions github-actions Bot added the db migration PR changes Alembic database migrations; maintainer must coordinate merge order label Jul 28, 2026
@mastertyko

Copy link
Copy Markdown
Contributor 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: 7f0025329c

ℹ️ 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/api.py
@mastertyko

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 1ceacc74bf

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

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: fcdf4d48af

ℹ️ 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 30, 2026
@Soju06

Soju06 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Deep review as promised in round 4. Verdict up front: the durable file-pin half is a real fix; the reservation-cleanup half is a feature-scale settlement protocol redesign riding in a fix: PR. Recommend splitting (details at the bottom).

Scope assessment

Two separable changes are bundled:

  1. Durable file pins — migration, app/modules/proxy/file_pin_repository.py, file_ops.py rewire, service.py session factory. This is the actual cross-replica bug fix and is well built (DB-clock expiry via clock_timestamp(), guarded post-claim refresh, batched lookup, conflict rejection).
  2. Single-owner reservation cleanup protocol — 3 new ContextVar signal channels (support.py), _ResponsesReservationCleanup / _guard_responses_startup_handoff / rewritten _probe_stream_startup_error (api.py, ~700 lines), _OwnerForwardOutcome taxonomy (owner_forwarding.py), delayed-200-as-handoff-acknowledgement semantics on the internal forward (http_bridge_forwarding.py), a settle_compact_usage wrapper replacing all ~25 settle sites in compact.py, and removal of retry re-reservation in http_bridge/streaming.py. This changes the wire semantics of every forwarded request carrying a reservation — not just file-pinned ones — and reverses the receiver-settles-on-forwarded-failure invariant (see the rewritten test_proxy_compact_forwarded_bridge_rejection_leaves_reservation_for_origin, which now asserts the row stays reserved where the fix(accounts): serialize cross-replica token refresh with per-account DB claims #1254-era fix asserted released). The PR template claims "no behavior change beyond the bug" while shipping three spec deltas including a large responses-api-compat requirement rewrite; by our own OpenSpec rules that is behavior change, so the fix-class soak exemption should not apply to part 2.

Migration: collides with main today (P1)

20260728_000000_add_file_account_pins.py:14 targets down_revision = 20260725_000000_add_http_bridge_pending_tool_calls. On current main that parent already has two children (20260728_..._merge_pending_tool_calls_and_rollup_repair_heads, 20260731_..._capability_lineage_markers) reconverged by the 20260803 merge head. Merging this as-is creates a second Alembic head; app/db/migrate.py:389 fails drift check (alembic_head_count_invalid expected=1) and startup migration refuses. Must retarget to 20260803_000000_merge_http_bridge_recovery_and_capability_lineage_heads (or ship a merge revision).

Collision map across open PRs (all sharing or predating this branch point):

PR migration down_revision status vs main
#1521 (this) add_file_account_pins 20260725 second head — retarget
#1417 sticky_session_continuity_abandoned_at 20260725 git-mergeable but same second-head problem
#1528 account_usage_limits 20260725 second head — retarget
#1536 api_key_fair_share_threshold 20260725 second head — retarget
#1491 account_plan_downgrade_observations 20260731 second head — retarget
#1555 / #1183 security-lineage chain + own merge revisions 20260720/20260722 lineage predates 20260803 merge — needs rework, not just retarget
#1309 subagent_prompt_cache_ttl 20260717 ancient — rework
#1577 oauth_live_policies 20260803 aligned today

Safe sequencing: serialize all db migration PRs; immediately before each merge, retarget that PR's down_revision to the then-current single head. Today: #1577 needs nothing; every other migration PR (this one included) must rebase after whichever merges first. Never land two in the same window without a merge revision.

Conflict sources (CONFLICTING label)

Findings

P1 — migration multi-head startup failure. As above.

P1 — rebase must be semantic, not textual. #1561 interaction as above; also the new finally: _signal_propagated_responses_service_cleanup_ready() in _settle_compact_api_key_usage must be re-checked against main's raise-on-failure path so a failed settlement doesn't both raise and suppress the API-layer release.

P2 — ambiguous-dispatch quota hold leans on a defeasible backstop. _responses_origin_may_release_reservation (api.py) intentionally leaks a reservation on DISPATCH_AMBIGUOUS until the stale reaper (reset_scheduler.py:19, 6h). The reaper cutoff is updated_at (api_keys/repository.py, release_stale_usage_reservations), which heartbeat touches keep bumping — exactly the leak class in #1594. A leaked heartbeat + deferred origin release = reservation held indefinitely, not 6h. This PR and the #1594 fix need to be co-designed; whichever lands second must re-verify the other's assumption.

P2 — unbounded buffering loop can stall forwarded streams. The new cleanup-ready wait in _probe_stream_startup_error (api.py) buffers frames until _signal_propagated_responses_service_cleanup_ready() fires, with no timeout. The "every submit path signals" invariant is convention-only; any future service path that yields events without signaling makes the receiver buffer everything and never return 200 — a silent replica-to-replica stall, same symptom family as #1534. Suggest a bounded wait that fails closed 503.

P2 — fail-closed flip converts successful upstream calls into 502s. _pin_file_account DB failure now surfaces as _FileOwnerPostSuccessError → 502 after a successful upstream create/finalize (file_ops.py), and every /responses with input_file takes a hard DB dependency (_resolve_file_account_for_responses raises file_owner_unavailable on any DB error). Defensible per design.md, but it is a best-effort→fail-closed availability trade on the hot path and deserves explicit owner sign-off — another reason this isn't "no behavior change beyond the bug".

P2 — retry re-reservation removal needs one more test. http_bridge/streaming.py drops the owner-recovery re-reserve and reuses the original reservation. Tests assert the reservation identity, but not the race where the first attempt's finalizer (which owns cleanup from submit under the new protocol) settles/releases before the retry submits — the retry would then run against a released reservation.

P3 — mixed-version rollout semantics. design.md documents drain requirements for pins only, but the delayed-200 handoff changes both sides of the internal forward. Idempotent release makes most old/new pairings benign, but new-origin + old-receiver (immediate 200, finalizer not yet installed, receiver crash) leaves the reaper as sole recovery. Worth one paragraph in the migration plan and/or a forward-contract version gate.

P3 — compact branch asymmetry. The terminal-compaction branch of _stream_responses binds only the cleanup-ready event, not the dispatched/rejected pair the stream path binds; if a terminal compact can owner-forward into dispatch ambiguity, the branch finally releases while the receiver may settle. If that path is unreachable, a comment saying why would prevent someone "fixing" it later.

Positive notes: the loop-prevention guard (bridge_forward_loop_prevented, streaming.py) addresses the earlier P1 about re-forward ambiguity; repository SQL is careful on both dialects; test breadth (~50 targeted cases incl. cancellation/handoff races) is well above repo norm.

Recommendation

@Soju06

Soju06 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Maintainer decision on the review above: we will NOT require the A/B split — the PR can proceed as a whole, but classified as behavior-changing (no fix-class soak exemption): it merges after a hard review pass and then soaks in beta before any stable cut. To get there, please: (1) do the semantic (not mechanical) rebase — main has moved through #1561/#1562 and today's #1558 settlement-confirmation gating, all in your touched region; the finally-signal in _settle_compact_api_key_usage must be reconciled with main's release-then-raise path; (2) retarget the migration's down_revision onto the current single alembic head; (3) address the P1/P2 findings above, in particular the unbounded cleanup-ready buffering wait (bound it and fail closed) and the DISPATCH_AMBIGUOUS reliance on the 6h stale reaper — note a first-party fix for #1594 is starting now and touches exactly that reaper/heartbeat surface, so plan to rebase on top of it. The fail-closed file-pin flip (DB error → 502 on successful upstream create) is accepted as part of the whole-PR review.

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

Labels

db migration PR changes Alembic database migrations; maintainer must coordinate merge order 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