fix: roll back unauthorized Global Ask turns - #399
Conversation
…x/global-ask-atomic-rollback-362
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude Re-review exact current head |
…ix/global-ask-atomic-rollback-362 # Conflicts: # CHANGELOG.md
139ecaa
into
ContextualWisdomLab:stack/board-source-detail-state-filter-4fdd-20260822
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
📝 Info: Rolled-back turn leaves the conversation reusable
For an existing conversation the ordinal is max+1 and the updated_at bump lives inside the transaction, so a rollback leaves no inserted turn, no ordinal gap, and no stale timestamp. A follow-up retry recomputes the ordinal cleanly, which the new test confirms.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| async def _ensure_citations_visible( | ||
| conn: asyncpg.Connection, | ||
| conversation_id: UUID, | ||
| turn_ordinal: int, | ||
| cited_post_count: int, | ||
| can_see_post: Callable[[asyncpg.Record], bool], | ||
| ) -> None: | ||
| """Lock and re-authorize new citations before their transaction commits.""" | ||
| rows = await conn.fetch( | ||
| """ | ||
| select relation.cited_post_id::text as post_id, | ||
| post.post_title, post.visibility_code, post.corporate_entity_id, | ||
| post.author_account_id, post.source_detail_state_code | ||
| from global_ask_turn_citation relation | ||
| join source_post post on post.post_id = relation.cited_post_id | ||
| where relation.global_ask_session_id = $1 | ||
| and relation.turn_ordinal = $2 | ||
| for share of post | ||
| """, | ||
| conversation_id, | ||
| turn_ordinal, | ||
| ) | ||
| if len(rows) != cited_post_count or any(not can_see_post(row) for row in rows): | ||
| raise GlobalAskEvidenceChanged |
There was a problem hiding this comment.
📝 Info: Re-authorization covers only cited posts, not sources
The re-check at global_ask_history.py re-authorizes cited posts under FOR SHARE, but not source posts that fed the answer without being cited. A source that becomes unauthorized between retrieval and persist can still have its content baked into the stored answer_text. ADR 0126 states the stored answer stays account-owned transcript data and only cited/source projections are re-filtered on read, so this is intentional.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if can_see_post is not None: | ||
| await _ensure_citations_visible( | ||
| conn, | ||
| conversation_id, | ||
| ordinal, | ||
| len(cited_ids), | ||
| can_see_post, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Row-lock re-check closes the retrieval/persist race
Sources are gathered on a separate connection, then persist_turn re-reads cited posts with FOR SHARE. Under read-committed this sees the latest committed source_post state and blocks in-flight writers until commit, so a revocation committed after gathering is caught and the whole turn rolls back. The len(rows) != cited_post_count check also catches concurrent deletes. Because the same _can_use_post_for_analysis predicate runs at gather and re-check, the normal path never rolls back spuriously.
Was this helpful? React with 👍 or 👎 to provide feedback.
) This integration branch diverged from the line that originally shipped #399/#374 before either landed, so it never picked up the fix: a cited post whose authorization changed between source selection and commit (visibility flip, corporate-entity move, etc.) would have its facts served in the Global Ask answer and its citation row persisted even though the request had no business seeing it anymore. Restored _ensure_citations_visible (row-share-locked re-check inside persist_turn's own transaction) and GlobalAskEvidenceChanged -> 503 wiring in ask_agent, byte-identical to the already-reviewed #399 fix. Added a regression test that forces the race from inside the fake chat client and confirmed it fails on the pre-fix code (leaks the revoked post's facts with a 200) before passing on the restored fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011EP69xAyLaJxa6oaF6D9eq
1 participant
What changed
persist_turntransaction with row-share locks.Validation
uv run --extra dev --extra backend pytest backend/tests/test_api.py -q— 110 passed, 6 skippeduv run --extra dev --extra backend pytest tests/test_global_ask_sources.py -q— 9 passeduv run --with pytest pytest tests/test_migration_replay.py -q— 9 passedpython -m compileallandgit diff --checkpassedStacked on #398; merge after the tenant-settings parent is accepted.
Closes #362