Busy-as-mutex locking, status-neutral updateData, and immediate InstanceData persistence - #877
Merged
Merged
Conversation
…ction, short status lock, options and logging - InstanceExecutionSnapshot record + IInstanceRepository.GetExecutionSnapshotAsync single-row projection (no includes) for admission checks - IInstanceStatusLock + InstanceStatusLock: short-lease (StatusLockLeaseSeconds) chain-reentrant status-flip lock over Aether IDistributedLockService - WorkflowExecutionOptions: UseBusyAsMutex flag, StatusLockLeaseSeconds, StatusLockRetry bounded wait - WorkflowLogs 10135-10139: busy rejection, reserve, chain ownership lost, settlement, status-lock failure - InstanceBusy added to pipeline ClientFacingErrorCodes (409 mapping already present) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-only) and policy (per-hop) - ITransitionValidationService gains ValidateSchemaAsync / ValidatePolicyAsync; ValidateAsync remains the composite used at intake points - TransitionPipeline.CreateAndValidateContextAsync now runs policy-only per hop: schema is validated once at request intake (HTTP app service pre-dispatch, async accept, start) — auto-chain hops and job re-entry no longer re-validate schema, removing the double validation cost on the async path - Event delivery already inherits intake validation by delegating to IInstanceCommandAppService; documented that invariant on EventAppService Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… behind UseBusyAsMutex flag - ITransitionAdmissionService + AdmissionKind (Normal / BypassBusyCheck / Unconditional / OwnerReentry); impl classifies via well-known transition extensions and pipeline directives - TransitionPipeline.RunWithBusyAdmissionAsync prologue (flag-on): context → Busy pre-check → policy validation → per-kind admission; chain runs with no held lease; per-hop chain-ownership re-assert via execution snapshot — a rotated token (cancel/exit takeover, reaper) stops the chain without faulting - InstanceBusyManager: TryReserveWithPropagationAsync (Busy + BeginChain token), TakeOverAsync (token rotation, no propagation), TryReleaseAsync (compensation) - AsyncTransitionStrategy (flag-on): Normal requests reserve at accept under the short status lock (immediate 409 for competitors), token stamped into the job payload; enqueue failure releases the reservation; legacy path unchanged - HTTP intake: Busy admission check runs before schema/policy validation - New error ChainOwnershipLost (Instance:100034, 409); job handler treats it as non-retryable so a cancelled chain is not re-driven or faulted - Tests: pipeline ctor updates; async-strategy busy-guard tests rewritten for the flag-on admission model (were failing on master against the commented-out legacy guard) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r busy-as-mutex - TransitionSettlement.ApplyAsync: optional chain-ownership guard — flag-on chains re-read the execution snapshot before settling; a rotated token (cancel/exit takeover, reaper) skips the settle instead of overwriting the new owner's status - TransitionPipeline.MarkInstanceFaultedAsync: fault flip serialized under the short status lock with token guard (skip when a takeover owns the instance) - PostCommitParentSnapshot now carries ChainToken; PostCommitParentMutationService uses the short status lock (flag-on) and skips settle/fault on token mismatch — mutation commits inside the lock scope before release - Subflow terminal services intentionally unchanged: their locks use disjoint per-subInstance ':sub:' keys serializing duplicate terminal deliveries, not instance status; parent mutations flow through the guarded paths above Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- WorkflowExecutionOptionsValidator: UseBusyAsMutex now requires EnableChainReaper (no lock lease to auto-expire — the reaper is the only crash recovery) and a positive StatusLockLeaseSeconds; fails fast at first options resolution - TransitionAdmissionServiceTests: classification matrix (4 kinds), Busy pre-check per kind, reserve outcomes (marked/already-busy/lock-conflict/completed), takeover token rotation, ownership verification, release compensation (25 tests) - TransitionPipelineTests: flag-on prologue dispatch — Normal reserves without the whole-chain lock, admission rejection is 409 before any step, bypass kind takes over and clears the foreign S8 checkpoint, owner re-entry verifies instead of reserving - WorkflowExecutionOptionsValidatorTests for the new guard rails Test verification: Application.Tests 23 failures and Domain.Tests 153 failures are identical on master (known AmbientServiceProvider baseline) — no regressions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…usy-as-mutex is the only model BREAKING CHANGE: the UseBusyAsMutex flag, chain ownership token, chain-token gate, whole-chain lock path and the chain reaper are all removed. The Busy flag is the sole execution mutex: the first request-handling hop takes the short status lock only for the Active→Busy check-and-set, then the pipeline and its entire auto-chain run with no lock and no per-hop checks. - Instance: ChainToken/ChainHeartbeatAt columns and BeginChain/EndChain/ MatchesChain/TouchChainHeartbeat removed; EF migration drops the columns and their indexes (DropInstanceChainTokenColumns) - TransitionPipeline: single admission path — Busy pre-check → policy validation → per-kind gate (Normal reserves; cancel/exit/timeout bypass; updateData unconditional; pre-reserved/internal-resume re-enter); legacy reserved suffixed-lock path, ChainLockRegistry registration, per-hop ownership re-assert and lease-extension block deleted - Job re-entry recognized by WorkflowExecutionContext.IsPreReserved (set by TransitionJobHandler) instead of a carried token; payload/contract/controller ChainToken fields removed - Admission: Reserve = TryMarkBusyWithPropagationAsync under the short status lock; TakeOver/VerifyOwnership removed; release compensation is token-less - Settlement/fault/post-commit: token guards removed; fault and post-commit mutations stay serialized under the short status lock - ChainReaperService + hosted service + leader-lease options + stuck-chain repo queries removed; StrictChainTokenGate, EnableChainReaper, EnableLockLeaseExtension, ChainReaper* options and appsettings keys removed - ChainOwnershipLost error/log, chain gate and reaper log methods removed - Tests updated to the token-less model; gate/reaper/reentrancy test files deleted; Application.Tests failure set identical to the pre-existing baseline (23), Domain.Tests 153 — no regressions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rward exemption, light busy check - Cancel/exit/timeout (BypassBusyCheck) now take the short status lock at admission and mark Busy under it (TakeOverAsync) — exempt from the 409 but serialized through the same distributed lock as every other status flip - REGRESSION FIX: a Busy parent with an active SubFlow correlation is admitted without a reserve instead of 409 — ForwardToActiveSubflowStep relays the request to the subflow (sync and async accept paths); snapshot projection gains HasActiveSubFlow (served by IX_InstancesCorrelations_ActiveBlockingSubFlow) plus Flow/FlowVersion - Pipeline-end settlement (Busy→Active) now serialized under the short status lock; post-commit settlement passes null (caller already holds the lock) - InstanceCommandAppService.TransitionAsync: light Busy fast-fail via the single-row execution snapshot BEFORE loading the full aggregate — a Busy rejection no longer pays the DataList/correlations load; ClassifyKey classifies cancel/exit/updateData from the cached workflow definition alone - StatusLockLeaseSeconds default 15 → 5 (real hold time is milliseconds) - Tests: takeover/forward/fast-fail coverage incl. aggregate-not-loaded assertion; suite matches the known pre-existing baseline (23) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Busy fast-fail loaded the workflow from the snapshot's Flow/FlowVersion, then the full path loaded it again. The second call was not a real round trip (LoadWorkflowAsync memoizes into IWorkflowContext and short-circuits on a Key match), but the duplication read as wasted work and leaned on that implicit memoization — which compares Key only, not version. Now the definition is resolved once from the snapshot and reused; it is reloaded only when there was no snapshot row, or when the snapshot and the aggregate resolved different rows, so the workflow always matches the instance's own Flow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… 409 CreateAndPrepareInstanceAsync seeds sub-items (IsSubItem) as Busy at creation — harmless before busy-as-mutex, fatal after: the child's own start transition classified as Normal and CheckAdmission rejected it (Instance:100031) against its birth-Busy status, failing StartSubflowJob and the parent's continuation job (subflow-orchestration-parent repro). Creation is the reservation: ExecuteStartTransitionAsync now stamps IsPreReserved when the just-created instance is already Busy, so the start classifies as OwnerReentry (no check, no reserve). Regular instances are created Active and keep the normal reserve path. Uniform across sync/async and local/cross-domain starts — every start funnels through StartAsync. TransitionContextFactory now maps IsPreReserved onto the execution context; the async accept classifies on the factory output, so the flag must survive that mapping (previously only the pipeline prologue copied it). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…write funnel
The BEFORE INSERT trigger (advisory lock + VersionNo := MAX+1 + conditional
latest demotion) is dropped; versioning moves application-side, POC-validated
(100k parallel writers, LockProgram.cs):
- WorkflowDbContext.SaveChangesAsync: when the change tracker holds new
InstanceData rows, the save runs inside a transaction (ambient UoW or a
local one) with SET LOCAL lock_timeout/statement_timeout, takes the
per-instance SELECT ... FOR UPDATE row lock on Instances, reads the
authoritative head, assigns monotonic VersionNos (head+1, chained across
multi-row saves) and demotes any stale latest row before the inserts run.
Every write path (start, pipeline steps, updateData, subflow output mapping,
definition seeds) funnels through with zero call-site changes.
- SemVer rebase: each new row carries its AppliedVersionStrategy (in-memory
only); when the in-memory base turns out stale under the lock, the strategy
is re-applied to the real head (InstanceData.RebaseVersion) — version
strings stay monotonic, duplicate SemVer is gone. First-row/explicit-version
appends keep their authored version.
- Migration DropInstanceDataVersioningTrigger (Down restores the 20260711114525
trigger). Unique indexes stay as the DB-level backstop. Rolling deploys are
safe in either order: funnel and trigger compute the same MAX+1 under the
same serialization.
- Options: WorkflowExecution:InstanceDataWrite:{LockTimeoutMs=5000,
StatementTimeoutMs=10000} (+ validator); lock timeout maps to 409
(Instance:100035), statement timeout to 503 (Instance:100036) via
UserFriendlyException.
- Supersedes the optimistic-reconciliation journal spec (content stays
full-merge last-writer-wins).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…antics updateData now updates the data AND drives the state process when it safely can, instead of always stopping at the data write: - OwnsStatus (WorkflowExecutionContext + TransitionExecutionContext): whether this execution owns the instance's Busy lifecycle. Set by admission — reserve/takeover/owner-reentry true, subflow forward false, updateData opportunistic. Propagated across inline continuation hops; post-commit settlement contexts always own (they act for the chain that handed off). - STABILIZATION: ResolveAvailableStep and TransitionSettlement now act only for status owners — a data-only updateData running beside an in-flight chain could previously resolve Active and steal the owner's Busy. - Opportunistic reserve (TryReserveOpportunisticallyAsync): updateData on an Active instance reserves Busy under the short status lock and runs the full pipeline (auto transitions evaluate against the fresh data, chain advances, settle returns it to Active). On a Busy instance it degrades to data-only — never rejected — and the owning chain sees the new data in its own steps. - SubFlow-state parent (Busy at rest, no competing chain): updateData owns the advance; HandleUpdateDataPreflightStep now jumps to Auto (90) with Target=Current instead of Finalize, so the parent's own auto transitions evaluate with the fresh data. No satisfied condition → same data-only exit as before. No forward to the subflow — updateData targets the parent. - RunAutomaticTransitionsStep: updateData without ownership skips auto evaluation; all other transition kinds unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… the subflow updateData now uses the standard pipeline end to end instead of the SubFlow-state preflight short-circuit: - HandleUpdateDataPreflightStep deleted (DI registration, LifecycleOrder constant and profile exclusion entries removed; its custom data write duplicated CreateTransitionRecordStep). On a SubFlow-state parent the data is now written by the same step as everywhere else, ChangeStateStep resolves $self, and RunAutomaticTransitionsStep evaluates the state's own autos with the fresh data — the chain advances when a condition is satisfied, otherwise the request exits data-only, exactly like a normal state. - ForwardToActiveSubflowStep: updateData is never forwarded to the active subflow — it executes on the instance it targets; a subflow's data is updated by addressing the subflow instance directly. - HandleSubFlowStep: updateData never starts, restarts or parks on a subflow — its $self pass through a SubFlow state leaves the machinery untouched and continues, covering both the active-correlation re-entry and the completed-correlation window (a restart from a data update would be catastrophic). - Ownership semantics from the previous commit unchanged: opportunistic reserve on Active, data-only beside an in-flight chain, advance ownership on a SubFlow-state parent at rest; ResolveAvailable/settlement stay owner-gated. - Stale preflight-only log methods removed; profile tests updated (the ForErrorBoundary expected-orders failure is pre-existing on master). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
updateData stops being a status owner and starts behaving like every other transition where it matters — the state process advances — while never touching the instance status itself: - Admission (Unconditional) no longer reserves. TryReserveOpportunisticallyAsync is deleted along with the OwnsStatus fabrication it fed: an updateData execution always runs with OwnsStatus=false, so ResolveAvailableStep and settlement (both owner-gated) leave the status exactly as they found it. - SetBusyStep skips updateData outright. Its 'safety net' Busy write was the one path that could mark an Active instance Busy under a non-owner, and nothing was then allowed to flip it back — the instance stayed Busy forever (observed in a runtime report as a permanently stalled instance). - RunAutomaticTransitionsStep's data-only guard is gone: the state's automatic transitions are evaluated against the freshly written data on every updateData, so a fan-in counter can gate a transition. - Ownership for a satisfied auto transition is acquired at the continuation boundary in RunChainAsync: a real ReserveAsync (short status lock) before any dispatch, so the chained transition runs as a genuine owner in both inline and enqueue mode. When a competing chain holds the instance the continuation is dropped with a warning instead of advancing ownerless; a reserve whose continuation then fails to dispatch is compensated. - New HandleUpdateDataDataOnlyStep (order 21): an updateData against a parent with an OPEN SubFlow correlation writes its data and skips to Finalize — no tasks, no schedule, no autos, and no risk of touching the subflow. Parents with SubProcess or already-completed correlations run the full pipeline, which is what makes the fan-in case work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The S8 crash-resume checkpoint is gone: Instance.ResumePointStepOrder (and its column, via DropInstanceResumePointColumn), the per-step stamping and the resume read in TransitionExecutor, and every ClearResumePoint call. It carried only a step order — no transition or job identity — while TransitionExecutor read it on EVERY pipeline entry. With transition-per-job any non-null value left on the row by a concurrent or failed execution made the next hop skip its own steps: a checkpoint at or past CreateTransition left no transition record, so a flow task on the entered state failed with 'requires a persisted instance transition id'; one between OnExecute and ChangeState let the state change commit with its tasks never run. Both were observed in a runtime report and neither is acceptable. A retried or redelivered transition now simply runs its pipeline from the top. Duplicate side effects stay covered by what actually keys on identity: the transition-record duplicate guard and the remote task journal (GetSuccessfulTaskIdsAsync, keyed by transition record id), which is untouched. Directive-based resume is unaffected and stays: StepOutcome.SkipTo, subflow and long-poll-ack ResumeFrom = ClearBusyOnResumeStep, timeout ResumeFrom = SetBusy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… match it Every DirectTrigger failure reached the incident as Task:Unknown:trigger-transition whatever its cause, so errorBoundary could not express 'retry on lock contention, fail fast on everything else' — and the documented ["409"] shape matched nothing. Two causes, both fixed: - The status was never resolved. ErrorNormalizer.Normalize(Error) only set StatusCode when the code literally parsed as a number. It now resolves it from a bare numeric code, from the trailing status segment of an already task-scoped code, or from the Aether error prefix (Conflict→409, Validation/NotSupported→400, NotFound→404, Unauthorized→401, Forbidden→403, Transient→503, Dependency→502). TriggerTaskExecutorBase.MapErrorToStatusCode now delegates to that single map instead of keeping its own copy. - The precise code was thrown away. An engine failure reaches the factory twice (engine → ExecutionError.ToError → Result.Fail → the coordinator re-wraps it), and the outer call site does not know the task type — Reference carries none — so it passed the literal "Unknown" and flattened Task:DirectTrigger:trigger-transition:409 into Task:Unknown:trigger-transition. CreateFromError now keeps a code that is already task-scoped, and appends the resolved status otherwise; CreateFromException does the same. errorCodes entries match a full code or a bare status, so ["409"] now works. Doc examples realigned with the codes the runtime actually emits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A $self automatic loop re-enqueues the same (instance, sourceState,
transitionKey) on every iteration, and the job name was derived from exactly
those three fields — so every iteration produced a byte-identical name. The
scheduler entry is keyed by that name and deleted BY NAME when a one-shot job
completes, so the finishing iteration deleted the NEXT iteration's trigger: the
chain died mid-loop, the instance stayed Busy with no job left to settle it, and
nothing could recover it (a job that never runs never reaches job-timeout
recovery). Observed in a runtime report as a finalize loop stopping at 2 of 3
with the instance still Busy ten minutes later. The same collision window exists
on the accept path, between MarkAsProcessed and the scheduler delete.
Physical and logical identity are now separate:
- JobName gains a trailing invocation segment for transition jobs (async and
scheduled), derived from the job's own id — unique per enqueue, still readable,
and traceable back to the row. Timeout / long-poll-ack / state-notify names are
unchanged (armed once). TryParse accepts the new three-field shape plus both
legacy shapes, so rolling deploys and existing rows keep working. With no
source state the segment is omitted: "{key}.{invocation}" would be
indistinguishable from "{sourceState}.{key}" coming back in.
- The "is a job for this transition already queued" guard moves off the name onto
the structured columns (AnyActiveTransitionJobAsync: instance + job type +
source state + transition key), which is what that check always meant. Client
semantics are unchanged: a duplicate request while one is active still gets 409.
AnyActiveByJobNameAsync is removed rather than left as a trap that now matches
nothing.
- AsyncTransitionStrategy built the name twice (guard, then insert); with a
unique name that would produce two different strings, so the id and the name
are built once and threaded through.
Cancellation already matched on the structured columns and cancelled by job id,
so it is unaffected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…xplicit service Architecture decision: no DbContext-level interception. The SaveChangesAsync override that silently versioned every pending InstanceData row is gone; persistence of new data versions is now an explicit call the write path makes itself, collected in one service so the semantics stay in one place. - New IInstanceDataWriteService.SaveWithVersioningAsync(instance): inside the ambient transaction when one is open (otherwise a local one) it applies SET LOCAL lock/statement timeouts, locks the parent Instances row FOR UPDATE, reads the authoritative head, assigns monotonic VersionNos and rebases stale semantic versions (the AssignVersions pure core moved verbatim), demotes any stale latest row, and owns the SaveChanges. Error mapping is unchanged: lock timeout -> Instance:100035 (409), statement timeout -> Instance:100036 (503). Per-instance grouping/Guid ordering is gone - one instance per call. - Every production write path now persists through the service (the complete inventory): instance start, CreateTransitionRecordStep, the three task steps (OnExecute/OnEntry/OnExit after ApplyScriptContextChanges), subflow output mapping, and DefinitionAppService publish + seed data (the seed path resolves the service from its own child DI scope, whose DbContext holds the entities). SubflowFault/Completion's later saves carry no pending data rows - the output mapping already persisted them - so they stay plain. - WorkflowDbContext keeps a 5-line assertion ONLY: an Added InstanceData row with an unassigned VersionNo fails the save with a pointed exception instead of corrupting the version sequence three steps later. It does no locking, no versioning, no transaction work. The funnel file and the context's two funnel-only ctor parameters are removed. The partial unique indexes on InstancesData remain the database-level backstop. No schema change - no migration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A state with automatic transitions PARKS Busy at rest — ResolveAvailableStep deliberately never resolves it (HasOnlyManualOrEventTransitions gate). Every fan-in wait state is such a state, so the updateData continuation reserve ALWAYS failed there and the gate could never fire: evaluation was fixed, the advance was not. Observed live on a testbed (parent parked Busy at its collect state; every satisfied gate dropped with Instance:100031). The handoff now tells the two Busys apart. A LIVE owner shows up as an active transition job for a DIFFERENT transition key (async accept intents, per-hop chain jobs, armed timers — which will fire and re-evaluate anyway); only then is the continuation dropped. A Busy with no such job is parked and ownerless: it is taken over (idempotent flip under the same short status lock) and the gate transition proceeds as a real owner. Rows for the execution's own transition key are its own accept intent — never an owner. An in-process sync chain leaves no job row and stays invisible to the probe; the duplicate transition-record guard and per-hop policy checks stop that loser, as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rsion
ApplyScriptContextChanges replayed snapshot-side task results onto the live
aggregate with AddDataWithVersion, carrying the version string the DETACHED
snapshot computed. A concurrent append that advanced the live head between
snapshot creation and the apply left that frozen version below the head — the
replay then either threw on a latest-only loaded aggregate ('Cannot append
version ... the target version line is not in memory', observed in production
with 1.2.2) or, worse, silently demoted the newer head: explicit-version rows
carry no AppliedVersionStrategy, so the InstanceData write service never
rebases them yet honors their in-memory takesLatest decision.
The replay now re-appends by STRATEGY (AddData with the row's own
AppliedVersionStrategy, Patch fallback): the version is recomputed from the
live head, so it can never land below it, and the row stays strategy-bearing —
the write service rebases it against the real database head under the
per-instance row lock. The AddDataWithVersion guard itself is untouched; it
keeps protecting genuine older-line appends on the publish/API path.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stored InstanceData row is the FULL merged state (full-merge model: NewVersion stores previous-full + delta), but the no-change dedup compared the RAW input's hash against the merged head's hash. For delta-only inputs — the recommended pattern for concurrent updateData callbacks — those never match, so an idempotent duplicate callback (re-stamping a key that is already set) silently created a byte-identical new version on every delivery. DataHash exists precisely to prevent that growth. AddData now merges first and compares the merged result against the head's hash: no content change → the existing head is returned and no row is created. The merge is computed once and handed to NewVersion (alreadyMerged) instead of being recomputed. Identical-full-input dedup behaves as before; any input that actually changes content still produces a new version. Also documented two review outcomes as code comments so they are not re-asked: AssignVersions' loop is required (one save can carry several Added rows — a task step applies all its tasks' snapshot outputs at once), and HistorySequence cannot be replaced by Version/VersionNo/IsLatest — same-version rows are routine (VersionStrategy.None default) and VersionNo is only assigned at persist time under the row lock, so unpersisted rows cannot order themselves without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nt task groups Two halves of one durability story — a step with N tasks that dies mid-way must neither lose the finished tasks' data nor re-run their side effects on retry. Retry wiring: the retry entry point always carried the faulted transition record's id (RetryInfo.TransitionId) but nothing consumed it, so every retry created a FRESH transition record. The task journal (InstanceTask rows) is keyed by transition record id, which means the completed-task bypass never matched anything: the whole bypass mechanism was dead code and every retry re-ran every task, side effects included. The id now flows through the context (RetryOfTransitionRecordId) into CreateTransitionRecordStep, which reuses the original record (falling back to normal creation when it no longer exists) — the journal lines up and journaled-complete tasks are skipped. Response rehydration for bypassed tasks is deliberately NOT done in this change. Group checkpointing: task outputs used to accumulate on the detached snapshot and reach the database only in one save at the END of the step — the journal could say "completed" while the data existed nowhere durable. The coordinator now accepts an optional per-order-group checkpoint, and the three task steps use it to apply the group's snapshot deltas onto the live aggregate and persist them through the versioning write service as soon as the group finishes. A crash in group N loses only group N; earlier groups are already on disk, and a retry finds both their journal rows (bypass) and their data (persisted). A failing group does not checkpoint. Parallel groups still merge before the checkpoint, so multi-row saves remain — which is exactly why AssignVersions keeps its loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…omputed under the lock Architecture decision: an InstanceData version is persisted the moment it is produced — task outputs included, parallel or sequential — and its WHOLE identity is computed under the per-instance FOR UPDATE row lock from the authoritative head: VersionNo = head + 1, Version = head version + strategy, and the no-change dedup from the merged content's hash. The in-memory version chain machinery this replaces (compute-then-rebase, AssignVersions, batched saves) is gone. - IInstanceDataWriteService: SaveWithVersioningAsync is replaced by AppendAsync (strategy append: head read WITH content under the lock, merge, hash dedup returning null on no-change, version from head, direct DbSet insert, aggregate refresh) and AppendExplicitAsync (publish path: as-authored payload, same-version short-circuit, older-line rows never steal the latest flag). Master-schema validation moved from the aggregate's [SchemaValidation] aspect into the service, unchanged in behavior. - Instance.AcceptPersistedData: the one way a persisted row enters an aggregate's memory — Id-idempotent (EF relationship fixup may have already attached it) and single-latest-preserving. Used for live aggregates and ScriptContext snapshots alike. - TaskExecutionEngine persists each task output immediately through the service and refreshes the snapshot; Extension/Function origins still never persist. Parallel branches write through their own scoped DbContext — the row lock serializes them; MergeParallelBranch no longer re-appends data, it only syncs the freshest persisted row into the coordinator's snapshot. - ApplyScriptContextChanges no longer replays data (nothing to replay) — it syncs the live aggregate with the snapshot's persisted latest and applies the non-data mutations. The per-group checkpoint from the previous commit is removed: immediate persistence supersedes it. - Callers rewired: transition payload (CreateTransitionRecordStep), start (instance row inserted FIRST, then the initial version appended), subflow output mapping, and the publish paths (AppendExplicitAsync). The retry task-journal bypass now finds every completed task's data already on disk — the crash-between-tasks gap is closed for good. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With every InstanceData row now persisted immediately by IInstanceDataWriteService (identity computed under the per-instance FOR UPDATE lock), the aggregate-side write machinery is dead weight — and a temptation for a second write path. Tear it out: - Instance: AddData, AddDataWithVersion and GetNextHistorySequence removed. AcceptPersistedData is the ONLY way a (persisted) row enters the aggregate's memory; reads (FindData, GetVersionHistory, LatestData, partial-load guards) are untouched. - InstanceData: NewVersion, RebaseVersion and AppliedVersionStrategy removed — the compute-then-rebase in-memory version chain has no callers left. Doc comments now describe the write service as the identity authority. - SchemaValidationAttribute aspect deleted: its only weave targets were the two removed methods; the service performs the same master-schema validation explicitly before persisting (SchemaValidationException unchanged, so the HTTP mapping is unaffected). The service's schema-load warning now goes through WorkflowLogs (InstanceDataSchemaLoadFailed, 10148); the dead InstanceDataVersionRebased message is dropped. - InstanceDataWriteService: the strategy append's identity computation is extracted into the pure static PlanAppend (merged content, merged-hash dedup verdict, version pair) so the 5879086 regression — dedup must compare the MERGED result, never the raw delta — stays unit-pinned without a database. - Tests: new InstanceDataSeeder in Domain.Tests seeds fully-identified rows via AcceptPersistedData with the old AddData/AddDataWithVersion signatures; all test call sites swapped. Rebase and aggregate-dedup test files deleted (the dedup pin moved to PlanAppend tests); the latest-invariant and version-format suites rewritten around the seeder. Removing the aspect also removes the AmbientServiceProvider dependency from these tests — 129 pre-existing parallel-collection failures in the Domain suite disappear (153 → 24 on the master baseline, zero new failures; Application and Infrastructure baselines unchanged). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…le tie-breaker HistorySequence existed to order not-yet-persisted rows sharing the same Version string, back when data accumulated in memory until a step-end save. With immediate persistence, every row's VersionNo is assigned under the per-instance FOR UPDATE lock before anyone can observe it — an unpersisted multi-row state is structurally impossible, so the column carries no information VersionNo doesn't. - InstanceData: property, ctor parameter and snapshot copy removed; InstanceDataVersionComparer's same-version tie-break moves to VersionNo. Instance.GetVersionHistory / GetLatestDataForVersion order by VersionNo. - EF: property mapping removed; UX_InstancesData_Instance_IsLatest is rebuilt without the column in its INCLUDE list. - Migration DropInstanceDataHistorySequence: index drop → column drop → index recreate; Down restores both. Applied to all system + domain schemas via DbMigrator (the stale unmanaged `public` template schema is untouched, as with prior drop-column migrations). - InstanceDataVersioningTests rewritten: they used to install a copy of the ALREADY-DROPPED versioning trigger and test that; they now run the real InstanceDataWriteService against Testcontainers PostgreSQL — 20 concurrent appends through separate DbContexts serialize on the row lock into VersionNo 1..20 with a single latest and a loss-free merged head, plus merged-hash dedup, head-derived version chains, the explicit older-line IsLatest invariant, and per-instance sequence independence. - Test seeder loses the sequence math and serializes concurrent seeding per instance (the removed aggregate methods did their whole read-compute-mutate under one lock; the seeder now does too). Suites at baseline (Domain adds zero failures vs master; Application 23, Infrastructure 11 unchanged). E2E on the migrated schema: contract-signing smoke PASS (20 sequential versions, single latest), updateData concurrency 15/15 PASS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The DbMigrator failed at startup under build-time DI validation: the Infrastructure module registers IInstanceDataWriteService, but two of its constructor dependencies lived in modules non-HTTP hosts never load — IWorkflowContext (registered only by the HttpApi base module) and IComponentCacheStore (registered only by the Application module). - WorkflowContext moves from HttpApi.Shared to the Domain project next to its interface (it is a plain scoped holder with no ASP.NET dependency), and the Infrastructure module TryAdd-registers it beside the write service so the module satisfies its own dependency; the API hosts' existing registration stays authoritative. - IComponentCacheStore is now resolved lazily inside the master-schema validation step. In workers and the migrator the workflow context is always empty, so that line is never reached; the null-store branch logs and skips as belt-and-braces. Verified: DbMigrator runs clean with DOTNET_ENVIRONMENT=Development (ValidateOnBuild active), Infrastructure suite at baseline, hosts healthy, updateData concurrency smoke 8/8 PASS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… share the ambient UoW connection
Found by the new data-integrity-lab E2E flow: a transition with four same-order
(parallel) tasks faulted with NpgsqlOperationInProgressException ("connection is
already in state 'Executing'"). Each parallel branch gets its own DI scope, but
the ambient (AsyncLocal) UnitOfWork flows into every branch and hands them all
the SAME schema-bound DbContext — and a Npgsql connection cannot execute two
commands concurrently. Before immediate per-record persistence, parallel
branches never touched the database mid-step, so this had no way to surface.
RunLockedAsync now takes a per-DbContext SemaphoreSlim gate (ConditionalWeakTable)
before entering the lock/persist scope. Appends arriving on the same context
serialize locally — exactly what the FOR UPDATE row lock would do to them anyway,
minus the broken shared-connection interleaving; writers on different contexts
are untouched and keep serializing on the row lock. Task bodies (HTTP calls etc.)
still run fully parallel; only the tail persist is serialized per connection.
New Testcontainers test pins the scenario: eight concurrent AppendAsync calls
through one shared DbContext produce VersionNo 1..8, a single latest and a
loss-free merged head instead of throwing.
Verified E2E on the new build: data-integrity-lab 10/10 PASS (sequential chain
+3 rows with dedup echo swallowed, 4 parallel branches loss-free under a
concurrent updateData storm, two-stage noop dedup probe, counter == accepted),
subflow-orchestration updateData concurrency 10/10 PASS, contract-signing smoke
PASS. Infrastructure suite at baseline.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… row
Publish regression: after an explicit older-line publish (e.g. 1.0.0 -> 1.1.0
-> 1.0.1) the latest row (1.1.0) carries VersionNo 2 while the older-line row
1.0.1 carries VersionNo 3 — it took a higher number without taking the latest
flag. The service derived every next VersionNo from the LATEST row (+1), so all
further appends for that instance computed 3 and died on
UX_InstancesData_Instance_VersionNo; no new version (1.0.2, 1.2.0, ...) could
ever be written. The old funnel/trigger numbered from MAX(VersionNo)+1 — this
restores that under the same FOR UPDATE lock: ReadHeadAsync now also returns
the instance-wide MAX(VersionNo) and both append paths number from it (the
latest row keeps feeding merge, dedup and the semantic version).
Also hardens the same-context gate found by the data-integrity-lab flake
("A second operation was started on this context"): GetDbContextAsync itself
can touch the shared ambient-UoW connection, so appends now take a striped
per-instance semaphore BEFORE resolving the context, in addition to the
per-context gate around the lock/persist scope.
Pinned by a PlanAppend unit test (MaxVersionNo > latest VersionNo) and a
Testcontainers test replaying the exact production sequence: explicit
1.0.0/1.1.0/1.0.1 then 1.2.0 (takes latest), 1.0.2 (older line) and a strategy
append (1.2.1) — VersionNo 1..6, single latest. Verified against the live
publish endpoint on the same sys_views instance that was stuck: 1.0.2, 1.2.0
and 1.0.3 all publish cleanly (VersionNo 4/5/6, latest on 1.2.0). E2E battery
green: data-integrity-lab 3x PASS, updateData concurrency 10/10, contract
smoke PASS.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Delete generated BuildHost debug artifacts from src/BBT.Workflow.Infrastructure/bin/Debug/net10.0 (both BuildHost-net472 and BuildHost-netcore). Removes Microsoft.Build.Locator, Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost binaries and related runtime/config/resource files (Newtonsoft.Json, System.CommandLine, System.* DLLs, .deps.json, runtimeconfig, and localized resource DLLs). These are build outputs and were accidentally committed; removing them reduces repository size and prevents tracking of generated files.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @yilmaztayfun, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Important Review skippedToo many files! This PR contains 145 files, which is 45 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (145)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideImplements Busy-as-mutex admission (Busy flag as execution mutex), makes updateData status-neutral while still driving the normal transition pipeline and auto evaluation, and rebuilds InstanceData persistence around an explicit write service that assigns per-row identity under a per-instance FOR UPDATE lock. Removes chain-token locking and crash-resume checkpointing, hardens task/journal behavior and InstanceData versioning, and updates tests and migrations accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
 |
|
Overall Grade |
Security  Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Aug 13, 2026 8:31a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 2 critical |
🟢 Metrics 150 complexity
Metric Results Complexity 150
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
11 tasks
A cache miss can fire from anywhere — including a parallel task branch. The backend already created its own DI scope, but the ambient (AsyncLocal) UnitOfWork flows through scopes and handed the load the caller's shared DbContext; a concurrent command on that connection then failed the branch with "A second operation was started on this context instance" (seen on cold cache after a Redis restart, faulting run-parallel transitions in data-integrity-lab). RuntimeCacheBackend now wraps both load paths in a RequiresNew, non-transactional unit of work: reads get their own context/connection and no longer ride the caller's transaction. Verified with a cold-cache E2E run (FLUSHALL + 10/10 data-integrity-lab PASS). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ch semantic version Review decision on PR #877: VersionNo is no longer an instance-global sequence but a 1-based ordinal WITHIN one semantic Version string: 1.0.0|1, 1.1.0|1, 1.1.0|2, 1.1.0|3, 1.2.0|1 Same-version appends (VersionStrategy.None) continue their line; every version bump or explicit publish of a new version starts a fresh line at 1. Cross-line ordering stays semantic (InstanceDataVersionComparer); EnteredAt carries global chronology. The comparer tie-break and every same-version read path (GetVersionHistory, GetLatestDataForVersion, FindData) already operated within one version line, so they are untouched by construction. - InstanceDataWriteService: the head read shrinks back to Version/DataHash/Data; the new ReadLineMaxAsync scalar (still under the FOR UPDATE lock) supplies MAX(VersionNo) of the TARGET version line, and both append paths number from it. PlanAppend now yields content/version/dedup only. - Unique backstop moves from (InstanceId, VersionNo) to (InstanceId, Version, VersionNo) — UX_InstancesData_Instance_Version_VersionNo. - Migration LineScopeInstanceDataVersionNo renumbers existing rows per (InstanceId, Version) preserving prior relative order (verified against a synthetic global-numbered fixture: 1.0.0|1, 1.1.0|2,4,5, 1.2.0|7 → 1.0.0|1, 1.1.0|1,2,3, 1.2.0|1); Down renumbers back to a per-instance chronological sequence and restores the old index. - Tests: Testcontainers suite rewritten to line semantics (the reviewer's exact table is pinned by SameVersion_Appends_Should_Grow_Their_Own_Line_Ordinal; 20 concurrent Patch appends now assert 20 distinct versions all at ordinal 1); the domain seeder numbers per line; the E2E harnesses assert per-Version groups (count == max == distinct, min == 1) instead of a global 1..N. E2E on the migrated schema: data-integrity-lab 10/10 (twice, once cold-cache), updateData concurrency 15/15, contract smoke PASS; publish scenario end-to-end: 1.1.0/1.0.1/1.0.2/1.2.0 all 200 with vn=1 on their own lines, latest on 1.2.0, re-publishing 1.1.0 dedups with 409/100002. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ry loop Review decision on PR #877: InstanceStatusLock no longer loops with jittered backoff waiting for the short status lease. One TryAcquire; a held lock means a concurrent hop is mid-flip, and the caller contract is unchanged — admission paths surface it as InstanceLockConflict (409, the client's dedupe/retry signal) and settlement/fault paths proceed unguarded as before. The now-unused StatusLockRetry option is removed. LockConflictRetryOptions / LockAcquireWait stay: the subflow terminal locks (SubItemTerminalLockRetry via TransitionLockScopeFactory) and the job handler's LockConflictRetry still use that machinery deliberately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review decision on PR #877: the WorkflowDbContext.SaveChangesAsync assertion (reject an Added InstanceData row with an unassigned VersionNo) scanned the change tracker on every save to catch a mistake reviews are expected to catch — the plus/minus call went against paying that on every runtime save. Writing instance data through anything other than IInstanceDataWriteService is now a convention enforced in review; the unique indexes on InstancesData remain the database-level backstop. Guard tests removed with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
middt
approved these changes
Aug 13, 2026
|
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch replaces the chain-token locking model with Busy-as-mutex (the Busy status itself is the execution mutex), redesigns updateData to be status-neutral, and rebuilds the InstanceData write model around immediate per-record persistence with row identity computed under a per-instance
FOR UPDATElock. It is the follow-up to the preprod lock+DB-pool cascade: the 330s-lease distributed lock is gone from the transition path entirely.What changed
1. Busy-as-mutex locking (breaking)
IInstanceStatusLock, 5s lease); the pipeline and auto-chain then run lock-free.Normal(409 on Busy),BypassBusyCheck(cancel/exit/timeout),Unconditional(updateData),OwnerReentry(job re-entry / subflow resume).ChainToken/ChainLockRegistry/ChainReaperare fully removed (columns dropped by migration). Known trade-off, accepted deliberately: a pod crash on the sync path leaves the instance Busy until the async-path job recovery picks it up.2. updateData v2 (breaking)
$selfstate change, auto evaluation at order 90); it never forwards to or restarts a subflow.ResumePointstep checkpoint is removed — retries re-run the pipeline from the top; duplicate protection lives in the transition-record guard and the task journal (F4/F6).{tx|sx}.{id}.{state}.{key}.{inv8hex}), fixing the$selfauto-loop killing its own next trigger under TransitionPerJob (F8).3. InstanceData write model v2 (breaking)
IInstanceDataWriteService(AppendAsync/AppendExplicitAsync). The DbContext-level write funnel, the DB versioning trigger,Instance.AddData/AddDataWithVersion, the in-memory rebase machinery and theHistorySequencecolumn are all removed.FOR UPDATElock from the authoritative head:VersionNo = MAX(VersionNo)+1,Version = head + strategy, and no-change dedup on the merged content's hash.VersionNois the single same-version tie-breaker.MAX(VersionNo), not the latest row — explicit older-line publishes (e.g.1.0.0 → 1.1.0 → 1.0.1) no longer brick the instance on the unique index;InstanceDataWriteServiceis constructible in non-HTTP hosts (DbMigrator/workers) —WorkflowContextmoved next to its interface in Domain,IComponentCacheStoreresolved lazily.RetryInfo.TransitionIdis now consumed, the retry reuses the original transition record and the task journal bypasses completed tasks whose data is already on disk.4. Migrations (in order)
DropInstanceChainTokenColumns→DropInstanceDataVersioningTrigger→DropInstanceResumePointColumn→DropInstanceDataHistorySequence(index rebuild + column drop; everyDownrestores). Rolling-deploy safe: trigger and service both assign the sameMAX+1during the overlap window.Why
Root cause chain from the preprod incident and the vnext-example field report (F1–F8): the long-lease distributed lock cascaded into DB-pool exhaustion; updateData could strand instances in Busy; the deferred-persistence data model lost task output on crash and raced on version identity. Each of the three pillars above removes one of those failure classes structurally.
Testing
SchemaValidationaspect eliminated 129 pre-existingAmbientServiceProviderparallel-collection failures in the Domain suite (153 → 24); zero new failures anywhere.VersionNo 1..20with a single latest and a loss-free merged head; shared-context concurrency; merged-hash dedup; explicit older-lineIsLatestinvariant; the exact production publish sequence that used to dead-end.data-integrity-lab(new flow): sequential chain with dedup echo, 4 parallel HTTP branches under a concurrent updateData storm, two-stage noop dedup probe, deterministic row math — 10/10;subflow-orchestrationupdateData concurrency — 20/20 (threshold 8, burst 6, ~1300 same-instant 409 dedupes, counter == accepted, no stranded Busy);contract-signingproduction-shaped smoke — 20 sequential versions, single latest, 3 children, rapid approvals without 409.Notes for reviewers
ExecutionKeyistransition+task+order); mappings should return delta-only output (a full echo overwrites concurrent writers' fresher values with stale ones); each accepted updateData produces two data rows (request payload + task output).WorkflowDbContext.SaveChangesAsynckeeps a 5-line assertion guard: an Added InstanceData row withVersionNo == 0(i.e. a write path bypassing the service) throws.🤖 Generated with Claude Code
Summary by Sourcery
Replace chain-token execution locks with Busy-as-mutex admission, make updateData transitions status-neutral, and adopt a centralized InstanceData write service with immediate per-record persistence and DB-level versioning under a per-instance row lock.
Enhancements:
Documentation:
Tests:
Chores: