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

[fuziontech]

Problem

On 2026-09-18 a tenant's hourly sqlmesh refresh failed 5 sources.db_* models (plus 1 on the retry an hour later, and 2 ad-hoc analyst queries) with:

Failed to execute query "SET TRANSACTION SNAPSHOT '00000351-0000EF66-1'": ERROR: snapshot "00000351-0000EF66-1" does not exist

Failures landed 1–7s into each statement, were confined to a ~1h window, and every 4-hourly run before and after was clean.

Root cause is ours. Attached databases are instance-global, exactly like secrets, and the worker never detaches client-attached ones. So a client's ATTACH ... AS db survives its session on the hot-idle worker and is inherited by whichever session that worker serves next:

1. An analyst ran one-shot psql sessions doing CREATE OR REPLACE TEMPORARY SECRET analyst_secret (...) + ATTACH IF NOT EXISTS '' AS db (TYPE postgres, SECRET analyst_secret, READ_ONLY) — 23 attaches over 9 workers in the failing hour.
2. sqlmesh then reused those workers. Its own ATTACH IF NOT EXISTS '' AS "db" (... SECRET "pipeline_secret") was a no-op, because db already existed. The scanner freezes the connection string at ATTACH time, so sqlmesh's scans silently ran against the analyst's endpoint.
3. That endpoint can't import a snapshot across connections, so postgres_scanner's parallel scan (pg_export_snapshot() on the main conn, SET TRANSACTION SNAPSHOT on each extra thread) failed.

The tenant query log separates cleanly: all 4 workers that failed had served such a psql attach beforehand; all 8 workers that had not were clean.

It is also a cross-user isolation hole, same class as the secrets wipe: an attached catalog keeps the credentials it was built from even after wipeUserSecrets drops the secret. Verified live — a catalog attached as one org user was queryable from a later session as a different user who held no such secret and ran no ATTACH.

Fix

detachUserCatalogs (duckdbservice/user_catalogs.go) runs right next to the secrets wipe:

• Mandatory on every shared-warm CreateSession — a failure fails the session, same semantics as the wipe (the CP's existing create-failure handling applies).
• Best-effort at DestroySession, so a parked worker doesn't hold a credential-bearing catalog between sessions.

Preserved: DuckDB-internal catalogs, the instance's primary database (resolved by lowest non-internal oid, since it is a file stem rather than memory when DataDir is set), and the worker-managed allowlist ducklake / delta / __ducklake_metadata_*.

The listing is catalog-qualified (system.main.duckdb_databases()): 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 when the schema version has moved. The session-init probe has normally paid for that already, so this is defensive. The detach also runs under its own deadline so it cannot starve the persistent-secret replay that follows.

If a detach ever failed persistently the CP retires that worker (createSessionOnWorker always passes retireOnFailure=true), so the org's next connection gets a fresh one rather than looping on a stuck worker.

Like the wipe, this is only safe under one-session-per-worker; it sits inside the same sharedWarmMode && maxSessions == 1 guard.

Tests

• duckdbservice/user_catalogs_test.go: allowlist, identifier quoting, file-backed primary, no-op, and that a post-detach ATTACH IF NOT EXISTS binds the NEW target.
• TestCreateSessionDetachesPreviousSessionCatalogs: pool-level, two consecutive sessions on one shared-warm worker — including the case where the best-effort destroy-time detach was skipped, so the CreateSession detach is the one that has to hold.
• e2e: persistent_user_secret_isolation gains an attached-catalog leak check modelled on the incident: root leaves a catalog holding a marker table, and a second user's ATTACH IF NOT EXISTS under the same alias must come up empty. Each side's statements ride ONE connection (fed to psql on stdin) — every pg call is its own session, whose session-create detach would otherwise remove the catalog before it could be observed. ATTACH parse-fails into the pin set, so both sides land on the org's standard worker rather than being split across the exploratory tier. Also asserts ducklake + memory survive the detach. Statement shapes verified against a local standalone server.
• Verified by hand against a real Postgres attach: the attached catalog stays queryable after wipeUserSecrets drops its secret (the bug), is gone after detachUserCatalogs, and a re-attach then correctly fails with "secret not found".
• CLAUDE.md documents the invariant, including that a new worker-managed ATTACH must be added to the allowlist.

Supersedes #1200

#1200 is an independent take on the same fix. This one additionally resolves the primary database by oid instead of reserving memory by name (a file-backed instance's primary is a file stem), qualifies the listing, gives the detach its own deadline, detaches best-effort at session destroy so a parked worker does not hold a live authenticated pool, and carries the cross-user IF NOT EXISTS e2e check. It adopts #1200's single-connection harness technique, its run-unique catalog name, and its system-catalogs-preserved assertion.

Not in this PR

Tables a client creates in the memory catalog also survive on a hot-idle worker — same class of stale state, separate change.

🤖 Generated with Claude Code

https://claude.ai/code/session_016ZnZTbiHkGWQ7485DoJB3E

[github-actions]

Test Impact Plan

Deterministic summary of how this PR changes tests, CI runners, and coverage-risk signals.

Summary

Area	Added	Changed	Deleted
Test files	1	1	0
E2E/journey files	0	0	0
Workflow files	0	0	0

Signals

• Test cases: +5 / -0
• Assertions: +33 / -0
• Skips or known failures added: 0
• Workflow continue-on-error added: 0
• Workflow path filters added: 0
• Test commands removed from justfile: 0
• E2E/journey retry lines added: 0

Coverage risk: neutral or increased

No coverage-reduction warnings detected.

[bill-ph]

Reviewed commit bd13b2f.

Findings:

• No P0 blockers found. The session-boundary detach is mandatory in CreateSession, best-effort in DestroySession, preserves the primary and worker-managed catalogs, quotes identifiers safely, and includes unit/pool/e2e coverage for cross-session isolation.
• Non-blocking: CI unit tests and the MW-dev e2e job were still in progress at review time; please confirm they complete successfully before merge.

— Robo Bill

[benben]

Automated review generated on behalf of @benben.

Approve. Adds a session-boundary DETACH of client-attached catalogs on shared-warm workers (mandatory at CreateSession, best-effort at DestroySession), mirroring the existing secrets wipe. The mechanism, guard placement, primary-DB resolution, identifier quoting and error unwind all check out; unit tests are real DuckDB round-trips including the "destroy-time detach skipped" case. The main gap is that the preserve-list is name-based and a client can pick a preserved name.

• major duckdbservice/user_catalogs.go:19 Client can persist a catalog by using an allowlisted name. ATTACH 'postgres://…' AS __ducklake_metadata_x (or AS delta when DeltaCatalogEnabled=false) survives every session boundary.
• minor tests/mw-dev/e2e/harness.sh:2563 Leak check can pass vacuously on a different worker. Nothing asserts root's and $u2's sessions shared one worker pod.
• minor duckdbservice/service.go:1264 Detach shares the 15s secretCtx budget with wipe and replay. A slow postgres catalog teardown can consume the remaining budget and fail the session or skip replay.

Model: opus, PR authored with fable.

[benben]

Automated review generated on behalf of @benben.

Approve (re-review of new commits). The new commits move the catalog detach onto its own 15s deadline and catalog-qualify the database listing to system.main.duckdb_databases(). Both changes hold up: no context is used after the relocated wipeCancel(), and the qualified lookup is covered by the existing new unit tests.

Model: opus, PR authored with fable.

[benben]

Automated review generated on behalf of @benben.

Approve (re-review of new commits). The new commit rewrites the e2e attached-catalog leak check to run each side's statements over a single psql session on stdin. The mechanics are right, but it hand-rolls the psql invocation instead of using the existing pg_script helper and so loses the harness-wide transient retry.

• major tests/mw-dev/e2e/harness.sh:2568 Inline psql bypasses pg_script's transient-retry set. A cold spawn ("failed to detect attached catalogs"/"capacity exhausted") on either session aborts the harness.
• minor tests/mw-dev/e2e/harness.sh:2576 Leak assertion passes even if CreateSession detach is broken. Root's best-effort destroy-time detach (or a different worker for $u2) already yields count=0.

Model: opus, PR authored with fable.