Skip to content

v3.0: redesign around sender / receiver / hosted processor (IHttpClientFactory-style consumption) #39

Closed

Description

@jasonshave

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>-poison after 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 IMeterFactory so meters are DI-owned and testable with MetricCollector<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 / Consumer span 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 AzureStorageQueueClient as 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 injects IQueueSender / IQueueReceiver or registers a processor.
  • ReceiveMessagesAsync(handle, handleException) → AddProcessor<T, THandler>().
  • IMessageConverter → IQueueMessageSerializer.
  • Base64 queues → set ClientOptions.MessageEncoding = Base64.

Activity

  1. jasonshave commented on Oct 15, 2026

    @jasonshave
    OwnerAuthor

    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 v3 working tree on 2026-10-15, HEAD c4e51f9, including the substantial uncommitted/untracked implementation refactor, compared with the 2.1 baseline f760477. 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 and WaitAllAsync paths).

    Cancellation of the host's shutdown token makes WaitAllAsync throw before _cancelHandlers.Cancel() runs. Renewal has its own cancellation source and continues too.

    An isolated probe confirmed that, after StopAsync exited through cancellation, the handler token remained uncancelled and three additional renewals occurred. This is especially relevant when the host deadline and ShutdownDrainTimeout coincide.

    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 before TryDeserialize runs.

    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 FormatException around 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 r0 while the simulated service held r1. 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 in StorageQueue.cs:113 is related.

    ProcessAsync is 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.Run that 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 RetryDelay callback can escape similarly. Track subsequently 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:88 and :112.

    Appending -poison makes 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 /orders and /orders/ yield the queue name orders, 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: CreateIfNotExists is inconsistent across operations

    Location: src/AzureStorage.QueueService/StorageQueue.cs:197.

    GetPropertiesAsync does not ensure creation. A first properties request can fail with queue-not-found despite CreateIfNotExists = 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.

    TagList is a struct. Passing it through Action<TagList> lets the callback modify a copy, leaving the original list empty.

    A listener probe confirmed that adding queue.name in 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.

    Area Assessment Recommended simplification
    Registration and caching Named options, a registry, keyed singletons, a queue cache, and a facade cache maintain overlapping identity and lifetime information. Let keyed DI own queue lifetimes. Make the public factory a thin lookup adapter. Retain only registration metadata needed for validation and diagnostics.
    Processor orchestration One processor combines polling, dispatch, task bookkeeping, scopes, renewal, retries, settlement, shutdown, and telemetry. Their interactions caused several findings above. Give one delivery routine ownership of the complete message lifecycle. Keep polling and concurrency scheduling outside it.
    Settlement state IsSettled loses whether the handler completed, abandoned, or dead-lettered. The processor reports all three as settled_by_handler and successful processing. Store an explicit settlement outcome. A small enum is sufficient; a hierarchy of State-pattern classes would add unnecessary machinery.
    Compatibility Legacy serialization forces serializer overrides into the new transport implementation and adds a second factory/cache path. Contain legacy behavior in a boundary adapter. If a clean breaking v3 is acceptable, keeping customers on the maintained 2.x branch is considerably simpler. That alternative changes this issue's existing compatibility commitment and requires a deliberate scope decision.
    Public test plumbing QueueMessageInfo, IQueueMessageOperations, and the public message constructor expose transport machinery partly to enable tests. The operations contract also exposes SDK receipts. Reconsider the consumer-facing delivery contract before freezing it. Consumers should fake the abstraction they consume without constructing transport internals.
    Configuration lifetime IOptionsMonitor suggests live configuration, while cached queues retain their original settings. Adopt an explicit startup snapshot unless runtime reconfiguration is a real requirement.

    Patterns that would earn their place

    • Strategy: keep IQueueMessageSerializer. It represents real variation and is already a useful boundary.
    • Adapter: isolate the 2.x facade, with compatibility behavior tested independently.
    • One delivery routine with a fixed lifecycle: acquire ownership -> deserialize -> create scope -> invoke -> settle -> release resources. This routine should contain the complete failure boundary.
    • Explicit state: keep receipt, renewal activity, and settlement outcome together under one owner.
    • A bounded worker pool: consider workers that each receive and process one message. This removes much of the semaphore/task-set coordination and avoids batching messages that cannot start. The tradeoff is more receive requests. Retain batching only if its throughput or cost benefit justifies the added scheduling complexity.

    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

    • The solution built through dotnet test AzureStorage.QueueService.sln --no-restore --verbosity minimal.
    • Native .NET 8 test execution was blocked: only .NET 10 runtimes were installed.
    • All 70 unit tests passed using DOTNET_ROLL_FORWARD=Major against the installed .NET 10 runtime. This is useful evidence but not a substitute for the native supported-runtime check.
    • The integration test reached Azurite and received a message, but failed in ASP.NET response serialization under roll-forward: ResponseBodyPipeWriter did not implement PipeWriter.UnflushedBytes. Therefore this review does not claim an integration pass or identify that runtime-mismatch failure as a client regression.
    • Isolated probes outside the repository exercised host-deadline cancellation, synchronous dispatch, handler-construction failure, simulated renewal cancellation, actual malformed-Base64 receive behavior against Azurite, SDK endpoint parsing/poison URI derivation, and actual activity tags.
    • No repository source, tests, documentation, or configuration were modified. The review does not replace the owner's requirement to run the full affected local checks before shipping changes.

    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.

  2. jasonshave commented on Oct 15, 2026

    @jasonshave
    OwnerAuthor

    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 v3 working tree on 2026-10-15, HEAD still c4e51f9, 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

    • Earlier F1: the host-deadline cancellation path now reaches unconditional cancellation in finally. The distinct drain-timeout issue below remains.
    • Earlier F2: malformed Base64 is returned with identity and Error; the mixed-batch Azurite integration test passes.
    • Earlier F3: normal handler completion awaits an active renewal rather than cancelling its request and unnecessarily losing the returned receipt.
    • Earlier F4: synchronous handler prefixes no longer prevent other workers from starting. Slow deserialization remains outside renewal protection; see R2.
    • Earlier F5: handler construction/scope disposal are within the delivery boundary, and a throwing retry callback has a fallback.
    • Earlier F6: explicit poison names and trailing-slash sibling URI construction address the original derivation issues. Startup validation still misses an invalid derived default; see R6.
    • Earlier F7/F8: properties now ensure creation, and populated span tags are passed directly.

    R1 — High: every completed delivery retains a linked cancellation registration until processor shutdown

    Locations: src/AzureStorage.QueueService/QueueProcessor.cs:375 and :382 (LeaseRenewal construction and StopAsync).

    Each delivery creates _stopWaiting with CancellationTokenSource.CreateLinkedTokenSource(stopAll). StopAsync cancels 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 _stopRenewals consequently 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. StopRenewalAsync is 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.Start only happens later in DeliverAsync. 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 ReceiveAsync returns 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 ShutdownDrainTimeout expires, _cancelHandlers.Cancel() is followed by await all.WaitAsync(cancellationToken). _stopRenewals.Cancel() is only in the subsequent finally. A handler that ignores cancellation therefore keeps its renewal running until the host token expires or that handler eventually returns. With StopAsync(CancellationToken.None) this can last indefinitely.

    Probe: a handler deliberately ignored its token, the drain deadline elapsed, and its token was correctly cancelled. StopAsync remained 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.Delay in the empty-queue branch is outside the receive exception boundary; DelayAsync catches only cancellation. Workers are started as detached tasks, and the host does not observe their failure during normal operation.

    Probe: configured EmptyQueueBackoffMin, EmptyQueueBackoffMax, and ErrorBackoffMax to 60 days. StartAsync accepted the options; the first empty receive caused the worker to fault with ArgumentOutOfRangeException. 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, and StorageQueue.cs:259-261.

    PoisonQueueName is 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, and QueueClientFactory.cs:76-80.

    The options validator checks poison naming only when PoisonQueueName is explicitly supplied. The default <source>-poison is checked later, during queue construction. Consequently ValidateOnStart can 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 first GetQueue threw 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 ValidateOnStart reject invalid derived names at startup, with and without trailing-slash endpoints; valid explicit short poison names pass.

    Over-engineering reassessment

    • Keep the worker-per-slot design. It eliminates the previous semaphore, in-flight task set, and batch-dispatch coordination. The additional idle receive requests are now documented. Reintroducing a batching scheduler without measured need would undo a useful simplification.
    • Keep the facade removal and startup snapshot decision. They removed genuine alternate code paths and conflicting lifetime expectations. The remaining work should not restore them indirectly.
    • Finish one delivery lifetime boundary. R1-R3 show that renewal, decoding/deserialization, drain, and cleanup still have different effective boundaries. The useful simplification is clear resource ownership from raw receipt through settlement and cleanup, not more middleware, state classes, or policy interfaces.
    • Keep the explicit settlement enum and SDK-independent visibility receipt. They improve the contract without a framework-sized abstraction.
    • Factory + keyed DI still overlap, but I would treat that as optional simplification rather than require another broad rewrite. If the factory remains the deliberate canonical owner, document that and keep DI as aliases. Avoid adding a third lifetime owner. The previous facade cache is already gone.
    • The AsyncLocal decoding collector is the most intricate new bridge. The real mixed-batch test is valuable. Add evidence for concurrent receive/peek isolation and reused SDK options if retaining it. Avoid expanding ambient state or additional callback plumbing to solve the delivery-lifetime issues above.
    • The public testing constructor/operations contract remains an API tradeoff, not a newly proven defect. I would not block this round solely to replace it with the earlier disposition-returning alternative. Resolve the demonstrated ownership and validation problems first.

    Validation

    Ran dotnet test AzureStorage.QueueService.sln --no-restore --verbosity minimal using the current .NET 10 targets and an existing local Azurite instance, with a unique default integration queue:

    • Solution build: passed.
    • Unit tests: 65 passed, 0 failed.
    • Integration tests: 2 passed, 0 failed, including the real malformed-Base64 mixed batch.
    • No runtime roll-forward workaround was needed this round.
    • Isolated probes confirmed all six findings above. R1 used runtime-private reflection only as review evidence; production changes/tests need not depend on runtime internals. R2 and R5 exercised actual Azurite behavior.
    • git diff --check reported trailing whitespace on the new 10.0.x workflow 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.

  3. jasonshave commented on Oct 15, 2026

    @jasonshave
    OwnerAuthor

    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 v3 working tree on 2026-10-15, HEAD still c4e51f9, 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:81 and :94; SdkQueueClientFactory.cs:9; QueueClientSettings.cs:80.

    QueueClientFactory validates the primary source settings correctly, then builds settings for its poison SDK client with ForSiblingQueue. Those settings leave PoisonQueueName unset. SdkQueueClientFactory.CreateClient calls the full settings.Validate() again, which now derives and validates <poison-name>-poison even 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 threw InvalidOperationException from poison-client construction, complaining that <source>-poison-poison was 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 ISdkQueueClientFactory bypasses 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-59 and :88; SdkQueueClientFactory.cs:14-17.

    EffectiveQueueName always prefers QueueName if populated. SDK construction uses that field only for connection-string authentication; for endpoint authentication it uses EndpointUri and ignores QueueName. Validation and actual transport identity therefore disagree.

    For example:

    settings.QueueName = "configured-alias";
    settings.EndpointUri = new Uri("https://example.queue.core.windows.net/orders");
    settings.TokenCredential = credential;
    settings.PoisonQueueName = "orders";

    Validation compares orders with configured-alias and accepts the settings, but both actual SDK clients target orders. 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 was orders; invoking its poison client factory produced an SDK client also named orders.

    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 becomes visibilitytimeout=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 400 OutOfRangeQueryParameterValue, explicitly identifying visibilitytimeout=0 and 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-91 and :137-141.

    Workers start in the processor's IHostedLifecycleService.StartedAsync. Other lifecycle services can still fail their own StartedAsync afterward; this callback is not a guarantee that all host startup callbacks completed. If startup fails, disposing the host disposes the processor. Its new Dispose method 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 StartedAsync waited for the first receive and then threw. host.StartAsync() failed. After host.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 StopAsync was 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 individual StartedAsync callback 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

    • R1 fixed: repeated the 1,000-completed-delivery probe. The processor-wide token retained 0 active child registrations afterward, compared with 1,000 before the fix. The disposal ownership is now explicit.
    • R2 fixed: repeated the real Azurite probe with a 1.8-second serializer and a one-second visibility timeout. The competing consumer no longer received the message before the first handler; renewal now protects deserialization.
    • R3 addressed: the drain deadline now cancels handler and renewal scheduling together, and the additional wait is bounded. The added regression passes.
    • R4 oversized-duration handling addressed; lower-bound gap remains as S3. Unexpected worker failure now has an explicit log/restart path.
    • R5 ordinary same-name configuration addressed; effective-endpoint identity bypass remains as S2.
    • R6 initial source derived-name validation addressed; repeated policy validation of the destination introduces S1.

    Over-engineering assessment

    The current changes do not justify another broad rewrite. LeaseRenewal now has a clear disposable lifetime, and the small internal IOwnedReceiver seam 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:

    1. Resolve the actual source transport identity once, following the same precedence as SDK construction.
    2. Validate source processing policy, including its poison destination, against that identity.
    3. Construct destination SDK clients without reapplying source processing policy to them.

    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 minimal on the current .NET 10 targets against existing local Azurite, using a unique default integration queue:

    • Solution build passed.
    • 78 unit tests passed, 0 failed.
    • 2 integration tests passed, 0 failed.
    • git diff --check reported no whitespace errors (only Git's line-ending conversion notices).
    • All four findings above were reproduced in isolated scratch probes. S1/S3 used actual Azurite; S2 used real SDK construction and inspected the resulting queue identities; S4 used a real Generic Host with a scripted queue.
    • Repeated the prior leak and slow-deserialization probes to verify those fixes rather than reopening them speculatively.
    • No repository files were modified by this assessment.

    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.

  4. jasonshave commented on Oct 15, 2026

    @jasonshave
    OwnerAuthor

    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 v3 working tree on 2026-10-15, HEAD still c4e51f9, 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 verify isSettled(). 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 calling onFailed, 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 LeaseRenewal over its real RenewLockAsync. Configured a custom SDK HTTP pipeline policy to throw InvalidOperationException for 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:31 and :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.Current is 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

    • S1: a real 50-character source queue now successfully dead-letters to its valid long poison destination. The full integration suite also verifies the 56-character source / 63-character poison boundary.
    • S2: the conflicting endpoint-plus-QueueName configuration is rejected; effective identity follows the selected transport.
    • S3: a 500 ms processor visibility value is rejected during validation.
    • S4: repeated partial-start failure through a real Generic Host. A later lifecycle callback threw, the host was disposed, and there were 0 subsequent receives. Worker launch now waits for ApplicationStarted, and disposal signals cancellation.

    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 minimal on .NET 10 with local Azurite and a unique default integration queue:

    • Build passed.
    • 96 unit tests passed, 0 failed.
    • 3 integration tests passed, 0 failed.
    • git diff --check found no whitespace errors; only line-ending conversion notices.
    • Isolated scratch probes confirmed both T1/T2 and the closure of S1-S4.
    • No repository files were changed by this review.

    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.

  5. jasonshave commented on Oct 15, 2026

    @jasonshave
    OwnerAuthor

    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.

    • T1: LeaseRenewal suppresses InvalidOperationException only when isSettled() is true. Other failures reach the normal failure callback/log path. Regression coverage checks both a genuine renewal failure and the legitimate settlement race.
    • T2: the internal Base64 handler is subscribed only while the SDK constructs and captures its configuration, then removed in 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:

    • New regressions were run against the old behavior first and failed as expected; the genuine settlement-race control passed.
    • Final full 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.
    • Repeated the original real-SDK probes: an unsettled message with a pipeline InvalidOperationException now reports 1 renewal failure; external SDK clients created before and after library construction both preserve decoding errors.
    • The preceding long poison destination, endpoint identity, visibility minimum, and partial-start cleanup probes continue to pass.

    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.

  6. jasonshave commented on Oct 16, 2026

    @jasonshave
    OwnerAuthor

    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 pass Timeout.InfiniteTimeSpan for a message that never expires. Doing that makes every send fail with RequestFailedException: 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.InfiniteTimeSpan is −1 millisecond, and the SDK truncates it to whole seconds, so it sends messagettl=0, which the service rejects.

    Verified against Azurite with Azure.Storage.Queues 12.28.1:

    timeToLive passed Result
    Timeout.InfiniteTimeSpan 400, query parameter invalid
    TimeSpan.FromSeconds(-1) OK, expires 9999-12-31

    Planned fix: in StorageQueue.SendAsync (StorageQueue.cs:63), map Timeout.InfiniteTimeSpan to TimeSpan.FromSeconds(-1) before calling the SDK, so the documented value works, and add a unit test that checks the SDK receives −1 second.

  7. jasonshave commented on Oct 16, 2026

    @jasonshave
    OwnerAuthor

    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, with messagettl=0 and 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.SendAsync now maps exactly Timeout.InfiniteTimeSpan to TimeSpan.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:

    • Unit cases: omitted/default TTL, .NET's -1 ms sentinel, the SDK's explicit -1 second sentinel, and a finite one-hour TTL.
    • Actual Azurite integration: send using the documented 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 minimal passed with 102 unit tests and 8 integration tests, 0 failures. The solution built successfully, and git diff --check reported 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.

  8. jasonshave commented on Oct 16, 2026

    @jasonshave
    OwnerAuthor

    Shipped in 3.0.0

    Release v3.0.0 via #42, published to nuget.org as AzureStorage.QueueService 3.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 maintained 2.x line.

    Fixed before release: the "never expire" send failure from my earlier comment (Timeout.InfiniteTimeSpan now 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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions