fix(gateway): owner-only engine bot decisions, and ENGINE_BOT in Helm - #65
Merged
Merged
Conversation
…e ENGINE_BOT in Helm Every gateway replica with a session in a bot game ran an EngineBotMover that chose moves from its own cached GameAuthority. A non-owner's copy never sees the owner's moves, so it asked the engine about an old position on every broadcast and routed the answer to the owner, which applied it whenever it was legal (reproduced: ...Nf6 applied at ply 4, computed for ply 1). That made per-replica ENGINE_BOT unsafe under the chart's default two gateway replicas. - RedisCommandRouter.prepareOwnership: the router's ownership step (valid lease or claim, then the takeover reload) without applying a command. holdsOwnership: a synchronous valid-lease-and-no-reload-debt check. - EngineBotMover asks the engine only after prepareOwnership succeeds, and submits only if it still owns the game and the bot is still to move at the same ply and FEN. An `ended` broadcast unregisters the game on every replica. - Helm: gateway.engineBot.enabled (default true) renders ENGINE_BOT="1" on the gateway. Stockfish still comes only from the gateway image. - Tests: scripted-gate unit tests; real-Redis two-node tests with the production router and registry; Helm snapshot pins enabled, disabled, per strategy, and the rest of the gateway env contract.
…b/sub The two simulated replicas shared one in-memory bus, so broadcasts crossed between them synchronously. Each node now uses the production createRedisPubSub, as serve.ts wires it, so the other node hears a broadcast asynchronously through Redis. Negative checks watch a named 500 ms window, the terminal test waits for both movers to let go, and the non-owner test asserts the exact number of ownership checks.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
PR Summary by QodoMake engine bot decisions owner-only and enable Helm parity
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
1.
|
 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…t-parity # Conflicts: # docs/PROJECT_STATE.md
- Reload debt belongs to the claim that recorded it. rehydrateIfStale tracked in-flight reloads by game only, so a slow reload begun for an earlier claim could finish after a later claim and mark a copy that predates another owner's moves as fresh. Claims now get a process-wide, never-reused number; only a reload for the current claim settles the debt. prepareOwnership re-checks the lease after reloading. (Qodo) - A result dropped by the post-engine check is followed by one more pass, so a lease that lapses mid-think no longer strands the bot's turn. (Greptile P1) - A copy that already shows the game over unregisters before any ownership traffic, and a non-owner re-checks every lease TTL, reloading its copy from the event log: a game whose owner died is claimed and continued, and a game whose ended broadcast was lost is unregistered. (Qodo x2) - Tests: lapsed-lease re-run, orphaned-game re-check, missed-ended cleanup, finished-game join, claim-generation race and dead-owner takeover on real Redis. Test engine fakes yield like real I/O. A test-only Redis preflight fails the multi-node file in seconds when Redis is unreachable; the clients under test keep production retry settings. - ADR-0080 amendment and PROJECT_STATE Increment 69 describe the final behaviour and its remaining limits.
…w notes - OwnershipRegistry recorded a SET NX claim as continuing ownership whenever a failed renewal had left the game in its owned set. If the lease had lapsed and another node advanced the game in between, the re-claim fired no onClaimed, so nothing reloaded and the node acted on a stale copy (route() and the bot alike). A SET NX that creates the key is now always a new claim; renewing a key still held is not. Found by the independent Gemini review. - stillCurrent drops its turn check: the same ply and FEN already encode the side to move, so the clause could never fail on its own. Its comment now says the apply can queue behind an earlier append and that the event log's expected-sequence check is the last line of defence. - Helm refuses a non-boolean gateway.engineBot.enabled: a quoted "false" is truthy and would have left the bot on. Snapshot pins the rejection. - Tests: real-Redis re-claim after an unnoticed lapse; wider negative window and wait deadlines for slow runners. - Docs: ADR-0080 wording (reload ordering, append check, costs of non-owner re-checks, the pre-existing route() lease re-check left as follow-up), chart README engine footprint, RUNNING.md prose, PROJECT_STATE Increment 69.
…ten docs - Real-Redis test: a replica whose copy still shows a game in progress claims it after the owner resigned the game and died; only the post-claim reload shows the ending, and the mover must unregister without an engine call. The post-reload `status.over` check had no test isolating it. - Docs, from the final architecture review: the rehydrateIfStale comment and ADR-0080 say the new reload starts at once and queues on the game's lock (for a cached game, which is every path here); chart README corrects the engine footprint (no worker until the first bot move; a 300 ms time-limited search means a throttled pod plays weaker, not slower); ADR-0010 notes the changed claim meaning; RUNNING.md no longer says the failure is silent; ADR-0080 and PROJECT_STATE list the new tests and eleven mutation checks.
Owner
Author
|
/review |
|
Code review by qodo was updated up to the latest commit fc810f4 |
…s leave Qodo and Greptile (on fc810f4): a non-owner's re-check reloaded the game from the event log every lease TTL, and a registration outlived the session that caused it, so one spectator visit made that replica replay a game's full history every TTL until the game ended. - RealtimeGateway gains an optional onGameUnloaded hook, fired when the last session in a game leaves a node (the pair of onGameLoaded). - The mover unregisters there unless the registry still lists the game as this node's (claimedHere, true even while renewals fail): a non-owner exists only to take over for a local player; the owner keeps the game for a player who reconnected through another replica. - The non-owner re-check no longer reloads: it is one ownership attempt, and a successful claim reloads through the router's takeover path. - Tests: session-leave rule (unit and real Redis, where the owner still moves for a player on the other replica) and the gateway hook. Mutations M1-M7 and M9-M13 rerun one at a time on this code; each is caught. - Docs: ADR-0080 and PROJECT_STATE updated; RUNNING.md now says exactly what is logged in each misconfiguration; chart README no longer claims memory headroom from hash size alone; claim() doc comment moved back to claim().
Owner
Author
|
/review |
|
Code review by qodo was updated up to the latest commit 1354e38 |
…ot a snapshot Found on 1354e38 by the architecture and concurrency reviews, Qodo and Greptile: localSessionsGone unregistered when the registry did not yet list the game, but a claim could be in flight (the mover's own prepareOwnership, or route() for a command sent just before disconnecting). The claim then succeeded, leaving an owner that renews the lease forever with no mover: the bot never moved again and no other replica could claim the game. - localSessionsGone now only marks the game as having no local player and takes a pass; the game is dropped when, and only when, a pass fails to own it. A join clears the mark. A claim in flight that succeeds keeps the game, and a former owner lets go on its first failed claim. claimedHere/listsAsOwned are gone. - The non-owner re-check reloads its copy again, so a connected non-owner that missed `ended` lets go within one lease TTL (Greptile). Registrations are now bounded by local sessions, so the replay runs only while someone on that replica is in the game. - Tests: claim in flight at session leave, former owner, rejoin, and the restored missed-ended re-check. Fifteen mutations, each introduced alone on the compiled build, are all caught. - Docs: ADR-0080 and PROJECT_STATE (scope now names packages/realtime-gateway).
Owner
Author
|
/review |
|
Code review by qodo was updated up to the latest commit 84bba34 |
…redis's ready check CI run 36203300566 attempt 1 failed "a finished game stops bot work on every replica" (timed out waiting for both movers to let go) after ioredis logged "Connection in subscriber mode, only subscriber commands may be used" from _readyCheck. ioredis writes SUBSCRIBE during the `connect` phase (it carries Redis's `loading` flag), so a subscription made within milliseconds of connecting switches the connection to subscriber mode before the ready check's INFO, which is then refused; ioredis resets the connection and the `ended` broadcast published in that gap is lost. Test nodes subscribe right after connecting, unlike a gateway whose rooms open long after startup, and a loaded runner widens the gap (the attempt-2 re-run passed; it never reproduced locally). The harness now passes enableReadyCheck: false to its nodes' createRedisPubSub; production pub/sub code and settings are unchanged.
Owner
Author
|
/review |
|
Code review by qodo was updated up to the latest commit 37fdfac |
…nnection Greptile (on 37fdfac): the multi-replica suite avoided the early-subscription failure by disabling the ready check in the test only, while gateways run with it enabled and do subscribe at startup (terminal-event workers). ioredis writes SUBSCRIBE during the handshake (it is allowed while Redis loads), so a subscription made right after connecting — startup, a reconnect — switches the connection to subscriber mode before the ready check's INFO, which Redis refuses; ioredis resets the connection and drops what was published meanwhile. createRedisPubSub now disables the check on the subscriber connection only (it waits for dataset loading, which pub/sub does not need); the publisher keeps it. The test override is removed, so the suite runs the production configuration. Also, from the final delegated reviews on 37fdfac: - Test: a claim still in flight when the last local session leaves, which then fails, lets the game go (the in-flight case only covered success before). - Docs: a pass that fails outright (Redis or the event log unreachable) schedules a re-check regardless of local players; ADR-0080, PROJECT_STATE and the scheduleRecheck comment now say so.
Owner
Author
|
/review |
|
Code review by qodo was updated up to the latest commit e728015 |
…e ADR-0008 From the delegated reviews of e728015: - Test quality: the subscriber ready-check fix had no direct test (the race appeared once on CI and does not reproduce on demand). createRedisPubSub now builds its options through an exported pubSubConnectionOptions, and redis-pubsub-options.test.ts pins that only the subscriber skips the ready check, that the publisher keeps ioredis's default, that caller options pass through, and that a caller cannot re-enable the check on the subscriber. Removing the subscriber option fails both tests. - Architecture (LOW): ADR-0008 now points to the ADR-0080 amendment.
Owner
Author
|
/review |
|
Code review by qodo was updated up to the latest commit a38b9ce |
This was referenced Sep 26, 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
1 participant
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.
Problem
The Helm chart never rendered
ENGINE_BOT, so a Helm release offered Play vs Computer with an opponent that never moved (audit Ops row). Compose enables it, and the gateway image already ships the pinned Stockfish andSTOCKFISH_PATH.Turning it on per replica was not safe on
main, and the chart defaults togateway.replicas: 2. Every replica with a session in a bot game ran anEngineBotMoverthat chose moves from its own cachedGameAuthority. A non-owner's copy never sees the owner's moves, which reach it as room broadcasts that never touch the authority. A reconnect to the other pod, a second tab, or a spectator was enough to make that replica ask the engine about an old position on every broadcast and route the answer to the owner. The owner applied it whenever it was legal. Reproduced with two nodes: the owner applied…Nf6at ply 4, computed by the non-owner for ply 1.Change
Gateway: only the owner decides
RedisCommandRouter.prepareOwnership(gameId)is the router's existing ownership step (valid lease, orclaim, then the existing takeoverrehydrateIfStale) with no command applied.holdsOwnership(gameId)is a synchronous check: valid lease and no reload debt. No lease logic is reimplemented.EngineBotMovercallsprepareOwnershipbeforeprovider.play(). A non-owner returns without an engine call. An unowned game (bot as White at ply 0, or an owner that has gone away) is claimed and rehydrated from the event log before its position is read.holdsOwnershipis still true and the local state still has the bot to move at the same ply and FEN. Otherwise the result is dropped, and the re-run queued by the broadcast that changed the game computes from the current position. The check androute()have no I/O between them. While the lease is valid no other node can own the game, and in the checked position only the bot has a legal move. No wire-protocol change.endedbroadcast unregisters the game on every replica. A non-owner's copy never reachesover.LocalCommandRouter): no ownership gate, and the post-engine position check still applies.Helm
gateway.engineBot.enabled(defaulttrue) rendersENGINE_BOT="1"on the gateway Deployment only;falseremoves it.STOCKFISH_PATHcome only from the gateway image. The chart adds no engine configuration.main: rolling, blue/green and canary differ only by that one variable. With the bot disabled, the render is byte-identical tomain.Review-round fixes (Qodo, Greptile, independent Gemini review)
rehydrateIfStale(shared withroute()) tracked in-flight reloads by game only: a slow reload begun for an earlier claim could land after a later claim and mark a copy that predates another owner's moves as fresh. Claims now get a process-wide, never-reused number; only a reload for the current claim settles the debt.prepareOwnershipre-checks the lease after reloading.SET NXre-claim is a new claim. When a renewal threw,OwnershipRegistrykept the game in its owned set, so a laterSET NXafter the lease had lapsed (and another node had advanced the game) fired noonClaimedand nothing reloaded. ASET NXthat creates the key is now always a new claim. This predates the PR and affectedroute()too.endedbroadcast was lost is unregistered. A copy that already shows the game over unregisters before any ownership traffic.RealtimeGatewayhookonGameUnloaded). When a game's last session on a node leaves, the mover marks it and takes a pass, and drops it only when a pass fails to own it (a join clears the mark). So a non-owner lets go, a claim still in flight that succeeds keeps the game (no ownerless-mover stall), a former owner lets go on its first failed claim, and the owner keeps the game for a player who reconnected elsewhere. Re-checks and their replays run only while someone on that replica is in the game. (Qodo/Greptile performance finding on fc810f4; race found on 1354e38 by Qodo, Greptile and the delegated reviews.)gateway.engineBot.enabled(a quoted"false"is truthy and would leave the bot on).Docs: ADR-0080 amendment (design, costs, remaining limits), ADR-0010 cross-reference,
docs/RUNNING.md, chart README (engine footprint), PROJECT_STATE Increment 69 (Increment 68 belongs to #64 and is untouched).Proof
services/gateway/test/engine-bot-multinode.integration.test.tsruns on real Redis (CIgateway-servicejob, zero-skip). It uses the productionRedisCommandRouter,OwnershipRegistryand per-nodecreateRedisPubSub, and covers:provider.play(), including on duplicate broadcasts; the non-owner holds a stale bot-to-move copy and never computes from it;engine-bot.test.tscovers the same gate and post-engine checks with a scripted gate, including the game advancing or ending mid-think.endedunregister, 3; the reload-debt clause, 1.scripts/helm-snapshot-test.shpins:ENGINE_BOTon by default, off when disabled, and still on under blue/green and canary;STOCKFISH_*env anywhere;falsefails 6 of these checks.npm test34/34 with no skips on Redis 7;helm lintpasses; kubeconform passes for the default, disabled, external-datastore, external-secrets, blue/green and canary renders; snapshot test 116/116.Not in scope
BOT_AUTO_ANALYZE, ANTICHEAT_AUTO_ANALYZE, the tournament reporter, the search indexer, the games projection, ratings, timed games and auth are untouched. There is no migration.
Merged latest
main(#64) normally; this PR is Increment 69.Mutation evidence (final code, 84bba34): fifteen deliberate defects were each introduced alone, against the compiled test build, one process at a time, and each failed named tests: pre-engine gate, post-engine check,
endedunregister, reload-debt clause, claim-generation check, re-run after drop, non-owner re-check, reload before re-check, finished-copy unregister, post-reload finished unregister,SET NXnew-claim rule, gateway unload hook, and the session-leave rule three ways (never releasing, releasing at once regardless of the claim, a join not clearing the mark). The multi-node suite fails in seconds by name when Redis is unreachable (test-only preflight; production retry settings unchanged).