marketbyorder-bot: track Reset Count per channel - #41
Open
armcconnell wants to merge 2 commits into
Open
Conversation
Port of the market-by-price bot fix. Reset Count is per publisher, and a group can carry two publishers interleaved on the same ports, so two steady-but-different values must not be read as a reset. Key resetCount by channel_id and scope the wipe to the resetting channel. This shard also owns snapCtx, keyed by (channel, snapshot_id), so resetChannel clears that too.
…l explicit in test
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.
Completes the design in #39. Ports the fix merged for the sibling bot in #38.
Problem
A group can carry two redundant publishers interleaved on the same ports, distinguished only by
channel_id. Reset Count is per publisher and stays constant while neither is resetting — so two steady-but-different values are the normal state, not a reset.Coordinatorheld it as a singleuint8, so every alternation between the two publishers tripped the reset barrier and wiped all shard state. On the sibling bot this produced 1.35M spurious barriers, wiping refdata faster than it could be relearned and leaving effectively every book read-out with an empty symbol.This lane is not currently exposed to it — no market-by-order feed is published today — so this is a latent fix, applied so the bug cannot surface later.
Change
Same shape as #38: key
resetCountbychannel_id, and scope the wipe to the resetting channel.Shard.reset()becomesresetChannel(ch), deleting only keys whose channel matches.The one structural difference from the sibling: this shard also owns
snapCtx, keyed bysnapKey{ch, snap}, and the coordinator ownssnapshotRouteon the same key. Both are pruned per channel rather than reassigned wholesale — the sibling has no equivalent maps, so this part has no counterpart in #38.Every shard is still drained on a barrier: ordering the wipe after all in-flight records is the barrier's job, and any shard may hold records for the resetting channel.
Tests
Two regression tests mirroring #38, both confirmed failing first — and failing for real reasons, not just a build error:
keepassertions fail, because the oldreset()reassigned all four maps)The test harness applies the wipe before acking, so the "did not survive" assertions exercise a real deletion rather than just an ack.
go vetandgo test -raceare clean, verified independently of the implementing agent.Notes
Coordinator.seqLastis written on every record and never read. Dead bookkeeping, deliberately left alone.metrics.goandshard_test.goare unformatted onmainalready; left untouched so the diff stays readable rather than burying the change in reformatting.SnapshotWriter.Reset, whosedirtymap is keyed by instrument id with no channel component. So one channel's reset still drops the other channel's pending snapshot work. The effect is a transient skipped snapshot tick that self-heals on the next update for that instrument — not the data-loss class this PR fixes — and it is structurally identical to the merged sibling. Channel-scopingdirtywould need its own change.