fix(#790): raise default FIXP keep-alive interval so reattach can survive a real disconnection - #792
Open
pedrosakuma wants to merge 6 commits into
Open
pedrosakuma wants to merge 6 commits into
pedrosakuma wants to merge 6 commits into
Conversation
The network-partition scenario holds a 10s partition expecting FIXP's Establish-reuse reattach to recover the session (per spec, a brief blip should never need a peer-forced session roll). But chaos-drill.yml stacked docker-compose.real-conformance.yml on top of the real stack, which swaps matching-platform's bridge config to the fast conformance profile (heartbeatIntervalMs=idleTimeoutMs=500ms) used by the SessionRoll conformance spec to force a roll in under a second. With that profile mounted, the venue always reaps the session (~1.5s) long before a 10s partition heals, guaranteeing a Renegotiated roll (and the resulting Ambiguous, permanently-unretried outbound mutations) on every single run — this is a timer/scenario-duration mismatch in the CI workflow, not a FIXP or reconnect-logic defect. Drop the real-conformance.yml overlay from every compose invocation in this workflow so trading-host boots against the base stack's production-like bridge (docker/real/exchange-simulator.bridge.json, heartbeat/idle=30s), which comfortably outlasts the 10s partition and lets the scenario exercise genuine reattach recovery. Refs #790. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The drill's assert_post_recovery_trade() crosses Buy+Sell from the same end-client (alice) at the same price, which the default-on self_trade_prevention (and, for the Sell leg, AllowShortSell) gate rejects. docker-compose.real-conformance.yml used to grant this same opt-out (for the unrelated ReferencePriceLiveSpec) as a side effect of an overlay that also swapped in fast conformance FIXP timers — removed in the previous commit precisely because that timer swap caused #790. Add a minimal docker-compose.chaos.yml overlay that grants only the risk opt-out alice needs, without touching matching-platform's bridge/timers, so trading-host keeps the production-like keepalive values needed for the network-partition scenario to exercise a genuine FIXP reattach. Refs #790. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The venue honors the client-negotiated keepAliveInterval (not its own bridge default) and terminates the session as idle after ~3x that interval of silence. The FIXP spec floor of 1000ms produced a ~3s terminate threshold, far shorter than a realistic reconnect window, forcing a spurious session roll (Renegotiate -> Ambiguous mutation reclassification) on any brief disconnection instead of a clean Reattach. 15s gives ~45s of tolerance, comfortably surviving the chaos drill's 10s network-partition scenario and realistic real-world blips while still detecting genuinely dead sessions promptly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…indows for new KeepAliveIntervalMs default The venue terminates a FIXP session as idle after ~3x the client- negotiated KeepAliveIntervalMs of inbound silence, NOT SuspendedTimeoutMs (5 min) as these tests' names imply -- these tests were (accidentally) exercising the keepalive-lapse boundary because the pre-#792 default (1000ms) made that boundary far tighter (~3s) than the venue's actual SuspendedTimeoutMs mechanism. After #792 raised the default to 15000ms (~45s threshold), the prior PastWindowDisconnect=5000ms constant no longer exceeded the venue's idle-terminate threshold, so PastSuspendedTimeout_Renegotiates_SurvivingOrderFlaggedStale timed out waiting for a roll that never happened (session simply reattached, sessionVerId stayed at 1) -- confirmed by the real-stack CI run on PR #792. The stuck (never-rolled) order then broke that test's cleanup path (which assumed a rolled/Ambiguous order needing VenueAbsent resolution), leaving a stray resting order that cascaded into failures on unrelated later specs sharing the same real-stack sandbox. Bump WithinWindowDisconnect 1000ms -> 5000ms and PastWindowDisconnect 5000ms -> 50000ms so both stay correctly positioned relative to the new ~45s venue-side threshold. Refs #790 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The CI run on b27e676 showed WithinSuspendedTimeout_Reattaches_OrderSurvivesNoStaleFlag newly failing at cleanup after bumping WithinWindowDisconnect 1000ms -> 5000ms. That change was speculative and unnecessary -- 1000ms was already comfortably under both the old (~3s) and new (~45s) venue idle-terminate threshold, so there was no reason to touch it. Revert to isolate the fix to the one constant that actually needed recalibrating (PastWindowDisconnect) and narrow the diff for the next CI iteration. Refs #790 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…vered Per code review: disconnectStartedUtc was captured before awaiting docker.DisconnectMatchingAsync(), which spawns several sequential docker subprocesses (docker availability check, network inspect, network disconnect) before the partition takes effect. Timing the WithinWindowDisconnect/PastWindowDisconnect budget from before that setup latency understates the real elapsed idle time the venue observes, eroding the margin against its ~3x KeepAliveIntervalMs idle-terminate threshold (45s post-#792) -- risking exactly the flake (session reattaches instead of rolling) this fix targets. Move the timestamp capture to after DisconnectMatchingAsync() returns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Fixes the 100%-reproducible
network-partitionchaos-drill failure (#790) by raising the default FIXPKeepAliveIntervalMsfrom 1000ms to 15000ms, plus two related workflow/compose corrections found along the way.Root cause
The venue (B3MatchingPlatform) honors the client-negotiated
keepAliveInterval, not its own bridge config, and terminates a FIXP session as idle after ~3x that negotiated interval of silence.ExchangeOptions.KeepAliveIntervalMs(trading-host) defaulted to the FIXP spec floor of 1000ms, giving a ~3-second venue-side terminate threshold in every environment. Any disconnection longer than ~3s — including the chaos drill's 10snetwork-partitionscenario, or a realistic production network blip — always got evicted server-side, forcingUNNEGOTIATED→ fresh Negotiate →Renegotiated→ in-flightPendingNewmutations reclassifiedAmbiguous(manual reconciliation required).This confirms @pedrosakuma's original read: disconnection duration alone should never force a session roll — FIXP's Establish-reuse/Reattach should absorb a brief blip. The defect wasn't in the reconnect logic; it was a keepalive interval configured far too aggressively for reattach to ever get a chance to work.
Changes
backend/src/B3.Trading.Infrastructure/ExchangeOptions.cs: defaultKeepAliveIntervalMs1000 → 15000 (comment explains the venue-side ~3x terminate math)..github/workflows/chaos-drill.yml: stop stackingdocker-compose.real-conformance.yml(a different conformance spec's fast-timer bridge profile: 500ms heartbeat/idle) onto the chaos drill's real stack — unrelated to the root cause but a real hazard for this workflow regardless.docker/docker-compose.chaos.yml(new): minimal overlay granting alice'sAllowShortSell/AllowSelfTraderisk opt-outs (previously an incidental side effect ofreal-conformance.yml) so the drill's same-user Buy+Sell self-cross assertion still passes now thatreal-conformance.ymlis no longer stacked.Validation
Live-dispatched
network-partitionscenario runs on this branch (no local Docker in this environment):KeepaliveIntervalLapsed→UNNEGOTIATED→Ambiguous, orders stuck).KeepAliveIntervalMsbump): still failed — same signature, proving the compose changes alone weren't the root cause.KeepAliveIntervalMsbump: PASS x2 — runs 33749020448 and 33749424888. Both baseline and post-recovery trades filled cleanly through the partition/reconnect cycle.Closes #790
Protocol evidence
This PR only changes our own client-side config default (
ExchangeOptions.KeepAliveIntervalMs) within the already-existing, unchanged FIXP negotiation/clamp behavior on the venue side (EstablishValidator.csin B3MatchingPlatform, read directly to determine the correct default — not modified, and no new template/field/enum is introduced or assumed).