Skip to content

fix(proxy): make durable bridge claim_session a real compare-and-set - #1643

Open
Komzpa wants to merge 8 commits into
mainfrom
fix-claim-session-cas
Open

fix(proxy): make durable bridge claim_session a real compare-and-set#1643
Komzpa wants to merge 8 commits into
mainfrom
fix-claim-session-cas

Conversation

@Komzpa

@Komzpa Komzpa commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

claim_session in durable_bridge_repository read the owner row, computed the next fencing epoch in Python, and only then entered sqlite_writer_section() for the UPDATE ... WHERE id = ? — with no epoch predicate. with_for_update() is a real row lock on PostgreSQL but a no-op on SQLite, so two concurrent claims in one process mint the SAME epoch (reproduced 40/40 with no barrier on the production SQLite engine config). The loser's release_session then closes the row the winner is serving, leaving a durable row that pairs one claim's account_id with another's latest_response_id.

That corruption is upstream of the cross-account anchor guard (#1638): its durable_lookup.account_id check reads the winner's account as the owner of the loser's response id, concludes "same account, safe to inject", and injects an anchor the account cannot resolve — the exact wedge. Fixing the CAS closes the wedge at its source.

Fix: the read, epoch computation, CAS UPDATE (WHERE includes session id + observed owner instance + observed epoch), alias cleanup and commit now happen in one sqlite_writer_section — the same pattern record_recovery_attempt already uses. PostgreSQL correctness unchanged.

Regression reproduces the duplicate-epoch / split-anchor state on origin/main and passes with the fix; 521 tests green; openspec change added.

@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: 396ffa75d0

ℹ️ 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 tests/unit/test_durable_bridge_sessions.py Outdated
@Komzpa
Komzpa force-pushed the fix-claim-session-cas branch from 396ffa7 to 8ac3b87 Compare August 6, 2026 14:16
@github-actions github-actions Bot added the db migration PR changes Alembic database migrations; maintainer must coordinate merge order label Aug 6, 2026
@Komzpa
Komzpa force-pushed the fix-claim-session-cas branch from 8ac3b87 to c38dc2b Compare August 6, 2026 17:48
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 6, 2026
@Komzpa
Komzpa force-pushed the fix-claim-session-cas branch from 3916b0f to ab5d30f Compare August 6, 2026 23:03
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 7, 2026
@Soju06

Soju06 commented Aug 10, 2026

Copy link
Copy Markdown
Owner

The CAS fix itself looks correct and well-executed — moving the read, epoch computation, fenced UPDATE ... WHERE id + observed owner_instance_id + owner_process_epoch + owner_epoch, alias cleanup, and commit into a single sqlite_writer_section (app/modules/proxy/durable_bridge_repository.py:554-676) matches the record_recovery_attempt pattern, and the coordinator-level regression added in response to the Codex P1 is exactly the right shape. A few things block merging in the current state:

  1. Stacked contents from fix(proxy): recover dead durable bridge anchors fast #1625. This branch carries rebased copies of the three bridge-restart-recovery commits (81b7463, bd40d2b, d6d287e), plus openspec/changes/recover-bridge-restart-anchors/ and the 20260806_120000_add_http_bridge_owner_process_epoch migration — all of which belong to fix(proxy): recover dead durable bridge anchors fast #1625. Merging this PR as-is would merge most of fix(proxy): recover dead durable bridge anchors fast #1625 out-of-band with different SHAs than its own head. Please either wait for fix(proxy): recover dead durable bridge anchors fast #1625 to merge and rebase this down to just the CAS delta (durable_bridge_repository.py + tests + openspec/changes/fix-claim-session-cas/), or state explicitly that this PR supersedes fix(proxy): recover dead durable bridge anchors fast #1625 so the maintainer can coordinate order (the db migration label exists for exactly this).

  2. Alembic single-head is broken against current main. Your merge revision 20260807_010000_merge_claim_with_current_main_head merges 20260806_120000 with 20260806_020000, but main has since landed 20260808_000000_tune_usage_history_autovacuum (down_revision = 20260806_020000). Merging today produces two heads; the PR's green "Migration check" ran before that migration landed. After rebasing, the merge revision needs to include the new main head.

  3. CodeQL jobs failed on a GitHub API rate-limit flake (##[error]API rate limit exceeded, 02:12 UTC Aug 7), not on your code — a re-run after the rebase should clear all three Analyze jobs.

Minor: ApiKeysRepository.update_last_used (app/modules/api_keys/repository.py:383-388) is re-added here but main removed it in the last_used_at coalescing work (#1627/#1665/#1667), and on current main nothing calls it — after rebasing it should be dropped as dead code.

Happy to re-review once this is rebased down to the CAS-only delta; the core change is merge-ready on its merits.

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.

2 participants