Fix unsafe actor state mutation in ProcessIndex deletion path - #1952
Open
stasimus wants to merge 4 commits into
Open
Fix unsafe actor state mutation in ProcessIndex deletion path#1952stasimus wants to merge 4 commits into
stasimus wants to merge 4 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem: in the
ProcessIndexdeletion path for stopped actors (Cassandra cleanup),cleanup.deleteAllEvents(...).map { ... }mutated the actor's mutableindexand replied to the client from the future callback — i.e. on a dispatcher thread concurrently with the actor's own thread. The callback also had no failure handling, and the deletion itself was not crash-safe: a node crash at the wrong moment left the journal and index permanently diverged (eitherActorDeletedjournaled with the instance events never removed, or the events removed with the instance still recovering as alive).Reproduce:
supportsCleanupOfStoppedActors), delete a passivated recipe instance whiledeleteAllEventsfails — thedeleteRecipeInstancecall times out and the instance stays undeletable in memory until restart;ActorDeletedjournal write — the instance resurfaces after recovery as a live instance with an empty journal and the deletion is never retried.Fix: deletion is now a durable two-phase operation running entirely on the actor thread.
ActorDeletionStartedis journaled first (new persisted event, instance enters the newDeletingstatus), then the events are deleted asynchronously with the result piped back to the actor as a message, andActorDeletedis journaled only after the events are actually gone. A deletion interrupted by a crash is resumed on recovery; a failed deletion repliesStatus.Failureto the caller and is retried by the retention sweep or a client retry. WhileDeleting, the instance cannot be reactivated or recreated, and the sweep does not launch duplicate concurrent deletions.Backward compatible for upgrades: the new proto fields are optional and old journals/snapshots deserialize unchanged. Not rollback-safe once the new version has run: older Baker versions cannot deserialize
ActorDeletionStartedevents found in the ProcessIndex journal, so a downgrade after deletions have been requested requires those journal entries to age out via snapshots.