Skip to content

persist: finish GC on a dropped shard before calling it finalized - #40524

Draft
mtabebe wants to merge 2 commits into
MaterializeInc:mainfrom
mtabebe:per-143-gc-after-drop
Draft

mtabebe wants to merge 2 commits into
MaterializeInc:mainfrom
mtabebe:per-143-gc-after-drop

Conversation

@mtabebe

@mtabebe mtabebe commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Context:
The storage controller keeps each dropped shard in a set until it is
finalized. Each tick it skips shards where is_finalized is true, calls
finalize_shard on the rest, and drops a shard once that returns Ok.

Problem:
It is possible for the storage controller to say it is done before
GC has finished. is_finalized holds as soon as the shard is a
tombstone with one empty batch, before any GC runs.

Retrying finalize_shard does not commit anything, so it does not ask
for GC and returns. Moreover, one GC may not be enough, because we bound
how many versions one GC run can remove.

Since nothing commits to a finalized shard, nothing asks for GC again,
and we can leak blobs.

Solution:
State::finalized_gc_step returns the next step on a finalized shard
(write a rollup, or GC through some seqno), or None when it is done.

finalize_shard runs those steps until None, and is_finalized requires
None.

If no progress is made (e.g., on stale state behind a concurrent GC) we
back off and retry up to a bounded time period.

Note:
Shards that left the controller's set before this change, or that were
finalized while persist_gc_remove_finalized_rollups is off, keep their
old rollups. is_finalized depends on that flag.

Testing:

  • finalize_retry_finishes_gc: skips GC after become_tombstone, then
    retries finalize_shard, with the flag on and off. Before this change
    the shard counted as finalized before GC ran, and a part leaked.
  • Check that no rollup blob is left that state does not reference.

Fixes PER-143

@mtabebe
mtabebe force-pushed the per-143-gc-after-drop branch 2 times, most recently from 6a8fc7e to 4982f2b Compare November 6, 2026 00:36
mtabebe and others added 2 commits November 6, 2026 10:28
Move the test `Consensus` that commits a CaS and then loses its response
from `schema.rs`'s tests to the shared test helpers in `lib.rs`, so tests
outside `schema.rs` can use it. No behavior change.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Context:
The storage controller keeps each dropped shard in a set until it is
finalized. Each tick it skips shards where `is_finalized` is true, calls
`finalize_shard` on the rest, and drops a shard once that returns `Ok`.

Problem:
It is possible for the storage controller to say it is done before
GC has finished. `is_finalized` holds as soon as the shard is a
tombstone with one empty batch, before any GC runs.

Retrying `finalize_shard` does not commit anything, so it does not ask
for GC and returns. Moreover, one GC may not be enough, because we bound
how many versions one GC run can remove.

Since nothing commits to a finalized shard, nothing asks for GC again,
and we can leak blobs.

Solution:
`State::finalized_gc_step` returns the next step on a finalized shard
(write a rollup, or GC through some seqno), or `None` when it is done.

`finalize_shard` runs those steps until `None`, and `is_finalized` requires
`None`.

If no progress is made (e.g., on stale state behind a concurrent GC) we
back off and retry up to a bounded time period.

Note:
Shards that left the controller's set before this change, or that were
finalized while `persist_gc_remove_finalized_rollups` is off, keep their
old rollups. `is_finalized` depends on that flag.

Testing:
- `finalize_retry_finishes_gc`: skips GC after `become_tombstone`, then
retries `finalize_shard`, with the flag on and off. Before this change
the shard counted as finalized before GC ran, and a part leaked.
- Check that no rollup blob is left that state does not reference.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@mtabebe
mtabebe force-pushed the per-143-gc-after-drop branch from 4982f2b to 100456f Compare November 6, 2026 15:44

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant