From 7af0de4e8cd0ffd95b046077f4d4d3670b1b7dcb Mon Sep 17 00:00:00 2001 From: J Date: Tue, 23 Jun 2026 16:54:18 +0800 Subject: [PATCH] fix(channel): skip orphaned installations in hub-boot active scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preflight deploy review: channel_installation dropped the workspace/agent FK (MUL-3515 §4), so unlike lark_installation it does not cascade away when its workspace is deleted or its agent is hard-deleted (e.g. runtime teardown). The hub-boot query then keeps opening a WebSocket for a bot whose owner is gone. JOIN ListActiveChannelInstallations to live workspace + agent so an orphaned installation is never connected, uniformly for every deletion path. The JOIN matches the old ON DELETE CASCADE semantics (row existence, not agent archival), so an archived-but-present agent's installation is still listed; the orphaned row's encrypted secret is thereby never decrypted/used. Tests: a real-DB handler test asserts a deleted-workspace/agent installation and a non-Feishu one are both excluded; the lark scope test's active-list assertion moved there since the JOIN now needs real workspace/agent fixtures. (Physically deleting dormant orphaned channel rows on workspace/agent deletion is a separate app-layer-cleanup follow-up.) MUL-3515 Co-authored-by: multica-agent --- server/internal/handler/lark_test.go | 58 +++++++++++++++++++ .../lark/channel_store_scope_test.go | 23 +------- server/pkg/db/generated/channel.sql.go | 19 ++++-- server/pkg/db/queries/channel.sql | 19 ++++-- 4 files changed, 91 insertions(+), 28 deletions(-) diff --git a/server/internal/handler/lark_test.go b/server/internal/handler/lark_test.go index 87f6cd19f..2f1f99700 100644 --- a/server/internal/handler/lark_test.go +++ b/server/internal/handler/lark_test.go @@ -1,6 +1,7 @@ package handler import ( + "context" "encoding/json" "log/slog" "net/http" @@ -160,3 +161,60 @@ func TestListLarkInstallations_NotConfigured_HardCodedInstallSupportedFalse(t *t t.Fatalf("install_supported must be false in the early-return branch even with a non-nil APIClient") } } + +// TestListActiveLarkInstallations_SkipsOrphans pins the MUL-3515 hub-boot +// guard: ListActiveChannelInstallations is JOINed to live workspace + agent, +// so an active channel_installation whose workspace or agent has been deleted +// (channel_* has no FK cascade) is never returned — otherwise the Hub would +// keep opening a WebSocket for a bot whose owner is gone. It also stays +// channel_type='feishu'-scoped. Runs against the real test DB. +func TestListActiveLarkInstallations_SkipsOrphans(t *testing.T) { + ctx := context.Background() + agentID := createHandlerTestAgent(t, "LarkActiveScopeAgent", []byte("[]")) + + const ( + liveApp = "cli_active_live" + orphanApp = "cli_active_orphan" + slackApp = "cli_active_slack" + // Deliberately non-existent workspace/agent so the JOIN drops the row. + orphanWS = "5d0a0000-0000-4000-8000-000000000001" + orphanAg = "5d0a0000-0000-4000-8000-000000000002" + ) + clean := func() { + _, _ = testPool.Exec(context.Background(), + `DELETE FROM channel_installation WHERE config->>'app_id' = ANY($1)`, + []string{liveApp, orphanApp, slackApp}) + } + clean() + t.Cleanup(clean) + + seed := func(ws, ag, channelType, app string) { + if _, err := testPool.Exec(ctx, ` +INSERT INTO channel_installation (workspace_id, agent_id, channel_type, config, installer_user_id, status) +VALUES ($1, $2, $3, jsonb_build_object('app_id', $4::text), $5, 'active') +`, ws, ag, channelType, app, testUserID); err != nil { + t.Fatalf("seed %s installation: %v", app, err) + } + } + seed(testWorkspaceID, agentID, "feishu", liveApp) // live workspace + agent -> listed + seed(orphanWS, orphanAg, "feishu", orphanApp) // deleted workspace + agent -> dropped + seed(testWorkspaceID, agentID, "slack", slackApp) // wrong channel_type -> dropped + + active, err := lark.NewChannelStore(testHandler.Queries).ListActiveLarkInstallations(ctx) + if err != nil { + t.Fatalf("ListActiveLarkInstallations: %v", err) + } + seen := map[string]bool{} + for _, inst := range active { + seen[inst.AppID] = true + } + if !seen[liveApp] { + t.Fatal("expected the live-workspace/agent Feishu installation to be listed") + } + if seen[orphanApp] { + t.Fatal("orphaned installation (deleted workspace/agent) must not be listed — the hub would connect a dead bot") + } + if seen[slackApp] { + t.Fatal("non-Feishu installation must not be listed by the Feishu hub") + } +} diff --git a/server/internal/integrations/lark/channel_store_scope_test.go b/server/internal/integrations/lark/channel_store_scope_test.go index f4644b767..4c9cb6b06 100644 --- a/server/internal/integrations/lark/channel_store_scope_test.go +++ b/server/internal/integrations/lark/channel_store_scope_test.go @@ -139,26 +139,9 @@ VALUES ($1, $2, 'slack', 'oc_scope_slack', 'om_scope_slack', 'pending') t.Fatalf("ListLarkInstallationsByWorkspace: got apps=%v, want exactly [%s]", apps, feishuApp) } - // list-active (hub boot): includes the Feishu install, never the Slack one - active, err := store.ListActiveLarkInstallations(ctx) - if err != nil { - t.Fatalf("ListActiveLarkInstallations: %v", err) - } - var sawFeishu, sawSlack bool - for _, r := range active { - switch r.AppID { - case feishuApp: - sawFeishu = true - case slackApp: - sawSlack = true - } - } - if !sawFeishu { - t.Fatal("ListActiveLarkInstallations: missing the active Feishu installation") - } - if sawSlack { - t.Fatal("ListActiveLarkInstallations: returned a Slack installation (hub would supervise another channel)") - } + // (ListActiveLarkInstallations channel-type + live workspace/agent scoping + // is covered by TestListActiveLarkInstallations_SkipsOrphans in the handler + // package, which has real workspace/agent fixtures the JOIN now requires.) // --- outbound reads: a Slack binding/card must not be seen as Feishu --- diff --git a/server/pkg/db/generated/channel.sql.go b/server/pkg/db/generated/channel.sql.go index fac27bee4..334d9d301 100644 --- a/server/pkg/db/generated/channel.sql.go +++ b/server/pkg/db/generated/channel.sql.go @@ -601,15 +601,26 @@ func (q *Queries) GetChannelUserBindingByUserID(ctx context.Context, arg GetChan } const listActiveChannelInstallations = `-- name: ListActiveChannelInstallations :many -SELECT id, workspace_id, agent_id, channel_type, config, status, ws_lease_token, ws_lease_expires_at, installer_user_id, installed_at, created_at, updated_at FROM channel_installation -WHERE status = 'active' - AND channel_type = $1 -ORDER BY created_at ASC +SELECT ci.id, ci.workspace_id, ci.agent_id, ci.channel_type, ci.config, ci.status, ci.ws_lease_token, ci.ws_lease_expires_at, ci.installer_user_id, ci.installed_at, ci.created_at, ci.updated_at FROM channel_installation ci +JOIN workspace w ON w.id = ci.workspace_id +JOIN agent a ON a.id = ci.agent_id +WHERE ci.status = 'active' + AND ci.channel_type = $1 +ORDER BY ci.created_at ASC ` // Boot path for a per-channel-type inbound hub: every active installation of // the given channel_type, so a hub claims leases and opens connections only // for its own platform and never supervises another channel's installation. +// +// The JOINs require the owning workspace and agent rows to still exist. +// channel_installation has no FK (MUL-3515 §4), so unlike the old +// lark_installation (which cascaded away on workspace/agent deletion) an +// installation can be orphaned when its workspace is deleted or its agent is +// hard-deleted (e.g. runtime teardown). Without this guard the hub would keep +// opening a WebSocket for a bot whose workspace/agent is gone. The JOIN matches +// the old ON DELETE CASCADE semantics: it filters on row existence, not agent +// archival, so an archived-but-present agent's installation is still listed. func (q *Queries) ListActiveChannelInstallations(ctx context.Context, channelType string) ([]ChannelInstallation, error) { rows, err := q.db.Query(ctx, listActiveChannelInstallations, channelType) if err != nil { diff --git a/server/pkg/db/queries/channel.sql b/server/pkg/db/queries/channel.sql index 4510a5f73..ecc8bdf1e 100644 --- a/server/pkg/db/queries/channel.sql +++ b/server/pkg/db/queries/channel.sql @@ -75,10 +75,21 @@ ORDER BY created_at ASC; -- Boot path for a per-channel-type inbound hub: every active installation of -- the given channel_type, so a hub claims leases and opens connections only -- for its own platform and never supervises another channel's installation. -SELECT * FROM channel_installation -WHERE status = 'active' - AND channel_type = sqlc.arg('channel_type') -ORDER BY created_at ASC; +-- +-- The JOINs require the owning workspace and agent rows to still exist. +-- channel_installation has no FK (MUL-3515 §4), so unlike the old +-- lark_installation (which cascaded away on workspace/agent deletion) an +-- installation can be orphaned when its workspace is deleted or its agent is +-- hard-deleted (e.g. runtime teardown). Without this guard the hub would keep +-- opening a WebSocket for a bot whose workspace/agent is gone. The JOIN matches +-- the old ON DELETE CASCADE semantics: it filters on row existence, not agent +-- archival, so an archived-but-present agent's installation is still listed. +SELECT ci.* FROM channel_installation ci +JOIN workspace w ON w.id = ci.workspace_id +JOIN agent a ON a.id = ci.agent_id +WHERE ci.status = 'active' + AND ci.channel_type = sqlc.arg('channel_type') +ORDER BY ci.created_at ASC; -- name: SetChannelInstallationStatus :exec UPDATE channel_installation