Repository navigation
Conversation
Member
|
resolved in #10363 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a saga consume fails and is retried in process, every retry throws
ObjectDisposedExceptionfrom a disposedDbContextinDbContextSagaStore.StartTransactionAsync, before the saga runs — whenever any earlier saga consume has already run in the process. The retry cannot succeed, the in-process budget is exhausted in microseconds, and the message waits for delayed redelivery. Nothing is logged for a handler fault, so the only symptom is a saga that stops progressing.This is the residue of #10327. That change gives each retry its own scope so saga-state contention can resolve, but the retry that is supposed to resolve it is the thing that faults. Retry policy is irrelevant: 25 attempts behave exactly like 3, because every attempt after the first is dead on arrival.
Live stack, captured in a first-chance handler against Postgres:
Root cause
Four pieces, each individually intended, that combine into a shared mutable feature holding a scope-bound service.
1. Attempt clones read the receive context's features through a fallback.
ConsumerRetryMiddleware.ExecuteAttemptAsyncrents a pooledReceiveContextper attempt, andReceiveContext.CopyToinstalls the outer context's collection as the clone's_defaults(clone._features.Initialize(_features)).PooledFeatureCollection.TryGetchecks the clone's own entries and then falls through, soGetOrSet<T>()on the clone returns the outer context's instance whenever the outer has one. This is what lets the parsed message be shared across attempts.2.
SagaConsumermutates whatever the fallback returns, with the attempt's scoped store.context.Servicesis the attempt's scope. IfGetOrSetreturned the outer'sSagaFeature, the attempt'sDbContextSagaStore— and its scopedDbContext— is now pinned on an object that outlives the attempt.3. The outer context has a
SagaFeaturewhenever its pooled instance previously served as a saga attempt clone.SagaFeature : IPooledFeature, so when a clone is returned,PooledFeatureCollection.Reset()parks it (Storenulled). When that instance is later rented as an outer receive context,ReceiveContext.Initialize()calls_features.Initialize()with no defaults, which restores every parked feature. ASagaFeaturewithStore == nullnow sits exactly where the next clone's fallback will find it.4. Attempt 1 fails and its scope is disposed; attempt 2 reads the same feature.
??=is a no-op becauseStoreis set — to the store from the disposed scope.#10327's description anticipated this: "
SagaConsumercaches the saga store onSagaFeature; without a reset a retry would reuse a store from the disposed scope.IScopeBoundFeaturemarks features that hold a scoped service;SagaFeatureis the one implementor and is reset before and after the attempt." The merged revision has neitherIScopeBoundFeaturenor that reset. Both went when the design moved from swappingcontext.Serviceson one context to running each attempt on a pooled clone — and the clone's fallback to the receive context is what makes the cached store reachable again.Why the existing guard did not catch it
SagaRetryScopeTests.Saga_Should_UseFreshStore_When_SaveIsRetriedpublishes one saga message on a fresh pool. No pooled instance has ever been a saga clone, the outer has noSagaFeature, each attempt creates its own on the clone, and the test passes without exercising the fallback. The same blind spot produced a false "requires concurrency" conclusion downstream: concurrency only makes the precondition in (3) near-certain and supplies the first failure via version conflicts. A single-threaded reproduction with one warm-up saga consume produced 294 disposals.Fix
SagaFeaturestops being pooled, andSagaConsumercreates it on the attempt instead of reading through the fallback.Saga.csreads the store throughGetSagaFeature()→GetOrSet<SagaFeature>(), which finds the attempt's own entry first, so no reader changes. WithIPooledFeaturegone the feature is dropped onReset()rather than parked, so a receive context can never carry one and the fallback can never surface one — the hazard is removed rather than patched around.The alternative of keeping the pooled feature and making the assignment unconditional (
Store =) also passes the new test, but leaves a shared, mutable, scope-holding feature in the fallback path. #10327 kept??=because "several test harnesses inject a store through the feature and rely on the cache"; all five of those sites (SagaTester,SagaExecutionTests,SagaStateMachineTests,SagaSchedulingTests,SagaTimeoutTests) callsaga.HandleEventdirectly and never go throughSagaConsumer, so that constraint does not apply here.SagaFeatureis the onlyIPooledFeaturein the repository that holds a scope-resolved service; the others carry flags, the consumer set or the parsed message, all meant to be shared across attempts.Behavior change
SagaFeatureno longer implementsIPooledFeature; itsInitialize/Resetmembers are removed. Public type, no in-repo callers outside the collection machinery.SagaConsumerresolvesISagaStorefrom the current attempt's scope on every consume. ASagaFeature.Storepre-seeded on the consume context is no longer honoured bySagaConsumer— only bySaga.HandleEventcalled directly, which is how every existing harness does it.Tests
SagaRetryScopeWarmupTestsisSagaRetryScopeTestsplus one warm-up saga consume before the message whose first save throws, with store ids recorded only once armed. The saga ends in a non-final state so the persisted count is observable.Expected: 2, Actual: 1)SagaRetryScopeTests(existing, single message)Mocha.Sagas.Tests: 154 total, 0 failed, 1 pre-existing skip. Not run here: the EF, Postgres and RabbitMQ suites. Downstream, the same defect reproduced end-to-end against Postgres and RabbitMQ (626ObjectDisposedExceptionagainst 280 genuine version conflicts in an 11-message fan-in; 0 disposals with in-process retries disabled).Related