Order the failed message status by the times the events happened - #5812
Open
johnsimons wants to merge 1 commit into
Open
Order the failed message status by the times the events happened#5812johnsimons wants to merge 1 commit into
johnsimons wants to merge 1 commit into
Conversation
A retry acknowledgement set Status to Resolved unconditionally, and an incoming processing attempt set it to Unresolved unconditionally. Inside one batch the statement order settles it, and FailedMessageBatchWriter says so. Across two transactions the last commit wins, whichever event actually happened first. --error-ingestion-only made that reachable: several hosts drain the same error queue into one database, so a confirmation and a later failure of the same message can be on different hosts entirely. A message that failed again after its retry succeeded could end up sitting at Resolved, and a redelivered attempt could pull an already resolved or archived message back to Unresolved. The retry acknowledgement carries the time the retry succeeded, so RecordSuccessfulRetry now takes it. Both persisters compare it against the newest attempt they have stored: - a confirmation resolves only a message whose newest stored attempt is no newer than the retry - an attempt moves the status only when it is strictly newer than the newest stored attempt, where the payload columns already take a tie Strict on one side and not the other is deliberate. Refreshing the payload from a redelivery of the newest attempt changes nothing; letting that same redelivery flip the status undoes a resolve or an archive that came after it. For EF the resolve becomes a dialect statement, because the guard is per row and ExecuteUpdate has nowhere to put it. The retry rows are still deleted whatever the guard decides: the retry itself completed, so its claim is released either way. For RavenDB the two patch scripts grow a branch, which the retention stamp moves inside: a late older attempt must not strip @expires off a message that is still resolved, and ExpirationManager now hands out the statement so it can sit in a branch rather than only at the end of a script. MessageExpiryTests gave both of its attempts the same AttemptedAt, which models a redelivery rather than a re-failure. They are now a minute apart, which is what the test name describes.
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.
The bug
Two unconditional writes to
FailedMessage.Status:ResolvedUnresolvedNeither looks at when the event it represents actually happened, so whichever
write lands last wins. Two symptoms:
Resolved, so the failure is invisible.message back to
Unresolved.On RavenDB the second one also strips the message's
@expiresstamp, becauseCancelExpirationruns down the same unconditional path, so the messagereappears in the failed message list and then never expires.
This does not need scale-out, or even concurrency
Ingestion runs
TransportTransactionMode.ReceiveOnly: the write commits, thenthe receive is acknowledged. Any attempt can therefore arrive twice, which is
why the EF side already has
Redelivery_of_a_stored_attempt_is_not_a_new_attempt.Redelivery alone reaches symptom 2, on one host, with one writer.
Symptom 1 is queue ordering rather than writer count. The acknowledgement and
the later failure are two separate messages on the error queue being received at
MaximumConcurrency. A single writer preserves batch order, not event order.When both land in the same batch it is worse:
ErrorIngestor.Ingestruns theerror processor before the retry confirmation processor, so a confirmation and a
re-failure in one batch always end
Resolvedno matter what the timestamps say.Verified by reverting the RavenDB guards in this branch and running the new
tests against them. Single writer, one process:
--error-ingestion-onlywidens the window, since several hosts drain one queueand the two messages need not even be on the same host, but it did not create it
and it did not touch this code.
How long
18bc698d6a6(2023-09-29)Statuswrite on RavenDBf9bc7c2b87b(2026-07-23)CancelExpirationjoined it, on the same unconditional pathThe July change was right about the case it targeted, which is that a genuine
re-failure must not keep a stale expiry. It just inherited the 2023 bug and
made it worse: before it, a wrongly un-resolved message still expired away
eventually, and after it the message stays forever.
The fix
The acknowledgement carries the time the retry succeeded, in the
ServiceControl.Retry.Successfulheader, soRecordSuccessfulRetrynow takesit. Both persisters compare it against the newest attempt they have stored:
newer than the retry
stored attempt
Strict on one side and not the other is deliberate, and differs from the payload
columns, which take the incoming attempt on a tie. Refreshing the payload from a
redelivery of the attempt already stored changes nothing. Letting that same
redelivery flip the status undoes a resolve or an archive that came after it.