fix(duckdbservice): wipe user-attached catalogs at session create on shared-warm workers - #1200
fuziontech wants to merge 2 commits into
Conversation
…shared-warm workers DuckDB ATTACH is instance-global and a hot-idle worker is reused across sessions of an org, but CreateSession only wiped user SECRETS — an attached catalog from a previous session (e.g. an external Postgres source, whose postgres_scanner pool holds live authenticated upstream connections) was silently inherited by the next session. That is the same cross-user isolation boundary as wipeUserSecrets, one level up, and it has a second-order correctness effect: postgres_scanner's pg_pool_max_connections SET has no set-callback and applies only at pool creation, so a session inheriting a stale attach cannot reconfigure the pool it inherits (ATTACH IF NOT EXISTS no-ops), which re-enables the parallel-scan snapshot dance behind load-balanced Postgres replicas and fails intermittently with 'snapshot ... does not exist'. CreateSession now detaches every non-internal attached catalog except the system-managed reserved set (ducklake, delta, memory) before the secret wipe, failing the session on wipe error exactly like secrets. Tests: duckdbservice/user_catalogs_test.go (unit: wipe drops user catalogs incl. quoted names, preserves ducklake/delta/memory/system/ temp; wiring: a catalog attached by session A is gone after session B's CreateSession on the same worker). E2E: user_catalog_wipe assertion in tests/mw-dev/e2e/harness.sh. Co-authored-by: Shelley <shelley@exe.dev>
Test Impact PlanDeterministic summary of how this PR changes tests, CI runners, and coverage-risk signals. Summary
Signals
Coverage risk: neutral or increased No coverage-reduction warnings detected. |
bill-ph
left a comment
bill-ph
left a comment
There was a problem hiding this comment.
Reviewed commit a6e104a.
No P0 blockers found. The shared-warm session boundary now detaches user catalogs before wiping secrets, preserves reserved/internal catalogs, quotes user-controlled identifiers safely, fails session creation on wipe errors, and adds unit, wiring, and mw-dev E2E coverage.
— Robo Bill
benben
left a comment
benben
left a comment
There was a problem hiding this comment.
Automated review generated on behalf of @benben.
Approve. Adds a session-create DETACH of every non-internal, non-reserved catalog on shared-warm workers, mirroring the secret wipe. The mechanism and tests are sound for user attaches, but the reserved set does not name DuckLake's Postgres-typed metadata catalog (__ducklake_metadata_ducklake); if duckdb_databases() does not flag it internal, the wipe detaches the org's own metadata catalog on every fresh session, and the unit tests (all :memory: attaches) cannot detect that.
- major
duckdbservice/user_catalogs.go:48DuckLake metadata catalog not reserved; wiped if not flagged internal. ConfirmSELECT internal FROM duckdb_databases() WHERE database_name='__ducklake_metadata_ducklake'is true on a real worker; otherwise every CreateSession detaches it and the reaper-configured pg pool from configureDuckLakeMetadataPool is lost. - minor
duckdbservice/service.go:1472DestroySession still leaves user catalogs (live upstream pools) on hot-idle worker. Secrets get a best-effort wipe at destroy but attached postgres_scanner pools stay open for the whole hot-idle TTL. - minor
duckdbservice/user_catalogs_test.go:48Tests only attach :memory: catalogs; no real DuckLake/type catalog covered. A regression where a system-owned non-internal catalog gets detached passes both unit tests.
Model: fable.
…nection Each pg/psql -c invocation is its own connection (and session), so the 'attached in its own session' probe ran in a SECOND session — whose CreateSession wipe had already detached the probe (ci-pr-21200-cnpg: 'probe catalog not attached in its own session (count=0)'). Feed both statements to psql via stdin so they ride one connection; also make the probe catalog name run-unique. Co-authored-by: Shelley <shelley@exe.dev>
benben
left a comment
benben
left a comment
There was a problem hiding this comment.
Automated review generated on behalf of @benben.
Approve (re-review of new commits). The new commit only reworks the harness's same-session check to ride one psql connection; that part is correct, but the "fresh session" leak assertion still cannot land on the worker holding the attach under the e2e's exploratory tier.
- major
tests/mw-dev/e2e/harness.sh:2582Leak check passes trivially: fresh SELECT never reuses the pinned worker. mw-dev sets DUCKGRES_EXPLORATORY_TIER_ENABLED=true and findIdleAssignedWorkerLocked matches on profile, so the ATTACH (DDL, pinned standard) and the plain SELECT (exploratory) land on different-shaped workers. - minor
tests/mw-dev/e2e/harness.sh:2579Inline psql bypasses pg_script backpressure retry; hard-fails on transient errors. A 'no Duckgres worker'/'capacity exhausted' response on the attach connection aborts the whole harness instead of retrying like every other assertion.
Model: fable.
|
Superseded by #1201 (merged as 261f430), which lands the same session-boundary catalog detach. #1201 adopted this PR's single-connection harness technique, run-unique catalog name and system-catalogs-preserved assertion, and additionally resolves the primary database by oid rather than reserving |
3 participants
Problem
DuckDB
ATTACHis instance-global, and a hot-idle worker pod is reused across sessions of an org.CreateSessionwiped user secrets at session create (wipeUserSecrets) but never detached user catalogs — so a catalog attached by one session (e.g. an external Postgres source, whose postgres_scanner pool holds live, authenticated upstream connections) was silently inherited by the next session on that worker.That is the same cross-user isolation boundary as the secret wipe, one level up. It also has a second-order correctness effect that surfaced in production: postgres_scanner's
pg_pool_max_connectionsSET has no set-callback and is only read when a catalog's pool is created — a session that inherits a stale attach (ATTACH IF NOT EXISTSno-ops) cannot reconfigure the pool, the parallel-scanpg_export_snapshot/SET TRANSACTION SNAPSHOTdance resumes across a load-balanced multi-replica Postgres frontend, and imports intermittently fail withsnapshot "..." does not exist.Fix
CreateSession(shared-warm mode, samemax_sessions=1guard as the secret wipe) now detaches every non-internal attached catalog before the secret wipe, preserving the system-managed reserved set (ducklake,delta,memory— activation and the pg_catalog compat layer own those). A wipe failure fails the session, exactly like secrets. The catalog wipe runs first because attached pools can reference the secrets about to be dropped.Tests
duckdbservice/user_catalogs_test.go— unit: wipe detaches user catalogs (incl. quote-unsafe names), preserves reserved/internal ones; wiring: a catalog attached by session A is gone after session B'sCreateSessionon the same worker (red-green verified against the unwired code).tests/mw-dev/e2e/harness.sh— newuser_catalog_wipeassertion: attach visible in-session, absent on the next fresh session,ducklake+memorypreserved.Co-authored-by: Shelley shelley@exe.dev