fix: make interrupted GridFS uploads and deletes recoverable - #516
Open
dragoangel wants to merge 1 commit into
Open
dragoangel wants to merge 1 commit into
dragoangel wants to merge 1 commit into
Conversation
dragoangel
force-pushed
the
gridfs-orphans
branch
2 times, most recently
from
September 13, 2026 17:03
c80c5d4 to
e0191e6
Compare
Contributor
Author
dragoangel
force-pushed
the
gridfs-orphans
branch
3 times, most recently
from
September 15, 2026 14:37
d823a2b to
8ee4802
Compare
GridFS chunks outlive the files document that names them. files_id is their only link and it is recorded nowhere else, so once the files document is gone the chunks are unreachable by any query the code can express. Three paths produce that state: an upload that never finishes, a delete interrupted between its two non-atomic halves, and the time-based GC pass itself on a message whose upload straddles its water mark. Every in-flight upload and delete now writes a marker document keyed by files_id and removes it once the operation is whole, so no interruption is invisible. The GC reads those markers and finishes what was started, within minutes rather than within maxQueueTime, and acts only on an operation known not to have completed. The state on the marker is what makes that safe: a file with no chunks means opposite things depending on which operation was in flight, and a collector that deleted on sight would destroy a message that had been queued successfully. A marker records when it stops being anybody's rather than when it was written, and the process holding the operation pushes that moment forward every minute for as long as it is still holding it. The timer lives in the very process doing the writing, so it cannot outlive it: a child killed, out of memory, or thrown out of its event loop takes its timers with it and the marker goes stale with no cleanup path having to run first. The beat only extends, never creates - an operation can be ended by a different process than the one beating for it, because a forked child hands its cleanup to the master over IPC, and an upserting beat would raise the marker from the dead and then beat for it forever. Both queues store: RemoteQueue is the one every SMTP submission takes, MailQueue the one behind the HTTP API, and the marker covers both. It is held until the message is queued rather than until the upload finishes, because a stored message with no delivery behind it is nobody's, and PUT /store/:id clears its own because nothing on that path ever queues one. An SMTP client that disappears mid-DATA needs no crash to reach the same state. smtp-server handles the socket error at session level and, in _onClose, calls this._dataStream.unpipe() and drops the stream without ending, erroring or destroying it, so nothing downstream is told anything: the upload stays open, removeMessage is never called, and maildrop.add's callback never fires. Erroring the size limiter from onClose is the whole fix, because the error then takes the route a size limit or a plugin rejection takes and the existing handlers unwind down to store(). A message that reached the terminating dot is left alone. The time-based pass stays where it was, as the coarse backstop for anything orphaned before the markers existed. No operator action is needed on upgrade. Signed-off-by: Dmytro Alieksieiev <1865999+dragoangel@users.noreply.github.com>
dragoangel
force-pushed
the
gridfs-orphans
branch
from
September 15, 2026 15:17
8ee4802 to
a5595ea
Compare
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.
Implements #515.
What the problem is
GridFS chunks outlive the
filesdocument that names them.files_idis their onlylink and it is recorded nowhere else, so once the
filesdocument is gone the chunksare unreachable by any query the code can express. Three paths produce that state: an
upload that never finishes, a delete interrupted between its two non-atomic halves,
and the time-based GC pass itself on the rare message whose upload straddles the water
mark.
That pass does eventually remove them, by age: it deletes files and chunks whose own
_idis below a water mark taken from the oldest queue row, and an orphan's_idsinks below that mark like anything else. What it cannot do is reason about
reachability, so it cannot tell an orphan from a message that is merely old, and it
can only act once the mark has moved past it. The mark is held back by the oldest
queued delivery, which is itself bounded by
maxQueueTime, so collection is laterather than absent: up to 30 days on the default setting, and indefinitely where
expiry is disabled.
What this changes
Every in-flight upload and delete writes a marker document keyed by
files_idinto<gfs>.pending, and removes it once the operation is whole, so no interruption isinvisible. The GC reads those markers and finishes what was started, within minutes
rather than within
maxQueueTime, and it acts only on an operation that is known notto have completed.
The
stateon the marker is what makes that safe. A file with no chunks meansopposite things depending on which operation was in flight, and a collector that
deleted on sight would destroy a message that had been queued successfully. So the
collector has four outcomes and only three of them delete anything:
deletingwith a files document still present: the delete stopped partway, finishit in the order it would have run in
writingwith no files document: an upload that never produced one, so its chunksare unreachable and go
writingwith a files document but no delivery naming it: the upload finished butthe message never reached the queue, so nothing will ever deliver or delete it
the repair
The delete itself is reordered to remove chunks before the files document, so every
interruption leaves either a marker or a complete file and the collector can tell
which.
Both queues store
RemoteQueueis the queue a forked SMTP child stores through, which is the path everysubmission arriving over the wire takes;
MailQueueis the one behind the HTTP API.Both write the marker, and both hold it until the message is queued rather than
until the upload finishes: a stored message with no delivery behind it is nobody's,
and only the caller knows when it stops being that.
store()hands the upload id backso the caller can say so,
mail-dropclears it after the push, andPUT /store/:idclears its own, because nothing on that path ever queues anything. That endpoint
answers 500 if the clear fails - unlike the same failure after a push, there is no
delivery to keep the body alive, so 200 would promise durability that will not hold.
How long a marker lives
A marker records when it stops being anybody's rather than when it was written, and
the process holding the operation pushes that moment forward once a minute for as long
as it is still holding it. The collector compares that against the clock and has
nothing to derive - which matters because the process that sweeps is generally not the
process that wrote, and in a split deployment the two do not share a configuration.
The heartbeat is a timer in the very process doing the writing, so it cannot outlive
it. A child killed, out of memory, or thrown out of its event loop takes its timers
with it and the marker goes stale on its own, with no cleanup path having to run
first. A timeout describes what should happen; a missed beat is what did.
It also covers the parts of an operation that write no chunks at all, which a deadline
tied to write progress would not: headers are parsed before the body reaches the
upload stream and are stored separately as file metadata, and after the stream
finishes the message still has to be scanned, have its metadata written and be pushed
to the queue.
The beat only extends, never creates. An operation can be ended by a different process
than the one beating for it, because a forked child hands its cleanup to the master
over IPC, and an upserting beat would raise the marker from the dead there and then
beat for it forever. Finding nothing to extend is also how a beat learns to stop.
The teardown half
An SMTP client that disappears mid-DATA is the producer that needs no crash at all.
smtp-serverhandles the socket error at session level and, in_onClose, callsthis._dataStream.unpipe()and drops the stream without ending, erroring ordestroying it, so nothing downstream is told anything: the upload stays open,
removeMessageis never called because no delete ever happens, andmaildrop.add'scallback never fires.
Erroring the size limiter from
onCloseis the whole fix, because the error thentakes the same route a size limit or a plugin rejection takes and the existing
handlers unwind the pipeline down to
store(), which removes the chunks it hadwritten. A message that reached the terminating dot is left alone: DATA ends before
the connection closes, so it is complete even if the client never waited for the
response.
The error is named
ClientDisconnectrather than only coded, becausemail-droptreats anything whose name ends in
Erroras a storage failure worth logging andreporting as NOQUEUE. A client that hung up is neither, and on a busy receiver it is
routine - it is logged at info as an outcome, so the error channel stays useful for
real storage faults.
onClosealso recordssession.connectionClosed, because a connection can be lostwhile the
smtp:datahooks are still running, before there is an in-flight messagefor the teardown to find. Piping a dead stream into a pipeline that is only being
built would suspend it for the life of the process with no way back, so that case
destroys the stream instead.
Existing orphans, and why there is no migration step
The time-based pass stays exactly where it was, as the coarse backstop. It is what
collects anything orphaned before the markers existed, and the chunks it strands
itself on a straddling upload, neither of which any marker describes. So this needs no
operator action on upgrade: the old backlog ages out the way it always has, and
everything new is collected in minutes.
Cost
The two passes do not compete.
collectPendingGridfsreads a collection that is emptywhenever nothing is in flight. The heartbeat is one small update a minute per
operation actually in flight. The time-based deletes are
_idrange scans on analways-indexed field, and the water mark sits below the oldest queued message, so
nothing a live message owns is ever in range.
Tests
test/mail-queue-gridfs-test.jsis new: nineteen cases covering the collector's fouroutcomes, the marker lifecycle, the heartbeat, the delete order, and a delete taking
over a stale upload marker.
test/remote-queue-gridfs-test.jsadds six for the storepath every SMTP submission takes, including which stream failures count as storage
failures and which are outcomes.
test/api-store-marker-test.jsadds three for thestore-only endpoint, and
test/smtp-interface-test.jsgains four for the teardown.All of them fail on the code before this change.
Compatibility
No new configuration. The marker collection is
<gfs>.pending, indexed onexpiresthrough
indexes.yaml, and nothing outside the process that opened an operation needsa number to know. An index named
mailpending, oncreated, existed in an earlierrevision of this branch and can be dropped by anyone who deployed from it once every
process is upgraded; a fresh deployment never sees it.
Fixes #515.