fix(inbound): guard re-approve on already-active grant against unique-index 500 - #2369
Open
detail-app[bot] wants to merge 1 commit into
Open
Conversation
…-index 500 `approve_inbound_grant`'s `inserted` CTE revived a revoked grant by inserting a fresh `active` row, but gated only on "no pending row" + "a revoked row exists". After an approve→revoke→re-approve cycle, the ledger holds both an `active` row (current) and a `revoked` row (history) for the same (connection_id, chat_id). A further approve took the `inserted` INSERT path and collided with the partial unique index `inbound_grants_live_uniq` (`WHERE status <> 'revoked'`), raising an unhandled `asyncpg.UniqueViolationError` that surfaced as HTTP 500 instead of the documented `ConflictError` (HTTP 409). The crash aborted the whole statement before the existing `if row is None: raise ConflictError(...)` fallthrough could run, so retried/scripted/double-submitted re-approvals of an already-admitted chat crashed with a bare "Internal Server Error" rather than a clean 409. Add a `AND NOT EXISTS (... status = 'active')` guard to the `inserted` CTE so it only inserts when no active row already exists. With both the `promoted` (pending→active) and `inserted` (revoked→new active) CTEs empty for an already-active chat, the outer `granted` CTE yields no row and the existing `row is None` → `ConflictError` branch returns HTTP 409 — atomic and idempotent, preventing the partial-index collision rather than catching it. This matches the query's own design and the function's existing conflict message; the codebase-conventional `try/except asyncpg.UniqueViolationError` alternative was not used because it catches the violation after the statement aborts, and the CTE guard makes the already-active case a first-class no-op within the single statement. Tests: - `tests/integration/test_inbound_grants.py`: two service-layer regression tests — re-approve on already-active with revoked history (the exact bug sequence) and re-approve on already-active with no revoked history (the previously-unasserted non-promotable 409 path). - `tests/integration/test_reapprove_http_contract.py`: end-to-end HTTP contract + concurrency tests mounting the real `connections.router` + real `install_exception_handlers` over a real DB pool (driven via `httpx.ASGITransport`) to assert the 409 aios error envelope, not a 500; plus a concurrent-approve probe asserting no 500 and exactly one active row. Co-Authored-By: Detail <dev@aios>
Contributor
Code reviewVerdict: pass Reviewed head No blocking findings. Ruff and mypy passed locally for the changed files; GitHub lint, unit, and integration checks completed successfully. |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detail bug report: View on Detail
Summary
Re-approving an already-
activeinbound grant (after an approve→revoke→re-approve cycle) raised an unhandledasyncpg.UniqueViolationErroron the partial unique indexinbound_grants_live_uniq, surfacing as HTTP 500 instead of the documentedConflictError(HTTP 409). This broke the idempotent re-approval contract for retried / scripted / double-submitted approve calls on an already-admitted chat.Substrate state changes
None — code-only change. No migration, env-var, or external-service contract change; the partial unique index is unchanged (only the query was missing a guard).
Bug & Fix
Bug:
approve_inbound_grant'sinsertedCTE revived a revoked grant by inserting a freshactiverow, gated only on "nopendingrow" + "arevokedrow exists". After approve→revoke→re-approve, the ledger holds both anactiverow (current) and arevokedrow (history). A further approve took theinsertedINSERT path and collided withinbound_grants_live_uniq(WHERE status <> 'revoked'), aborting the whole statement before the existingif row is None: raise ConflictError(...)fallthrough could run — so the API returned a bare "Internal Server Error" 500, and the constraint name went only to server logs.Fix: Added
AND NOT EXISTS (SELECT 1 FROM inbound_grants WHERE ... AND status = 'active')to theinsertedCTE so it only inserts when noactiverow already exists. For an already-active chat both thepromoted(pending→active) andinserted(revoked→new active) CTEs are empty, the outergrantedCTE yields no row, and the existingrow is None→ConflictErrorbranch returns HTTP 409. This is atomic/idempotent and prevents the partial-index collision rather than catching it, matching the query's own design and the function's existing conflict message. Atry/except asyncpg.UniqueViolationErroralternative (used elsewhere in the codebase) was not chosen because it catches after the statement aborts; the guard makes the already-active case a first-class no-op within the single statement.Test plan
tests/integration/test_inbound_grants.py): two service-layer tests — the exact approve→revoke→re-approve→re-approve sequence (assertsConflictErrorand ledger["active","revoked"]), and the previously-unasserted already-active-never-revoked 409 path. Verified to fail with the bug's exactUniqueViolationErrorwhen the fix is reverted.tests/integration/test_reapprove_http_contract.py): mounts the realconnections.router+ realinstall_exception_handlersover a real migrated DB pool (driven viahttpx.ASGITransport) to assert the 4th approve returns HTTP 409 with the aios{"error":{"type":"conflict",...}}envelope, not a 500; plus a concurrent-approve probe asserting all concurrent results areConflictErrorand exactly oneactiverow remains.tests/unit/test_inbound_admission.pydecision-logic suite passes.mypy src tests,ruff check, andruff format --checkall pass;openapi.jsonand the generated SDK show no drift (API surface unchanged).Risk / rollback
Code-only and low-risk: the guard only suppresses an INSERT that would have crashed, steering it to the already-existing
ConflictErrorbranch. Roll back by reverting the commit; no migration to undo and no state to repair (the bug never produced corrupt rows — the INSERT was rejected by the unique index).Automatic Fixes PRs can be configured here.