ref(mcp): retain app-scoped teardown prototype (toby)

[OliverBryant]

Status

This PR is a draft prototype and reference. It is no longer the current merge target and should not be merged as one unit.

The split is tracked by #2030 and must land strictly in this order:

1. Add stable MCP lifecycle generation identities (toby) #2031 — stable PublicMCPApp and UserMCPServer generation identities
2. Fence MCP OAuth persistence by association generation (toby) #2032 — OAuth producer lifecycle fence, after Add stable MCP lifecycle generation identities (toby) #2031 merges
3. Add the atomic app-scoped MCP teardown primitive (toby) #2033 — atomic app-scoped teardown primitive, after Fence MCP OAuth persistence by association generation (toby) #2032 merges

Each later PR must branch from the latest upstream main only after its predecessor has merged. Feature branch stacking is not allowed. The downstream xagent-saas PRs #949 and #912 remain frozen; route enablement is the final downstream layer.

Program context: https://github.com/xorbitsai/xagent-saas/issues/909

Verified prototype value

• Demonstrates locked catalog, server, and association revalidation before destructive writes.
• Demonstrates one local transaction for credential, grant, flow, association, and final server cleanup.
• Preserves external OAuth revocation as post-commit best effort so provider I/O does not hold database locks.
• Demonstrates sanitized failure logging that excludes credentials and raw upstream detail.
• Adds SQLite write-intent and PostgreSQL two-session concurrency coverage, with real PostgreSQL CI routing.
• Demonstrates producer-first and teardown-first OAuth persistence ordering and exact callback-flow revalidation.
• CI passed on prototype head 9cf987b.

Unresolved blockers carried into the split

• Association ABA: a stale operation can accept replacement association B after original association A was deleted because the current fence uses only user and server identity. Stable association generation is required by Add stable MCP lifecycle generation identities (toby) #2031, consumed by Fence MCP OAuth persistence by association generation (toby) #2032 and Add the atomic app-scoped MCP teardown primitive (toby) #2033.
• SQLite catalog identity: plain INTEGER PRIMARY KEY can reuse the maximum ROWID after delete and recreate, so catalog generation must not rely on the integer primary key. This is addressed by Add stable MCP lifecycle generation identities (toby) #2031 and consumed by Add the atomic app-scoped MCP teardown primitive (toby) #2033.
• PostgreSQL lock evidence: the catalog-lock test currently updates the same row already protected by FOR UPDATE, so it does not isolate the table SHARE lock. Add the atomic app-scoped MCP teardown primitive (toby) #2033 requires a different catalog row, deterministic lock evidence, and a mutation that fails when the table lock is removed.

Reference boundary

No additional code should be added to this PR. Useful implementation and test evidence may be ported into the serial child PRs only when it fits their independent acceptance criteria.

[gemini-code-assist]

Code Review

This pull request introduces a robust mechanism to atomically tear down and disconnect MCP app servers. It adds teardown_mcp_app_server in src/xagent/web/api/mcp.py which uses table-level locking in PostgreSQL to serialize catalog ownership changes and prevent race conditions. It also refactors external OAuth token revocation to use snapshots of encrypted credentials before database commits, and adds comprehensive unit and integration tests. The reviewer suggested logging the original exception details with exc_info=True during teardown failures to preserve the debug trace and avoid swallowing unexpected errors.

[rogercloud]

Summary

This PR adds a new function, teardown_mcp_app_server (~250 new lines in src/xagent/web/api/mcp.py), intended to fix known race conditions and partial-deletion bugs in MCP app disconnection: it validates ownership via a preflight-issued immutable-PK token, serializes catalog mutations under a table lock plus row-level FOR UPDATE locks, deletes all related credentials/grants/flow-states/associations/server-row in one transaction, and defers external OAuth token revocation to after the commit (avoiding holding DB locks during a slow network call) via an encrypted snapshot captured pre-commit. It ships with two new test files (SQLite unit tests and PostgreSQL "two-session barrier" concurrency tests).

However, this new function currently has zero production callers. The actual user-facing endpoint, DELETE /api/mcp/servers/{server_id} (delete_mcp_server), is untouched by this diff and still exhibits every bug the PR's own description says it fixes: a split commit before calling manager.remove_server(...), deletion resolved by a mutable server name, no row/table locking, and external OAuth revocation still awaited inside the transaction. So today this PR changes no user-visible behavior at all, while introducing new, unreachable code that itself has several independently-confirmed correctness/security issues (see below).

Blocking: yes — recommended event: REQUEST_CHANGES

Design-level notes (non-blocking, for the author's awareness)

• The "single commit + post-commit best-effort revocation" ordering fix is genuinely correct and valuable, but it currently ships as a parallel, unreachable implementation rather than a fix to the actual endpoint (see F1).
• The identity-verification mechanism (table lock + reimplemented ownership resolution) works around a missing schema relationship: MCPServer has no FK to PublicMCPApp, so ownership is inferred from a mutable name field or an unstamped auth JSON blob. A real FK/backfill (the codebase already has partial _adopt_builtin_auth-style stamping) would eliminate the owner-token apparatus, the table lock, and the namespace-collision edge cases (see F4) in one move. Not requesting this as a blocking change, but worth considering before wiring this primitive into the real endpoint.
• teardown_mcp_app_server commits/rolls back a caller-owned, shared Session directly, which conflicts with the documented repo convention in connector_team_scope.py that a shared/caller-owned session must not be committed or rolled back by a callee. Not a live risk today since the function has no callers, but must be addressed before any caller (e.g. a SaaS preflight caller, per the function's own docstring) is wired up.
• The function is placed in an HTTP router module and raises HTTPException directly despite being described as a reusable "primitive" with no route decorator. Consistent with this file's existing convention for private helpers, so not a new inconsistency, but worth reconsidering if broader reuse is intended.

Findings

F1 — CRITICAL — Blocking: YES

src/xagent/web/api/mcp.py:3612 (new teardown_mcp_app_server) has zero production callers. Confirmed via grep -rn "teardown_mcp_app_server" src/ tests/: only the two new test files reference it, and it carries no @router decorator. The endpoint users actually hit, delete_mcp_server (DELETE /api/mcp/servers/{server_id}, src/xagent/web/api/mcp.py:3864), is completely unmodified by this diff and still has every bug this PR's description says it fixes: a split commit at src/xagent/web/api/mcp.py:4021 before calling manager.remove_server(server_name) at src/xagent/web/api/mcp.py:4046 (non-atomic), deletion resolved by mutable server_name rather than a validated app id, no row/table locking anywhere in the function, and external OAuth revocation still awaited inside the transaction (src/xagent/web/api/mcp.py:4005, before the commit at line 4021) — precisely the anti-pattern this PR's teardown primitive claims to fix.

Impact: the PR's stated purpose (fix teardown race conditions) is not achieved through any code path a real user can reach. Merging this PR changes zero observable production behavior while adding ~250 lines of new, unreachable, and independently buggy code (see F2-F4).

Suggested fix: either wire teardown_mcp_app_server into DELETE /api/mcp/servers/{server_id} (replacing or delegating from delete_mcp_server), or narrow the PR's scope/description to state this is preparatory/unreachable code pending a follow-up cutover.

F2 — HIGH — Blocking: YES

src/xagent/web/api/mcp.py:3827 (commit) / src/xagent/web/api/mcp.py:3837 (revocation) / src/xagent/web/api/mcp.py:3852 (swallow site). Post-commit external OAuth revocation is unretryable and effectively unobservable.

• The transaction commits at line 3827, deleting the local MCPOAuthGrant/MCPOAuthClient rows, before attempting external token revocation at lines 3833-3840 ("best-effort" by explicit design per the adjoining comments). If that post-commit HTTP call fails, there is no local record left to retry from — the encrypted grant/client rows are already gone. No dead-letter table, outbox, retry queue, or metric exists anywhere in the codebase for this. Net effect: a live third-party OAuth token can remain valid indefinitely at the provider while the user is told (and the DB agrees) the app was disconnected, with zero operator-visible signal.
• Observability regression: the failure-swallowing logger.error(...) at line 3852 has no exc_info=True and no exception argument at all. This is the exact same call site a prior review from gemini-code-assist[bot] already flagged (inline comment on this line: "always log the original exception details with exc_info=True on the server ... to prevent swallowing unexpected failures"). That comment was never addressed and remains valid.
• Additionally, this PR's diff removes the exception object/detail from several existing logger.warning calls in the shared revocation helper (src/xagent/web/api/mcp.py:1010 and src/xagent/web/api/mcp.py:1043, confirmed by diffing against base commit 9b2b8cad): those call sites previously logged %s", ..., exc and now log nothing. This is a regression that also affects the pre-existing (non-app-scoped) delete flow and the grant-delete endpoint, not just the new code.

Suggested fix: log the exception (exc_info=True) at every swallow site touched by this PR, including addressing the outstanding gemini-bot comment on line 3852; consider marking the grant revocation_pending/writing an outbox record in the same transaction instead of hard-deleting it, so a background job can retry.

F3 — HIGH — Blocking: YES

src/xagent/web/api/mcp.py:3806-3811. Data-loss race: teardown can silently delete another user's brand-new connection to a shared server. The "last user" check does SELECT ... FROM user_mcpservers WHERE mcpserver_id = ... FOR UPDATE LIMIT 1 to decide whether any other user is still connected before deleting the shared MCPServer row. Under PostgreSQL's default READ COMMITTED isolation, FOR UPDATE only locks rows that exist at query time — it creates no predicate/gap lock, so it cannot block a concurrent INSERT of a new UserMCPServer row by a different user connecting to the same server in parallel. Every install/connect code path that creates a UserMCPServer row (the connect endpoint, OAuth connect, ensure_builtin_oauth_server_visibility_for_user in mcp_apps.py, the OAuth callback path in auth.py) takes no lock on the parent MCPServer row before inserting, so teardown's earlier with_for_update() on MCPServer never contends with them.

UserMCPServer.mcpserver_id has ondelete="CASCADE" to mcp_servers.id, so db.delete(server) at commit time cascades and destroys any UserMCPServer row present at that time — including one a second user just committed after teardown's "other users" check ran but before teardown's own commit. This is realistic for a shared catalog connector (e.g. a team Slack/Gmail integration) where one member disconnects while another is connecting. Untested by either new test file.

Suggested fix: take a lock that actually serializes against concurrent inserts (e.g. pg_advisory_xact_lock keyed by server id, acquired by both the install and teardown paths), or re-check "other users" again immediately before the final delete under a lock the insert path also honors.

F4 — MEDIUM — Blocking: YES

src/xagent/web/api/mcp.py:3591 (_server_belongs_to_exact_catalog_app). The non-OAuth-transport ownership shortcut only compares server.name == app_id and never consults the auth["app_id"] stamp, unlike the OAuth-transport branch and unlike the codebase's general resolver get_app_for_mcp_server (src/xagent/web/api/mcp_apps.py:729), which trusts the stamp regardless of transport. Since PUT /servers/{server_id} (update_mcp_server) lets an owning user freely rename any of their connected servers (it only blocks squatting a new catalog name, not renaming away from an existing catalog-provisioned one), a legitimate user can rename their connected non-OAuth catalog server and then find teardown_mcp_app_server permanently returns 403 "MCP app teardown owner changed" for that server — a false-positive, permanent lockout with no adversarial action involved.

(Note: a related theory that ambiguous display-name collisions create a security bypass — permit when it should reject — was investigated and refuted; MCPServer.name is always provisioned from the app's own unique app_id, never a mutable display name, so that specific bypass does not exist. Only the false-negative/lockout direction above is real.)

Suggested fix: have the non-OAuth branch also honor a genuine auth["app_id"] stamp when present, consistent with the OAuth branch and with get_app_for_mcp_server.

M1 — MAJOR — Blocking: NO

src/xagent/web/api/mcp.py:3663 (pinning) / src/xagent/web/api/mcp.py:3736 (use). The owner-token re-validation pins PublicMCPApp.id + app_id but not provider_name, which is read fresh (not pinned) later in the function to decide which OAuth credentials to delete. provider_name is mutable via an admin PATCH endpoint (protected only for builtin apps). If an admin changes an app's provider_name between a user's preflight and their teardown call, teardown can delete/revoke a different provider's credentials than what was validated at preflight. Requires admin action to trigger and doesn't cross a privilege boundary for the admin, but does misdirect credential deletion for an uninvolved end user. Untested.

Suggested fix: pin provider_name at preflight time and re-validate it alongside id/app_id.

M2 — MAJOR — Blocking: NO

src/xagent/web/api/mcp.py:3730-3775 vs src/xagent/web/api/mcp.py:3990-4008, and src/xagent/web/api/mcp.py:3580-3609 vs mcp_apps.py:729. ~90-110 lines of near-duplicated security-sensitive logic between the new teardown_mcp_app_server and the existing delete_mcp_server: the OAuth credential-cleanup + sibling-provider-retention scan is near-verbatim duplicated, and the owner-resolution algorithm in _server_belongs_to_exact_catalog_app reimplements the identical owners == {app_id} ambiguity logic already in get_app_for_mcp_server (mcp_apps.py:729-780) instead of reusing it. A future fix to one copy (e.g. the sibling-retention scan) can easily be missed in the other.

Suggested fix: extract the shared OAuth-cleanup logic into a common helper; have the ownership gate call get_app_for_mcp_server once and reuse its result instead of re-querying/re-implementing it and then calling it again later for app_info.

M3 — MAJOR — Blocking: NO

The PR's core concurrency mechanism (table lock + row-level with_for_update()) is not exercised by any test that actually runs in CI. .github/workflows/ci.yml filters out all postgresql-marked tests in every job (-m "not slow and not postgresql" / -m "slow and not postgresql"), and .github/workflows/test-migrations.yml's hand-maintained postgres test allowlist does not include the new tests/web/api/test_mcp_app_teardown_postgresql.py, so that file never runs in CI. SQLite is the documented default production database for this app, and the new table-lock helper explicitly skips itself on non-Postgres dialects while SQLAlchemy's with_for_update() is a known no-op on SQLite, so even the SQLite tests that do run exercise none of the actual locking behavior. The PR description itself admits the 3 PostgreSQL tests were never run locally either. Combined with F3/F4 above (real concurrency bugs found in the code this mechanism is supposed to protect), this significantly undercuts confidence that the concurrency guarantees this PR is built around actually hold.

Suggested fix: add the new PostgreSQL test file to test-migrations.yml's allowlist so it actually runs in CI.

Minor (non-blocking)

• tests/web/api/test_mcp_app_teardown_postgresql.py's schema setup (tables=[...] at two call sites) omits mcp_oauth_clients/mcp_oauth_grants/mcp_oauth_flow_states, which the code under test queries unconditionally — if this file were ever run, test_catalog_mutation_waits_while_teardown_holds_identity_locks would error with "relation does not exist" rather than pass (the other parametrized test happens to short-circuit with a 403 before reaching that code). This further undermines the "7 tests passed" validation claim in the PR description.
• Inactive/revoked MCPOAuthGrant rows are excluded from teardown's deletion (status == "active" filter, src/xagent/web/api/mcp.py:3782) and can survive with live encrypted tokens still in the DB — but this filter is inherited from pre-existing code in delete_mcp_server, not new to this PR, and the affected grant would already have had external revocation attempted when it was individually revoked earlier, so this is low urgency.
• Missing test coverage for: non-owner teardown attempt (403 only manually verified, not asserted in a test), malformed-identity/argument validation branch, the multi-user path where the server survives teardown (every existing test has exactly one user), the sibling-provider-retention branch (the single largest untested logical block), two concurrent teardowns of the same app, and the F3 race scenario (concurrent install racing teardown).
• Confusing parameter naming: app_id: str in teardown_mcp_app_server's signature maps to PublicMCPApp.app_id, while expected_catalog_app_id: int maps to PublicMCPApp.id (the PK) — the name without "app_id" in it is the one that isn't the app_id. Purely a readability nit; the logic is correct.
• Assorted style nits: @dataclass(frozen=True) on _MCPOAuthRevocationSnapshot gives false immutability since it holds a mutable dict field; duplicated ~5-line test-setup boilerplate between the two PostgreSQL test functions instead of a shared fixture; redundant @pytest.mark.asyncio decorators given asyncio_mode = "auto" in pyproject.toml; malformed-argument validation returns 403 instead of a 500/assertion for what are effectively internal-caller programming errors; three distinct "owner changed" failure conditions collapse into one identical opaque 403 message (arguably should be 409 Conflict, and distinguishable for debugging).

Blocking status & recommended decision

• src/xagent/web/api/mcp.py:3612 — CRITICAL — new teardown primitive has zero production callers; the real endpoint keeps every bug this PR claims to fix. [new]
• src/xagent/web/api/mcp.py:3827 — HIGH — post-commit revocation is unretryable/unobservable; carries forward an unaddressed prior review comment on line 3852 about missing exc_info=True. [new]
• src/xagent/web/api/mcp.py:3806 — HIGH — FOR UPDATE "last user" check has no gap lock, allowing a concurrent connect to be silently cascade-deleted. [new]
• src/xagent/web/api/mcp.py:3591 — MEDIUM — non-OAuth ownership check ignores the auth["app_id"] stamp, causing a permanent 403 lockout after a legitimate rename. [new]

Recommended event: REQUEST_CHANGES

[OliverBryant]

Review follow-up summary

Accepted fixes:

• The primitive contract now requires expected_provider_name together with the exact app_id and immutable PublicMCPApp.id. The catalog lock and row lock revalidate all three values before any destructive write; provider drift fails closed with 403.
• Locked ownership validation now returns the validated catalog row. Target credential cleanup consumes that row directly and no longer invokes the mutable generic resolver a second time.
• Teardown deletes every grant status for the disconnecting user/server. Only active grants produce external-revocation snapshots; revoked and inactive encrypted token rows are still removed locally. Other users and their grants remain intact while the shared server remains.
• PostgreSQL fixtures now create the OAuth client, grant, and flow tables. The PostgreSQL suite covers delete, rename/reassignment, provider drift, catalog-lock serialization, and a concurrent association insert against the locked parent server.
• The migration workflow now routes the teardown PostgreSQL file to the real PostgreSQL service, and the push-path and runtime allowlists remain mirrored.
• A regression test injects a secret-bearing arbitrary exception and verifies that neither the raw upstream detail nor the credential marker enters logs.

Deliberately unchanged findings:

• The generic upstream DELETE route is not rewired. This is a preparatory cross-repository primitive for xagent-saas A3 PR feat(workforce): REST API/SDK deployment #949, whose preflight can supply stable SaaS identity; the generic route cannot supply that identity without a separate contract change.
• External revoke remains post-commit best-effort. Local destructive state is atomic. An outbox is not added because the primitive deliberately avoids remote-success/local-rollback splits and unbounded network I/O while catalog/server locks are held.
• Arbitrary exception objects and tracebacks are not logged. Reproduction confirmed that exc_info=True emits potentially secret-bearing exception messages and causes, while SQLAlchemy hide_parameters only redacts SQL bind parameters. Sanitized operator signals remain available through grant id, token kind, auth method, HTTP status, and safe app/server identifiers.
• Non-OAuth ownership continues to require provisioned name provenance rather than trusting a caller-authored auth blob. Normal catalog users receive non-owner/non-editor associations and cannot rename the global row; an administrator mutation intentionally invalidates a stale preflight and fails closed.

Validation on this head:

• 7 directly related SQLite teardown tests passed.
• 20 targeted sibling OAuth delete/revoke and app-resolution tests passed.
• Both required mutations were verified: removing the provider gate makes the provider-drift test fail, and restoring the active-only grant filter leaves revoked/inactive grants and makes the multi-user test fail.
• Ruff, Python compilation, diff checks, workflow YAML parsing, and normalized workflow allowlist mirroring passed.
• 5 PostgreSQL cases collect locally. They were not executed locally because XAGENT_TEST_POSTGRES_URL is not configured.
• CI is currently running on 666d07eb: Test SQLite Migrations has passed, while Test PostgreSQL Migrations and the remaining pull-request checks are still in progress. The PostgreSQL concurrency finding will not be closed until that real PostgreSQL job passes.

[rogercloud]

This PR adds an app-scoped MCP teardown primitive in src/xagent/web/api/mcp.py that revalidates immutable catalog identity and provider data under PostgreSQL catalog/server locks, performs local credential/grant/flow/association/server cleanup in one transaction, and snapshots OAuth material for post-commit revocation. It also adds SQLite and PostgreSQL teardown coverage and routes the PostgreSQL concurrency file through the migration workflow. The core direction fixes split-commit and mutable-identity hazards, but an in-flight OAuth connect/callback can still write a flow or grant after disconnect commits. Blocking: yes — recommended event: REQUEST_CHANGES

Approach verdict

acceptable-with-reservations

The core approach is sound: immutable catalog-row identity plus provider pinning, lock-time revalidation, one local commit, rollback on local failure, and post-commit best-effort revocation address the intended split-commit and mutable-catalog hazards without holding database locks over provider I/O. Round 0's design-level reservation is the OAuth producer race described below; it is the same root as new finding N2, not an additional duplicate. The no-production-caller observation is intentionally not reported as a finding: the linked xagent-saas PR #949 documents this as a preparatory cross-repository primitive whose future consumer supplies the SaaS preflight identity.

New findings

[new] N2 — OAuth producers can write after teardown

File: src/xagent/web/api/mcp.py:3781
Severity: major
Blocking: yes

Reachable trigger: A shared remote-MCP OAuth server has users U1 and U2. U1's callback can read an active association, claim and commit its flow state, and then wait for the provider code exchange. While that network request is in flight, U1's app teardown locks the server, deletes U1's currently visible grants, flow state, and association, observes U2's association, and commits while retaining the shared server and OAuth client. The callback then resumes with its already-loaded flow data and upserts/commits a fresh grant. The generic OAuth connect path has the analogous window: it checks the association before discovery or dynamic client registration, then persists a flow after that external I/O.

Concrete impact: The post-teardown grant was absent from the teardown revocation snapshot, so its encrypted access/refresh token remains locally and the provider token remains live. The grant or flow can be orphaned from U1's association and can become visible again if U1 reconnects; a later server teardown may cascade the row without ever having snapshotted that token. This violates the app-scoped disconnect contract even though the original teardown returned success.

Contract/invariant evidence: Teardown only fences rows visible when it queries them. MCPOAuthGrant and MCPOAuthFlowState reference the server, user, and OAuth client, but neither has a UserMCPServer foreign key. Therefore PostgreSQL's parent/server lock can delay a producer write but cannot reject it when U2 deliberately keeps the parent server. The callback's active-association check is an unlocked preflight before claim, provider I/O, and grant persistence, not a final membership invariant. The primitive is latent in the current repository because it has no in-repository production caller, but this is a confirmed defect in the new app-scoped teardown contract and is blocking for the intended SaaS consumer.

Specific suggestion: Make the producer and teardown use one lifecycle fence: immediately before persisting a flow or grant after external I/O, lock the server and revalidate the user's active UserMCPServer association (and flow/cancellation state). If teardown won, reject local persistence and revoke the token just obtained, or durably queue the encrypted material for revocation; do not rely on the parent lock alone or hold database locks across provider I/O.

[new] N1 — SQLite teardown has no ownership fence

File: src/xagent/web/api/mcp.py:3576
Severity: major
Blocking: no

Reachable trigger: The added helper takes a public_mcp_apps table lock only when the dialect is PostgreSQL; the SQLite branch at this line is a no-op. With two independent SQLite sessions/connections in WAL mode, a supported admin catalog PATCH, DELETE/recreate, provider update, or name reassignment can commit after teardown has read and validated the catalog row, server, association, and legacy owner set, but before the first destructive DML. SQLite compiles with_for_update() away, so those revalidation reads do not provide a write-intent fence.

Concrete impact: The stale validated_app can drive cleanup under an owner/provider identity that no longer matches the catalog: provider drift can delete the wrong provider-scoped credential, legacy name ownership can select the wrong app's credential or association/server, and delete/recreate can remove a server for a replacement catalog definition. If the SQLite snapshot must be upgraded after the competing write, the broad handler can return a sanitized 500 rather than a fail-closed 403.

Contract/invariant evidence: expire_all() runs only before the revalidation reads, and there is no catalog foreign key tying MCPServer to PublicMCPApp; neither prevents a second connection's commit in the read-to-first-write window. This is major but non-blocking for the current head because the primitive has no in-repository production caller. Once the intended downstream SaaS caller wires this primitive into supported SQLite use, the same data-integrity impact would independently meet the blocking bar.

Specific suggestion: Establish a dialect-appropriate SQLite write-intent/transaction-safe fence before any ownership read, such as an explicit BEGIN IMMEDIATE equivalent compatible with the caller's transaction contract, and keep it through local cleanup. Add a file-backed two-connection WAL regression test that mutates provider/name or delete-recreates the catalog row after preflight and proves teardown fails closed without deleting credentials or the server.

[new] N3 — PostgreSQL catalog-lock test can pass before the mutation is sent

File: tests/web/api/test_mcp_app_teardown_postgresql.py:216
Severity: minor
Blocking: no

Reachable trigger: After submitting the rename worker, the test waits only for mutation_committed to remain unset for 250 ms. There is no statement-sent or worker-start barrier. Thread scheduling, connection setup, or the worker's initial SELECT can consume that interval before it reaches the dirty-row UPDATE or commit attempt.

Concrete impact: The assertion can pass even if the catalog SHARE lock is removed or ineffective. Releasing teardown then allows the rename to commit, and the later final assertions still pass, so CI can report success without exercising the intended PostgreSQL serialization contract.

Contract/invariant evidence: With autoflush=False, assigning app.name is in memory; the UPDATE is emitted by mutation_db.commit(), and mutation_committed.set() occurs only afterward. The adjacent association-insert test already observes a statement-sent event before asserting that commit is blocked, but this catalog mutation test does not.

Specific suggestion: Add a SQLAlchemy before_cursor_execute listener filtered to the mutation thread and UPDATE public_mcp_apps, set a mutation_sent event, wait for that event, and only then assert mutation_committed remains unset until teardown is released. Remove the listener in finally.

Update summary

The initial implementation landed in e7da6b39. Update 666d07eb adds required expected_provider_name pinning and locked-row reuse, deletes every local grant status while snapshotting only active grants, completes PostgreSQL OAuth-table setup, mirrors workflow path/detector/runtime routing, and adds regression and secret-safe logging coverage. Those changes explain the P6, P7, P8, P9, and P10 outcomes below; the generic route remains intentionally unwired for the cross-repository handoff, and the N2 producer fence is still missing.

Prior-findings checklist

Root	Status	Evidence and history
P1	DROPPED	The no-production-caller condition is an intentional cross-repository preparatory boundary, tracked by xagent-saas PR #949, not a current xagent contract failure. The exact identity primitive remains directly exercised by tests and is documented for the downstream preflight. Sources: review 5075764771, inline 3902244043 → reply 3902539424, conversation 5491613761.
P2	DROPPED	Verified-safe sanitized exception boundaries: app/server, grant, token-kind, auth-method, and HTTP-status signals remain, while arbitrary exception text and tracebacks are not required by a demonstrated contract; the f-string request was style-only. Sources: review 5075436104, inline 3901976139 → reply 3902204616, inline 3902244351 → reply 3902539715, conversation 5491613761.
P3	DROPPED	The post-commit provider revoke is explicitly local-atomic and best-effort, with no current eventual-revocation, retry, outbox, or outcome-channel contract. This is distinct from P2's logging root. Sources: review 5075764771 F2, inline 3902244351 → 3902539715, conversation 5491613761, and the linked issue #719 and issue #659.
P4	DROPPED	Verified-safe under the supported PostgreSQL schema: the existing parent MCPServer FOR UPDATE conflicts with the child FK KEY SHARE, so an association insert is observed first or waits and receives an FK failure if the parent is deleted. The added barrier test corroborates this contract. Sources: review 5075764771 F3, inline 3902244592, conversation 5491613761.
P5	DROPPED	Verified-safe: catalog-provisioned associations are non-owner/non-editor, ordinary users cannot rename the shared definition, and an administrator mutation intentionally invalidates stale preflight and fails closed. Sources: review 5075764771 F4, inline 3902244821 → reply 3902539897, conversation 5491613761.
P6	FIXED	expected_provider_name is now required and revalidated with the immutable catalog PK and public app ID; the locked row drives both cleanup branches. Sources: review 5075764771 M1, inline 3902245009 → reply 3902540119, conversation 5491613761.
P7	DROPPED	The target resolver duplication was refactored away; the remaining sibling scan and generic-route overlap intentionally differ in transaction, grant-status, revocation, and server-removal contracts, with no demonstrated present impact. Sources: review 5075764771 M2, inline 3902245196 → reply 3902540338, conversation 5491613761.
P8	FIXED	The PostgreSQL teardown file is now present in the push-path and detector allowlists and has an explicit runtime step in the migration workflow. Sources: review 5075764771 M3, conversation 5491613761.
P9	FIXED	The PostgreSQL fixture now creates MCPOAuthClient, MCPOAuthGrant, and MCPOAuthFlowState along with their dependencies at every setup callsite. Sources: review 5075764771 minor schema note, conversation 5491613761.
P10	FIXED	The teardown selects and deletes all grant statuses locally; only active grants are snapshotted for external revocation. Sources: review 5075764771 minor active-filter note, conversation 5491613761.
P11	DROPPED	Coverage-only gaps do not establish a product failure. The F3/concurrent-install bullet is the P4 root and is not a second finding; shared-server survival and the association barrier are covered. Sources: review 5075764771 minor coverage list, conversation 5491613761.
P12	DROPPED	The app_id versus immutable PublicMCPApp.id distinction is explicit in the keyword-only signature, type checks, and documentation; no supported misuse or impact was established. Source: review 5075764771 body-only naming note.
P13	DROPPED	Frozen-dataclass, setup repetition, asyncio-marker, malformed-argument, and opaque-error-shape suggestions are style/defensive preferences without a demonstrated present contract failure. Source: review 5075764771 body-only style/error-shape notes.
P14	DROPPED	Transaction ownership is an intentional use-case boundary: subordinate hooks do not commit, while this atomic primitive owns its local commit/rollback. No supported production caller currently creates the alleged shared-session impact. Sources: review 5075764771, conversation 5491613761.
P15	DROPPED	A direct MCPServer to PublicMCPApp FK/backfill is an alternative redesign, not a required fix; current immutable-token and fail-closed provenance checks establish no supported wrong-owner failure. Source: review 5075764771; follow-up context in conversation 5491613761.
P16	DROPPED	The helper's router-module placement and HTTPException use follow existing mcp.py conventions, and the undecorated function is not an HTTP route. Source: review 5075764771 body-only module-boundary note.

History notes

The complete prior review bodies, conversation record, inline comments, and reply chains were considered. P2 and P3 share inline root 3902244351 only as distinct underlying roots (sanitized logging versus provider-revocation durability); P1's manifest overlap 3901976139 belongs to P2, not P1. The concurrent-install item in P11 is the P4 root. P1 is tracked by #949, while P3's explicit best-effort contract is documented by #719 and #659. The prior request-changes review is from rogercloud and the current PR author is OliverBryant; normal status handling applies and no qinxuye waiver was applied.

Blocking status & recommended decision

Blocking: yes.

Blocking issue:

• src/xagent/web/api/mcp.py:3781 — major — [new] An in-flight OAuth producer can persist a fresh encrypted grant/flow after teardown commits, leaving the provider token live and violating disconnect cleanup.

N1 remains major, Blocking: no for this head because the primitive has no in-repository production caller, and N3 remains minor, Blocking: no; neither belongs in the blocking-issues list.

Recommended event: REQUEST_CHANGES

[OliverBryant]

Implemented the accepted N1, N2, and N3 fixes on head 8ae2e6f and resolved all three review threads.

N1 — SQLite lifecycle fencing

• Teardown now establishes a real DBAPI BEGIN IMMEDIATE write reservation before any identity read.
• SQLAlchemy logical autobegin is reset only for a read-only preflight. Pending ORM writes or an existing real DBAPI transaction fail closed without committing or rolling back caller state.
• File-backed WAL two-connection cases cover provider drift and delete/recreate with mutation statement barriers.

N2 — OAuth lifecycle resurrection

• Connect and callback now perform all discovery, dynamic registration, and token exchange before entering the final persistence fence.
• The shared lock order is server, active current-user association, then exact original flow for callbacks.
• Callback identity pins flow id, state, server, user, and client, so a fast reconnect cannot make an old callback valid again.
• Teardown-first rejects producer persistence. Producer-first commits under the server lock and teardown subsequently removes the new rows.
• Issued access and refresh tokens are snapshotted before revalidation. A lifecycle or database persistence failure rolls back first, then performs best-effort revocation.

N3 — PostgreSQL barrier accuracy

• The catalog rename test observes the rename worker and exact UPDATE public_mcp_apps statement through before_cursor_execute before checking that commit is blocked.
• The listener is always removed in finally.

Additional self-review fixes

• Connect final persistence now owns an immediate rollback boundary for every exception after acquiring lifecycle locks.
• Callback database persistence failures revoke the newly issued token after rollback.
• Post-claim redirects use a captured safe path instead of accessing an ORM flow row that teardown may already have deleted.
• The broad callback persistence error log now uses fixed text and does not include arbitrary exception messages, causes, credentials, or raw upstream details.

Race evidence and mutations

• Separate tests preserve both producer-first and teardown-first ordering, fast reconnect with an old flow, shared-server multi-user survival, and statement-sent barriers.
• Removing BEGIN IMMEDIATE fails the SQLite provider-drift barrier case.
• Removing the active-association fence makes the connect revalidation case fail.
• Removing exact flow identity makes the fast-reconnect callback case fail.
• Removing failed-persistence token revocation makes the revocation and safe-logging case fail.

Validation

• 89 directly related local SQLite and OAuth tests pass.
• Ruff format check, Ruff lint, py_compile, and git diff --check pass.
• The PostgreSQL file collects 8 cases locally. XAGENT_TEST_POSTGRES_URL is not configured locally, so no local PostgreSQL runtime pass is claimed.
• The Test Database Migrations workflow and its dedicated Test app-scoped MCP teardown concurrency PostgreSQL step were triggered for head 8ae2e6f. At the time of this comment that step had not been confirmed complete, so no PostgreSQL pass is claimed here.

The branch was rebased without conflicts onto upstream/main ddcabb7 and updated with an explicit force-with-lease protecting the previous remote head 666d07e.

[rogercloud]

This PR adds an app-scoped MCP teardown primitive that revalidates catalog identity, performs local credential/grant/flow/association/server cleanup in one transaction, and snapshots OAuth material for post-commit revocation. It adds SQLite and PostgreSQL concurrency coverage and CI routing, and now fences remote MCP OAuth connect/callback persistence against teardown; the remaining review concerns are a replacement-association ABA and a reused SQLite catalog token, plus a partial PostgreSQL lock test. Blocking: yes — recommended event: REQUEST_CHANGES

Approach verdict

acceptable-with-reservations

The operation-level direction is coherent: the caller supplies public app identity, catalog-row identity, and provider identity; teardown acquires a dialect-appropriate catalog fence, expires stale ORM state, revalidates under locks, performs one local commit, and revokes captured OAuth material only after the commit. The remote MCP OAuth producers now use the same server/association/flow lock order after provider I/O, which closes the original teardown-first resurrection window without holding locks over network calls.

The reservations are concrete rather than architectural preferences. The lifecycle fence still identifies UserMCPServer only by (user_id, server_id, is_active), so it cannot distinguish an old association A from a replacement B. The supposedly immutable PublicMCPApp.id is also a reusable SQLite ROWID, so a delete/recreate can cross the catalog generation boundary despite the new BEGIN IMMEDIATE fence. Finally, the PostgreSQL test now waits for the UPDATE statement to be submitted, but the same-row FOR UPDATE lock independently blocks that UPDATE, so the test does not prove that the catalog table SHARE lock is effective.

Update summary

Since the previous review at 666d07ebd31f55a51ef05929566b7281bbd504a5, the relevant update commits added the SQLite BEGIN IMMEDIATE ownership fence, the final server/association/flow fences for remote MCP OAuth connect and callback persistence, failed-token compensation, and the PostgreSQL mutation statement barrier; 9cf987b47a79e82317bf3fbd97ef8ab0d551734a then normalized PostgreSQL test imports. These changes make N1 fixed and N2 fixed and remove N3's worker-start/initial-SELECT timing gap, but they do not add association-generation identity and do not make the N3 assertion specific to the catalog table lock.

Prior-findings checklist

Root	Status	Sources and current-code evidence
P1	DROPPED	The no-production-caller observation remains true, but it is an intentional cross-repository preparatory boundary. Sources: review 5075764771, inline 3902244043 → reply 3902539424, and conversation 5491613761. xagent-saas PR #949 and issue #911 explicitly track the downstream caller and describe this layer as no-production-caller preparation. The helper is still undecorated and the generic DELETE route is still separate, but those pre-existing route facts are not a PR-caused contract failure here.
P2	DROPPED	Sources: Gemini review 5075436104, review 5075764771 F2, inline 3901976139 → reply 3902204616, and the P2 portion of inline 3902244351 → reply 3902539715, with conversation 5491613761. Current changed catch sites log bounded app/server/grant/status/token-kind signals without arbitrary exception text or tracebacks, including src/xagent/web/api/mcp.py:1043-1049, 1076-1083, 1107-1111, 4082-4089, 4112-4122. The linked xagent-saas PR #949 contract deliberately excludes credentials and arbitrary upstream details, so the requested exc_info is not required.
P3	DROPPED for this review (accepted/tracked)	Sources: P3 portion of review 5075764771 F2, inline 3902244351 → reply 3902539715, and conversation 5491613761. The technical behavior remains local commit followed by best-effort provider revocation, with no durable retry/outcome channel, but xagent-saas issue #719 and issue #659 explicitly document and track that boundary. It is not re-reported as a new finding.
P4	DROPPED	Sources: review 5075764771 F3, inline 3902244592 → reply 3902604271, and conversation 5491613761. Under the supported immediate, non-deferrable UserMCPServer.mcpserver_id FK, a child insert takes PostgreSQL KEY SHARE on the parent and conflicts with teardown's MCPServer FOR UPDATE at src/xagent/web/api/mcp.py:3925-3930; the current barrier test covers the resulting wait/rejection at tests/web/api/test_mcp_app_teardown_postgresql.py:390-482.
P5	DROPPED	Sources: review 5075764771 F4, inline 3902244821 → reply 3902539897, and conversation 5491613761. Catalog-provisioned associations are non-owner/non-editor, so the alleged normal-user rename trigger is unavailable; administrators can mutate catalog rows, but the resulting stale preflight is intentionally rejected. The non-OAuth branch at src/xagent/web/api/mcp.py:3832-3835 therefore remains a fail-closed provenance rule rather than a reachable ordinary-user lockout.
P6	FIXED	Sources: review 5075764771 M1, inline 3902245009 → reply 3902540119, and conversation 5491613761. expected_provider_name is required at src/xagent/web/api/mcp.py:3862, revalidated with the catalog PK and public app ID at :3910-3917, and the locked row's provider_name drives cleanup at :3981-3995; provider drift fails closed before destructive work.
P7	DROPPED	Sources: review 5075764771 M2, inline 3902245196 → reply 3902540338, and conversation 5491613761. Target resolver duplication was removed: _locked_catalog_app_for_server at src/xagent/web/api/mcp.py:3824-3854 reuses the locked row, while the remaining sibling resolver and generic-route cleanup intentionally have different transaction and revocation contracts. The generic route is byte-for-byte unchanged from base, and no present divergence impact was established.
P8	FIXED	Sources: review 5075764771 M3 and conversation 5491613761. The PostgreSQL teardown file is now in the push-path and detector lists at .github/workflows/test-migrations.yml:55-59, 148-152, with an explicit PostgreSQL runtime step at :359-365.
P9	FIXED	Sources: review 5075764771 minor schema note and conversation 5491613761. The shared PostgreSQL fixture now creates MCPOAuthClient, MCPOAuthGrant, and MCPOAuthFlowState with their parents in tests/web/api/test_mcp_app_teardown_postgresql.py:29-55, eliminating the previously omitted OAuth tables at all setup call sites.
P10	FIXED	Sources: review 5075764771 minor active-filter note and conversation 5491613761. The current teardown loads and deletes every grant status at src/xagent/web/api/mcp.py:4025-4042; only active grants are snapshotted for external revocation at :4034-4040. The SQLite test at tests/web/api/test_mcp_app_teardown.py:431-508 preserves sibling rows while removing active, revoked, and inactive rows for the disconnecting user.
P11	DROPPED	Sources: review 5075764771 coverage list and conversation 5491613761. Remaining omissions are coverage breadth only, without a demonstrated product failure; multi-user survival and the parent association race are covered, and the concurrent-install bullet is the P4 root rather than a separate finding.
P12	DROPPED	Source: review 5075764771 body-only naming note. The keyword-only signature at src/xagent/web/api/mcp.py:3857-3879 documents that app_id is the public key while expected_catalog_app_id is the PK, and typed callers use the named arguments; no supported misuse or impact was established.
P13	DROPPED	Sources: review 5075764771 body-only style/error-shape notes and conversation 5491613761 context. The frozen-dataclass, setup repetition, redundant marker, malformed-argument status, and opaque-403 suggestions remain style/defensive preferences without a demonstrated contract failure.
P14	DROPPED	Sources: review 5075764771 design note and conversation 5491613761. The primitive intentionally owns its single local commit/rollback at src/xagent/web/api/mcp.py:4076, subordinate hooks do not commit, and no supported production caller currently supplies unrelated pending writes to this preparatory helper.
P15	DROPPED	Sources: review 5075764771 design note and conversation 5491613761. A direct MCPServer–PublicMCPApp FK/backfill is an alternative redesign, not a required fix: arbitrary custom servers and ambiguous legacy provenance make a mandatory relation incompatible with the established catalog-delete contract, while the current resolver fails closed.
P16	DROPPED	Sources: review 5075764771 module-boundary note and conversation 5491613761. The undecorated primitive and direct HTTPException use follow existing mcp.py helper conventions and match the downstream status-mapping contract; no present compatibility failure was established.
N1	FIXED	Sources: review 5076655033 N1, inline 3902969821 → reply 3903292257, and conversation 5492749959. _lock_catalog_for_app_teardown now dispatches SQLite to _begin_sqlite_write_intent; BEGIN IMMEDIATE is acquired before identity reads at src/xagent/web/api/mcp.py:3614-3661, and local cleanup remains in that transaction. The file-backed WAL mutation barriers cover the post-fence provider-drift/delete-recreate window.
N2	FIXED	Sources: review 5076655033 N2, inline 3902969812 → reply 3903291847, and conversation 5492749959. Remote MCP connect now fences persistence after discovery/DCR through the server and active-association lock at src/xagent/web/api/mcp.py:3673-3821; callback snapshots issued tokens and revalidates server, active association, and exact original flow before persisting at :4802-4829, revoking on a lost lifecycle race.
N3	PARTIAL — minor, Blocking: no	Sources: review 5076655033 N3, inline 3902969827 → reply 3903292175, and conversation 5492749959. The added mutation_sent listener and wait at tests/web/api/test_mcp_app_teardown_postgresql.py:327-369 fix the worker-start/initial-SELECT gap, but the rename still targets the same catalog row teardown holds with FOR UPDATE at src/xagent/web/api/mcp.py:3910-3917. Thus the test can still pass if the catalog table SHARE lock is removed, and the 250 ms negative wait remains timing-based; this surviving issue is reported below.

Confirmed findings

[new] A2 — replacement association ABA crosses the lifecycle fence

Location: src/xagent/web/api/mcp.py:3700 (related teardown association lock at :3937-3944; connect preflight/persistence at :4903-5008)

Severity: major
Blocking: yes

Reachable trigger. A shared MCPServer is retained by another user, a team-ownership rule, or a platform key. User U1 has association A and starts a supported remote-MCP OAuth connect; the route reads A, then performs discovery or dynamic client registration. While that provider I/O is in flight, a disconnect deletes A and commits, and U1 reconnects before the delayed producer reaches its final fence, creating active association B for the same server. The same A→B sequence is possible for the downstream app teardown: its preflight observes A, A is deleted, and B is created before the teardown helper acquires the server lock.

Concrete impact. _lock_active_mcp_oauth_lifecycle accepts B because its predicate matches only user_id, mcpserver_id, and is_active; it does not prove that B is the association the old connect started from. The stale connect can therefore persist a new flow/client under B, allowing an old authorization attempt to continue in the new lifecycle. A stale teardown likewise locks and deletes B, including the newly reconnected association's grants and flow state (and potentially the shared server if no other association remains). This causes user-visible reconnection loss and can remove fresh credential state created after the disconnect.

Contract and invariant analysis. The existing final fence correctly fixes N2 when the original association is gone, but it has no generation identity once a replacement row exists. The teardown query at :3937-3944 has the same tuple-only identity problem, and connect_mcp_oauth discards the association returned by _get_user_mcp_server_or_404 at :4903-4906 before provider I/O. The (user_id, mcpserver_id) uniqueness constraint only prevents simultaneous duplicates; after A is deleted it permits B, and the supported association creators do not acquire the server lock before creating B. PostgreSQL FK KEY SHARE serialization prevents an insert racing a held parent FOR UPDATE, but it cannot distinguish or reject B created before the stale operation obtains that lock. SQLite write-intent fencing serializes later writes but likewise does not make the association identity generation-stable.

Specific fix. Add a non-reusable immutable lifecycle token to UserMCPServer (preferably a UUID or schema-guaranteed version, not a plain SQLite integer key). Capture it in connect and teardown preflight, pass it through both lifecycle helpers, and include it in the locked association predicate. Add PostgreSQL and SQLite A→B races proving that stale teardown rejects B and stale connect cannot persist under B.

[new] A1 — SQLite catalog ROWID reuse defeats the owner-generation token

Location: src/xagent/web/api/mcp.py:3912 (related token contract at :3868-3874; masking test setup at tests/web/api/test_mcp_app_teardown.py:614, 699-712)

Severity: major
Blocking: no

Reachable trigger. PublicMCPApp.id is a plain SQLite INTEGER PRIMARY KEY, so it aliases ROWID and may be reused after deleting the maximum row. A caller can preflight a newly created, highest-id catalog app and record (id=N, app_id=A, provider=P). Before teardown_mcp_app_server acquires its BEGIN IMMEDIATE, a supported administrator deletes that app and recreates the same app_id with the same provider_name; SQLite can assign the replacement the same id=N.

Concrete impact. The locked predicate at :3912-3917 then accepts the replacement because all three values still match. The stale teardown proceeds to use the replacement provider and delete its user credential, grants, flows, association, and possibly shared server. This crosses exactly the delete/recreate boundary that the docstring at :3868-3874 claims the catalog PK distinguishes, producing an incorrect destructive result rather than a fail-closed 403.

Contract and invariant analysis. The new BEGIN IMMEDIATE fence is effective only after the helper starts; it cannot identify a delete/recreate that committed between the caller's preflight and helper entry. The existing schema uses mapped_column(Integer, primary_key=True) and the original migration creates a plain integer primary key without AUTOINCREMENT; there is no tombstone or separate generation consulted by teardown. The new tests avoid the supported trigger by inserting a pk-keeper row and, in the provider-drift case, changing the provider, so they do not exercise maximum-row reuse with identical provider identity. The primitive has no in-repository production caller, which is why this major correctness defect is Blocking: no on the current head, but the failure is real whenever the intended consumer invokes it on supported SQLite.

Specific fix. Introduce a durable non-reusable catalog-generation token, preferably a random UUID with a migration/backfill for existing rows, carry it through the downstream preflight, and revalidate it under the existing locks. Alternatively, perform a real SQLite AUTOINCREMENT migration for this table on every supported deployment. Add a regression that deletes and recreates the highest or only eligible row with the same app_id and provider_name, without a keeper row, and asserts 403 with all user/server state intact.

[prior] N3 — the PostgreSQL catalog-lock test is still false-green

Location: tests/web/api/test_mcp_app_teardown_postgresql.py:368 (related listener at :327-337 and teardown row lock at src/xagent/web/api/mcp.py:3910-3917)

Severity: minor
Blocking: no

Reachable trigger. The test now waits for mutation_sent, so the original scheduling gap is gone. However, the mutation updates expected_pk, the same PublicMCPApp row that teardown has already selected with FOR UPDATE. PostgreSQL therefore blocks the UPDATE even if LOCK TABLE public_mcp_apps IN SHARE MODE is removed. In addition, before_cursor_execute fires before DBAPI execution and mutation_committed.wait(timeout=0.25) is still a timing-based absence check.

Concrete impact. CI can report success without proving the catalog table SHARE lock serializes catalog writes. The test would continue to pass under a broken or removed table-level fence, so the central legacy-name ownership guarantee remains insufficiently exercised.

Contract and invariant analysis. The test's gated_identity pauses after the locked identity helper returns, which keeps the expected catalog row lock held. The rename worker then updates that exact row and signals mutation_sent before the driver executes it. The later assertions only require the mutation to commit after teardown releases; they do not distinguish row-lock blocking from table-lock blocking or deterministically prove the lock state.

Specific fix. Mutate a different catalog row that is not held by teardown's expected-row FOR UPDATE lock, or otherwise isolate the table-lock assertion from that row lock. Retain the statement-sent barrier, but replace the 250 ms negative wait with a deterministic lock-timeout or equivalent lock-state assertion and verify the mutation outcome after releasing teardown.

Verification note

This review is based on static inspection of the exact base/head code, changed tests, workflow, and complete supplied history. No local tests, builds, linters, formatters, or other validation were run. The Simplification Lens was unavailable due to its usage limit, so no simplification opportunities are reported.

Blocking status & recommended decision

Blocking: yes.

• src/xagent/web/api/mcp.py:3700 — major, [new], Blocking: yes — trigger: association A is deleted and replaced by active B on a retained shared server while an old connect or teardown is in flight; impact: tuple-only matching accepts or deletes B, so stale work can persist old OAuth state under B or remove fresh grants/association state.
• src/xagent/web/api/mcp.py:3912 — major, [new], Blocking: no — trigger: SQLite reuses the maximum PublicMCPApp ROWID after same-app/provider delete/recreate before BEGIN IMMEDIATE; impact: stale teardown accepts the replacement and performs destructive cleanup.
• tests/web/api/test_mcp_app_teardown_postgresql.py:368 — minor, [prior], Blocking: no — trigger: the test's same-row FOR UPDATE and timing wait mask removal of the catalog table lock; impact: CI can pass without proving the intended PostgreSQL catalog serialization.

A2 is the only final blocking issue. A1 and N3 are confirmed but do not block this head: A1 is latent in the intentionally unwired cross-repository primitive, and N3 is a test-coverage accuracy defect rather than a production failure. Recommended event: REQUEST_CHANGES

[OliverBryant]

Third-round review conclusion

All three findings are accepted and their threads are resolved:

• Association ABA is real: tuple-only lifecycle matching cannot distinguish replacement association B from original association A. Stable association generation is introduced by Add stable MCP lifecycle generation identities (toby) #2031, consumed by the OAuth producer fence in Fence MCP OAuth persistence by association generation (toby) #2032, and required by teardown in Add the atomic app-scoped MCP teardown primitive (toby) #2033.
• SQLite catalog ROWID reuse is real: PublicMCPApp integer identity is not a durable generation across delete and recreate. Add stable MCP lifecycle generation identities (toby) #2031 introduces the cross-dialect catalog generation, and Add the atomic app-scoped MCP teardown primitive (toby) #2033 pins it in the teardown contract.
• The PostgreSQL catalog-lock test still conflates the expected-row FOR UPDATE lock with the table SHARE lock. Add the atomic app-scoped MCP teardown primitive (toby) #2033 requires a different-row mutation, deterministic lock evidence, and a mutation that fails when the table lock is removed.

The prototype has been split under tracking issue #2030 into a strict serial sequence:

1. Add stable MCP lifecycle generation identities (toby) #2031 — stable catalog and association generation schema
2. Fence MCP OAuth persistence by association generation (toby) #2032 — OAuth producer lifecycle fence, only after Add stable MCP lifecycle generation identities (toby) #2031 merges
3. Add the atomic app-scoped MCP teardown primitive (toby) #2033 — atomic app-scoped teardown primitive, only after Fence MCP OAuth persistence by association generation (toby) #2032 merges

Every later PR must be created from the latest upstream main after its predecessor has merged. Feature branch stacking is not allowed. Tests and CI routing remain with the behavior they prove.

PR #2000 is now a draft prototype and reference, not a merge target. Its verified atomic-local-transaction, post-commit revocation, sanitized logging, OAuth race-ordering, and cross-database test work may be ported into the child PRs when it satisfies their independent acceptance criteria. No additional code, commit, or push was made for this split.

The downstream xagent-saas PRs #949 and #912 remain frozen. Route enablement is the final downstream layer after #2031, #2032, and #2033 have merged and the consumer contract pins both generations.

No re-review is requested for this draft reference.