Make stream deletion asynchronous, resumable across restarts - #1770
Make stream deletion asynchronous, resumable across restarts#1770prabhaks wants to merge 6 commits into
Conversation
Lays groundwork for background stream deletion: a durable tombstone marker outside the deleted prefix, an in-memory `deleting` flag on resident streams, and guards in the reload/query/info-endpoint code paths that reject a stream once either is set. Purely additive, no behavior change to the current delete handlers, since nothing yet sets a tombstone or the flag. Prepares for the actual async-delete rewrite in a follow-up PR.
…erable list_dirs_relative only surfaces child directories on every backend (S3/GCS/Azure via list-with-delimiter's common_prefixes, LocalFS via read_dir + is_dir), never leaf objects. A tombstone stored as a bare key named after the stream was therefore invisible to any future scan that needs to discover tombstoned streams rather than check one known name at a time. Move the marker one level deeper, under a directory named after the stream, and add list_tombstoned_streams for that scan.
…lehq#1763) DELETE /logstream/{stream} now writes a tombstone, notifies ingestors, and returns 202 Accepted immediately instead of blocking on the full recursive object-store delete. The actual deletion runs in a deduplicated background task, resumes automatically if the node crashes or restarts mid-delete (via the tombstone left by PR parseablehq#1768's safety net), and self-heals nodes that missed the live notification. Only the node that receives the original DELETE request ever runs the physical delete; ingestors flag the stream as deleting and wait for the tombstone to clear, so a large deletion doesn't get redundantly re-run by every node in the cluster. list_streams() on the local filesystem backend is also fixed to treat a stream mid-deletion as absent rather than erroring out the whole listing.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
|
CI status update: the two Quest integration test failures (Distributed and Standalone) are an expected consequence of this PR's intentional API contract change, not a bug in the implementation. Root causes, confirmed from the CI logs:
Both are already called out under "API contract changes" in the PR description. Quest ( |
parseablehq/parseable#1770 makes DELETE /logstream/{stream} return 202 Accepted instead of 200 OK, since deletion now runs in the background rather than blocking the response. It also makes recreating a stream immediately after deleting it return 409 while the old stream's deletion is still in flight, instead of succeeding right away. Updates DeleteStream to expect 202, and adds a bounded retry-on-409 to the stream creation helpers so tests that delete and immediately recreate the same stream name (a common setup/teardown pattern here) keep working without needing changes at every call site.
|
Opened a companion fix for the Quest test suite: parseablehq/quest#126 (updates the hardcoded 200 assertions to 202, and adds retry-on-409 for tests that recreate a stream right after deleting it). |
…discovery set_metadata replaced the whole LogStreamMetadata wholesale, so a reload racing a delete (e.g. a schema update landing after mark_deleting()) could silently clear the deleting flag back to false despite it being documented as monotonic. Now ORs it in instead of overwriting. list_tombstoned_streams trusted list_dirs_relative's raw directory listing as proof of a marker's existence, but a directory can exist under the tombstone root without the marker itself (e.g. an interrupted write). Each candidate is now re-verified with is_tombstoned before being reported. list_old_streams (unused elsewhere in this codebase, but kept consistent with list_streams) didn't exclude TOMBSTONE_ROOT_DIRECTORY, so dir_with_old_stream would treat it as a corrupt stream directory the same way list_streams did before the earlier fix.
…c-stream-deletion
check_or_load_stream's resident-stream fast path doesn't itself check is_tombstoned (flagged in CodeRabbit's review of parseablehq#1768), so a concurrent request on the same node could slip through in the window between the tombstone becoming durable and mark_deleting() actually running. Moving mark_deleting() before the tombstone write, with no await point in between, closes that window entirely for the initiating node. Cross-node propagation is still bounded by the existing fan-out push and self-heal, not synchronous -- that's an accepted, already-documented limitation of this design, not something this reorder attempts to fix.
|
How the tombstone-based deletion works The core problem: today, The fix, in three parts:
Cross-node correctness (this is a distributed system, not a single process):
Split into two PRs:
Happy to expand on any specific part (crash-recovery ordering, ingestor self-heal timing, why 202 vs 200, etc.) if useful. |
1 participant
Summary
Stacked on #1768.
Closes the second half of #1763: deleting a large stream currently blocks the DELETE request on a full recursive object-store delete, which can take a long time for TB-scale streams even though the underlying delete itself is already reasonably efficient (batched, concurrent). This PR moves the actual deletion to the background, building on the tombstone/
deleting-flag safety net added in #1768.DELETE /logstream/{stream}now writes a durable tombstone, best-effort deletes the smallstream.jsonso the stream disappears from listings almost immediately, flags the streamdeletingin memory, fans the delete out to ingestors, and responds202 Acceptedinstead of200 OKonce all of that is durably in place -- before the slow part even starts.list_streams()on the local filesystem backend would fail the entire listing if it encountered a stream mid-deletion, since that backend treats a stream directory withoutstream.jsonas corrupt rather than "not a stream."API contract change
DELETE /logstream/{stream}now returns202 Accepted(body: "log stream {name} deletion started") instead of200 OKonce the deletion has finished. Any client code checking for exactly200will need updating.Test plan
cargo build --libcargo test --lib(449 passed)cargo fmt --checkcargo clippy --lib --all-targetsACTIVE_STREAM_DELETIONSdedup semantics, andlist_streams()correctly skipping (not erroring on) a stream mid-deletion on the local filesystem backend, including a control case confirming a genuinely corrupt directory still errors