Files
multica/server/pkg/db
Bohan Jiang 33bd8aeaa9 MUL-4134: fix(lark): preserve same-agent bindings when reconnecting a revoked Feishu bot (#4997)
* fix(lark): allow rebinding a revoked Feishu bot to a different agent

When a Feishu/Lark Bot is disconnected from agent A (status → revoked),
the row is preserved for audit but still holds the (channel_type,
config->>app_id) unique index slot. Binding the same Bot to agent B
would fail with:

  duplicate key value violates unique constraint
  "idx_channel_installation_type_appid" (SQLSTATE 23505)

because UpsertChannelInstallation conflicts on (workspace_id, agent_id,
channel_type) — a different agent_id means no conflict match, so it tries
INSERT and hits the app_id unique index.

Fix: before the upsert, inside the same transaction, hard-delete any
revoked installation with the same app_id in the same workspace. The
delete is fenced to status=revoked so an active installation can never
be silently removed. If no revoked row exists the delete is a no-op
(deletes zero rows, returns nil error) and the upsert proceeds normally.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>

* fix(lark): preserve same-agent bindings when reconnecting a revoked Feishu bot

The cleanup added in the previous commit hard-deletes every revoked
channel_installation sharing the app_id in the workspace before the
upsert — including the row belonging to the agent currently being
(re)installed. That regresses the common "disconnect then reconnect the
same bot to the same agent" flow: disconnect only flips status to
'revoked' (bindings are preserved), and UpsertChannelInstallation
conflicts on (workspace_id, agent_id, channel_type), so before this the
same agent's row was reactivated in place — installation_id and every
channel_user_binding / channel_chat_session_binding kept. Deleting it
first forces an INSERT with a fresh installation_id, orphaning every
member's account link (they must re-link) and all chat-session
continuity; only the installer is re-bound.

Fence the delete with `agent_id <> $agent_id` so it only clears a
DIFFERENT agent's revoked row (the genuine app_id-slot blocker). The
same agent's revoked row is left for the upsert to reactivate losslessly.
Since idx_channel_installation_type_appid is globally unique on
(channel_type, app_id), at most one row ever holds a given app_id, so the
excluded row is exactly the one the upsert will reuse.

Adds DB-backed regression tests: same-agent revoked row preserved,
different-agent revoked row deleted, active row never deleted, other
workspace fenced, plus end-to-end reactivation semantics (same agent
keeps installation_id + bindings; different agent gets a fresh id).

Co-authored-by: multica-agent <github@multica.ai>

* fix(lark): clean dependent rows when hard-deleting a rebound Feishu installation

Addresses review on #4997 (MUL-4134). channel_* has no FK/cascade
(MUL-3515 §4), so hard-deleting a different-agent revoked installation
left application-owned rows dangling at a removed installation_id:

- channel_chat_session_binding: the outbound patcher would resolve a
  binding, then fail loading the deleted installation — turning a clean
  no-op into error logs.
- channel_binding_token: a still-unexpired bind link (15 min TTL) could
  be redeemed into the deleted installation, reporting "bound" against a
  bot that no longer reaches the user.
- channel_inbound_audit: dangling installation_id, where migration 124
  models the old ON DELETE SET NULL as an app-layer NULL.
- channel_user_binding: dead member links (a different agent is a
  distinct connection; links do not follow and can never be reused).

Rework RemoveRevokedInstallationByAppID to resolve the single row holding
the app_id and act only when it is revoked, in this workspace, and owned
by another agent; then, on the caller's transaction, clear chat-session
bindings, pending binding tokens and member links, NULL the audit
references, and finally delete the row via the fenced query (defense in
depth). Same-agent reconnect and active/other-workspace rows are no-ops.

Adds DeleteChannelUserBindingsByInstallation,
DeleteChannelBindingTokensByInstallation, and
NullChannelInboundAuditInstallationID queries, plus a DB-backed test
(TestChannelStore_RebindCleansDependentRows) asserting every dependent is
cleaned and the audit row survives detached. Verified the test fails when
the cleanup is skipped.

Co-authored-by: multica-agent <github@multica.ai>

* fix(lark): make the rebind cleanup race-safe with a guarded delete gate

Addresses the concurrency must-fix on #4997 (MUL-4134). The prior shape
read the candidate installation, checked revoked/workspace/agent in Go,
cleaned the dependent rows, then ran the fenced delete. That read-then-
clean-then-delete order has a TOCTOU: while B is rebinding the bot to a
different agent, A can reconnect to the SAME agent and reactivate the row
to 'active' in between. B still wipes A's user/chat/token bindings and
NULLs its audit based on the stale "it was revoked" read, then the fenced
delete no-ops (status is no longer revoked) — so A's installation
survives active but its bindings are gone. Concurrent same-agent data
loss, reintroduced.

Make the guarded DELETE the atomic gate. DeleteChannelInstallationByAppID
becomes DeleteRevokedChannelInstallationByAppID `:one ... RETURNING id`,
and RemoveRevokedInstallationByAppID keys all dependent cleanup off the
id the delete actually claimed. No separate read. Under READ COMMITTED a
concurrent reactivation makes the DELETE re-check status='revoked'
against the live row (EvalPlanQual): it claims nothing, returns
pgx.ErrNoRows, and no dependents are touched. With no FK the cleanup can
follow the claiming delete in the same transaction; any failure rolls the
whole thing back.

Adds TestChannelStore_RebindGuardedDeleteRaceWithReactivation: two real
transactions race on one revoked installation — one reactivates and holds
the row lock, the other runs the rebind cleanup and blocks on the guarded
delete — asserting the installation and every binding stay intact.
Verified this test fails on the old read-then-clean-then-delete shape and
passes (also under -race) on the gated version.

Co-authored-by: multica-agent <github@multica.ai>

---------

Co-authored-by: jiangliangyou <jiangliangyou@xiaomi.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>
Co-authored-by: J <j@multica.ai>
2026-07-07 15:07:02 +08:00
..