Repository navigation
v3.0: redesign around sender / receiver / hosted processor (IHttpClientFactory-style consumption) #39
Description
Why a major version
The 2.1 release (stability fixes, named-options registration, keyed-service injection, IAzureStorageQueueClient) tightens what exists but does not change its shape. The remaining customer pain lives in the shape: one class bundles sending, receiving-with-a-policy, and admin operations, and every consumer writes their own polling loop. Fixing that changes how the package is consumed, so it is a 3.0.
Guiding model: IHttpClientFactory. Keep what we already copied correctly (named registration, one cached SDK client per name, keyed-service injection, an explicit default client). Add the parts we are missing: a per-client builder handle, a split between the shared connection and the way it is consumed, a library-owned consumption loop, and a testing seam.
1. Split "the connection" from "how you consume it"
Today AzureStorageQueueClient does three unrelated jobs: send, receive-with-a-processing-policy, and admin (create/clear/peek). The receiving policy is where every customer complaint comes from, and it is welded to the same object used for sending.
Replace with:
IQueueSender— send only.IQueueReceiver— pulls a batch and returns each message wrapped in a context (QueueMessage<T>:MessageId,DequeueCount,InsertedOn, raw body) that the caller completes explicitly:CompleteAsync,AbandonAsync,DeadLetterAsync,RenewLockAsync. Same model as the Azure Service Bus receiver. The current two-delegate API hides the message identity, which is why consumers cannot detect a poison message or extend a lease.- A processor (item 2) that composes the receiver with a handler.
ReceiveMessagesAsync(handle, handleException) stays as a thin convenience over the receiver, [Obsolete], for the whole of 3.x.
2. A hosted processor as the primary way to consume
Every consumer currently writes their own loop and each one has slightly different bugs. The library should own the loop, the way ServiceBusProcessor or a Functions trigger does:
services.AddAzureStorageQueueClient(b =>
b.AddClient("orders", configuration.GetSection("Queues:Orders"))
.AddProcessor<OrderMessage, OrderHandler>(o =>
{
o.MaxConcurrentMessages = 4;
o.MaxDequeueCount = 5; // then move to orders-poison
o.RenewVisibility = true; // keep the lease alive while the handler runs
}));The processor owns:
- backoff when the queue is empty or storage errors
- bounded concurrency
- renewing the visibility timeout while a handler is still running — this removes the duplicate-processing problem instead of asking consumers to guess a timeout
- moving a message to
<queue>-poisonafter N attempts - passing cancellation into handlers
- draining in-flight messages on shutdown (
IHostedLifecycleService)
Handlers are classes, not lambdas: IQueueMessageHandler<T> with HandleAsync(QueueMessage<T> message, CancellationToken ct), resolved from a new DI scope per message so scoped services (e.g. DbContext) work the way they do in a controller action.
3. Per-client builder handle
AddClient(...) should return a small per-client builder (the IHttpClientBuilder equivalent) carrying the name, so extension methods can attach to one specific client — AddProcessor above depends on this, and it lets third parties extend the library (.AddDeadLetterToServiceBus()) without us owning every feature.
This is the one breaking change that cannot be hidden: source-compatible for single-line x.AddClient(...) usage, binary-breaking otherwise. Also take the other known breaks here: AddDefaultClient returning the builder, ApplicationException → InvalidOperationException, fix or remove QueueClientSettings.SectionName, drop the synchronous PeekMessages, remove the three [Obsolete] LoggerMessageExtensions methods.
4. Consider building on Microsoft.Extensions.Azure
Microsoft already ships named Azure SDK clients from DI and configuration (AddAzureClients, IAzureClientFactory<T>), including credentials from configuration ("Credential": "managedidentity", client secret, etc.). Our TokenCredential property cannot be bound from appsettings.json, so every identity-based consumer writes code for it. If our factory became a thin layer over theirs, our value would be purely serialization + processing, which is where it actually is.
Trade-off: one more dependency and their configuration schema instead of ours. Prototype before committing. Lean: yes, to avoid reimplementing credential handling badly.
5. Serialization: per-client, pluggable, no guessing
IMessageConverter is three generic methods on one global object; it cannot be registered per message type or per client cleanly. Replace with a per-client IQueueMessageSerializer. Move Base64 handling entirely to the SDK's QueueClientOptions.MessageEncoding and delete the sniffing heuristic. Add first-class CloudEvent support (a handler that receives the typed data out of the envelope), since Event Grid → storage queue is the package's stated use case.
6. Telemetry that matches what dashboards expect
- Use
IMeterFactoryso meters are DI-owned and testable withMetricCollector<T>instead of static fields. - Tag by the OpenTelemetry messaging semantic conventions (
messaging.system,messaging.destination.name,messaging.operation) so data lands in standard Grafana / App Insights views. Producer/Consumerspan kinds with trace context carried inside the message (opt-in envelope), giving one end-to-end trace from sender through the queue to the handler.
7. A testing seam for consumers
AddInMemoryQueues() swaps the sender/receiver for an in-memory fake so a handler can be tested end-to-end in a unit test without Azurite. The HttpMessageHandler-stub equivalent. Small to build, large for adoption.
Keep as-is
One cached SDK client per name as a singleton (no handler-rotation machinery — it does not apply to QueueClient), named options for configuration, keyed services for injection, the explicit default client.
Sequencing
- 3.0: items 1, 2, 3 — one piece of work. The processor is what stops consumers writing the loop that takes their host down, and it needs the receiver and the per-client builder. Keep
AzureStorageQueueClientas an[Obsolete]façade over sender + receiver for all of 3.x so nobody is forced to migrate on day one. - 3.x minors: items 5, 6, 7, each independently.
- Before 3.0 starts: prototype item 4 and decide; write the proposed public API down as a design doc first, since the API shape is the decision that is expensive to get wrong.
Migration guide (to write with the release)
IQueueClientFactory.GetQueueClient()continues to return the façade; new code injectsIQueueSender/IQueueReceiveror registers a processor.ReceiveMessagesAsync(handle, handleException)→AddProcessor<T, THandler>().IMessageConverter→IQueueMessageSerializer.- Base64 queues → set
ClientOptions.MessageEncoding = Base64.
Activity
jasonshave commentedon Oct 15, 2026
Independent v3 review: correctness, simplification, and over-engineering
Assessment: the public direction is useful, but I would hold the preview and simplify the implementation before adding features. The strongest opportunities are in processor ownership, registration, and the compatibility layer. Over-engineering is a first-class concern in this review, not a secondary cleanup item.
Requested by the repository owner as assessment only. No repository files were changed, and this comment is feedback for the implementing agent rather than an implementation or an instruction to change the agreed scope without considering its tradeoffs.
Review scope: current local
v3working tree on 2026-10-15, HEADc4e51f9, including the substantial uncommitted/untracked implementation refactor, compared with the 2.1 baselinef760477. Several reviewed implementation files are not present in the committed branch snapshot. Source coordinates below refer to the working tree reviewed; line numbers may move as work continues.Verified correctness findings
F1 — High: host shutdown cancellation leaves handlers and renewal running
Location:
src/AzureStorage.QueueService/QueueProcessor.cs:108(also the shutdown cancellation andWaitAllAsyncpaths).Cancellation of the host's shutdown token makes
WaitAllAsyncthrow before_cancelHandlers.Cancel()runs. Renewal has its own cancellation source and continues too.An isolated probe confirmed that, after
StopAsyncexited through cancellation, the handler token remained uncancelled and three additional renewals occurred. This is especially relevant when the host deadline andShutdownDrainTimeoutcoincide.Recommendation: make shutdown cleanup unconditional, with distinct deadlines for draining, cancelling handlers, and stopping renewal. Ensure the host deadline cannot bypass cancellation of work owned by the processor.
Regression coverage: cancel the host shutdown token before the configured drain timeout; assert handler cancellation and cessation of renewal, including while the handler remains blocked.
F2 — High: malformed Base64 bypasses poison handling and fails the entire receive batch
Location:
src/AzureStorage.QueueService/StorageQueue.cs:105.The SDK decodes before the library can construct messages with
Error. With Base64 configured, an invalid encoded body throws beforeTryDeserializeruns.An Azurite probe containing one malformed message and one valid message produced
FormatException; neither reached the processor. Consequently, its deserialization and dequeue-limit policies cannot handle that batch. This defeats the intended per-message poison handling and can repeatedly hide healthy messages alongside the invalid one.The SDK exposes a separate message-decoding failure mechanism.
Recommendation: bring decoding failures into the same per-message failure policy, preserving message identity and settlement capability. Merely catching
FormatExceptionaround the batch receive is insufficient.Regression coverage: a real/emulated queue with Base64 enabled and a mixed valid/malformed batch; assert that the invalid message can be dead-lettered and the healthy message still reaches its handler.
F3 — High: finishing a handler can cancel a renewal after its receipt changed remotely
Location:
src/AzureStorage.QueueService/QueueProcessor.cs:314(StopRenewalAsync, also called before settlement).The code cancels both the renewal timer and any active renewal request. If Azure accepted the update but cancellation prevents its response from arriving, completion uses the old receipt.
A simulated accepted-update/pending-response probe confirmed cancellation followed by deletion using
r0while the simulated service heldr1. This probe demonstrates the local race; it is not a claim that a live Azure transport was forced into this exact timing window.Recommendation: stop scheduling renewals, await any active update, then settle using its receipt. Keep that operation bounded separately. General network ambiguity cannot be eliminated, but cancellation initiated by the processor should not unnecessarily create it.
Regression coverage: finish the handler while a renewal has been accepted remotely but its response is pending; ensure normal completion waits for the updated receipt rather than cancelling that request.
F4 — High: synchronous handler work delays other messages whose leases already started
Location:
src/AzureStorage.QueueService/QueueProcessor.cs:179; batch deserialization inStorageQueue.cs:113is related.ProcessAsyncis invoked directly and runs synchronously until an incomplete await. A synchronous handler, or a substantial synchronous prefix, therefore delays dispatching the remaining batch.With concurrency set to two, a one-second visibility timeout, and 1.2 seconds of synchronous work, the second handler started 1.203 seconds after the first. Its initial lease had already expired before its renewal loop started. Batch deserialization introduces another delay before any processing starts.
Recommendation: establish lease ownership immediately after receiving and schedule work independently of handler synchronous behavior. Address both dispatch and deserialization rather than adding a
Task.Runthat leaves the earlier lease gap intact.Regression coverage: multiple messages, a synchronous/blocking handler prefix longer than the initial visibility interval, and slow deserialization. Assert that every received delivery has active ownership/renewal before it can wait behind other work.
F5 — Medium: handler construction and scope failures disappear without logging or retry settlement
Location:
src/AzureStorage.QueueService/QueueProcessor.cs:245; task tracking at approximately line 204.Handler resolution falls outside the handler exception boundary. Scope disposal and a throwing
RetryDelaycallback can escape similarly.Tracksubsequently removes faulted tasks without observing their exceptions.A throwing-constructor probe produced an unsettled message, zero abandon calls, and zero logged exceptions. Repeated dependency failures can eventually send otherwise valid messages to poison.
Recommendation: put one exception boundary around the entire delivery lifecycle, including scope creation, resolution, invocation, and disposal. Explicitly observe all tracked task failures. Retry-policy failures also need a defined, logged fallback.
Regression coverage: handler constructor failure, async scope-disposal failure, and a throwing retry callback; assert logging, defined settlement/fallback behavior, and continued processing.
F6 — Medium: poison queue derivation breaks for valid queue configurations
Location:
src/AzureStorage.QueueService/QueueClientFactory.cs:88and:112.Appending
-poisonmakes valid source queue names longer than 56 characters generate invalid poison names. Azure permits at most 63 characters.Separately, relative URI resolution makes an accepted endpoint ending in
/orders/generate/orders/orders-poison. An SDK constructor probe confirmed that both/ordersand/orders/yield the queue nameorders, while the current derivation produces different destinations.Recommendation: support an explicit poison destination, validate it at startup, and derive sibling queues with structured URI handling. This also avoids making a naming convention an unavoidable restriction on valid source queues.
Regression coverage: source names of 56, 57, and 63 characters; endpoint URIs with and without a trailing slash; explicit poison destination validation.
F7 — Medium:
CreateIfNotExistsis inconsistent across operationsLocation:
src/AzureStorage.QueueService/StorageQueue.cs:197.GetPropertiesAsyncdoes not ensure creation. A first properties request can fail with queue-not-found despiteCreateIfNotExists = true, unlike send, receive, peek, and clear. This finding follows directly from the source; it was not a separate live-service probe.Recommendation: apply the creation policy consistently or narrow its documented contract.
Regression coverage: call properties first against a missing queue with automatic creation enabled.
F8 — Medium, inherited: receive/send span tags are discarded
Location:
src/AzureStorage.QueueService/Telemetry/ActivityExtensions.cs:7.TagListis a struct. Passing it throughAction<TagList>lets the callback modify a copy, leaving the original list empty.A listener probe confirmed that adding
queue.namein the callback leaves the resulting span without that tag. This predates the v3 implementation but remains relevant to the full-client review.Recommendation: construct and pass the populated tags directly, or use an API that returns the modified value.
Regression coverage: enable an activity listener and verify actual receive/send span tags, rather than only checking that an activity exists.
Over-engineering and simplification assessment
Fixing the individual bugs will leave much of the current complexity intact. The complexity is concentrated in overlapping ownership and mixed contracts, not simply the number of files or interfaces.
IsSettledloses whether the handler completed, abandoned, or dead-lettered. The processor reports all three assettled_by_handlerand successful processing.QueueMessageInfo,IQueueMessageOperations, and the public message constructor expose transport machinery partly to enable tests. The operations contract also exposes SDK receipts.IOptionsMonitorsuggests live configuration, while cached queues retain their original settings.Patterns that would earn their place
IQueueMessageSerializer. It represents real variation and is already a useful boundary.Avoid adding middleware pipelines, a generic processing framework, multiple retry-policy interfaces, or a class for every settlement state. Those would make this library harder to understand. The goal is fewer independent mechanisms and clearer ownership, not additional pattern names.
If changing the API shape is on the table
Simplify the managed-processing path further. Most handlers need a typed payload, read-only metadata, and cancellation. They currently receive nullable deserialization results, transport receipts, renewal methods, settlement methods, and automatic settlement behavior in the same object.
A cleaner managed handler could receive a successfully deserialized message and return a small disposition such as complete, retry, or dead-letter. The processor would own renewal and settlement throughout. Keep explicit message operations for advanced manual consumers if that usage warrants the separate API. This is an architectural alternative to assess, not an assertion that the manual consumption path should be removed without considering existing requirements.
Binding the message type at registration would also prevent accidentally attaching incompatible typed processors to the same queue. Today, the generic-per-operation API leaves that contract entirely to callers.
What should be preserved
The sender/receiver separation, per-message DI scopes,
TimeProvider, distinct peeked-message types, raw-body poison replay, and serialized receipt updates address real problems. Their value should survive the simplification.Also avoid promises that visibility renewal guarantees no duplicate processing. Treat delivery as at-least-once and describe renewal as reducing avoidable concurrent redelivery.
Validation and limitations
dotnet test AzureStorage.QueueService.sln --no-restore --verbosity minimal.DOTNET_ROLL_FORWARD=Majoragainst the installed .NET 10 runtime. This is useful evidence but not a substitute for the native supported-runtime check.ResponseBodyPipeWriterdid not implementPipeWriter.UnflushedBytes. Therefore this review does not claim an integration pass or identify that runtime-mismatch failure as a client regression.Recommended order: resolve F1-F5, simplify delivery ownership and registration, and settle the compatibility/API direction before publishing the preview. Address F6-F8 before considering the client ready. The implementing agent should treat the architectural recommendations as a design assessment and the numbered findings as concrete behavior to reproduce and correct.
jasonshave commentedon Oct 15, 2026
Follow-up v3 review: six remaining/new findings after the incorporated changes
The simplification is meaningful. Worker-per-slot processing, removal of the 2.x facade and its alternate serialization/cache path, explicit settlement outcomes, and startup snapshots improve the design. I would preserve those changes. There are still six concrete findings below, including two incompletely closed portions of the earlier review.
Scope: local
v3working tree on 2026-10-15, HEAD stillc4e51f9, including the updated uncommitted/untracked implementation. Locations refer to that working tree, not necessarily the committed branch contents. Assessment only: no repository source, tests, configuration, or documentation were changed by this review. Findings were checked with isolated scratch probes outside the repository.Previous review: #39 (comment)
What is now addressed
finally. The distinct drain-timeout issue below remains.Error; the mixed-batch Azurite integration test passes.R1 — High: every completed delivery retains a linked cancellation registration until processor shutdown
Locations:
src/AzureStorage.QueueService/QueueProcessor.cs:375and:382(LeaseRenewalconstruction andStopAsync).Each delivery creates
_stopWaitingwithCancellationTokenSource.CreateLinkedTokenSource(stopAll).StopAsynccancels it and awaits the loop, but never disposes the linked source. Cancelling a linked source does not unregister its callback from the parent source. The long-lived processor's_stopRenewalsconsequently retains one linked cancellation source per delivery until shutdown.Probe: processed 1,000 messages to completion, waited until the worker had returned to an empty receive, forced collection, and inspected the parent's active cancellation-registration list. It still contained 1,000 registrations. This is accumulation proportional to all messages processed, rather than current concurrency.
Recommendation: make the per-delivery renewal resource have one explicit end of lifetime that disposes its linked cancellation source and unregisters from the parent.
StopRenewalAsyncis currently called multiple times per delivery, so distinguish repeatable stop/join from final disposal or make the combined operation safely idempotent. Dispose processor-owned cancellation resources at the appropriate service lifetime boundary too.Regression: a long-lived processor repeatedly completes messages while its parent renewal token remains uncancelled; completed deliveries must not remain retained through cancellation registrations. Test disposal/collection or equivalent resource-lifetime evidence, without requiring production code to expose runtime-private registration internals.
R2 — Medium, earlier F4 partially open: deserialization can exhaust the visibility timeout before renewal starts
Locations:
src/AzureStorage.QueueService/StorageQueue.cs:122,src/AzureStorage.QueueService/QueueProcessor.cs:157, and:214.The worker still calls typed
ReceiveAsync, which deserializes before returning the message.LeaseRenewal.Startonly happens later inDeliverAsync. Worker-per-slot removes the batch-dispatch gap but does not protect the already received message while its serializer runs.Actual Azurite probe: used a valid one-second visibility timeout and a custom serializer that takes 1.8 seconds. A competing SDK consumer received the same message with dequeue count 2 before the first handler started. The first processor had not yet started renewal.
Recommendation: put deserialization inside the managed delivery's lease ownership boundary. An internal raw-receive/owned-delivery phase can do this without changing the manual typed receiver's public contract. Starting renewal after typed
ReceiveAsyncreturns cannot close this interval. Avoid documenting earlier F4 as completely resolved until this part is covered.Regression: a real/emulated queue, slow custom deserialization, and another consumer; assert that ownership is maintained during deserialization and that the second consumer cannot take the message during that protected interval.
R3 — Medium, distinct remaining shutdown gap: drain timeout cancels the handler but leaves its renewal running while waiting for the handler to exit
Locations:
src/AzureStorage.QueueService/QueueProcessor.cs:113-122.After
ShutdownDrainTimeoutexpires,_cancelHandlers.Cancel()is followed byawait all.WaitAsync(cancellationToken)._stopRenewals.Cancel()is only in the subsequentfinally. A handler that ignores cancellation therefore keeps its renewal running until the host token expires or that handler eventually returns. WithStopAsync(CancellationToken.None)this can last indefinitely.Probe: a handler deliberately ignored its token, the drain deadline elapsed, and its token was correctly cancelled.
StopAsyncremained pending and renewal continued (one further renewal in the next 600 ms with a one-second visibility setting).This is not a demand to forcibly terminate arbitrary application code. The processor can independently stop scheduling new renewals even when it cannot terminate the handler.
Recommendation: at the drain deadline, stop scheduling renewals as well as cancelling handlers. Preserve the earlier F3 fix by allowing an already submitted renewal request to finish under an explicit bounded policy. Define whether any further wait for uncooperative handlers is bounded by the host token or a separate timeout.
Regression: cancellation-ignoring handler, expired drain deadline, and an uncancelled host token; assert no newly scheduled renewal after the drain boundary. Then release the handler so the test can clean up.
R4 — Medium: accepted timing options can silently fault the worker and stop processing permanently
Locations:
src/AzureStorage.QueueService/QueueProcessorOptions.cs:63,QueueProcessor.cs:175, and:189.Validation checks positivity and ordering but does not check the timer API's supported range. An exception from
Task.Delayin the empty-queue branch is outside the receive exception boundary;DelayAsynccatches only cancellation. Workers are started as detached tasks, and the host does not observe their failure during normal operation.Probe: configured
EmptyQueueBackoffMin,EmptyQueueBackoffMax, andErrorBackoffMaxto 60 days.StartAsyncaccepted the options; the first empty receive caused the worker to fault withArgumentOutOfRangeException. No replacement worker was started. The fault was visible only by inspecting the worker task / eventually awaiting shutdown.Recommendation: validate configured timer durations against the actual supported range at startup, including drain/renewal timing where applicable. Ensure an unexpected worker fault is surfaced immediately rather than silently reducing the running worker count. This does not require a new supervision framework; startup validation plus a simple explicit task-failure observation boundary is sufficient.
Regression: oversized timer settings fail startup clearly; a worker failure cannot remain invisible until shutdown.
R5 — Medium: the poison destination can be the source queue, creating endless recycling with reset retry counts
Locations:
src/AzureStorage.QueueService/QueueClientFactory.cs:76-83,QueueClientSettings.cs:67, andStorageQueue.cs:259-261.PoisonQueueNameis checked for naming syntax, but not for being different from the source queue. If they are equal, dead-lettering successfully sends a new copy to the source and deletes the original. The next delivery has a fresh message ID and a dequeue count of 1, defeating both poison isolation and the dequeue limit.Actual Azurite probe: configured source and poison to the same valid name, received invalid JSON, and dead-lettered it. The source still contained the same bad body with a different ID and dequeue count 1.
Recommendation: validate that the effective source and poison queue identities differ, for both connection-string and endpoint registrations. Fail clearly before consuming messages.
Regression: source-equals-poison configuration is rejected for both connection styles; an explicit different poison name remains allowed.
R6 — Medium: ValidateOnStart does not validate the derived poison name
Locations:
src/AzureStorage.QueueService/QueueClientSettings.cs:67,QueueClientSettingsBuilder.cs:112, andQueueClientFactory.cs:76-80.The options validator checks poison naming only when
PoisonQueueNameis explicitly supplied. The default<source>-poisonis checked later, during queue construction. ConsequentlyValidateOnStartcan report successful startup for settings that the factory will deterministically reject on first use. A processor happens to resolve its queue at startup, but a send-only host does not.Probe: registered a valid 60-character source name with no explicit poison name and called
ValidateOnStart. The host started successfully; the firstGetQueuethrew because the derived poison name exceeded 63 characters.Recommendation: share effective-destination derivation/validation between startup validation and construction. Keep this as a small pure helper; there is no need for another validator/factory abstraction layer.
Regression: send-only hosts using
ValidateOnStartreject invalid derived names at startup, with and without trailing-slash endpoints; valid explicit short poison names pass.Over-engineering reassessment
Validation
Ran
dotnet test AzureStorage.QueueService.sln --no-restore --verbosity minimalusing the current .NET 10 targets and an existing local Azurite instance, with a unique default integration queue:git diff --checkreported trailing whitespace on the new10.0.xworkflow lines (dotnet.yml:30,nuget.yml:24); this is housekeeping, not an additional correctness finding.Recommendation: fix R1 and finish the delivery ownership/shutdown behavior in R2-R3, then close the validation holes in R4-R6. The overall direction is substantially better; another framework or wholesale public API redesign is not necessary to address these findings.
jasonshave commentedon Oct 15, 2026
Third v3 review: four verified remaining/new findings
The previous changes improve ownership and cleanup. The linked-registration leak and slow-deserialization lease gap are now fixed in repeated probes. This review found four concrete remaining/new problems. The main design lesson this round is to separate source-queue processing policy from construction of an SDK client for a destination; the current reuse of validation crosses that boundary.
Scope: updated local
v3working tree on 2026-10-15, HEAD stillc4e51f9, including uncommitted/untracked implementation changes. Source locations below refer to that working tree. Assessment only: no repository source, tests, configuration, or documentation were changed. Scratch probes ran outside the repository.Previous follow-up: #39 (comment)
S1 — High: constructing a valid poison destination validates an unnecessary poison-of-poison destination and fails
Locations:
src/AzureStorage.QueueService/QueueClientFactory.cs:81and:94;SdkQueueClientFactory.cs:9;QueueClientSettings.cs:80.QueueClientFactoryvalidates the primary source settings correctly, then builds settings for its poison SDK client withForSiblingQueue. Those settings leavePoisonQueueNameunset.SdkQueueClientFactory.CreateClientcalls the fullsettings.Validate()again, which now derives and validates<poison-name>-poisoneven though this SDK client is only a destination and will never use another poison destination.Consequently, default source queue names of 50-56 characters pass initial validation because their poison names are valid at 57-63 characters, but dead-lettering fails because the second validation invents a 64-70-character poison-of-poison name. Explicit poison names longer than 56 characters have the same problem.
Actual Azurite probe: registered a 50-character source, constructed/resolved the queue successfully, received invalid JSON, and called
DeadLetterAsync. It threwInvalidOperationExceptionfrom poison-client construction, complaining that<source>-poison-poisonwas 64 characters. The intended poison name was valid at 57 characters. The processor would catch this and repeatedly abandon the original instead of moving it to poison.Recommendation: validate source processing policy once at the source registration/construction boundary. SDK destination construction should validate its connection/endpoint and actual destination, without requiring a further poison destination. Keep shared name/connection helpers, but distinguish the responsibilities they validate.
Regression: exercise actual SDK poison-client construction for default source lengths 49, 50, and 56 and explicit poison lengths 57 and 63. Mocking
ISdkQueueClientFactorybypasses the failing second validation and would miss this regression.S2 — Medium, previous R5 partially open: endpoint registrations can bypass the source-equals-poison guard when QueueName is also populated
Locations:
src/AzureStorage.QueueService/QueueClientSettings.cs:56-59and:88;SdkQueueClientFactory.cs:14-17.EffectiveQueueNamealways prefersQueueNameif populated. SDK construction uses that field only for connection-string authentication; for endpoint authentication it usesEndpointUriand ignoresQueueName. Validation and actual transport identity therefore disagree.For example:
Validation compares
orderswithconfigured-aliasand accepts the settings, but both actual SDK clients targetorders. This reopens the self-poison loop from R5: dead-lettering copies back to the source with a fresh identity/dequeue count.Probe using real SDK constructors, without network access:
settings.Validate()accepted the configuration; the resolved source queue's actual name wasorders; invoking its poison client factory produced an SDK client also namedorders.Recommendation: resolve the effective source identity using exactly the same transport/authentication precedence as SDK construction. For endpoint authentication, use the queue from the endpoint, or explicitly reject conflicting redundant settings. Use that resolved identity for poison derivation and equality checks.
Regression: endpoint registration with a populated conflicting
QueueName; assert that source-equals-poison is rejected based on the actual endpoint target. Also cover both fields agreeing and connection-string precedence.S3 — Medium, previous R4 partially open: sub-second visibility passes startup validation and becomes a permanently failing receive
Location:
src/AzureStorage.QueueService/QueueProcessorOptions.cs:75.The validation error text says visibility must be between one second and seven days, but the actual lower-bound check is only
<= TimeSpan.Zero. A positive 500 ms value passes. The SDK sends whole seconds, so this becomesvisibilitytimeout=0, which the receive operation rejects. The processor keeps backing off and retrying rather than failing the bad configuration at startup.Actual Azurite probe:
new QueueProcessorOptions { VisibilityTimeout = TimeSpan.FromMilliseconds(500) }.Validate(...)succeeded. Receiving with the same SDK duration returned HTTP 400OutOfRangeQueryParameterValue, explicitly identifyingvisibilitytimeout=0and a minimum allowed value of 1.Recommendation: enforce the stated one-second minimum for processor receive visibility and define/document any normalization of fractional seconds. Keep abandon's intentional zero-delay semantics separate from receive/renewal validation.
Regression: reject positive sub-second processor visibility at startup; accept one second and the maximum boundary. The oversized-duration tests added for R4 do not cover this lower boundary.
S4 — Medium: failed host startup plus Dispose leaves already-started workers polling
Locations:
src/AzureStorage.QueueService/QueueProcessor.cs:84-91and:137-141.Workers start in the processor's
IHostedLifecycleService.StartedAsync. Other lifecycle services can still fail their ownStartedAsyncafterward; this callback is not a guarantee that all host startup callbacks completed. If startup fails, disposing the host disposes the processor. Its newDisposemethod only disposes cancellation sources; it never cancels them. A disposed, uncancelled source does not signal cancellation to tokens the workers already captured.Actual Generic Host probe with a scripted queue: registered the processor followed by another lifecycle service whose
StartedAsyncwaited for the first receive and then threw.host.StartAsync()failed. Afterhost.Dispose(), the worker performed three additional receives in 100 ms. This can leave orphan queue activity and processing attempts against a disposed service provider.Recommendation: cancellation must precede disposal of processor-owned sources, including when normal
StopAsyncwas never reached. Ensure any workers launched during partial startup are signalled to stop. If the documented requirement is strictly to begin after successful application startup, use the actual application-start signal rather than interpreting an individualStartedAsynccallback as that signal.Regression: a later lifecycle service fails startup after a processor worker begins; dispose the host without first calling its successful-stop path and assert that queue polling ceases. Test the lifecycle through a real Generic Host, not only direct calls to processor methods.
Status of the preceding six findings
Over-engineering assessment
The current changes do not justify another broad rewrite.
LeaseRenewalnow has a clear disposable lifetime, and the small internalIOwnedReceiverseam addresses a real difference between managed processing and manual typed consumption. Keep the worker-per-slot design and the single delivery routine.The most useful further simplification is validation by responsibility:
These can be small internal helpers. Another hierarchy of configuration, resolver, and validator interfaces would add machinery without solving a new requirement. The regressions in S1 and S2 come from inconsistent scope/identity in reused logic, not a shortage of abstractions.
Also keep the lifecycle fix direct: owned worker cancellation followed by resource disposal, with a real-host failure-path test. A new supervision framework is unnecessary.
Validation
Ran
dotnet test AzureStorage.QueueService.sln --no-restore --verbosity minimalon the current .NET 10 targets against existing local Azurite, using a unique default integration queue:git diff --checkreported no whitespace errors (only Git's line-ending conversion notices).Recommendation: fix the destination-validation regression S1 and identity mismatch S2, then close the timing and partial-start lifecycle gaps in S3-S4. Preserve the simplifications already made.
jasonshave commentedon Oct 15, 2026
Fourth v3 review: previous four findings resolved; two remaining medium-severity edge cases
The source/destination validation split, transport identity checks, visibility minimum, and host lifecycle changes address all four findings from the preceding review. I repeated their failure scenarios and confirmed the changed behavior. The overall architecture is substantially clearer; the remaining work below is targeted error handling and SDK-options isolation, not another broad refactor.
Scope: current local
v3working tree on 2026-10-15, HEAD stillc4e51f9, including uncommitted/untracked implementation. Source coordinates refer to that working tree. Assessment only: no repository source, tests, configuration, or documentation were modified.Previous review: #39 (comment)
T1 — Medium: a real renewal failure throwing InvalidOperationException is mistaken for successful settlement and silently stops renewal
Location:
src/AzureStorage.QueueService/LeaseRenewal.cs:64.The unconditional
catch (InvalidOperationException)assumes the handler settled the message between the settled check and renewal. It does not verifyisSettled(). An SDK pipeline policy or transport extension can throw the same exception while the message remains unsettled. In that case the renewal loop exits without callingonFailed, so the normal renewal-failed log is suppressed and the handler continues with a lease that will expire.Probe with actual SDK/Azurite: received a message normally and started
LeaseRenewalover its realRenewLockAsync. Configured a custom SDK HTTP pipeline policy to throwInvalidOperationExceptionfor the visibility-update PUT. The policy was invoked, the message remained unsettled, and the renewal-failure callback count was 0. This was not merely a stub throwing directly from a mocked renewal operation.Recommendation: suppress this exception only when the message is actually settled, for example with a settlement-aware catch filter. Other InvalidOperationException failures should reach the ordinary renewal-failure reporting path. No new exception hierarchy or policy abstraction is needed.
Regression: an unsettled message whose actual renewal path throws InvalidOperationException reports one failure; the legitimate renew-versus-settle race remains harmless and unreported.
T2 — Medium: mutating caller-supplied SDK options leaks the decoding policy into other SDK clients and silently omits malformed messages
Locations:
src/AzureStorage.QueueService/SdkQueueClientFactory.cs:14;DecodingFailures.cs:31and:48.SDK client creation adds the internal decoding-failure handler directly to the caller's
QueueClientOptions. If the caller reuses those options to construct another QueueClient outside this library, that SDK client also captures the internal handler. Outside a wrapped receive,DecodingFailures.Currentis null, so the handler returns successfully without collecting or reporting the message. The SDK interprets that as a handled decoding failure and omits it from the returned batch.Actual SDK/Azurite probe: created a Base64 SDK client with shared options before the library attached its handler, then created another SDK client with the same options after the library built a client. Both queues contained the same invalid Base64 text. The first SDK client threw FormatException; the second returned an empty batch with no exception. The message is not deleted, but it is hidden for the visibility interval and the external caller receives neither the message nor an error to handle.
Recommendation: keep the internal collector's behavior scoped to wrapped receives. When there is no active internal collector, preserve an appropriate decoding-failure behavior rather than silently accepting the failure. Alternatively isolate the SDK options owned by the library so caller reuse cannot acquire hidden callback behavior. Keep the solution small; a new global dispatcher or ambient-state framework would increase the problem's complexity.
Regression: share one Base64 QueueClientOptions instance between the wrapper and direct SDK clients created before and after wrapper construction. Verify that malformed messages are not silently suppressed outside an active collector and that concurrent wrapped receives still isolate their failures.
Previous findings verified closed this round
Design / over-engineering reflection
The useful simplifications have held: worker-per-slot scheduling, one managed delivery routine, a disposable renewal lifetime, removal of the legacy facade path, explicit settlement outcomes, startup snapshots, and source-policy validation separated from destination connection construction. I do not see a reason in this pass to replace those with another general processing framework.
The decoding collector remains the most intricate bridge because it combines a global AsyncLocal scope, mutation of SDK options, and event-handler capture. T2 is a concrete consequence of that complexity. Address its behavior outside the wrapper without expanding the mechanism. T1 likewise needs a precise exception boundary rather than another abstraction.
Validation
Ran
dotnet test AzureStorage.QueueService.sln --no-restore --verbosity minimalon .NET 10 with local Azurite and a unique default integration queue:git diff --checkfound no whitespace errors; only line-ending conversion notices.Assessment: the earlier broad correctness/ownership problems have been addressed. Close T1 and T2 with focused regression coverage; further wholesale restructuring is not warranted by the evidence from this pass.
jasonshave commentedon Oct 15, 2026
T1/T2 fixed locally, followed by another review
The owner authorized local implementation of the two findings in the fourth review, then requested another review. Both fixes are applied to the current working tree; these changes have not been committed or pushed.
LeaseRenewalsuppressesInvalidOperationExceptiononly whenisSettled()is true. Other failures reach the normal failure callback/log path. Regression coverage checks both a genuine renewal failure and the legitimate settlement race.finally. Library client construction using the same options is serialized. The caller's options are restored for subsequent SDK clients, and the old global attachment-tracking table is removed. An internal callback with no active collector fails explicitly rather than silently omitting a message.Follow-up review of the initial fix found that simply throwing outside the collector could interfere with an existing caller decoding handler. The final implementation restores the options instead. Actual SDK/Azurite integration coverage verifies receive and peek behavior, both with default SDK errors and with caller-provided decoding handlers. Existing wrapped mixed-batch handling and concurrent collector-isolation tests continue to pass.
Validation:
dotnet test AzureStorage.QueueService.sln --no-restore --verbosity minimal: 98 unit tests and 7 integration tests passed, 0 failed, with local Azurite on .NET 10.git diff --check: no whitespace errors.InvalidOperationExceptionnow reports 1 renewal failure; external SDK clients created before and after library construction both preserve decoding errors.Follow-up assessment: no additional material issues found in the changed paths. The fixes remain focused and remove attachment bookkeeping; no further broad restructuring is warranted by this pass.
jasonshave commentedon Oct 16, 2026
Pre-release review: "never expire" sends fail
Status: found during the pre-release review of
v3; not fixed yet.What's broken: the docs on
SendOptions.TimeToLive(IQueueSender.cs) say to passTimeout.InfiniteTimeSpanfor a message that never expires. Doing that makes every send fail withRequestFailedException: Value for one of the query parameters specified in the request URI is invalid(HTTP 400).Why: the service's "never expire" value is exactly −1 second.
Timeout.InfiniteTimeSpanis −1 millisecond, and the SDK truncates it to whole seconds, so it sendsmessagettl=0, which the service rejects.Verified against Azurite with
Azure.Storage.Queues12.28.1:timeToLivepassedTimeout.InfiniteTimeSpanTimeSpan.FromSeconds(-1)Planned fix: in
StorageQueue.SendAsync(StorageQueue.cs:63), mapTimeout.InfiniteTimeSpantoTimeSpan.FromSeconds(-1)before calling the SDK, so the documented value works, and add a unit test that checks the SDK receives −1 second.jasonshave commentedon Oct 16, 2026
Never-expire TTL finding verified and fixed locally
The latest pre-release finding is valid. Before changing production code, the new Azurite regression reproduced HTTP 400
InvalidQueryParameterValue, withmessagettl=0and a service response requiring a positive number or -1 for infinite TTL. The unit regression likewise showed the SDK receiving -1 ms rather than -1 second.StorageQueue.SendAsyncnow maps exactlyTimeout.InfiniteTimeSpantoTimeSpan.FromSeconds(-1)before calling the SDK. Other TTL values are forwarded unchanged, and the caller's SendOptions are not modified. This makes the existing public documentation accurate without adding an API or abstraction.Regression coverage:
Timeout.InfiniteTimeSpan, assert the receipt's expiration year is 9999, and verify the stored message has the same expiration. The test uses a unique queue and deletes it afterward.Full local validation:
dotnet test AzureStorage.QueueService.sln --no-restore --verbosity minimalpassed with 102 unit tests and 8 integration tests, 0 failures. The solution built successfully, andgit diff --checkreported no whitespace errors.Self-review: checked that normalization applies only to message TTL and only to the .NET infinite sentinel; null/default, finite, and explicit -1-second values retain their behavior. No additional material issues found in the TTL change. Changes remain local and uncommitted.
3.0: sender/receiver split, per-client builder, hosted processor
3.0: sender/receiver split, per-client builder, hosted processor (#42)
jasonshave commentedon Oct 16, 2026
Shipped in 3.0.0
Release v3.0.0 via #42, published to nuget.org as
AzureStorage.QueueService3.0.0.Delivered: items 1–3, the separate sender, receiver and processor, the hosted processor, and the per-client builder. Also the per-client serializer from item 5, with Base64 handled by the SDK, and the 2.x → 3.0 upgrade guide in the README. The 2.x API was removed rather than kept as an
[Obsolete]façade; the design doc has the reasoning. .NET 8 users stay on the maintained2.xline.Fixed before release: the "never expire" send failure from my earlier comment (
Timeout.InfiniteTimeSpannow maps to the −1 second the service expects), plus the shutdown, startup-validation and poison-queue fixes listed in #42.Carried forward as separate issues:
Microsoft.Extensions.Azure/ credentials from configuration (item 4, still undecided)