#6501 Prune connections map on game teardown - #6548
Merged
matthewevans merged 1 commit intoAug 1, 2026
Merged
Conversation
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
Contributor
|
Important Review skippedToo many files! This PR contains 860 files, which is 560 over the limit of 300. To get a review, narrow the scope: Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (155)
📒 Files selected for processing (860)
You can disable this status message by setting the 📝 WalkthroughWalkthroughBackground maintenance now removes ChangesConnection Map Cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
matthewevans
requested changes
Jul 23, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Verdict: blocked — the teardown cleanup has no discriminating regression test.
🔴 Blocker
crates/phase-server/src/main.rs:1106 and :1147 remove expired game codes from the connections map, but this PR adds no test that drives either expiry path and proves the entry is removed while another game’s connection remains. The author’s cited compile check cannot fail when either conns.remove(game_code) is reverted.
Suggested fix: add a focused server/lifecycle test that seeds two connection-map entries, exercises both relevant teardown routes, and asserts only the expired game’s entry is pruned.
✅ Clean
The cleanup follows the existing state → connections → spectators lock order and leaves the send-before-removal behavior intact.
Recommendation: add the discriminating teardown regression test, then return for approval.
Member
|
The requested regression coverage from the current review is still outstanding on |
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| key: scryfall-${{ steps.cache-keys.outputs.day }} | ||
|
|
||
| - name: Download preview inputs | ||
| uses: actions/download-artifact@v4 |
There was a problem hiding this comment.
P1: actions/download-artifact@v4 has 5 supply-chain risks
Superagent correlated 5 risks for this dependency. Top signal: Detects suspicious autostart persistence mechanisms. Location: download-artifact-d3f86a106a0bac45b974a628896c90dbdf5…
Review actions/download-artifact@v4 before merging. Remove or replace it if this behavior is unexpected.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name=".github/workflows/deploy.yml">
<violation number="1" location=".github/workflows/deploy.yml:332">
<priority>P1</priority>
<title>actions/download-artifact@v4 has 5 supply-chain risks</title>
<evidence>Superagent correlated 5 risks for this dependency. Top signal: Detects suspicious autostart persistence mechanisms. Location: download-artifact-d3f86a106a0bac45b974a628896c90dbdf5c8093/dist/index.js. Rules: threat-filesystem-autostart. Rules: threat-filesystem-autostart, threat-runtime-obfuscation-base64exec, threat-runtime-obfuscation-steganography
Dependency path: .github/workflows/deploy.yml → jobs → card-data → steps → 5 → actions/download-artifact</evidence>
<recommendation>Review actions/download-artifact@v4 before merging. Remove or replace it if this behavior is unexpected.</recommendation>
</violation>
</file>
| '@tailwindcss/vite': | ||
| specifier: ^4.2.1 | ||
| version: 4.3.0(vite@6.4.3(@types/node@25.6.2)(jiti@2.7.0)(lightningcss@1.32.0)(terser@5.48.0)) | ||
| version: 4.3.0(vite@6.4.3(@types/node@25.6.2)(jiti@2.7.0)(lightningcss@1.32.0)(terser@5.49.0)) |
There was a problem hiding this comment.
P1: terser@5.49.0 has 2 supply-chain risks
Superagent correlated 2 risks for this dependency. Top signal: Detects base64 decoding followed by code execution. Location: package/dist/bundle.min.js. Rules: threat-runtime-obfu…
Review terser@5.49.0 before merging. Remove or replace it if this behavior is unexpected.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="client/pnpm-lock.yaml">
<violation number="1" location="client/pnpm-lock.yaml:38">
<priority>P1</priority>
<title>terser@5.49.0 has 2 supply-chain risks</title>
<evidence>Superagent correlated 2 risks for this dependency. Top signal: Detects base64 decoding followed by code execution. Location: package/dist/bundle.min.js. Rules: threat-runtime-obfuscation-base64exec. Rules: threat-runtime-obfuscation, threat-runtime-obfuscation-base64exec
Dependency path: client/pnpm-lock.yaml → terser</evidence>
<recommendation>Review terser@5.49.0 before merging. Remove or replace it if this behavior is unexpected.</recommendation>
</violation>
</file>
| with: | ||
| cache-shared-key: rust-tool | ||
|
|
||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
P1: actions/setup-node@v4 has 6 supply-chain risks
Superagent correlated 6 risks for this dependency. Top signal: Detects suspicious autostart persistence mechanisms. Location: setup-node-49933ea5288caeca8642d1e84afbd3f7d6820020/d…
Review actions/setup-node@v4 before merging. Remove or replace it if this behavior is unexpected.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name=".github/workflows/deploy.yml">
<violation number="1" location=".github/workflows/deploy.yml:313">
<priority>P1</priority>
<title>actions/setup-node@v4 has 6 supply-chain risks</title>
<evidence>Superagent correlated 6 risks for this dependency. Top signal: Detects suspicious autostart persistence mechanisms. Location: setup-node-49933ea5288caeca8642d1e84afbd3f7d6820020/dist/cache-save/index.js. Rules: threat-filesystem-autostart. Rules: capability-filesystem-read, threat-filesystem-autostart, threat-filesystem-read, threat-runtime-obfuscation-base64exec, threat-runtime-obfuscation-steganography
Dependency path: .github/workflows/deploy.yml → jobs → preview-inputs → steps → 2 → actions/setup-node</evidence>
<recommendation>Review actions/setup-node@v4 before merging. Remove or replace it if this behavior is unexpected.</recommendation>
</violation>
</file>
Superagent Supply Chain ScanSuperagent flagged 2 dependencies introduced by this pull request. High risk:
|
matthewevans
force-pushed
the
fix/prune-connections-map-740b
branch
from
August 1, 2026 16:53
7e6226a to
867e448
Compare
matthewevans
requested changes
Aug 1, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Request changes — current head 867e44871482a6901ea4b05a16f606016cbc268b still lacks a regression that reaches either teardown integration.
prune_game_connections is correct in isolation, and the two new tests prove that helper removes only the requested key. They do not invoke the disconnect-grace expiry or lobby-expiry paths where the helper is wired in (crates/phase-server/src/main.rs:1329-1362 and :1396-1400). Revert either of those two call sites and both tests still pass, so the leak this PR fixes can return unnoticed.
Please add focused coverage that exercises each teardown route (or factor the route-owned cleanup into a testable unit that both production paths call) with an expired and a surviving game, then assert only the expired connection entry is removed. Keep the existing helper tests as the low-level boundary checks.
matthewevans
force-pushed
the
fix/prune-connections-map-740b
branch
from
August 1, 2026 17:26
867e448 to
8fa62b3
Compare
matthewevans
approved these changes
Aug 1, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Maintainer completion verified on 8fa62b3: current CI, including the security scan, is green. The remaining security threads are stale findings on the pre-rebuild merge-history commit; the rebuilt server-only head no longer contains those dependency changes.
matthewevans
requested changes
Aug 1, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Request changes — current head 8fa62b3db2c6dc67c8be4ffa664c4b48bdacfa42 still lacks a discriminating regression for the production teardown wiring.
prune_game_connections is correct in isolation, and the two added tests prove that helper removes only the requested key. But neither invokes the disconnect-grace expiry route or the lobby-expiry route where that helper is wired (crates/phase-server/src/main.rs:1353 and :1399). Revert either production invocation and both tests remain green, so the completed-game connection leak can return unnoticed.
Please add focused coverage that exercises each teardown route, or factor route-owned cleanup into a testable unit that each production route calls, with an expired and surviving game and an assertion that only the expired connection entry is removed. Keep the existing helper tests as low-level boundary coverage.
The unresolved Superagent supply-chain threads are outdated and refer to files absent from this rebuilt server-only diff; they do not block this review.
matthewevans
approved these changes
Aug 1, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Re-approving the unchanged rebuilt head after queue revalidation: current CI and security scan remain green; the historical scanner findings are stale pre-rebuild threads.
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 #6501: the in-memory
connectionsmap (crates/phase-server/src/main.rs) was inserted into on every host / join / reconnect but agame_codeentry was never removed, unlike the parallelgame_spectatorsmap which is pruned at the same teardown points. This leaks oneconnectionsentry per finished game.Root cause
connections: HashMap<String /*game_code*/, HashMap<PlayerId, Sender>>grows without bound: every completed or expired game leaves a permanentgame_code -> {seat -> dead Sender}entry.MAX_GAMESbounds only concurrent live sessions, not this map, so a long-running server accumulates one dead entry per game played.Fix
Mirror the existing
game_spectatorscleanup —connections.remove(game_code)in both background-reaper teardown loops:let conns→let mut connsand remove after the GameOver fan-out),connectionsprune block).Lock order is preserved throughout (
state -> connections -> spectators). No behavior change beyond releasing the dead entry.Files changed
crates/phase-server/src/main.rs—+14 / -1.Verification
cargo fmt --all— clean.cargo check -p phase-server— clean (compiles, borrow-checks). The change sits inside async websocket teardown paths that are not unit-testable in isolation; correctness follows from mirroring the adjacent, already-establishedgame_spectatorscleanup and preserving lock order.Summary by CodeRabbit