From 20a3eefb1e461ba4b4e4e69249cc61e55822c34a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:20:28 -0400 Subject: [PATCH] fix(WRK-32): pre-baseline pump for write-completion correlation + honest contract docs Write-completion correlation is recency-based per (server_handle, item_handle): OnWriteComplete carries no per-call token, so the executor attributes the first completion whose version exceeds a pre-call baseline to that write. A stale completion from an earlier timed-out write to the same handle could ride the next write's reply if it was already queued when the baseline was captured. Add CaptureCompletionBaseline (Write, Write2, WriteSecured, WriteSecured2) to run one pumpStep() immediately before snapshotting CurrentVersion, so an already-queued stale completion folds into the baseline instead of being available to the next wait. This narrows, but per the finding cannot close, the correlation gap -- a completion queued after the pump step and before the COM call returns (or during the wait) is still indistinguishable from the new write's own completion; closing it needs an MXAccess-side correlation token that does not exist. Rewrite the AwaitWriteCompletion doc comment to state the recency-based contract honestly instead of implying exact per-call matching, and add the same contract to docs/MxAccessWorkerInstanceDesign.md's write-completion section. Deferred: the reply-field comment in src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto is not edited here. A comment-only proto change still requires regenerating every generated artifact (C#, Go, Python, Java, Rust, protoset) in the same commit per repo convention, and this macOS box cannot verify all five client generator pins together for a Low-severity doc-only change. The contract is documented in MxAccessWorkerInstanceDesign.md instead; the existing proto comment ("concurrent writes... can swap rows") is not wrong, just less precise than the new doc section. --- docs/MxAccessWorkerInstanceDesign.md | 24 +++++++++ .../MxAccess/MxAccessCommandExecutor.cs | 53 +++++++++++++++++-- 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/docs/MxAccessWorkerInstanceDesign.md b/docs/MxAccessWorkerInstanceDesign.md index 28f49a1..fab6491 100644 --- a/docs/MxAccessWorkerInstanceDesign.md +++ b/docs/MxAccessWorkerInstanceDesign.md @@ -317,6 +317,30 @@ whose write workload is effectively fire-and-forget — no consumer reads the reply's `statuses` — can lower it, or set `0` to skip the wait entirely and reply on acceptance alone. +**Write-completion correlation is recency-based, not per-call.** MXAccess's +`OnWriteComplete` callback carries no correlation token — only the server/item +handle pair — so `MxAccessCommandExecutor` correlates a completion to a write +by *version*: it snapshots `MxAccessWriteCompletionCache.CurrentVersion` for +the handle as a baseline before the COM call, then reports the first +completion whose version exceeds that baseline as the write's outcome +(`AwaitWriteCompletion`). This is exact for the common case, but if an earlier +write to the same handle times out unconfirmed — its `OnWriteComplete` still +undelivered when the wait gives up — and the caller immediately issues another +write to the same tag, the stale callback can dispatch during the second +write's pump-wait and be reported as *its* outcome. The executor mitigates the +most common instance of this (`CaptureCompletionBaseline`) by running one +`pumpStep()` immediately before capturing the baseline, so a completion already +queued for the handle folds into the baseline instead of being available to +ride the next write's reply; this closes the gap for a callback that arrived +before the new write started, but a callback that MXAccess queues after that +pump step and before the COM call returns (or during the wait itself) is still +indistinguishable from the new write's own completion. Full closure is not +possible without an MXAccess-side correlation token, which does not exist — +this is a property of the COM surface, not a gap this worker introduced (a +native client correlating by handle has the identical ambiguity). The exposure +is bounded: `statuses` is an enrichment field, and `Hresult`/`ProtocolStatus` +stay acceptance-only regardless of which completion (if any) was attributed. + ## COM Creation The MXAccess analysis source at `C:\Users\dohertj2\Desktop\mxaccess` identifies diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessCommandExecutor.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessCommandExecutor.cs index 62b4e94..46cbc7e 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessCommandExecutor.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessCommandExecutor.cs @@ -445,7 +445,8 @@ public sealed class MxAccessCommandExecutor : IStaCommandExecutor // also fire-and-forget in MXAccess, and its commit outcome only exists // in the later OnWriteComplete callback. MxAccessWriteCompletionCache completionCache = session.WriteCompletionCache; - ulong completionBaseline = completionCache.CurrentVersion( + ulong completionBaseline = CaptureCompletionBaseline( + completionCache, writeCommand.ServerHandle, writeCommand.ItemHandle); @@ -485,7 +486,8 @@ public sealed class MxAccessCommandExecutor : IStaCommandExecutor // Same pre-call baseline rule as ExecuteWriteSecured. MxAccessWriteCompletionCache completionCache = session.WriteCompletionCache; - ulong completionBaseline = completionCache.CurrentVersion( + ulong completionBaseline = CaptureCompletionBaseline( + completionCache, write2Command.ServerHandle, write2Command.ItemHandle); @@ -523,7 +525,8 @@ public sealed class MxAccessCommandExecutor : IStaCommandExecutor // immediately after WriteSecured bumps the version past this snapshot, // so a fast commit still correlates (no missed-callback window). MxAccessWriteCompletionCache completionCache = session.WriteCompletionCache; - ulong completionBaseline = completionCache.CurrentVersion( + ulong completionBaseline = CaptureCompletionBaseline( + completionCache, writeSecuredCommand.ServerHandle, writeSecuredCommand.ItemHandle); @@ -564,7 +567,8 @@ public sealed class MxAccessCommandExecutor : IStaCommandExecutor // Same pre-call baseline rule as ExecuteWriteSecured. MxAccessWriteCompletionCache completionCache = session.WriteCompletionCache; - ulong completionBaseline = completionCache.CurrentVersion( + ulong completionBaseline = CaptureCompletionBaseline( + completionCache, writeSecured2Command.ServerHandle, writeSecured2Command.ItemHandle); @@ -994,7 +998,31 @@ public sealed class MxAccessCommandExecutor : IStaCommandExecutor } /// - /// Bounded pump-wait for the OnWriteComplete row matching a + /// Captures the write-completion baseline for / + /// immediately before a Write/Write2/WriteSecured/ + /// WriteSecured2 COM call. Pumps once first (WRK-32) so an OnWriteComplete + /// already queued for this handle — most commonly the still-undelivered callback of an + /// earlier write to the same tag that this worker gave up waiting on — folds into the + /// baseline instead of being available to ride this write's reply. + /// + /// + /// This narrows, but does not close, the correlation gap documented on + /// : a completion that MXAccess queues after this pump + /// step but before the COM call returns is still indistinguishable from this write's own + /// completion, because OnWriteComplete carries no per-call token — only the + /// server/item handle pair. + /// + private ulong CaptureCompletionBaseline( + MxAccessWriteCompletionCache completionCache, + int serverHandle, + int itemHandle) + { + pumpStep(); + return completionCache.CurrentVersion(serverHandle, itemHandle); + } + + /// + /// Bounded pump-wait for the OnWriteComplete row matching a Write/Write2/ /// WriteSecured/WriteSecured2 call, copied onto the reply when it /// arrives in time. The executor holds the STA thread but pumps /// Windows messages each poll (ReadBulk precedent) so the COM callback @@ -1002,6 +1030,21 @@ public sealed class MxAccessCommandExecutor : IStaCommandExecutor /// statuses — the consumer's unconfirmed path, never a synthesized /// failure. Protocol status/hresult stay acceptance-only either way. /// + /// + /// Correlation is recency-based per handle, not per call: MXAccess's + /// OnWriteComplete callback carries no correlation token, only the server/item + /// handle pair, so this method reports the first completion whose version exceeds + /// as this write's outcome. If an earlier write to + /// the same handle times out unconfirmed and its callback is still undelivered, that + /// stale completion can dispatch during this write's pump-wait and ride this reply + /// instead — 's pre-call pump only folds in a + /// completion already queued before this write started; it cannot fold in one queued + /// during the COM call or the wait itself. The Hresult/ProtocolStatus + /// fields stay acceptance-only regardless, so the exposure is limited to the enrichment + /// statuses field, and it is inherent to the COM surface — a native client + /// correlating by handle has the identical ambiguity. Full closure needs an + /// MXAccess-side correlation token, which does not exist. + /// private void AwaitWriteCompletion( MxCommandReply reply, MxAccessWriteCompletionCache completionCache,