fix(connectors): lock the connector definition row on edit and delete

[AlexLiu190625]

Split out of #1661, which is being broken into pieces that can each be reviewed on
their own. The lock is independent of the team-ownership work on that branch: it
fixes concurrency defects that exist today, on routes that have no team overlay,
so it stands on its own.

Three connector routes read a definition row and then write it, with no lock in
between:

Route	How it reads the definition row
PUT /api/custom-apis/{id}	through the caller's link row relationship
DELETE /api/custom-apis/{id}	through the caller's link row relationship
PUT /api/mcp/servers/{id}	a two-table join with the caller's link row

A relationship load and a two-table join both read the definition row without
locking it, so two requests can hold the same snapshot at the same time. This PR
takes a SELECT ... FOR UPDATE on that row, ahead of anything that reads or
mutates it, in every request that writes it — which is every request on two of
the three routes, and every request but one payload shape on the third. The
exception is described under "Which requests take the lock" below.

What goes wrong without the lock

Two concurrent edits lose one of them. Alice sends
PUT {"description": "billing lookup"} and Bob sends PUT {"url": ".../v2"} at
the same moment against Custom API 7, which starts as
{url: ".../v1", description: "old"}.

   Alice                                  Bob
   read row -> {url: v1, desc: old}
                                          read row -> {url: v1, desc: old}
   set desc = "billing lookup"
                                          set url = ".../v2"
   COMMIT -> {url: v1, desc: billing}
                                          COMMIT -> {url: v2, desc: old}

Final row: {url: ".../v2", description: "old"}. Alice got HTTP 200 and her
description is gone. With the lock, Bob's transaction blocks on Alice's until she
commits, re-reads {url: v1, desc: billing}, and the final row is
{url: ".../v2", description: "billing lookup"} — both edits kept.

A rename tells the connector team hook a name that is already stale. The PUT
routes call rename_team_connector(db, ..., old, new) so an installing
application can rewrite agent selectors that refer to the connector by name. The
old argument used to be read before the write, off an unlocked row.

   first renamer                          second renamer
   read name -> "billing"
                                          read name -> "billing"
   rename to "billing-api", COMMIT
   hook called with ("billing" -> "billing-api")
                                          rename to "billing-v2", COMMIT
                                          hook called with ("billing" -> "billing-v2")

The second call asks the application to rewrite selectors that say "billing".
Nothing says "billing" any more — the first renamer already rewrote them all to
"billing-api". The second rewrite matches nothing, reports no error, and every
selector the first rename produced is left pointing at a name the row no longer
has. With the lock, the second renamer reads its old off the row it holds
locked, gets "billing-api", and the hook is called with
("billing-api" -> "billing-v2").

An edit and a delete of the same connector can deadlock. PUT locks the
definition row and writes the link row afterwards. DELETE deletes the link row
and the definition row in that order, in one transaction. That is the same two
rows in opposite orders, which is the textbook deadlock shape, and PostgreSQL
resolves it by killing one of the two transactions with a 40P01. Removing just
the FOR UPDATE from the delete route and running the suite added here
reproduces it directly:

psycopg2.errors.DeadlockDetected: deadlock detected
DETAIL:  Process 269 waits for ShareLock on transaction 959; blocked by process 268.
         Process 268 waits for ShareLock on transaction 960; blocked by process 269.
CONTEXT: while deleting tuple (0,1) in relation "custom_apis"

The delete route now takes the same definition-row lock, and takes it before it
calls the connector-team delete hook — the position the PUT already had
relative to its own rename hook. Both routes therefore reach the same two rows in
one order, and cross the hook boundary in the same direction: definition row
first, hook-side rows second.

That ordering matters beyond this repository's two tables. An earlier revision of
this PR took the delete route's lock after the hook, which left the two routes
waiting in opposite directions across the hook boundary. A probe that installed
hooks taking their own SELECT ... FOR UPDATE on a second table reproduced a
40P01 against that arrangement — the cycle closing on the hook's own table —
and stopped reproducing it once the lock moved ahead of the hook. What remains
outside this repository's reach is a hook that locks rows of its own in some
other order relative to the statements here; only the installing application can
arrange those.

A row that vanishes mid-request produces a confusing 500. The access read can
find the row and still lose a race to a concurrent delete. The lock statement is
a fresh query, so it returns None in that case and the route answers with the
404 it already has for a missing connector. Without that branch the write path
runs against a row that is gone and surfaces as:

500 Failed to update MCP server: UPDATE statement on table 'mcp_servers'
    expected to update 1 row(s); 0 were matched.

Why the two custom-api routes become plain def

FOR UPDATE waits. When another transaction holds the row, the statement does
not return until that transaction commits or rolls back — that waiting is the
whole point of the lock, and there is no bound on how long it lasts.

FastAPI runs an async def route on the event loop thread itself. A blocking
wait there is not a wait for one request; it is a wait for every request the
process is serving, because nothing else can run on that thread meanwhile. A
plain def route runs in the threadpool, where the same wait occupies one worker
and leaves the loop free.

So "this route takes a row lock" and "this route must not be a coroutine" are the
same fact, and both custom-api routes are converted here. The MCP PUT is
already a plain def and needs no change. test_custom_api.py pins all three.

Which requests take the lock

PUT /api/custom-apis/{id} is the one route whose payload decides which row the
request writes. Ten of the eleven fields of CustomApiUpdate write the shared
CustomApi definition row; is_active writes the caller's own UserCustomApi
link row and nothing else. A payload that sets only is_active therefore has no
write on the definition row to serialize, and taking the definition-row lock for
it made an activate/deactivate request queue behind an unrelated concurrent edit
of the same connector.

The route now decides from api_data.model_fields_set: any field other than
is_active takes the lock, is_active alone does not. The set is used rather
than the values because an explicitly-null runtime_input_schema is written to
the definition row even though its value is None, while an absent field is not
written at all. The set is a superset of the writes that follow, so the lock is
taken in a few cases that turn out not to need it and skipped in none that do.

Both paths run the same fresh single-table read with populate_existing(); only
the FOR UPDATE clause is conditional. The vanished-definition 404 and the
response snapshot are therefore identical on both, and cannot drift apart.

PUT /api/mcp/servers/{id} is split the same way. Seven of the nine fields of
MCPServerUpdate target the shared MCPServer definition row; is_active and
user_env write the caller's own UserMCPServer link row and nothing else, so a
payload carrying only those two takes no lock. An earlier revision of this
description said the MCP update path writes the shared row for every payload.
That is not right: the rebuild assigns the existing values back, and SQLAlchemy
emits no UPDATE when the assigned value equals the loaded one, so on a server
created through the normal route that carries no global env or auth, there is
no shared-row write at all. The difference is not only a wait — under
PostgreSQL REPEATABLE READ the lock statement follows a snapshot the route's
first read already fixed, so a definition edit committed in between raises
SQLSTATE 40001 and the request ends as HTTP 500 with the requested activation
state unwritten.

Unlike the custom-api route, the field set here is not a superset of the writes
that follow: a server carrying a global env or auth has its secret
re-encrypted on every update, and Fernet ciphertext differs each time, so the
rebuild still writes the definition row on the lock-free path. That write, and
its behavior under REPEATABLE READ, is what this route already did before this
change; the isolation-level contract is tracked in #1945.

Lock placement

Each lock is taken after the access and permission gates that precede it in its
route, so a request refused by those gates never acquires it. Refusals that need
data read under the lock necessarily come after it: both PUT routes validate
the payload once locked — the MCP PUT including a 403 when a non-owner submits
a changed shared configuration — and the custom-api DELETE additionally has two
403s that read the delete hook's answer, because the hook has to be called with
the definition row already held for the lock order above to hold. So a delete
that is going to be refused does briefly hold the row, until the raised exception
propagates out and the request's session is closed without committing, and it
holds the row for the duration of the hook's own work. Both are accepted costs of
the single lock order. Each query uses
populate_existing(), which forces the locked row to replace the one already in
the session's identity map — without it the query would return the unrefreshed
pre-lock instance and the lock would protect a row the route does not actually
read.

Authority after the wait

FOR UPDATE waits, and the gates above it ran before that wait. Everything a
route decided from the caller's link row -- that the link exists at all, and what
it permits -- was therefore established before a wait with no bound on its length,
and nothing re-established it afterwards. Two supported operations can commit
inside that wait: an administrator deleting a user removes that user's link rows
and leaves every definition row standing, and a disconnect removes the caller's
own link while another user's link keeps the definition alive. A request that
resumes after either one would write the shared definition row on an authority it
no longer has, commit it, and fail only later -- while building its response off
the row that is gone, which surfaces as HTTP 500 over a durable change. The
delete route was worse: it answered 204 and removed the shared definition row,
cascading away every other user's link, for a caller who no longer had one.

Each locking path now re-reads the caller's link row once it holds the definition
row, before it calls a connector-team hook and before it writes or deletes
anything shared, and re-derives its authority from that read: a link that is gone
is the route's existing 404, and a link that no longer permits the operation is
the route's existing 403. The object that read returns replaces the pre-lock one
for the rest of the route, so the mutation, the hook, the refresh and the response
all read the row the transaction actually verified. populate_existing() on the
definition query covers the definition row and only it, which is why the link row
needs a statement of its own.

The two routes read "no longer permits the operation" differently. On the two
custom-api routes the link row carries its own can_edit/can_delete flags, so a
link with either flag cleared is refused with 403 directly. The MCP edit path gates a
shared-config change on ownership rather than on those flags -- the link row's own
can_edit column is not read here: what the re-read changes is can_edit_global,
which the route's existing owner-only guard already consumes -- a non-owner's
payload that actually changes the shared configuration still gets that guard's
403, while a non-owner's payload that changes nothing gets 200 with the shared
configuration silently dropped, exactly as a request from a non-owner always did.

Two boundaries this does not move. The window between that re-read and the commit
is the window these routes had before they took any lock -- in process, with no
wait in it -- and closing that one belongs with the team-authorization work, not
here. The paths that skip the definition lock add no wait, so they add no exposure
and are left alone; the MCP delete route takes no definition lock at all and is
likewise unchanged.

Tests

Which backends this actually serializes

FOR UPDATE is a PostgreSQL/MySQL row lock. SQLAlchemy renders no locking clause
at all on SQLite — the statement emitted there is byte-for-byte the one emitted
without the call — so on a SQLite deployment these read-modify-write sequences are
not serialized and the interleavings above remain possible. All three lock sites
now say so in a comment, so a reader is not left believing otherwise.

Closing that gap needs a dual-dialect fence (a no-op write that takes SQLite's
writer lock before the read), which this repository already has in two places.
That is repo-wide work across 33 with_for_update() call sites in 18 files
rather than a change to these three, and is tracked in #1944. Two further findings
from this PR's review are likewise tracked rather than changed here: the engine
never checks the isolation level a PostgreSQL server hands it (#1945), and no lock
site bounds how long a waiter may wait (#1946).

Tests

FOR UPDATE is a no-op on SQLite, so nothing in the ordinary suite can tell a
real lock from a statement that silently does nothing. Two Postgres-only suites
run the real statement against a real server:

• tests/web/api/test_mcp_server_edit_lock_postgresql.py
• tests/web/api/test_custom_api_edit_lock_postgresql.py

They cover, on two real connections with a barrier between them: a second editor
blocking until the first commits; the second editor's rename reporting the first
editor's committed name; an edit and a delete blocking each other in the same
order; an is_active-only edit completing while a concurrent definition edit
holds the row; and the vanished-row 404 for the edit route's locking path, for the
edit route's association-only path, and for the delete route, the delete case also
asserting that the refusal happens before the delete hook is called and that
nothing is committed.

Both suites also cover revocation on the other side of the lock: the caller's link
row deleted, and the link row's permission flag cleared, each committed by a second
connection while the route holds the definition row locked -- asserting the
404/403, that no shared row was written or deleted, that nothing was committed,
and (for the delete route) that the connector-team hook was never called.

Each timing test now also reopens an independent session once both writer sessions
have closed and asserts the committed definition, association and configuration
fields. Without that, a route that produced the right hook calls and the right
timing but wrote nothing durable would still pass.

The delete-versus-edit test's concurrent editor sets a definition field rather than
is_active, since an is_active-only edit no longer holds the row that test is
ordered against.

Both suites are wired into the existing Postgres job in test-migrations.yml,
alongside the other *_postgresql.py suites. That workflow's two hand-maintained
path lists also gain the three production modules these suites import beyond the
two route files already listed — connector_team_scope.py, models/custom_api.py
and models/mcp.py — so a change confined to one of them can no longer leave the
Postgres jobs skipped while CI reports success.

Lock ordering between the two custom-api routes is statement order, which is
dialect-independent, so it is pinned in the ordinary SQLite suite by counting the
statements the delete route issues before its first DELETE, and by counting the
statements already issued when the delete hook is called.

The ordinary suite also pins the two mixed payloads that carry is_active next to a
field the write guards skip -- an explicit-null description, and the definition's
current name -- as writing no definition field: the link row's activation is
persisted and no UPDATE against custom_apis is emitted.

[gemini-code-assist]

Code Review

This pull request introduces row-level locking using with_for_update and populate_existing on CustomApi and MCPServer definition rows during updates and deletions to prevent concurrent modification anomalies and deadlocks. To ensure that database lock waits do not block the FastAPI event loop, the affected endpoints have been converted from asynchronous (async def) to synchronous (def) functions. New tests, including real PostgreSQL integration tests, have been added to verify lock ordering and blocking behavior. Feedback is provided to use StaticPool with the in-memory SQLite database in tests to avoid potential flakiness or database loss across multiple sessions.

[rogercloud]

PR #1913 adds pessimistic SELECT ... FOR UPDATE locking to the Custom API PUT/DELETE and MCP PUT definition-row paths, converts the two async Custom API handlers to synchronous functions so lock waits do not block FastAPI's event loop, and adds PostgreSQL concurrency suites plus CI registration. It targets stale read-modify-write updates and stale rename-hook inputs, and it aligns the repository's definition/link deletion order while preserving the existing vanished-row 404 guard. Blocking: yes — recommended event: REQUEST_CHANGES.

Update summary

No commits have been added since the previous reviewed head (0d365f0c8c76188d7aef1f2083218813bf818c74), so this pass reviews the same PR head as that review. Independently of the prior feedback, this pass found two new confirmed issues: a lifecycle-hook lock inversion and missing DELETE-specific vanished-row coverage.

Approach verdict

acceptable-with-reservations. The pessimistic definition-row lock is an appropriate root-cause fix for stale reads and stale rename values, and making the two waiting Custom API routes synchronous keeps blocking database waits off the event loop. The per-definition lock boundary is an intentional and acceptable serialization for the current scope; the reservation is that application lifecycle hooks execute in the same request transaction and may lock or mutate their own tables, so their lock order must be coordinated with the repository's lock order.

Findings

Major — lifecycle hooks can deadlock across the definition-row lock

• Location: src/xagent/web/api/custom_api.py:516 (DELETE's lock; related PUT lock at :328-333, rename hook at :430-437, and delete hook at :480-482).
• Severity: major. Blocking: yes.
• Trigger and impact: A supported connector-team hook may mutate or take a row lock on an application table in the same SQLAlchemy session/transaction. A concurrent PUT takes the Custom API definition lock (D) and then calls rename_team_connector, while DELETE calls delete_team_connector before it acquires D; if the DELETE hook acquires its application row lock (H), the requests can wait in opposite orders (PUT D→H, DELETE H→D). PostgreSQL can then abort one transaction with 40P01, and the uncaught failure is returned as HTTP 500.
• Fix: Acquire the Custom API definition lock before delete_team_connector, or split hook preflight from post-lock mutation and define/enforce one lock order that covers the application-hook boundary. The explanatory comment does not establish that protocol.

Minor — DELETE vanished-row interleaving is untested

• Location: tests/web/api/test_custom_api_edit_lock_postgresql.py:269 (the vanished-row test covers only update_custom_api; the production guard is src/xagent/web/api/custom_api.py:523-527).
• Severity: minor. Blocking: no.
• Trigger and impact: Request A can pass its initial association/permission gate, then a concurrent request deletes the definition/link before A's fresh FOR UPDATE query. The current None guard returns 404; without it, the default delete path reaches db.delete(api) with api=None, raises UnmappedInstanceError, and exposes HTTP 500. No current PostgreSQL test exercises this DELETE interleaving, so that regression can pass CI.
• Fix: Add a DELETE-specific PostgreSQL interleaving that commits the concurrent deletion between the gate and lock query, asserts 404, and verifies that no unintended side effects are committed.

Prior-findings checklist

Canonical root	Status	Evidence / disposition
StaticPool recommendation at tests/web/api/test_custom_api.py:540	DROPPED / verified-safe	Independent verification agrees with the author's reply 3881898866 to the prior inline finding 3881493452: this synchronous, same-thread factory uses SQLAlchemy's per-thread SingletonThreadPool, and no cross-thread shared in-memory connection is required.

The previously validated edit-vs-delete lock-test false-green gap and the PostgreSQL timing-window gaps are tracked in #1816 from the linked #1661 discussion, and are not re-raised here.

No local tests were run by review default; CI is green.

Blocking status & recommended decision

Blocking: yes because the hook inversion is a confirmed PR-caused deadlock path that can return HTTP 500 for concurrent Custom API lifecycle requests.

Blocking issues:

• src/xagent/web/api/custom_api.py:516, major, cross-boundary lifecycle-hook lock inversion can deadlock concurrent PUT/DELETE and surface as PostgreSQL 40P01 / HTTP 500 [new]

Recommended event: REQUEST_CHANGES

[rogercloud]

Summary

This PR adds definition-row SELECT ... FOR UPDATE plus populate_existing() to Custom PUT/DELETE and MCP PUT so concurrent edits serialize before rename/delete work. It also converts Custom PUT/DELETE to synchronous handlers and adds PostgreSQL-only concurrency suites with workflow registration.

Update since the prior reviewed head

The f90b5d7a7f4a27691fb4c2ae93a42c9c15935e4f update moves the Custom DELETE definition-row lock ahead of delete_team_connector and adds the PostgreSQL vanished-row DELETE test. Those changes address the prior hook-order and missing-coverage concerns, but do not address the current findings below.

Prior-review checklist

• P1 — StaticPool recommendation: DROPPED / verified safe. The current helper is safe for the SingletonThreadPool usage it has, and there is no concurrent TestClient/thread usage that requires StaticPool.
• P2 — hook-order inversion: FIXED in f90. Current Custom PUT and DELETE both acquire the definition-row lock before the team hook.
• P3 — vanished DELETE row: FIXED in f90. The current PostgreSQL test asserts the 404 result, entity ordering, no hook invocation, and no commit.

The test-quality gaps tracked separately in #1816—including the DELETE lock-order false positive and the one-second negative windows—are excluded from this review and are not re-raised. The automated review-history extractor hit its usage limit; full raw review and inline-comment exports were manually reconstructed.

Design verdict

The core direction is sound with reservations: definition-row serialization, populate_existing(), lock-before-hook ordering, synchronous route conversion, and PostgreSQL test placement are all reasonable. However, SQLite is a supported backend and PostgreSQL isolation is not constrained, so the two correctness gaps below remain blocking; the remaining findings are capacity, assertion, and workflow-coverage gaps.

Confirmed findings

C1 — SQLite FOR UPDATE is not a serialization fence (major, blocking)

Trigger: Two overlapping writes to the same definition through file-backed SQLite can both read the old row and then write based on that stale state.

Evidence: SQLite is the documented/default local backend (src/xagent/config.py:2447-2463, example.env:29-32). The new locks are at src/xagent/web/api/custom_api.py:328-333 (Custom PUT), src/xagent/web/api/custom_api.py:512-517 (Custom DELETE), and src/xagent/web/api/mcp.py:3256-3261 (MCP PUT). The SQLite dialect removes FOR UPDATE, so the nominal lock is only a plain SELECT; the file-backed SQLite probe observed the second nominal lock return immediately while the first transaction remained open, with both transactions seeing the original row and the stale second write succeeding. WAL and busy_timeout serialize actual writes, but they do not fence the earlier read/configuration computation. Converting Custom PUT/DELETE to synchronous handlers also permits same-process thread-pool overlap that the previous single-event-loop execution would not interleave.

Impact: The promised lock/freshness behavior is absent on a supported backend: stale names/configuration can overwrite newer edits and rename hooks can receive the same old name more than once. PostgreSQL-only tests do not cover this path.

Fix: Add a shared dialect-aware lock helper: use a SQLite write-before-read fence (for example, an appropriate BEGIN IMMEDIATE strategy or a harmless write such as UPDATE target SET id=id) and retain FOR UPDATE for locking dialects. Add two-session, file-backed SQLite concurrency coverage for each affected route or for the shared helper.

C2 — PostgreSQL isolation contract is unenforced (major, blocking)

Trigger: A PostgreSQL deployment using REPEATABLE READ can update the definition after the route's initial relationship/join read but before the new definition-row lock.

Evidence: The normal non-SQLite engine accepts the full database URL without enforcing an isolation level (src/xagent/web/models/database.py:176-216). Custom first reads the personal-link/definition relationship at src/xagent/web/api/custom_api.py:297-316 and then issues the new lock at src/xagent/web/api/custom_api.py:328-334; MCP first reads its joined row at src/xagent/web/api/mcp.py:3229-3234 and then locks at src/xagent/web/api/mcp.py:3256-3262. Under PostgreSQL READ COMMITTED, the lock waits and re-evaluates as intended. Under REPEATABLE READ, however, a concurrent update after that first snapshot makes SELECT ... FOR UPDATE abort with SQLSTATE 40001 (“could not serialize access due to concurrent update”); populate_existing() cannot change the transaction snapshot, and neither route has a whole-transaction retry. The resulting failure escapes as an HTTP 500 rather than a successful edit, including cases where the base code could have updated only the personal association.

Impact: The route's correctness depends on an isolation setting that the application neither enforces nor documents as a restriction. A supported PostgreSQL configuration can therefore turn concurrent Custom/MCP edits into 500s instead of providing the advertised serialization behavior.

Fix: Either enforce and document READ COMMITTED for the normal PostgreSQL engine, validating/rejecting unsupported isolation settings, or support REPEATABLE READ with a whole-transaction retry that restarts before the initial relationship/join read. Add a targeted REPEATABLE READ regression test.

C4 — Lock-held hooks have unbounded wait/capacity impact (minor, non-blocking)

Trigger: A slow or blocked rename/team hook, one lock holder, and a hot-key burst can make every waiter remain in the database transaction while waiting for the definition row. The new Custom PUT, Custom DELETE, and MCP PUT locks have no NOWAIT, SKIP LOCKED, lock timeout, or statement timeout. Custom DELETE explicitly runs delete_team_connector after acquiring the lock (src/xagent/web/api/custom_api.py:497-503, src/xagent/web/api/custom_api.py:512-555), and the PUT hooks likewise run while their locks are held.

Evidence and impact: With the normal non-SQLite pool defaults (10 connections plus 20 overflow) and the default 40 synchronous worker tokens, one holder plus roughly 29 hot-key waiters can pin all 30 database connections and up to 40 workers; unrelated requests then wait or fail. This interaction is introduced by the new locks, although ordinary edits serialize as intended, so this is minor/non-blocking rather than a correctness blocker.

Fix: Bound lock/statement waits or use NOWAIT with a stable busy response, and keep slow/external hook work outside the transaction or otherwise constrain its duration.

C5 — PostgreSQL tests lack durable final-state assertions (minor, non-blocking)

Trigger: The four new PostgreSQL edit/rename tests assert timing completion and rename-hook tuples, but do not use a fresh independent session after the writer sessions close to verify the committed combined state. The Custom tests are at tests/web/api/test_custom_api_edit_lock_postgresql.py:90-176 and :179-273; the MCP tests are at tests/web/api/test_mcp_server_edit_lock_postgresql.py:73-159 and :162-256.

Impact: A regression that drops or leaves stale the description, configuration, association, or another durable field—or that fails to commit while preserving the timing and hook assertions—could pass these tests.

Fix: After both writer sessions have completed, open a fresh independent session and assert the final definition plus the relevant association/configuration fields for each scenario.

C6 — Workflow dependency paths are incomplete (minor, non-blocking)

Trigger: The workflow's new path lists include the route and test files but omit direct dependencies src/xagent/web/services/connector_team_scope.py, src/xagent/web/models/custom_api.py, and src/xagent/web/models/mcp.py. The push list is at .github/workflows/test-migrations.yml:54-57, while the detector's RELEVANT_PATHS mirror is at .github/workflows/test-migrations.yml:145-148.

Impact: Both pull-request and merge-group detection relies on RELEVANT_PATHS; a change only in one omitted dependency can leave should-test=false, so the PostgreSQL suites at .github/workflows/test-migrations.yml:459-471 are skipped while the migrations summary accepts the skipped jobs. Other CI does not provide an alternate PostgreSQL run.

Fix: Add all three dependency paths to both the push paths and RELEVANT_PATHS, or derive the trigger from the import/dependency graph so direct implementation changes cannot silently skip these suites.

C7 — Custom-only parent lock causes avoidable association fan-in (minor, non-blocking)

Trigger: A Custom PUT payload containing only is_active still takes the new CustomApi definition-row lock at src/xagent/web/api/custom_api.py:328-333, even though the only write is to the separate UserCustomApi association at src/xagent/web/api/custom_api.py:439-441; shared definition fields are untouched, validation is pure, and no rename is needed.

Impact: Personal association-only edits unnecessarily serialize on the parent definition and add lock latency plus database-connection/synchronous-worker fan-in. This is a narrower, Custom-only issue; the MCP occurrence is intentionally not reported because its current PUT still rebuilds and writes shared server configuration at src/xagent/web/api/mcp.py:3314-3353 and src/xagent/web/api/mcp.py:3360-3376.

Fix: Classify the fields set in the payload and use an association-only path that updates the link safely, handles a vanished definition as the same 404, and builds the response from the current snapshot. Retain the parent lock for shared-field/configuration and rename paths until a true association-only MCP path exists.

Blocking: yes — recommended event: REQUEST_CHANGES

[rogercloud]

PR #1913 adds transaction-level locking and fresh reads for shared connector definitions in Custom API PUT/DELETE and MCP PUT, aligns Custom DELETE with the hook lock order, and moves blocking Custom handlers into FastAPI's synchronous worker execution. It also adds real PostgreSQL concurrency/vanished-row coverage, durable post-commit assertions, and the workflow dependencies needed to run those suites, while keeping Custom association-only is_active updates off the shared-row lock. The current head still has one PostgreSQL MCP association-only isolation regression and one residual Custom over-lock.
Blocking: yes — recommended event: REQUEST_CHANGES

Approach verdict

acceptable-with-reservations. The design directly addresses the shared-definition read-modify-write race with a database linearization point, preserves the existing API/model boundary, uses populate_existing() to avoid stale identity-map state, and keeps lock-waiting database work out of the event loop. No macro design concern passed the Finding Admission Standard, so there is no admitted design-level finding.

The reservations are explicit boundaries rather than silently fixed issues: SQLite still has no real FOR UPDATE fence (tracked in #1944), the engine does not enforce or document a PostgreSQL isolation-level contract (tracked in #1945), and lock waits/hooks remain unbounded with worker/connection fan-in (tracked in #1946). Those underlying behaviors remain reservations and are not re-reported here.

Update since the last review

Since last reviewed head f90b5d7a7f4a27691fb4c2ae93a42c9c15935e4f, e28fb9f89 conditionally skips the Custom parent FOR UPDATE for is_active-only association writes. It adds fresh-session durable assertions to four PostgreSQL tests (two Custom and two MCP cases) and adds the three previously missing workflow dependency paths to both the push-path list and RELEVANT_PATHS. CI preflight was green (all checks success); the new review outcome is CHANGES_REQUESTED because the MCP association-only path remains a major blocker and a smaller Custom association-only residual remains.

Prior-findings checklist

The review-history extraction failed because of the usage limit. I manually reconstructed and verified all records from the raw exports (12 review-body records, 0 conversation comments, and 18 inline records comprising 9 roots plus 9 replies), together with the linked issue exports. This is an accounting/reconstruction result, not a claim that tracked underlying behavior is fixed; the statuses below distinguish fixed, verified-safe, and tracked-but-still-present roots.

Canonical root	Status	Occurrence-level evidence and preserved history
StaticPool recommendation for the in-memory SQLite lock-order helper	DROPPED / verified-safe	Sources: body 5052150208, inline 3881493452 (tests/web/api/test_custom_api.py:540), reply 3881898866, duplicate checklist occurrences body 5059841366 and body 5060560802, plus the empty-parent context review 5052620624. The helper at test_custom_api.py:537-540 uses the SQLite default SingletonThreadPool; both direct callers (:609 and :670) are same-thread synchronous sessions, so the reply's explanation is independently verified. No tracking issue owns this root.
Custom hook-order inversion	FIXED	Sources: body 5059841366, inline 3888359930 (historical src/xagent/web/api/custom_api.py:516), reply 3888580892, and duplicate checklist body 5060560802. The linked contextual discussion PR #1661 comment 5450509171 is not a separate occurrence. Current Custom definition-writing PUT takes the definition lock before rename_team_connector, and DELETE takes it before delete_team_connector; e28's lock-free association branch has no effective rename-hook edge.
Vanished DELETE row / 404 coverage	FIXED	Sources: body 5059841366, inline 3888359932 (tests/web/api/test_custom_api_edit_lock_postgresql.py, historical line 269/current review line 316), reply 3888581059, and duplicate checklist body 5060560802. The current test at :460-547 and route lock/guard at src/xagent/web/api/custom_api.py:560-578 verify 404, no hook, no commit, and correct query ordering. The separate #1816 false-green test-quality root is recorded below, not merged into this one.
SQLite FOR UPDATE no-op (C1)	DROPPED-by-tracking #1944 (underlying behavior remains)	Sources: body C1 / review 5060560802, inline 3889122216, and reply 3889611207. All three lock sites remain affected (custom_api.py:365-370, custom_api.py:560-565, and mcp.py:3265-3270): SQLite renders no locking clause for with_for_update(). The author reply explicitly confirms/reproduces that defect and says it is not fixed here; it is owned by #1944, whose complete comments export is empty. This is tracked, not verified-safe or fixed.
PostgreSQL isolation contract (C2)	PARTIAL / split	Sources: body C2 / review 5060560802, inline 3889122220 (src/xagent/web/api/mcp.py:3269), and reply 3889611514; the pre-existing definition-writing half is tracked by #1945. Occurrence statuses: Custom association-only is FIXED; Custom/MCP definition-writing under REPEATABLE READ is DROPPED-by-tracking #1945 because the same 40001 failure is present in the base; MCP association-only is NOT FIXED and is the major finding below. The reply's Custom matrix is correct, but its MCP conclusion is incomplete.
Unbounded lock wait / capacity (C4)	DROPPED-by-tracking #1946 (underlying behavior remains)	Sources: body C4 / review 5060560802, inline 3889122222, and reply 3889611768. The current routes still have no NOWAIT/timeout policy and hooks run while the definition lock is held; the reply confirms the arithmetic, says the behavior is not fixed in this PR, and points to #1946. It is not a separate follow-up finding here.
Durable final-state assertions (C5)	FIXED	Sources: body C5 / review 5060560802, inline 3889122225 (tests/web/api/test_custom_api_edit_lock_postgresql.py:94), and reply 3889612049. e28 adds independent fresh-session reads to all four named PostgreSQL tests: Custom :186-199 and :301-313, MCP :165-181 and :283-298. Each writer pair has finished and its sessions are closed before the assertions, so no residual C5 issue remains.
Workflow dependency path omission (C6)	FIXED	Sources: body C6 / review 5060560802, inline 3889122227 (.github/workflows/test-migrations.yml), and reply 3889612343. Both push paths (:54-60) and RELEVANT_PATHS (:148-154) now contain the two suites, two routes, and the three formerly missing direct dependencies. The reply's reference to #1270 is a distinct shared-test-path issue, not a duplicate or owner of C6.
Custom association lock (C7)	PARTIAL — residual remains	Sources: body C7 / review 5060560802, inline 3889122228 (historical custom_api.py:329, current classifier block :318-370), and reply 3889610607. The exact {is_active} case is fixed, but mixed effective-association payloads with explicit null/no-op non-runtime keys still take the parent lock; this same root is the minor finding below and has no tracking owner.
DELETE lock-order test false-green	DROPPED / tracked #1816 (underlying test gap remains)	The Round 1 raw occurrence is tests/web/api/test_custom_api_edit_lock_postgresql.py:444; the later checklist references are body 5059841366 and body 5060560802. The canonical tracking root is #1816, with earlier duplicate/tracking acknowledgements in PR #1661 comment 5442083844 and comment 5450509171. Removing the DELETE-side FOR UPDATE can still leave the test green because the later parent DELETE blocks under PostgreSQL MVCC; this remains a real test-quality gap but is explicitly tracked and is not re-reported.

Final confirmed findings

Major — src/xagent/web/api/mcp.py:3269 — Blocking: yes [prior]

The current unconditional MCPServer SELECT ... FOR UPDATE at src/xagent/web/api/mcp.py:3269 runs after the initial joined access read, whose snapshot is already established. A supported MCPServerUpdate payload of {"is_active": false} is association-only: is_active is persisted to UserMCPServer.is_active, while a normal route-created server's rebuilt shared configuration assigns the existing values (including concurrent_tools=[]) back to the same MCPServer instance. The current model/config path and SQLAlchemy net-dirty behavior therefore leave only the association row dirty; there is no shared-row write that justifies this lock.

Under PostgreSQL REPEATABLE READ, if another supported editor commits a definition update after that initial join and before this new lock query, the FOR UPDATE raises SQLSTATE 40001 / SerializationFailure. The generic exception handler converts it to HTTP 500 and rolls back, leaving the requested association state unchanged (is_active remains true). The exact base route, which has no parent lock, returns HTTP 200 and persists is_active=false in the same interleaving. PostgreSQL with a configured DATABASE_URL is a supported deployment/isolation combination, and the update model explicitly permits this payload.

I saw the prior author reply that MCP always writes the shared row: reply 3889611514. I re-checked MCPServerUpdate, _update_server_from_config, the UserMCPServer.is_active assignment, and the normal model defaults; the shared assignments are equal/no-op for this supported payload, so that explanation does not hold for MCP association-only updates. This is the surviving MCP occurrence of C2, not a new duplicate. The targeted local PostgreSQL probe run_mcp_rr_probe3() compared the exact base and current routes and observed current HTTP 500/40001 with final active=true versus base HTTP 200 with association false; the separate run_pg_rr_lock_probe() reproduced 40001 directly at SELECT ... FOR UPDATE.

Please classify model_fields_set before entering the shared path and skip the parent lock and shared config rebuild for {is_active,user_env}-only payloads, while retaining the lock/shared path for any actual definition-field write. Add a PostgreSQL REPEATABLE READ regression that performs the initial join, commits a concurrent definition edit, sends the association-only update, and asserts HTTP 200 plus a durable UserMCPServer.is_active=false.

Minor — src/xagent/web/api/custom_api.py:335 — Blocking: no [prior]

writes_definition_row = bool(fields_set - {'is_active'}) still takes the parent CustomApi lock for supported payloads such as {"is_active": false, "description": null} or {"is_active": false, "name": <current name>}. CustomApiUpdate accepts these optional fields; the guards at src/xagent/web/api/custom_api.py:394-427 skip the null/non-changing non-runtime values, and the only actual mutation in these examples is UserCustomApi.is_active at :475-477. The request consequently waits behind a concurrent definition editor despite not writing the shared row, preserving avoidable lock latency and synchronous worker/connection fan-in. This is a minor performance/capacity issue, not an established state or availability failure, so it is non-blocking.

I saw the author reply that model_fields_set fixes the exact is_active-only case and protects explicit-null runtime writes: reply 3889610607. That is correct but explicitly admits over-locking; the actual non-runtime guards above leave a residual effective-association-only case. This is the same C7 root, not a new issue.

Please classify effective writes (or normalize no-op values) before adding the parent lock, preserving locks for explicit-null runtime fields and other real definition writes, and add boundary tests for null and same-name companion fields.

Review limitations and evidence

• The history extractor failed with a usage-limit error; all raw PR exports and linked tracking exports were manually reconstructed and verified as described above. This does not imply that the underlying tracked SQLite/isolation/wait behavior is fixed.
• The Simplification Lens was unavailable because its Spark run hit the usage limit; no simplification opportunities are asserted.
• No full tests, builds, linters, or formatters were run by review default. The only runtime evidence was the workers' narrow local PostgreSQL probes: persistent run_mcp_rr_probe3() comparing exact base/current route behavior, plus raw run_pg_rr_lock_probe(). Their result was current HTTP 500/40001 versus base HTTP 200 with association false. CI preflight was green (all checks success).

Blocking status & recommended decision

Blocking: yes. The final confirmed set contains one blocker: the supported PostgreSQL REPEATABLE READ MCP association-only update can return HTTP 500 and fail to persist the user's requested active state. The Custom C7 residual is minor and non-blocking.

Blocking issues:

• src/xagent/web/api/mcp.py:3269 — major, Blocking: yes — unconditional parent FOR UPDATE turns a supported association-only is_active update into HTTP 500/40001 and leaves the association unchanged under PostgreSQL REPEATABLE READ. [prior]

Recommended decision: REQUEST_CHANGES (review outcome: CHANGES_REQUESTED).

[rogercloud]

Minor; Blocking: no — same C7 root [prior]. I saw reply 3889610607 that model_fields_set fixes exact is_active-only updates and protects explicit-null runtime writes, while explicitly admitting some over-locking. That is correct but incomplete: for supported {"is_active": false, "description": null} or {"is_active": false, "name": <current>}, fields_set - {"is_active"} is nonempty, yet the guards at custom_api.py:394-427 skip the companion field and only UserCustomApi.is_active is written at :475-477. The request still waits on the shared row, causing avoidable lock latency and worker/connection fan-in. Classify effective writes (or normalize no-ops) before adding the parent lock, preserve locking for real/runtime definition writes, and add null/same-name boundary tests.

[AlexLiu190625]

Confirmed as described: a payload like {"is_active": false, "description": null} has fields_set - {"is_active"} non-empty (description is present), so it takes the lock at custom_api.py:335, then writes zero definition-row fields -- the description is not None guard at :404-405 skips the null, and is_active only ever writes UserCustomApi.is_active. Same for {"is_active": false, "name": <the current name>}: name is in fields_set, the lock is taken, and then if api_data.name and api_data.name != api.name at :394 is false so nothing is written. Both are real extra waits with no write behind them.

I looked at moving to a "what will actually be written" classification instead of "which fields are present," and I'd rather not, for two reasons.

First, it isn't a free win even on correctness: runtime_input_schema writes on an explicit null (:457-458, if "runtime_input_schema" in fields_set: mutable_api.runtime_input_schema = ..., unconditional on the value), so a value-based classifier has to special-case that field or it silently drops back to no-lock for a real write -- it's not one rule, it's the write logic re-derived a second time in a second place. Every time the write guards below change, this second copy has to change with them, and the failure direction if it doesn't is an unlocked write to the shared row -- a lost update -- which is worse than the extra wait it's meant to remove.

Second, the name-unchanged case can't be decided from the request body at all: the check is api_data.name != api.name, and api.name only exists once the definition row has been read. Deciding "is this actually a no-op" ahead of the lock would mean reading that row once to check, then reading it again (locked) to write -- an extra query added specifically to the path this suggestion is trying to make cheaper.

So I'm keeping the field-presence criterion as is. The extra locking it causes is confined to no-op companion payloads riding alongside is_active, and getting that wrong in the other direction is the more expensive mistake.

[AlexLiu190625]

Correcting one sentence in my earlier reply on this thread: I wrote that checking the definition's current name at the classifier "would mean reading that row once to check, then reading it again (locked) to write -- an extra query." That's wrong. By the time the classifier (custom_api.py:335) runs, the entry gate above it has already lazy-loaded user_api.custom_api (the not user_api.custom_api check at custom_api.py:306), which pulls the definition row's columns, including name, into memory on the same statement. A classifier keyed on user_api.custom_api.name would cost zero additional queries — it's already sitting in the session.

The conclusion doesn't change, but the reason does. The real problem with a value-based classifier isn't its cost — it's that the only name it could compare against is the one read at that earlier, unlocked point, and the write logic downstream compares against a different, later read instead (the fresh, locked-when-taken read at custom_api.py:366). A payload that matches the earlier read but not a concurrently-renamed later one would score as a no-op under a value-based classifier and skip the lock, while the write logic still fires off the later read — removing the lock removes what serializes that write against a concurrent commit landing in the same gap. That's a lost-update risk, not an extra-query one. The full argument and the two new boundary tests are on the other thread on this line, since it carries this round's specific ask.

[rogercloud]

PR #1913 adds fresh populate_existing() definition reads and conditional SELECT ... FOR UPDATE to Custom API PUT/DELETE and MCP PUT, converts the blocking Custom handlers to synchronous FastAPI endpoints, and adds real-PostgreSQL concurrency coverage plus workflow wiring. The change targets stale read-modify-write edits, rename-hook ordering, and vanished rows while avoiding the parent lock for the supported association-only payloads it can prove are parent-clean. The overall row-lock direction is appropriate for PostgreSQL READ COMMITTED, but the current code still needs post-lock association revalidation and retains a smaller mixed-payload over-lock.
Blocking: yes — recommended event: REQUEST_CHANGES

Update summary since e28

Since the last reviewed head e28fb9f89bdc27540dd252b4542efa7b4cfc4bf0, a7338947bccf99592c657764df6fe7553c674961 makes MCP PUT's parent lock conditional on model_fields_set - {"is_active", "user_env"}, so association-only updates no longer issue the parent FOR UPDATE. It adds create-route-shaped PostgreSQL coverage for the REPEATABLE READ interleaving, with fresh-session assertions that the activation change and concurrent definition edit both persist, and adds the corresponding association-only vanished-row 404 coverage. CI preflight remains green at 14/14 checks; the surviving reportable roots are the stale-association authorization blocker and the prior Custom mixed no-op over-lock.

Approach verdict

acceptable-with-reservations. The core design is sound for its intended PostgreSQL READ COMMITTED boundary: a fresh definition-row read establishes the lock-time snapshot, populate_existing() prevents the identity map from returning the pre-lock object, Custom DELETE now follows the same parent-before-hook order as definition-writing PUT, and synchronous handlers keep blocking ORM waits off the event loop. The route/API boundary is preserved, and the new PostgreSQL suites are the right evidence layer for row-lock semantics.

The reservations are explicit scope boundaries, not silently fixed behavior. SQLite still renders FOR UPDATE as an unlocked SELECT and is tracked in #1944; the effective PostgreSQL isolation level is not enforced and is tracked in #1945; unbounded waits and hook hold time are tracked in #1946; and the lock-test/timing-window gaps are tracked in #1816. Those underlying behaviors remain where the checklist says so and are not re-reported as findings here. The separate secret-bearing MCP materialization behavior was compared with the exact base and is not attributed to this PR.

Prior-findings checklist

The history extractor failed because of the usage limit. I manually accounted for the complete raw exports (15 review records, 22 inline records including replies, and 0 conversation comments) and the complete linked exports for #1661, #1944, #1945, #1946, #1816, and #1270. The table preserves every canonical source, duplicate body occurrence, inline occurrence, reply, and tracking citation; a tracked row explicitly says when its underlying behavior remains.

Canonical root	Status	Occurrence-level evidence and preserved history
StaticPool recommendation for the in-memory SQLite lock-order helper	DROPPED / verified-safe	Sources: review body 5052150208, inline 3881493452 (tests/web/api/test_custom_api.py:540), reply 3881898866, the empty review context 5052620624, and duplicate checklist bodies 5059841366, 5060560802, and 5061222712. The helper at test_custom_api.py:537-540 uses SQLite's default SingletonThreadPool; its current callers at :609 and :670 are synchronous and same-thread, with no cross-thread shared-engine use. The root is independently verified safe, not tracked or silently waived.
Custom hook-order inversion	FIXED	Sources: body 5059841366, inline 3888359930, reply 3888580892, duplicate checklist bodies 5060560802 and 5061222712, plus linked contextual PR #1661 comment 5450509171. Current definition-writing Custom PUT takes the definition lock before rename_team_connector, and DELETE takes it before delete_team_connector; the association-only PUT path has no effective rename-hook edge. The #1661 item is context, not a separate occurrence.
Vanished DELETE row / 404 coverage	FIXED	Sources: body 5059841366, inline 3888359932, reply 3888581059, and duplicate checklist bodies 5060560802 and 5061222712. The current DELETE fresh read and None guard at src/xagent/web/api/custom_api.py:560-572, together with tests/web/api/test_custom_api_edit_lock_postgresql.py:460-547, verifies 404 before the hook or route commit. This is distinct from the surviving root below, where the parent survives but the child authority is gone.
SQLite FOR UPDATE no-op	DROPPED-by-tracking #1944 (underlying behavior remains)	Sources: body 5060560802, inline 3889122216, reply 3889611207, checklist 5061222712, and tracking issue #1944. The current Custom PUT, Custom DELETE, and MCP PUT sites still emit no locking clause on SQLite; the author explicitly confirmed the defect is not fixed here and tracked it in #1944. This is tracked, not verified-safe or fixed, and is not re-reported.
PostgreSQL isolation contract (C2)	PARTIAL / split	Sources: body 5060560802, inline 3889122220, reply 3889611514, later inline 3889796175 and reply 3892921621, checklist 5061222712, and tracking issue #1945. Occurrence outcomes are split: the original Custom association-only half is FIXED by e28, the later normal MCP association-only occurrence is FIXED by a733, and definition-writing REPEATABLE READ behavior is pre-existing and DROPPED-by-tracking #1945. The earlier reply's Custom matrix is retained as evidence; its MCP conclusion was incomplete and was corrected by the later occurrence, not re-reported as a separate root.
Unbounded lock wait / capacity	DROPPED-by-tracking #1946 (underlying behavior remains)	Sources: body 5060560802, inline 3889122222, reply 3889611768, checklist 5061222712, and tracking issue #1946. The current lock sites still have no NOWAIT or timeout policy and hooks still run while the parent lock is held; the author confirmed that behavior is not fixed here and opened #1946. It remains an underlying tracked capacity concern, not a follow-up finding.
Durable final-state assertions	FIXED	Sources: body 5060560802, inline 3889122225, reply 3889612049, and checklist 5061222712. Fresh independent-session assertions now cover the four edit/rename tests, and the later association-only coverage also checks durable state after the writer sessions complete. All occurrences are fixed.
Workflow dependency path omission	FIXED	Sources: body 5060560802, inline 3889122227, reply 3889612343, checklist 5061222712, and contextual tracking issue #1270. The two workflow lists now both contain the two suites, both route files, and the three direct production dependencies at test-migrations.yml:54-60 and :148-154; #1270 remains a separate broader tests/shared/ issue, not an owner of this root.
Custom association lock C7	PARTIAL — residual remains	Sources: body 5060560802, inline 3889122228, reply 3889610607, later follow-up inline 3889796183 and reply 3892930108, and checklist 5061222712. The exact {is_active} occurrence is FIXED by e28, but mixed effective-association payloads with explicit null or same-name companion fields remain NOT FIXED at custom_api.py:335; that surviving occurrence is the minor finding below.
DELETE lock-order test false-green	DROPPED-by-tracking #1816 section 1 (underlying test gap remains)	Sources: body-only review 5059841366, review 5060560802, and checklist 5061222712, plus linked PR #1661 comment 5442083844, comment 5450509171, and tracking issue #1816 section 1. The Round 1 occurrence is tests/web/api/test_custom_api_edit_lock_postgresql.py:444; removing the DELETE-side FOR UPDATE can still leave the test green because the later parent DELETE blocks under PostgreSQL MVCC. This remains a real gap, but is explicitly tracked and is not re-reported.
Negative timing windows	DROPPED-by-tracking #1816 section 3 (underlying test gap remains)	Sources: body-only review 5059841366 and review 5060560802, the adjacent tracking reference in checklist 5061222712, linked PR #1661 comment 5442083844, separate section-1 context comment 5450509171, and tracking issue #1816 section 3. The five not event.wait(timeout=1.0) sites still lack a positive lock-reached handshake or test-level timeout; this is the tracked underlying gap, not a new finding.

Confirmed findings

Major — stale association/authorization after the definition lock — src/xagent/web/api/custom_api.py:369, :564; src/xagent/web/api/mcp.py:3300 — Blocking: yes [new]

Reachable trigger. A caller can pass the initial Custom UserCustomApi gate at custom_api.py:297-316, the Custom DELETE gate at :493-512, or the MCP UserMCPServer gate at mcp.py:3228-3244, and then wait on the new definition-row lock at Custom PUT :365-370, Custom DELETE :560-565, or MCP PUT :3296-3300. During that wait, a supported team DELETE can remove only the caller's association while another association keeps the parent alive, or an administrator can delete the user and its child association rows without deleting the shared definition. The request then resumes with the pre-lock association object and pre-lock authority decision; no current path re-queries that association after the parent lock.

Concrete impact. Definition-writing Custom PUT and MCP PUT can still mutate and commit the shared definition after the caller's edit authority has been revoked. Their later response construction can access the deleted/expired association and return HTTP 500 even though the shared mutation is durable. A stale standalone/admin Custom DELETE can return 204 and delete the surviving definition, cascading away another user's association in the shared case, after the original caller no longer has delete authority. This is the surviving-association case, not the separately fixed vanished-definition 404 case.

Contract and invariant evidence. CustomApi/MCPServer definitions and UserCustomApi/UserMCPServer associations are separate rows; multiple users may reference one definition. The Custom team-delete branch explicitly removes only the caller's link when another link remains. The supported admin-user deletion path removes association children but does not reverse-delete the shared parent, and an in-flight authenticated request retains its user id. A parent FOR UPDATE therefore does not conflict with a child-only revocation, and populate_existing() refreshes only the definition object, not user_api or user_mcp. Nothing in the types, FK direction, or FastAPI request lifecycle guarantees that the association remains present or authorized after the initial gate.

Independent evidence. Targeted current-route probes using temporary file-backed SQLite to force the gate-to-definition-query boundary reproduced the supported interleavings: a team DELETE followed by queued Custom PUT left the victim link absent while the parent description was durably changed and response serialization raised ObjectDeletedError; MCP DELETE followed by queued MCP PUT left the parent mutation durable and returned HTTP 500 when db.refresh(user_mcp) failed; exact admin-user deletion reproduced the same durable Custom/MCP PUT mutations and HTTP 500/ObjectDeletedError; and stale Custom DELETE returned 204, deleting the parent in sole-association and shared variants, with the shared variant also losing the remaining association. The PR's PostgreSQL lock suites establish that the current definition-row statements are waitable on PostgreSQL; no project test suite was run for this review.

Fix. Keep the early gate for fast rejection, but after acquiring the definition lock issue a fresh association query for the current user and definition, optionally locking that child row while preserving parent-before-child order. Re-run the route-specific existence and authority checks (can_edit for PUT, can_delete for Custom DELETE, and can_edit_global for MCP), and use the fresh association object for all subsequent mutation and response work before invoking hooks or changing the shared definition. Association-only paths that skip the parent lock still need a child-row revalidation/lock appropriate to their own write. Add two-session PostgreSQL regressions where revocation occurs after the initial gate but before the parent lock completes, asserting 403/404, no shared mutation, and no misleading success response.

Minor — Custom mixed no-op companion fields still take the parent lock — src/xagent/web/api/custom_api.py:335 — Blocking: no [prior]

Reachable trigger. A supported CustomApiUpdate can set is_active=False together with description=None, or with name equal to the current definition name. The classifier at custom_api.py:334-335 uses field presence, so either payload sets writes_definition_row=True and waits on the parent FOR UPDATE at :368-370. The current guards at :394-405 skip the null description and unchanged name, while the only actual persistence change is UserCustomApi.is_active at :475-477.

Concrete impact. These association-effective requests unnecessarily queue behind unrelated definition edits, adding avoidable lock latency and synchronous worker/connection fan-in. No incorrect state, data loss, or severe availability failure was established, so this remains minor and non-blocking.

Contract and reply evidence. CustomApiUpdate accepts the companion fields, explicit-null runtime fields remain a real write, and the name no-op can only be known after reading the current definition. I saw the author's explanation in reply 3889610607 and the explicit confirmation of both residual cases in reply 3892930108; those replies correctly explain why a naive value classifier could under-lock, but they do not mitigate the demonstrated unnecessary wait. This is the same C7 root, not a new issue.

Fix. Classify effective writes after preserving explicit-null runtime_input_schema and other real definition writes, or otherwise split/normalize the proven no-op companion cases without weakening locks for fields that are actually assigned. Add boundary coverage for is_active combined with description=None and with the current name.

Review limitations and evidence

• CI preflight was green: all 14/14 checks completed successfully. Review decision remains CHANGES_REQUESTED because the major root above is blocking.
• No project test suites, builds, linters, or formatters were run, per the review assignment. The workers' targeted probes were narrow route harnesses, not project-suite validation.
• The history extractor failed with a usage-limit error. The complete raw PR exports and linked issue exports were still read and manually accounted for; this limitation does not convert any tracked underlying behavior into a fix or a new finding.
• Both Simplification Lens runs were unavailable because of usage limits; no simplification opportunities are asserted, and no Simplification section is included.
• Targeted authorization probe cells and outcomes: Prepare targeted authorization race repro established the gate-to-definition-query harness with current route functions and a temporary file-backed SQLite database; Reproduce MCP revoke during lock wait removed the victim MCP association while a queued PUT resumed, leaving a durable parent mutation and HTTP 500; Reproduce admin deletion during Custom lock wait removed the victim user/link, left the Custom parent description durable, and produced ObjectDeletedError/HTTP 500; Reproduce admin deletion during MCP lock wait produced the corresponding durable MCP mutation and HTTP 500; Reproduce admin deletion during Custom DELETE lock wait reproduced a stale 204 and parent deletion after authority removal; and Verify orphaned-definition DELETE race confirmed the sole/shared fixtures, including loss of the remaining association in the shared case. The local probes controlled the authorization boundary on SQLite; the PostgreSQL lock-wait behavior was not exercised by running the project suites.

Blocking status & recommended decision

Blocking: yes. The final confirmed set contains one blocker: a supported Custom or MCP definition-writing request can pass its association/permission gate, wait for the parent lock, and then commit shared state after that authority has been revoked. The Custom mixed no-op companion-field root is minor and non-blocking.

Blocking issues:

• src/xagent/web/api/custom_api.py:369 — major, Blocking: yes — stale pre-lock association/authority can let Custom/MCP writes commit after revocation and can produce a misleading HTTP 500 or stale Custom DELETE 204. [new]

Recommended decision: REQUEST_CHANGES.

[AlexLiu190625]

Closing this in favour of two smaller pull requests, split by connector route family:

• fix(connectors): lock the Custom API definition row on edit and delete #2059 — the Custom API side: PUT and DELETE /api/custom-apis/{id}
• fix(connectors): lock the MCP server definition row on edit #2060 — the MCP side: PUT /api/mcp/servers/{id}

The reason for the split is scope: this branch changed two independent route
families at once, so every review pass had to hold both in view. The two halves
touch disjoint production files and disjoint test files and depend on neither
each other's code nor each other's merge order, so each can now be read and
judged on its own.

Nothing was dropped in the split. The production change and the tests are the
same, taken per file from this branch's head; the only edit was to two comments
in mcp.py that described the Custom API route, reworded so the MCP half does
not read as depending on the other one having landed. Together the two pull
requests carry exactly this branch's line count.

The limitations that were accepted here as tracked separately rather than fixed
are restated in each new pull request, under "Known limitations tracked
separately", with the issue each one lives in: #1944, #1945, #1946, #1932 and
#1816. Each is listed on the side it applies to, so neither review has to
rediscover that they were already settled.

Thank you for the review work on this branch — the shape both new pull requests
carry is the one it arrived at here.