fix: free iceberg shards slots by catalog id, not name - #1368
ferhatelmas wants to merge 2 commits into
Conversation
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, aligns behavior across allocation/freeing paths, and is backed by targeted regression tests covering the id-vs-name mismatch scenario.
Pull request overview
This PR fixes an Iceberg sharding resource leak by making shard slot freeing use the same identifier as allocation (catalog id), instead of mixing catalog id for allocation and catalog name for freeing.
Changes:
- Update
TenantAwareRestCatalogto free shard resources usingcatalog.idinstead ofparams.warehouse(catalog name). - Update
IcebergCatalogReconcilerto consistently usecatalog.idwhen finding/reserving/confirming shard allocations. - Add/extend tests to cover the “catalog id differs from name” scenario for both mutation and reconciliation paths.
File summaries
| File | Description |
|---|---|
| src/storage/protocols/iceberg/catalog/tenant-catalog.ts | Uses catalog.id when freeing shard resources to match the allocation resource key. |
| src/storage/protocols/iceberg/catalog/tenant-catalog.test.ts | Adds a regression test ensuring the freed shard resource matches the allocated one when id != name. |
| src/storage/protocols/iceberg/catalog/reconciler.ts | Switches shard resource lookups/reservations/confirmations to use catalog.id instead of catalog.name. |
| src/storage/protocols/iceberg/catalog/reconciler.test.ts | Adds a test asserting the reconciler uses catalog IDs for shard resource matching and allocation. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Coverage Report for CI Build 34260580560Coverage increased (+0.6%) to 82.901%Details
Uncovered Changes
Coverage Regressions19 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings about migration/backfill of existing resource_id rows, I checked that this change is consistent with the rest of the file — createTable already used catalog.id for bucketName (lines 227, 271 pre-existing), so this fix just aligns dropTable and the reconciler to match, rather than introducing a new pattern.
Extended reasoning...
This is a small, targeted bugfix: reconciler.ts and tenant-catalog.ts's dropTable previously used catalog.name (or params.warehouse) instead of catalog.id when building the shard resource key passed to the sharder, causing a free/reserve mismatch after a catalog rename. I verified createTable in tenant-catalog.ts already used catalog.id at lines 227 and 271, confirming the fix brings dropTable and the reconciler into line with the existing convention rather than inventing a new one — reducing risk of the fix itself being wrong. New tests were added in both test files exercising the exact create-then-drop scenario with a catalog whose id differs from its name, asserting reserve/confirm/freeByResource are all called with the same bucketName.
No security-sensitive surface is touched (no auth, crypto, or permission logic); this is purely an internal resource-key consistency fix in shard bookkeeping.
Given the inline findings already raised (about whether existing/stale shard_slots or shard_reservation rows keyed by name need a migration/backfill), a human should weigh in on the operational rollout question, which is a legitimate concern the code review alone cannot settle.
|
@claude review always |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a new job that can delete shard reservations/slots in production, so it warrants final human verification of operational safety and rollout expectations.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
dc03b20 to
9bb68ce
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding on batch resumability, I checked two other angles and ruled them out: the reclaimer's "local table" guard matches only on (tenant_id, namespace_id, name), not the candidate's shard_id/slot_no, but iceberg_tables has a unique index per (tenant_id, namespace_id, catalog_id, name), so a table can only have one live shard binding at a time — no masked duplicate-leak path. I also verified the reconciler's Promise.all → sequential-loop refactor doesn't drop restorations on error: an unswallowed exception just stops that pass, and the remaining missing tables are picked up on the next reconciliation run.
Extended reasoning...
This run's confirmed inline finding (admin endpoint missing afterReservationId, combined with runBatch/inspect re-throwing non-timeout upstream errors, meaning an operator has no way to resume past a permanently-failing candidate) is new and not something I raised in my prior two reviews of this PR, so it warrants surfacing. Beyond that, I independently re-examined the two candidate issues surfaced this run and was able to concretely rule both out from the code and schema: the iceberg_tables unique index on (tenant_id, namespace_id, catalog_id, name) means the reclaimer's name-only local-table check cannot mask a genuinely orphaned duplicate reservation, since only one shard binding can be live per table at a time; and the reconciler's switch from Promise.all to a sequential per-table loop with only 404/NoSuchTable swallowed doesn't lose any missing-table restoration work, it just defers it to the next reconciliation pass on error, matching prior (fail-fast) behavior. My earlier inline comment about the advisory lock being held across external REST-catalog calls during batch reclamation remains open and unaddressed by the latest commit, but since it was already posted in a prior round I am not repeating it here.
aeefe88 to
9545253
Compare
139d5fc to
8e79b30
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new reclaimer’s “allocation unchanged” guard query does not verify the reservation row’s tenant/resource match, which can allow freeing a live reservation if rows become temporarily inconsistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
839d2d1 to
9579f7a
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces new operational behavior (admin endpoint + queue worker + locking/rollback semantics) in addition to the bug fix, which warrants final human validation in the target deployment environment.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Critical reservation-collision handling and moderate reclamation retry issues must be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, this run also checked two other candidates and ruled them out: the reclamation continuation's job-id check in reclaim-shard-slots.ts (Event.send() can return null on an exactly_once policy drop, but the continuation's singletonKey is unique per batch so that path isn't actually reachable here), and the new ReconciliationRollbackError aborting reconcile() on a rollback failure in reconciler.ts (this matches the PR's stated intent to fail fast rather than silently swallow errors, not an oversight).
Extended reasoning...
This run's inline finding (namespace bucket-name property never being sent upstream by tenant-catalog.ts's createNamespace, so syncUpstreamOrphanTables' catalogName lookup always falls back to findFirstCatalog) is a genuine new issue not raised in prior rounds. Two other candidates surfaced by the hunt were investigated and ruled out rather than posted: the nextJobId === undefined check in reclaim-shard-slots.ts:57 is technically checking the wrong sentinel (pg-boss send() signals a policy drop via null, not undefined) but the continuation's singletonKey (runId + the new afterReservationId) is always fresh, so the exactly_once collision this guards against cannot actually occur on the continuation path; and reconciler.ts's new ReconciliationRollbackError aborting the whole reconcile() on a rollback failure is the PR's deliberate fail-fast design (explicitly called out in the PR description as replacing silent Promise.allSettled swallowing), not an accidental behavior change. Given the PR already carries a large number of open threads from prior rounds (several resolved only by the author, not independently), and this run adds one more confirmed, unaddressed issue, a full body reiterating "needs human review" would be redundant — the inline finding and the still-open prior threads already make that clear.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/storage/protocols/iceberg/catalog/reconciler.ts— syncUpstreamOrphanTables resolves the owning catalog via namespaceResp.properties['bucket-name'], but tenant-catalog.ts's createNamespace never sends that property upstream (commented out at tenant-catalog.ts:237), so catalogName is always undefined and it always falls back to findFirstCatalog (arbitrary catalog, no ORDER BY). For tenants with more than one catalog, an orphan table gets attributed to the wrong catalog.id/bucket_name on restore. Fix: resolve the catalog from the local iceberg_namespaces row for dbNamespaceId (source of truth) instead of an upstream property that is never populated, or actually set bucket-name when creating namespaces. Pre-existing, but this diff extends the same lookup to feed migrateLegacyAllocation/reserve with catalog.id.Extended reasoning...
In syncUpstreamOrphanTables (reconciler.ts:127-172), catalogName is read from namespaceResp.properties?.['bucket-name']. tenant-catalog.ts createNamespace (lines 230-240) has the properties block that would set 'bucket-name' commented out, so no REST catalog namespace ever carries this property. Therefore catalogName is always undefined, findCatalogByName is never invoked, and every orphan-table restoration falls through to findFirstCatalog(tnx, tenantId), which does
SELECT id, name FROM iceberg_catalogs WHERE tenant_id=$1 AND deleted_at IS NULL LIMIT 1with no ORDER BY. For a tenant with 2+ catalogs, this can attribute a restored table to the wrong catalog_id/bucket_name in iceberg_tables and in the shard reservation (bucketName: catalog.id), silently misrouting a recovered table to an unrelated catalog.Verification: pre-existing. The mechanism is real and reachable: reconciler.ts:148 reads
catalogName = namespaceResp.properties?.['bucket-name']; the upstreambucket-nameproperty is commented out in tenant-catalog.ts:234-239 ("Underline catalog doesn't support this"), socatalogNameis always undefined,findCatalogByNameis never invoked, and control falls tofindFirstCatalog(reconciler.ts:154).…
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
9579f7a to
5a8a41e
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces new production reclamation logic that can delete shard allocations and adds an admin-triggered worker/endpoint, so it merits careful human validation of operational safety despite strong tests.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
I reviewed this PR's latest push (commit 5a8a41e, "add job to reclaim leaked shards") and found no new bugs. The concerns I raised in earlier rounds on this PR — legacy catalog-name-keyed reservations not being migrated when the reconciler restores an upstream-orphan table, the orphan-restore loop aborting entirely on one table's failure, the namespace advisory lock spanning external REST-catalog calls, and the admin endpoint lacking a way to resume a scan past a poison reservation — all now appear addressed in the current code (a migrateLegacyAllocation step, per-table try/catch, the listNamespaces probe moved outside the lock/transaction with bounded lock/statement timeouts, and an exposed afterReservationId body field). Given the size and sensitivity of this change (new background worker, admin endpoint, reworked reconciler, and shard-slot locking logic) and outstanding third-party review threads, a human look is still worthwhile.
Extended reasoning...
Overview
This run reviews the final state of a large, multi-push PR that adds an Iceberg shard-slot reclamation background worker and admin endpoint, and reworks the reconciler and locking logic that allocates/frees shard slots for Iceberg tables. The diff spans 17 files and ~2000 changed lines, including a substantial new commit (5a8a41e) pushed after all of my prior reviews on this PR.
Security risks
No new injection, auth-bypass, or data-exposure risk was found in this run. The admin route is behind API-key auth and gated on multitenant+queue-enabled mode; SQL queries use parameter binding throughout the reconciler and reclaimer.
Level of scrutiny
This PR touches shard allocation, locking, and reconciliation logic that is directly responsible for preventing data loss/corruption and resource leaks across tenants — high-sensitivity code that merits careful review, which is consistent with the many rounds of feedback already exchanged on it.
Other factors
I traced each of my four previously-raised concerns (reconciler.ts legacy-resource-id migration, per-table loop isolation, namespace-lock-across-network-calls, and the admin endpoint's missing resume parameter) against the current code and found each one substantively addressed by the latest commit. I did not re-verify the Copilot bot's more recent inline comments (reconciler.ts:285, reclaim-shard-slots.ts:152) against the current code, since their content isn't visible to me and the file has changed significantly since those were posted — that residual uncertainty, plus the overall scope of the change, is why I'm deferring to a human rather than approving outright.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Iceberg shard slots are allocated by id but freed by name so leaks.
What is the new behavior?
It's now always using id consistently.
Add admin triggered reclaim job that only does when slot identity matches tenant/resource, no local table metadata, warehouse is reachable, table is gone.