Repository navigation
Remove deleted vhost files from the replication index - #2335
Merged
Merged
Conversation
|
No issues found. |
jage
force-pushed
the
fix/vhost-delete-replicated-files
branch
from
October 5, 2026 11:30
f693e93 to
87794f4
Compare
carlhoerberg
approved these changes
Oct 5, 2026
VHost#delete only removed the data dir, so with clustering enabled the leader kept an index entry for every file of every deleted vhost, and followers kept the files on disk along with an open fd and digest for each. Repeated vhost create/delete grew memory without bound, also on a single node with clustering enabled. Add Replicator#delete_dir, which drops every index entry under a dir and tells followers to delete those files, and call it from VHost#delete before the dir is removed. Close #2334
jage
force-pushed
the
fix/vhost-delete-replicated-files
branch
from
October 6, 2026 16:05
87794f4 to
b953ec5
Compare
This was referenced Oct 7, 2026
jage
added a commit
that referenced
this pull request
Oct 7, 2026
Fixes #2334. With clustering enabled, `VHost#delete` closed the vhost and removed its data dir, but never told the replicator. The leader kept a file index entry (some holding an `MFile`) for every file of every deleted vhost, and followers kept the files on disk with an open fd and a SHA1 digest each. Repeated vhost create/delete grew memory without bound, also on a single node with clustering enabled and no followers. It also keeps deleted vhosts from coming back after a failover. A follower kept the deleted vhost's dir, which is named after the SHA1 of the vhost name, so once that follower was promoted, creating a vhost with the same name loaded the old queues, messages and definitions. This adds `Replicator#delete_dir`, which drops every index entry under a dir and sends a file delete for each of them to the followers. `VHost#delete` calls it after `close` and before removing the dir. Working from the index rather than walking the dir covers exactly what was replicated, also entries whose files are already gone. Followers need no protocol change: they already handle per-file deletes and prune empty parent dirs. Covered by a regression spec in `spec/clustering_spec.cr`. (cherry picked from commit ec9571b)
jage
added a commit
that referenced
this pull request
Oct 7, 2026
Fixes #2334. With clustering enabled, `VHost#delete` closed the vhost and removed its data dir, but never told the replicator. The leader kept a file index entry (some holding an `MFile`) for every file of every deleted vhost, and followers kept the files on disk with an open fd and a SHA1 digest each. Repeated vhost create/delete grew memory without bound, also on a single node with clustering enabled and no followers. It also keeps deleted vhosts from coming back after a failover. A follower kept the deleted vhost's dir, which is named after the SHA1 of the vhost name, so once that follower was promoted, creating a vhost with the same name loaded the old queues, messages and definitions. This adds `Replicator#delete_dir`, which drops every index entry under a dir and sends a file delete for each of them to the followers. `VHost#delete` calls it after `close` and before removing the dir. Working from the index rather than walking the dir covers exactly what was replicated, also entries whose files are already gone. Followers need no protocol change: they already handle per-file deletes and prune empty parent dirs. Covered by a regression spec in `spec/clustering_spec.cr`. (cherry picked from commit ec9571b)
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.
What does this pull request do?
Fixes #2334. With clustering enabled,
VHost#deleteclosed the vhost and removed its data dir, but never told the replicator. The leader kept a file index entry (some holding anMFile) for every file of every deleted vhost, and followers kept the files on disk with an open fd and a SHA1 digest each. Repeated vhost create/delete grew memory without bound, also on a single node with clustering enabled and no followers.It also keeps deleted vhosts from coming back after a failover. A follower kept the deleted vhost's dir, which is named after the SHA1 of the vhost name, so once that follower was promoted, creating a vhost with the same name loaded the old queues, messages and definitions.
This adds
Replicator#delete_dir, which drops every index entry under a dir and sends a file delete for each of them to the followers.VHost#deletecalls it aftercloseand before removing the dir. Working from the index rather than walking the dir covers exactly what was replicated, also entries whose files are already gone. Followers need no protocol change: they already handle per-file deletes and prune empty parent dirs.How can it be tested?
New spec in
spec/clustering_spec.cr: create a vhost with a durable queue and a published message, wait for the follower to catch up, delete the vhost, and expect the vhost dir to be gone on the follower. It fails without the change. The clustering, vhost, message store and definitions replication specs pass, andmake lintis clean.