Repository navigation
GH-4509: sweep expired handled envelopes on CosmosDb, and make DeleteAllHandledAsync work - #4587
Merged
Merged
Conversation
…AllHandledAsync work
CosmosDbMessageStore.MarkIncomingEnvelopeAsHandledAsync stamped a keepUntil that nothing ever read
back. CosmosDbDurabilityAgent's only sweep was tryDeleteExpiredDeadLetters, there is no equivalent of
the RDBMS DeleteExpiredHandledEnvelopesCommand, no ttl is written, and MigrateAsync creates the
container with default ContainerProperties, so container TTL is off. Handled inbox documents were kept
forever, bodies included, since mark-handled is a read/modify/replace that leaves body alone.
That bites harder here than on a SQL store: IncomingMessage.PartitionKey is the envelope's destination,
so every handled envelope for a listening endpoint piles into ONE logical partition, against a 20 GB
hard ceiling and a 10k RU/s cap. FetchCountsAsync, which CheckHealthAsync calls on every health check,
counts that pile cross-partition too. And there was no way to clean up after the fact:
DeleteAllHandledAsync threw NotSupportedException, so the built-in clear-handled command failed on this
provider.
Now:
- tryDeleteExpiredHandledEnvelopes, mirroring the dead-letter sweep, on its own
HandledMessageCleanupPollingTime cadence and bounded by HandledMessageCleanupBatchSize and
HandledMessageCleanupMaxBatchesPerCycle. Those three knobs read as general durability settings but
were referenced only from Wolverine.RDBMS.DurabilityAgent, so on Cosmos they did nothing at all.
- DeleteAllHandledAsync implemented with the same query minus the keepUntil predicate.
Unlike the dead-letter sweep this query is necessarily cross-partition -- dead letters share one
partition key, handled inbox documents do not -- so each delete carries the document's own partition
key, which is why the projection selects it.
## The comparison has to be on timestamps, not strings
keepUntil is persisted as an ISO-8601 string that keeps whatever offset it was written with; documents
in the test container carry "-05:00". Cosmos compares two strings lexicographically, so the obvious
`c.keepUntil < @now` against a UTC-rendered parameter is meaningless the moment the offsets differ.
The first version of this patch did exactly that and deleted a handled document half an hour short of
its expiry -- caught only because the test seeds a deliberately-retained document alongside the expired
one. DateTimeToTimestamp normalises both sides to epoch milliseconds and reads the offset, so it is also
correct for documents written by an older version.
Worth a look separately: tryDeleteExpiredDeadLetters compares c.expirationTime the same string-wise way.
Bug_4286_dead_letter_expiration passes only because its keeper is a full DAY out, so the dates differ
and the lexicographic comparison happens to land correctly. That is a latent instance of the same bug
and it is not touched here.
## On the tests
Assertions are by envelope id, not through FetchCountsAsync().Handled. A running Wolverine host handles
its own control traffic, so it mints fresh handled documents with the default five-minute retention
while the test watches -- a count never reaches zero and says nothing about whether the SEEDED document
was swept. That is GH-4499's lesson again: a count does not tell you whether the rows are the same rows.
The first draft asserted the count and failed for that reason rather than the real one.
Three documents per sweep test: one expired handled, one handled but well inside its window, one never
handled. Only the first may disappear. Both tests fail on unmodified main.
CosmosDbTests 125 green against the emulator. Pinned Release build of wolverine.slnx clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VDUrBeB4tTnKj4AExCS1nj
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.
Closes #4509. Thanks to @r0ss88 for the report — the diagnosis in the issue was exactly right, including the pointer at
Bug_4286_dead_letter_expirationas the place for the test.What was broken
MarkIncomingEnvelopeAsHandledAsyncstamped akeepUntilthat nothing ever read back:CosmosDbDurabilityAgent's only sweep wastryDeleteExpiredDeadLettersDeleteExpiredHandledEnvelopesCommandttlis written, andMigrateAsynccreates the container with defaultContainerProperties, so container TTL is offSo handled inbox documents were kept forever, bodies included, since mark-handled is a read/modify/replace that leaves
bodyalone. And there was no way to clean up afterwards either:DeleteAllHandledAsyncthrewNotSupportedException, so the built-inclear-handledcommand failed outright on this provider.It bites harder on Cosmos than on a SQL store, as the issue notes:
IncomingMessage.PartitionKeyis the envelope's destination, so every handled envelope for a listening endpoint piles into one logical partition — 20 GB hard ceiling, 10k RU/s cap.FetchCountsAsync, whichCheckHealthAsynccalls on every health check, counts that pile cross-partition too.What this adds
tryDeleteExpiredHandledEnvelopes, mirroring the dead-letter sweep, on its ownHandledMessageCleanupPollingTimecadence and bounded byHandledMessageCleanupBatchSize/HandledMessageCleanupMaxBatchesPerCycle. Those three knobs read as general durability settings but were referenced only fromWolverine.RDBMS.DurabilityAgent— on Cosmos they did nothing at all, which the issue also flagged.DeleteAllHandledAsyncimplemented with the same query minus thekeepUntilpredicate.Unlike the dead-letter sweep this query is necessarily cross-partition — dead letters share one partition key, handled inbox documents do not — so each delete carries the document's own partition key, which is why the projection selects it.
The comparison has to be on timestamps, not strings
This is the part worth reviewing.
keepUntilis persisted as an ISO-8601 string that keeps whatever offset it was written with; documents in the test container carry-05:00. Cosmos compares two strings lexicographically, so the obvious form:is meaningless the moment the offsets differ. The first version of this patch did exactly that, and it deleted a handled document half an hour short of its expiry — caught only because the test seeds a deliberately-retained document alongside the expired one.
DateTimeToTimestampnormalises both sides to epoch milliseconds and reads the offset, so it is also correct for documents already written by an older version.A latent instance of the same bug, deliberately not touched here:
tryDeleteExpiredDeadLetterscomparesc.expirationTimethe same string-wise way.Bug_4286_dead_letter_expirationpasses only because its keeper is a full day out, so the dates differ and the lexicographic comparison happens to land correctly. Worth its own issue.On the tests
Assertions are by envelope id, not through
FetchCountsAsync().Handled. A running Wolverine host handles its own control traffic, so it mints fresh handled documents with the default five-minute retention while the test watches — the count never reaches zero and says nothing about whether the seeded document was swept. That is #4499's lesson again: a count does not tell you whether the rows are the same rows. The first draft asserted the count and failed for that reason rather than the real one.Three documents per sweep test — one expired handled, one handled but well inside its window, one never handled — and only the first may disappear. Both tests fail on unmodified
main.Verification
CosmosDbTests125 green against the emulator (Testcontainers self-provisions it). Clean pinned build:dotnet build wolverine.slnx -c Release -f net9.0.🤖 Generated with Claude Code
https://claude.ai/code/session_01VDUrBeB4tTnKj4AExCS1nj