Skip to content

Commit f811747

Browse files
committed
test(tenant): add regression coverage for id collision and ghost rows
Extend the tenant lifecycle and SHOW dispatch test suites to lock in the two bugs addressed on this branch. Id allocation tests (pgwire_auth_tenants/lifecycle.rs): - create_tenant_without_id_allocates_distinct_ids: two consecutive CREATE TENANTs without an explicit ID must receive different ids; the count-based allocator assigned id 1 to both, silently overwriting the first tenant. - dropped_tenant_id_is_not_reused: the id of a dropped tenant must not be reassigned to a subsequent CREATE TENANT. Ghost-row tests (pgwire_show_dispatch.rs): - show_roles_omits_dropped_role: SHOW ROLES must not list a role after DROP ROLE. - show_tenants_omits_dropped_tenant: SHOW TENANTS must not retain the id-slot or name of a dropped tenant; the regression guard specifically checks for the empty-name ghost signature left by an orphaned admin user. - drop_tenant_refuses_when_real_users_remain: DROP TENANT must return 42501 and name the remaining users when operator-owned accounts still belong to the tenant, leaving both the tenant and the users intact.
1 parent 47f651e commit f811747

2 files changed

Lines changed: 198 additions & 0 deletions

File tree

nodedb/tests/pgwire_auth_tenants/lifecycle.rs

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,70 @@ async fn create_tenant() {
2020
assert!(events.last().unwrap().detail.contains("acme"));
2121
}
2222

23+
/// Two `CREATE TENANT`s without an explicit `ID` must receive distinct
24+
/// ids. The pre-fix allocator derived the id from a lazily-populated
25+
/// traffic counter, so with no traffic every auto-allocated tenant got
26+
/// id 1 and the second create silently overwrote the first.
27+
#[tokio::test]
28+
async fn create_tenant_without_id_allocates_distinct_ids() {
29+
let state = make_state_with_catalog();
30+
let su = superuser();
31+
ddl_ok(&state, &su, "CREATE TENANT alpha").await;
32+
ddl_ok(&state, &su, "CREATE TENANT beta").await;
33+
34+
let catalog = state.credentials.catalog();
35+
let catalog = catalog.as_ref().expect("catalog must be wired");
36+
let a = catalog
37+
.find_tenant_by_name("alpha")
38+
.unwrap()
39+
.expect("tenant alpha must exist");
40+
let b = catalog
41+
.find_tenant_by_name("beta")
42+
.unwrap()
43+
.expect("tenant beta must survive (not be overwritten by alpha's slot)");
44+
assert_ne!(
45+
a.tenant_id, b.tenant_id,
46+
"two auto-allocated tenants must not share an id"
47+
);
48+
// Reserved slots 0 (system) and 1 (bootstrap) are never handed out.
49+
assert!(a.tenant_id >= 2 && b.tenant_id >= 2, "{a:?} {b:?}");
50+
}
51+
52+
/// An auto-allocated id is never reused after the tenant is dropped:
53+
/// the durable high-water-mark only moves forward, so a dropped id
54+
/// cannot be silently reassigned to a different tenant.
55+
#[tokio::test]
56+
async fn dropped_tenant_id_is_not_reused() {
57+
let state = make_state_with_catalog();
58+
let su = superuser();
59+
ddl_ok(&state, &su, "CREATE TENANT gamma").await;
60+
let catalog = state.credentials.catalog();
61+
let dropped_id = catalog
62+
.as_ref()
63+
.unwrap()
64+
.find_tenant_by_name("gamma")
65+
.unwrap()
66+
.expect("gamma must exist")
67+
.tenant_id;
68+
69+
ddl_ok(&state, &su, &format!("DROP TENANT {dropped_id}")).await;
70+
ddl_ok(&state, &su, "CREATE TENANT delta").await;
71+
72+
let reused = state
73+
.credentials
74+
.catalog()
75+
.as_ref()
76+
.unwrap()
77+
.find_tenant_by_name("delta")
78+
.unwrap()
79+
.expect("delta must exist")
80+
.tenant_id;
81+
assert_ne!(
82+
reused, dropped_id,
83+
"a fresh tenant must not reuse a dropped tenant's id"
84+
);
85+
}
86+
2387
#[tokio::test]
2488
async fn drop_system_tenant_rejected() {
2589
let state = make_state();

nodedb/tests/pgwire_show_dispatch.rs

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,140 @@ async fn show_roles_lists_created_role() {
125125
);
126126
}
127127

128+
/// After `DROP ROLE`, `SHOW ROLES` must not list the dropped role. A
129+
/// `DROP` that leaves the entry visible in `SHOW` is a ghost — the
130+
/// catalog and the introspection view disagree on whether the role
131+
/// exists.
132+
#[tokio::test]
133+
async fn show_roles_omits_dropped_role() {
134+
let server = TestServer::start().await;
135+
server
136+
.exec("CREATE ROLE ghost_role_to_drop")
137+
.await
138+
.expect("CREATE ROLE must succeed");
139+
server
140+
.exec("DROP ROLE ghost_role_to_drop")
141+
.await
142+
.expect("DROP ROLE must succeed");
143+
144+
let rows = server
145+
.query_named_rows("SHOW ROLES")
146+
.await
147+
.expect("SHOW ROLES must not error");
148+
149+
assert!(
150+
!rows
151+
.iter()
152+
.any(|r| r.values().any(|v| v == "ghost_role_to_drop")),
153+
"SHOW ROLES must not list a role that was dropped: {rows:?}"
154+
);
155+
}
156+
157+
// ── SHOW TENANTS after DROP TENANT ───────────────────────────────────
158+
159+
/// After `DROP TENANT`, `SHOW TENANTS` must not list the dropped tenant
160+
/// — neither by name nor as a ghost id-slot.
161+
///
162+
/// `CREATE TENANT` auto-provisions a `<name>_admin` user in the new
163+
/// tenant. `SHOW TENANTS` derives its row set from the union of
164+
/// catalog-registered tenants and every user's `tenant_id`, so an
165+
/// orphaned admin user resurrects the dropped tenant as a ghost row
166+
/// whose `name` is empty (the catalog row is gone) but whose
167+
/// `tenant_id` slot is retained. The regression guard below asserts no
168+
/// such empty-named ghost survives for the dropped id.
169+
#[tokio::test]
170+
async fn show_tenants_omits_dropped_tenant() {
171+
let server = TestServer::start().await;
172+
// Explicit high id so the new tenant does not collide with the
173+
// bootstrap superuser's home tenant; the auto-provisioned
174+
// `<name>_admin` is then the only user in it.
175+
server
176+
.exec("CREATE TENANT ghost_tenant_to_drop ID 4242")
177+
.await
178+
.expect("CREATE TENANT must succeed");
179+
180+
let before = server
181+
.query_named_rows("SHOW TENANTS")
182+
.await
183+
.expect("SHOW TENANTS must not error");
184+
let tenant_id = before
185+
.iter()
186+
.find(|r| {
187+
r.get("name")
188+
.map(|n| n == "ghost_tenant_to_drop")
189+
.unwrap_or(false)
190+
})
191+
.and_then(|r| r.get("tenant_id"))
192+
.cloned()
193+
.expect("created tenant must be visible in SHOW TENANTS before drop");
194+
195+
server
196+
.exec("DROP TENANT ghost_tenant_to_drop")
197+
.await
198+
.expect("DROP TENANT must succeed");
199+
200+
let after = server
201+
.query_named_rows("SHOW TENANTS")
202+
.await
203+
.expect("SHOW TENANTS must not error");
204+
205+
assert!(
206+
!after.iter().any(|r| r
207+
.get("name")
208+
.map(|n| n == "ghost_tenant_to_drop")
209+
.unwrap_or(false)),
210+
"SHOW TENANTS must not list a tenant dropped by name: {after:?}"
211+
);
212+
// Regression guard for the specific silent failure mode: a retained
213+
// id-slot with a cleared (empty) name is the ghost signature.
214+
assert!(
215+
!after.iter().any(|r| r.get("tenant_id") == Some(&tenant_id)),
216+
"SHOW TENANTS must not retain the id-slot of a dropped tenant \
217+
(ghost row with empty name): {after:?}"
218+
);
219+
}
220+
221+
/// `DROP TENANT` must refuse (`42501`) when an operator-owned user
222+
/// still belongs to the tenant, rather than orphaning it (which leaves
223+
/// a `SHOW TENANTS` ghost) or silently hard-deleting it. The user and
224+
/// the tenant must both survive the refused drop.
225+
#[tokio::test]
226+
async fn drop_tenant_refuses_when_real_users_remain() {
227+
let server = TestServer::start().await;
228+
server
229+
.exec("CREATE TENANT keep_tenant ID 4243")
230+
.await
231+
.expect("CREATE TENANT must succeed");
232+
server
233+
.exec("CREATE USER keep_tenant_member WITH PASSWORD 'pw' TENANT 4243")
234+
.await
235+
.expect("CREATE USER must succeed");
236+
237+
let err = server
238+
.client
239+
.simple_query("DROP TENANT keep_tenant")
240+
.await
241+
.expect_err("DROP TENANT must be refused while a real user remains");
242+
let code = err.code().map(|c| c.code().to_string()).unwrap_or_default();
243+
assert_eq!(
244+
code, "42501",
245+
"DROP TENANT with a remaining real user must fail with 42501, got: {err}"
246+
);
247+
248+
// No silent deletion: the real user must still exist.
249+
let users = server
250+
.query_named_rows("SHOW USERS")
251+
.await
252+
.expect("SHOW USERS must not error");
253+
assert!(
254+
users.iter().any(|r| r
255+
.get("username")
256+
.map(|n| n == "keep_tenant_member")
257+
.unwrap_or(false)),
258+
"the operator-owned user must survive a refused DROP TENANT: {users:?}"
259+
);
260+
}
261+
128262
// ── SHOW STATS / SHOW SERVER STATS / SHOW METRICS / SHOW MEMORY ──────
129263

130264
/// `SHOW STATS` must reach a real handler. The session-parameter fallback

0 commit comments

Comments
 (0)