Extended session support - #309
Conversation
|
Chris Gillum (@cgillum) sorry to bother - could you please provide your feedback? 🙏 |
|
Chris Gillum (@cgillum) , apologies for pinging - could you please review this one? 🙏 |
|
Hey Usein Mambediiev (@usemam), sorry for missing your pings! Adding sophiatev to take a look at this PR as she has the most context on extended sessions these days. |
|
Hey Usein Mambediiev (@usemam) can you fix the merge conflicts, then I'll take a look :) |
|
sophiatev apologies about the delay. Conflicts are fixed now. Please take a look. Thank you! |
andystaples
left a comment
There was a problem hiding this comment.
Thanks for putting this together. The overall approach—implementing Core's IOrchestrationSession, defaulting the feature off, and retaining the SQL instance lock across checkpoints—is the right model for this backend. I found the following blockers that need to be addressed before merge:
-
The PR does not compile.
SqlOrchestrationSessionimplementsIOrchestrationSessionbut does not implementEndSessionAsync(). The current head fails with CS0535 for netstandard2.0, net8.0, and net10.0. A no-op implementation is appropriate if lock release remains inReleaseTaskOrchestrationWorkItemAsync. -
Lease ownership is not generation-safe. Every session receives the process-wide
lockedByValue, while checkpoint, fetch, renew, and release identify ownership using that reusable value. If a lease expires and the same process reacquires the instance, the old session can still checkpoint or clear the replacement session's lease. Also, the checkpoint guard checksLockedBybut not whetherLockExpirationis still valid, and_RenewOrchestrationLockshas no owner guard. Please use a unique token per lock acquisition and validate that token plus an unexpired lease in checkpoint/fetch/renew/release; lock-loss should surface asSessionAbortedException. -
The retained
EventPayloadMapcollides across ContinueAsNew generations. Value-tracked payloads are keyed only by(EventType, TaskEventId)and inserted withDictionary.Add. Activity and sub-orchestration IDs restart after ContinueAsNew, so scheduling ID 0 in a later generation can throw before checkpointing. Reset/replace mappings after a successful checkpoint or include execution identity in the key. -
_FetchOrchestrationMessagesreturns an undefined event order. It usesTOP (@BatchSize)withoutORDER BY. Core only ensures thatExecutionStartedis first; it does not restore order among external events or task responses. Please addORDER BY N.SequenceNumber ASC. -
All sessions share the service's stateful
BackoffPollingHelper. The mutable interval, exponent,Random, and auto-reset event are shared with the main orchestration poller and every concurrent session. Session polling therefore changes global polling latency, and reset signals can wake an unrelated waiter. Each session should own an independent helper and reset it after finding work. -
Azure Functions timeout mapping is inconsistent with Azure Storage.
SqlDurabilityOptionsonly assignsExtendedSessionIdleTimeoutwhen the common option is greater than zero. Explicit zero/negative values silently retain SQL's 30-second default, while Azure Storage clamps the same option to zero; this can also disagree with the isolated worker cache configuration. Please align the mapping (for example, clamp to zero) or reject invalid values consistently.
Please add repository-local coverage for:
- the Core interface/build contract;
- lease expiry followed by same-process reacquisition, proving stale checkpoint, renew, fetch, and release calls are rejected;
- order-sensitive external events or responses across a session;
- ContinueAsNew followed by activities and sub-orchestrations whose task IDs restart;
- Azure Functions adapter mapping for default, custom, zero, and negative timeout values;
- Durable Entity behavior in both queue modes, or explicitly prevent sessions from being attached to entity work items until supported;
- graceful shutdown/drain and lock release.
The cross-repository package pins, extension bundle publication, public docs/tooling schemas, and rollout telemetry can be handled as maintainer-owned follow-ups rather than requirements for this contribution.
|
Hi Usein Mambediiev (@usemam) , thanks for your contribution. Independent of Sophia's review, I had an agent take a look and it produced the findings above. Please take a look and address where applicable, and thanks again! |
Summary
Keep orchestration in memory until it's idle for a configured timeout.
When extended sessions enabled, check-pointing the orchestration instance does not reset
Instances.LockedBy/LockExpirationvalues. ASqlOrchestrationSessioninstance is provided to the dispatcher, through which new messages for the orchestration instance are fetched.Links