fix(worker): detach client-attached catalogs at the session boundary

CLAUDE.md

@@ -740,6 +740,24 @@ Invariants for anyone touching this path:
   + the `__default_*`/`duckgres_*` prefixes, which activation re-creates). It
   MUST run before replay on every CreateSession in shared-warm mode, and a
   wipe failure MUST fail the session.
+- **Client-ATTACHed catalogs get the same treatment, in the same place.**
+  `detachUserCatalogs` (`duckdbservice/user_catalogs.go`) runs right after the
+  secrets wipe on every shared-warm CreateSession and a failure MUST fail the
+  session (best-effort again at DestroySession). Attached databases are
+  instance-global exactly like secrets, and an attached catalog freezes its
+  connection string — credentials included — at ATTACH time, so it outlives
+  the wipe of the secret it was built from: without the detach, user B can
+  query user A's attached Postgres without ever holding A's credential. It is
+  also a correctness bug, not just isolation: clients attach with
+  `ATTACH IF NOT EXISTS ... AS db`, a no-op when the previous session left a
+  `db` behind, so the new session silently reads the PREVIOUS session's target
+  (2026-09-18: a tenant's sqlmesh run inherited an analyst's `db` endpoint and
+  its parallel postgres scans died with `SET TRANSACTION SNAPSHOT ... snapshot
+  does not exist`). Preserved: DuckDB-internal catalogs, the instance's primary
+  database (resolved by lowest oid — it is a file stem, not `memory`, when
+  DataDir is set), and the `isSystemCatalog` allowlist (`ducklake`, `delta`,
+  `__ducklake_metadata_*`). **A new worker-managed ATTACH must be added to that
+  allowlist** or the next session create will detach it.
 - **Execute-then-persist ordering.** Persist only statements DuckDB accepted;
   a store failure after a successful exec is an ERROR telling the user the
   secret will NOT survive the session. Replay failures at session create are
@@ -762,10 +780,11 @@ Invariants for anyone touching this path:
   `usersecrets.RedactErrorForLog(query, errMsg)` guards those error sinks
   (`logQueryError`, `logQuery`); keep new error logging behind it too, and pass
   the original (un-redacted) query so it can classify.
-- Touching the interception, wipe/replay, or payload shape → update
-  `server/conn_user_secrets_test.go`, `duckdbservice/user_secrets_test.go`,
-  and the `persistent_user_secret`(+`_isolation`) assertions in
-  `tests/mw-dev/e2e/harness.sh`.
+- Touching the interception, wipe/replay, catalog detach, or payload shape →
+  update `server/conn_user_secrets_test.go`,
+  `duckdbservice/user_secrets_test.go`, `duckdbservice/user_catalogs_test.go`,
+  and the `persistent_user_secret`(+`_isolation`, which also carries the
+  attached-catalog leak check) assertions in `tests/mw-dev/e2e/harness.sh`.
 
 ## Admin Console (VPC-private web UI, `kubernetes` tag)
 

duckdbservice/service.go

@@ -1257,6 +1257,25 @@ func (p *SessionPool) CreateSession(username, memoryLimit string, threads int, s
 		if len(wiped) > 0 {
 			slog.Info("Wiped user secrets left by previous session.", "user", username, "count", len(wiped))
 		}
+		// Same isolation step for client-ATTACHed databases: they are
+		// instance-global too, and an attached catalog keeps the credentials
+		// it was built from even after the secret above is gone. Mandatory for
+		// the same reason as the wipe — see detachUserCatalogs.
+		// Own deadline: a slow detach must not eat the replay's budget below.
+		detachCtx, detachCancel := context.WithTimeout(context.Background(), userSecretOpTimeout)
+		detached, detachErr := detachUserCatalogs(detachCtx, conn)
+		detachCancel()
+		if detachErr != nil {
+			secretCancel()
+			_ = conn.Close()
+			p.mu.Lock()
+			p.reserved--
+			p.mu.Unlock()
+			return nil, nil, fmt.Errorf("detach user catalogs before session start: %w", detachErr)
+		}
+		if len(detached) > 0 {
+			slog.Info("Detached user catalogs left by previous session.", "user", username, "count", len(detached), "catalogs", detached)
+		}
 		secretWarnings = replayUserSecrets(secretCtx, conn, username, secretStatements)
 		secretCancel()
 	}
@@ -1453,7 +1472,14 @@ func (p *SessionPool) DestroySession(token string) error {
 		if _, err := wipeUserSecrets(wipeCtx, session.DB); err != nil {
 			log.Warn("Failed to wipe user secrets on session destroy.", "user", session.Username, "error", err)
 		}
+		// Likewise for client-ATTACHed databases, which carry their own copy
+		// of the credentials. Best-effort here; mandatory at CreateSession.
 		wipeCancel()
+		detachCtx, detachCancel := context.WithTimeout(context.Background(), userSecretOpTimeout)
+		if _, err := detachUserCatalogs(detachCtx, session.DB); err != nil {
+			log.Warn("Failed to detach user catalogs on session destroy.", "user", session.Username, "error", err)
+		}
+		detachCancel()
 	}
 
 	// Best-effort restore of the cache-proxy S3 transport if this session

duckdbservice/user_catalogs.go

@@ -0,0 +1,109 @@
+package duckdbservice
+
+import (
+	"context"
+	"fmt"
+	"strings"
+)
+
+// systemCatalogNames are the attached databases the worker itself manages on a
+// shared-warm instance: activation attaches (and re-attaches) them, so a
+// session-boundary detach must leave them alone.
+var systemCatalogNames = map[string]struct{}{
+	"ducklake": {},
+	"delta":    {},
+}
+
+// systemCatalogPrefixes covers catalogs DuckDB extensions attach on the
+// worker's behalf — DuckLake attaches its metadata store under this prefix.
+var systemCatalogPrefixes = []string{"__ducklake_metadata_"}
+
+// isSystemCatalog reports whether an attached database is worker-managed.
+// DuckDB catalog names are case-insensitive.
+func isSystemCatalog(name string) bool {
+	name = strings.ToLower(name)
+	if _, ok := systemCatalogNames[name]; ok {
+		return true
+	}
+	for _, p := range systemCatalogPrefixes {
+		if strings.HasPrefix(name, p) {
+			return true
+		}
+	}
+	return false
+}
+
+// detachUserCatalogs detaches every database a client ATTACHed on the shared
+// per-org instance, preserving only what the worker manages itself: DuckDB's
+// internal catalogs (system/temp), the instance's primary database, and the
+// isSystemCatalog allowlist.
+//
+// Attached databases are instance-global, exactly like secrets, so an ATTACH
+// survives its session on a hot-idle worker and is inherited by whichever
+// session the worker serves next. Two distinct failures follow:
+//
+//   - Cross-user isolation: an attached catalog freezes its connection string —
+//     credentials included — at ATTACH time, so it keeps working after
+//     wipeUserSecrets has dropped the secret it was built from. User B of the
+//     org could query user A's attached Postgres without ever holding A's
+//     credential.
+//   - Silent mis-routing: clients attach with `ATTACH IF NOT EXISTS ... AS db`,
+//     which is a no-op when a previous session left a `db` behind, so the new
+//     session reads whatever target the PREVIOUS session chose. On 2026-09-18 a
+//     tenant's sqlmesh run inherited an analyst's `db` (a different endpoint)
+//     and its parallel postgres scans failed with `SET TRANSACTION SNAPSHOT
+//     ... snapshot does not exist`.
+//
+// Like the secrets wipe this is only safe with one session per worker: with a
+// concurrent session it would detach a catalog out from under a live query.
+//
+// Returns the names that were detached.
+func detachUserCatalogs(ctx context.Context, h secretDBHandle) ([]string, error) {
+	// The primary database is the one the instance was opened on — the lowest
+	// non-internal oid, since everything else is attached afterwards. It is
+	// `memory` on k8s workers but a file stem when DataDir is set, so resolve it
+	// rather than hard-coding a name.
+	//
+	// The function is catalog-qualified on purpose. By this point in session
+	// create the default catalog is `ducklake`, and sessionmeta documents that
+	// an unqualified `duckdb_databases()` there starts a DuckLake transaction —
+	// which pays a full catalog reload (tens of seconds on a large tenant) when
+	// the schema version has moved. Naming `system` means the lookup never has
+	// to consult the default catalog. The session-init probe has normally paid
+	// for that reload already, so this is belt and braces, not a measured win.
+	rows, err := h.QueryContext(ctx, `
+		SELECT database_name,
+		       database_oid = (SELECT MIN(database_oid) FROM system.main.duckdb_databases() WHERE NOT internal) AS is_primary
+		FROM system.main.duckdb_databases()
+		WHERE NOT internal`)
+	if err != nil {
+		return nil, fmt.Errorf("list attached databases: %w", err)
+	}
+	var candidates []string
+	for rows.Next() {
+		var name string
+		var isPrimary bool
+		if err := rows.Scan(&name, &isPrimary); err != nil {
+			_ = rows.Close()
+			return nil, err
+		}
+		if isPrimary || isSystemCatalog(name) {
+			continue
+		}
+		candidates = append(candidates, name)
+	}
+	if err := rows.Err(); err != nil {
+		_ = rows.Close()
+		return nil, err
+	}
+	_ = rows.Close()
+
+	var detached []string
+	for _, name := range candidates {
+		if _, err := h.ExecContext(ctx, "DETACH DATABASE IF EXISTS "+quoteSecretIdent(name)); err != nil {
+			return detached, fmt.Errorf("detach database %q: %w", name, err)
+		}
+		detached = append(detached, name)
+	}
+	return detached, nil
+}

duckdbservice/user_catalogs_test.go

@@ -0,0 +1,193 @@
+package duckdbservice
+
+import (
+	"context"
+	"database/sql"
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/posthog/duckgres/server"
+)
+
+func attachedDatabases(t *testing.T, db *sql.DB) []string {
+	t.Helper()
+	rows, err := db.Query("SELECT database_name FROM duckdb_databases() WHERE NOT internal ORDER BY database_name")
+	if err != nil {
+		t.Fatalf("list databases: %v", err)
+	}
+	defer func() { _ = rows.Close() }()
+	var names []string
+	for rows.Next() {
+		var n string
+		if err := rows.Scan(&n); err != nil {
+			t.Fatalf("scan: %v", err)
+		}
+		names = append(names, n)
+	}
+	return names
+}
+
+func mustExecDB(t *testing.T, db *sql.DB, q string) {
+	t.Helper()
+	if _, err := db.Exec(q); err != nil {
+		t.Fatalf("exec %q: %v", q, err)
+	}
+}
+
+// Regression for the 2026-09-18 incident: a client `ATTACH ... AS db` survived
+// its session on the hot-idle worker, so the next session's
+// `ATTACH IF NOT EXISTS ... AS db` was a no-op and it read the PREVIOUS
+// session's target. The detach must remove client catalogs and keep the
+// worker-managed ones.
+func TestDetachUserCatalogs(t *testing.T) {
+	db := openSecretTestDB(t)
+	dir := t.TempDir()
+	// Worker-managed catalogs, stood in for by plain DuckDB files: only the
+	// names matter to the allowlist.
+	mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "lake.duckdb")+"' AS ducklake")
+	mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "meta.duckdb")+"' AS __ducklake_metadata_ducklake")
+	mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "delta.duckdb")+"' AS delta")
+	// Client catalogs, including one needing identifier quoting.
+	mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "a.duckdb")+"' AS db")
+	mustExecDB(t, db, "ATTACH ':memory:' AS \"My \"\"odd\"\" Db\"")
+
+	detached, err := detachUserCatalogs(context.Background(), db)
+	if err != nil {
+		t.Fatalf("detachUserCatalogs: %v", err)
+	}
+	sort.Strings(detached)
+	if got, want := strings.Join(detached, ","), `My "odd" Db,db`; got != want {
+		t.Errorf("detached = %q, want %q", got, want)
+	}
+
+	got := strings.Join(attachedDatabases(t, db), ",")
+	if want := "__ducklake_metadata_ducklake,delta,ducklake,memory"; got != want {
+		t.Errorf("remaining databases = %q, want %q", got, want)
+	}
+
+	// The point of the fix: the next session's IF NOT EXISTS attach must now
+	// take effect instead of silently inheriting the old target.
+	mustExecDB(t, db, "ATTACH IF NOT EXISTS '"+filepath.Join(dir, "b.duckdb")+"' AS db")
+	var path string
+	if err := db.QueryRow("SELECT path FROM duckdb_databases() WHERE database_name = 'db'").Scan(&path); err != nil {
+		t.Fatalf("read db path: %v", err)
+	}
+	if !strings.HasSuffix(path, "b.duckdb") {
+		t.Errorf("db path = %q, want the new session's target (b.duckdb)", path)
+	}
+}
+
+// The primary database is a file stem, not `memory`, when the instance is
+// opened on a file. It must never be a detach candidate regardless of name.
+func TestDetachUserCatalogsKeepsFileBackedPrimary(t *testing.T) {
+	dir := t.TempDir()
+	db, err := sql.Open("duckdb", filepath.Join(dir, "alice.duckdb"))
+	if err != nil {
+		t.Fatalf("open: %v", err)
+	}
+	t.Cleanup(func() { _ = db.Close() })
+	mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "other.duckdb")+"' AS db")
+
+	detached, err := detachUserCatalogs(context.Background(), db)
+	if err != nil {
+		t.Fatalf("detachUserCatalogs: %v", err)
+	}
+	if len(detached) != 1 || detached[0] != "db" {
+		t.Errorf("detached = %v, want [db]", detached)
+	}
+	if got := strings.Join(attachedDatabases(t, db), ","); got != "alice" {
+		t.Errorf("remaining databases = %q, want %q", got, "alice")
+	}
+}
+
+func TestDetachUserCatalogsNoop(t *testing.T) {
+	db := openSecretTestDB(t)
+	detached, err := detachUserCatalogs(context.Background(), db)
+	if err != nil {
+		t.Fatalf("detachUserCatalogs: %v", err)
+	}
+	if len(detached) != 0 {
+		t.Errorf("detached = %v, want none", detached)
+	}
+}
+
+func TestIsSystemCatalog(t *testing.T) {
+	for name, want := range map[string]bool{
+		"ducklake":                     true,
+		"DuckLake":                     true,
+		"delta":                        true,
+		"__ducklake_metadata_ducklake": true,
+		"db":                           false,
+		"ducklake2":                    false,
+		"memory":                       false, // protected as the primary, not by name
+	} {
+		if got := isSystemCatalog(name); got != want {
+			t.Errorf("isSystemCatalog(%q) = %v, want %v", name, got, want)
+		}
+	}
+}
+
+// End-to-end at the pool level: two consecutive sessions on one shared-warm
+// worker. Session A attaches `db`; session B — possibly a different user of the
+// org — must not inherit it, and B's own IF NOT EXISTS attach must win.
+func TestCreateSessionDetachesPreviousSessionCatalogs(t *testing.T) {
+	db := openSecretTestDB(t)
+	dir := t.TempDir()
+	pool := &SessionPool{
+		sessions:       make(map[string]*Session),
+		stopRefresh:    make(map[string]func()),
+		duckLakeSem:    make(chan struct{}, 1),
+		warmupDB:       db,
+		warmupDone:     make(chan struct{}),
+		cfg:            server.Config{SessionInitTimeout: time.Second},
+		maxSessions:    1,
+		sharedWarmMode: true,
+		activation:     &activatedTenantRuntime{payload: ActivationPayload{OrgID: "analytics"}, db: db},
+	}
+	close(pool.warmupDone)
+
+	a, _, err := pool.CreateSession("alice", "", 0, nil)
+	if err != nil {
+		t.Fatalf("CreateSession(alice): %v", err)
+	}
+	if _, err := a.Conn.ExecContext(context.Background(),
+		"ATTACH IF NOT EXISTS '"+filepath.Join(dir, "alice_target.duckdb")+"' AS db"); err != nil {
+		t.Fatalf("alice attach: %v", err)
+	}
+	if err := pool.DestroySession(a.ID); err != nil {
+		t.Fatalf("DestroySession(alice): %v", err)
+	}
+	// The destroy-time detach is best-effort, but with nothing in the way it
+	// should already have cleared the catalog off the hot-idle worker.
+	if got := strings.Join(attachedDatabases(t, db), ","); got != "memory" {
+		t.Errorf("databases after alice's session = %q, want only memory", got)
+	}
+
+	// Simulate the destroy-time detach having been skipped (it is best-effort):
+	// the CreateSession detach is the one that must hold.
+	mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "alice_target.duckdb")+"' AS db")
+
+	b, _, err := pool.CreateSession("bob", "", 0, nil)
+	if err != nil {
+		t.Fatalf("CreateSession(bob): %v", err)
+	}
+	defer func() { _ = pool.DestroySession(b.ID) }()
+	if got := strings.Join(attachedDatabases(t, db), ","); got != "memory" {
+		t.Fatalf("bob inherited alice's catalogs: %q", got)
+	}
+	if _, err := b.Conn.ExecContext(context.Background(),
+		"ATTACH IF NOT EXISTS '"+filepath.Join(dir, "bob_target.duckdb")+"' AS db"); err != nil {
+		t.Fatalf("bob attach: %v", err)
+	}
+	var path string
+	if err := b.Conn.QueryRowContext(context.Background(),
+		"SELECT path FROM duckdb_databases() WHERE database_name = 'db'").Scan(&path); err != nil {
+		t.Fatalf("read db path: %v", err)
+	}
+	if !strings.HasSuffix(path, "bob_target.duckdb") {
+		t.Errorf("bob's db path = %q, want bob_target.duckdb", path)
+	}
+}