Repository navigation
[DurableTask.ServiceBus] Fix blob container listing and purge for hub names with dashes and culture-dependent container names - #1424
Open
Davide Montanari (davidemontanari) wants to merge 3 commits into
Open
Davide Montanari (davidemontanari) wants to merge 3 commits into
Davide Montanari (davidemontanari) wants to merge 3 commits into
Conversation
…ent container names
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Purging a hub can delete another hub’s overflow containers when their naming prefixes overlap.
1 open issue
What changed in this PR
This PR fixes Service Bus overflow blob naming and expiration handling for dashed hub names and culture-dependent formatting, with regression tests and small test utilities cleanups.
Changes:
- Parse expiration suffixes from the end and use invariant date formatting and lowercasing.
- Tighten suffix validation and add regression tests for dashed hubs, cultures, and invalid characters.
- Return
-1for missing queues and replace fake test credentials with a token credential stub.
| File | Description |
|---|---|
| test/DurableTask.ServiceBus.Tests/WaitForOrchestrationTests.cs | Replace the fake connection string with a credential stub. |
| test/DurableTask.ServiceBus.Tests/NoOpTokenCredential.cs | Add a credential stub for offline tests. |
| test/DurableTask.ServiceBus.Tests/BlobStorageClientHelperTest.cs | Add naming, expiration, and suffix validation tests. |
| src/DurableTask.ServiceBus/Tracking/BlobStorageClientHelper.cs | Fix dashed-name parsing, invariant formatting, and suffix validation. |
| src/DurableTask.ServiceBus/Tracking/BlobStorageClient.cs | Delegate lowercasing to the invariant helper. |
| src/DurableTask.ServiceBus/ServiceBusOrchestrationService.cs | Handle missing queues in delivery-count lookup. |
Review effort: Balanced
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.

Summary
Fixes how
DurableTask.ServiceBusnames, lists and purges overflow blob containers, so that:Also includes two small test-related cleanups.
Fixes
Expired containers were never purged for hub names with dashes
Overflow containers are named
{hubName}-dtfx-{type}-{yyyyMMdd}.BlobStorageClientHelper.IsContainerExpiredrequired exactly 4 dash-separated segments, so for a hub likemy-hubevery container was skipped.PurgeOrchestrationHistoryAsyncnever deleted its overflow blobs, and storage grew without limit.The check now reads the fixed segments from the end and requires the
dtfxmarker, so any hub name works.Container listing included other hubs' and malformed containers
BlobStorageClient.ListContainersAsyncfiltered by the{hubName}-dtfxprefix only. Because hub names can contain dashes, this returned:alpha, the listing includedalpha-dtfx-backup-dtfx-session-20150516, which belongs to hubalpha-dtfx-backup;alpha-dtfx-session-backuporalpha-dtfx-session-20150230.Deleting a hub (
DeleteBlobStoreContainersAsync) therefore also deleted these, including another hub's overflow messages and session state.ListContainersAsyncnow returns only containers owned by this hub (new internalBlobStorageClientHelper.IsContainerOwnedByHub):-{type}-{date}must equal this hub's{hubName}-dtfxprefix;yyyyMMddcalendar date (invariant culture).Both
DeleteExpiredContainersAsyncandDeleteBlobStoreContainersAsyncuse this listing.Container names depended on the host culture
th-TH, where 2015 is 2558), containers got far-future dates. The expiry check parses dates with the invariant culture, so these containers never expired. Dates are now written withCultureInfo.InvariantCulture. Blob reads and writes are not affected, because the blob key carries the exact container name.ToLower(). Ontr-TR,Ibecomes a dotlessı, which is not valid in a container name. Now usesToLowerInvariant(), and a redundantToLower()inBlobStorageClientis removed.Container name suffix validation accepted invalid characters
The regex
@"^[a-z0-9\\-]+$"also matched a backslash (\\in a verbatim string is a literal backslash), and$matched before a trailing\n. It is now^[a-z0-9-]+\z, which accepts only lowercase letters, digits and dashes. It is built once as a static field.Cleanups
ServiceBusOrchestrationService.GetHubQueueMaxDeliveryCountsAsync(internal, used only by tests):Single→SingleOrDefault, so a missing queue returns the intended-1instead of throwing.WaitForOrchestrationTestsno longer uses a fake connection string containingSharedAccessKey=..., which could trigger secret scanners. It now uses a new sharedNoOpTokenCredentialtest helper with the host-name +TokenCredentialconstructor.Behaviour changes / upgrade notes
BlobStorageClient.ListContainersAsync(public) no longer returns:{hubName}-dtfx-{type}-{yyyyMMdd}format. The framework never creates these, but any created manually are no longer listed or deleted with the hub.TaskHubClient.PurgeOrchestrationHistoryAsync. For hubs whose names contain dashes, the first purge after upgrading deletes the backlog of the hub's own overflow containers older than the threshold, which may be a large one-off batch.2558) are not purged automatically and need a one-time manual cleanup.AzureTableInstanceStoreneeds a dash-free hub name; that is unchanged.Tests
BlobStorageClientHelperTest, offline:IsContainerExpiredHubNameWithDashesTestContainerNamesAreCultureInvariantTest(th-TH,tr-TR)ParseKeyRejectsInvalidSuffixCharactersTest(backslash, trailing newline)IsContainerOwnedByHubTest(overlapping hubs, dashed hubs, empty type, non-date suffixes, invalid calendar dates)BlobStorageClientTest, storage-backed:TestDeleteExpiredContainersPreservesOverlappingHub: with hubsalphaandalpha-dtfx-backupin one account, purgingalphadeletes only its own expired container; the other hub's containers remain and its session blob is still readable.TestDeleteBlobStoreContainersPreservesOverlappingHub:alphalists only its own containers, and deleting the hub keeps the other hub's containers.TestDeleteBlobStoreContainersIgnoresMalformedContainers:{prefix}-session-backupand{prefix}-session-20150230are neither listed nor deleted with the hub.BlobStorageClientHelperTestBlobStorageClientTest(run against Azurite)ServiceBusUtilsTestsWaitForOrchestrationTests