fix: resolve the Rust+ map once per connected window - #87
Merged
Conversation
`GetMonumentsAsync` went straight to the socket, where it is `GetMap` — a full ~683 KB map JPEG download — to read a static monument list. The #map composer calls it on every compose, so it fired once per `MapRefreshInterval` (30s), forever. Measured on a live bot after #85: 10 bursts of ~683 KB over 309s, 77 MB/h, 99.6% of all Rust+ inbound traffic, 1.85 GB/day per server. The list was already in memory and thrown away: `GetRigPositionsAsync` fetches it once per connected window, keeps the two oil rigs and drops the rest. Fixes #86. `GetMap` answers geometry, monuments and the image in one response, so the three per-purpose wrappers on `IRustServerConnection` each paid for the whole JPEG while wanting a third of it. They collapse into one `GetServerMapAsync` returning a `ServerMapSnapshot`, and the three are deleted — leaving them is what invites this regression back. `ServerMapWindowCache` resolves that snapshot once per connected window and serves every reader from it, replacing the `DimensionsHolder` #85 added. Resolution is single-flight: on connect the marker poll and the map service's initial compose both arrive before either resolves, and each miss is a full map download. A failed fetch is not cached, and each reader fetches under its own token, so one reader cancelling never poisons another. The world size that completes `MapDimensions` comes from `GetInfo`, not `GetMap`, so it is resolved separately and retried per read. Folding it into the map fetch would let a transient `GetInfo` rate limit — likely, given the heavy call it follows on connect — latch "no dimensions" for the whole window and leave #map dark until the next reconnect. For the same reason the marker poll now reads dimensions inside its loop rather than latching one value before it. Connected-window cost drops from two `GetMap` round trips to one, and #map refreshes stop fetching entirely: ~120 full-map downloads an hour become one per window, 77 MB/h becomes ~0.3 MB/h. The trade is that a window holds its JPEG until it disconnects, whether or not the guild uses #map — one bounded ~683 KB array per connected server, against a second full download per window if the image were fetched separately. Regression tests (both watched failing first): - GetMonuments_ServesRepeatReadsFromTheConnectedWindow_WithoutRefetching saw 7 map fetches where 2 suffice. - ConnectedWindow_IssuesASingleMapFetch_ForDimensionsMonumentsAndImage saw 4 where 1 suffices. The supervisor's map queries now guard their own degradation: the collapsed socket call throws where the old dimensions and image wrappers swallowed everything, and an escaping exception tears down the consuming render loop for the rest of the process. Their existing tests staged the fault after connect, which the window cache would now serve from cache, so they stage it before connect instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core connected-window socket/query behavior (including caching and concurrency), so a final human review is warranted despite strong test coverage.
Pull request overview
This PR reduces redundant Rust+ socket traffic by collapsing multiple map-related socket calls into a single per-connected-window map snapshot, then serving dimensions/monuments/image from an in-memory window cache (with single-flight resolution and non-cached failures).
Changes:
- Replaced per-purpose socket map accessors with
GetServerMapAsync()returning aServerMapSnapshot. - Introduced
ServerMapWindowCacheto resolve the map once per connected window (single-flight), while resolving world size (GetInfo) separately and retrying it per read. - Updated supervisor/map-query behavior and expanded tests to lock in “single map fetch per window” and degradation semantics.
File summaries
| File | Description |
|---|---|
| tests/RustPlusBot.Features.Connections.Tests/ServerQueryTests.cs | Updates failure staging to occur pre-connect due to connected-window caching. |
| tests/RustPlusBot.Features.Connections.Tests/ServerMapWindowCacheTests.cs | Adds isolated coverage for window cache single-flight, failure behavior, cancellation behavior, and world-size retry. |
| tests/RustPlusBot.Features.Connections.Tests/MapImageQueryTests.cs | Adds regression tests asserting repeat reads don’t refetch and the connected window uses a single map fetch. |
| tests/RustPlusBot.Features.Connections.Tests/Fakes/FakeRustSocketSource.cs | Updates test fake to model unified map fetch (GetServerMapAsync) and count full-map round trips. |
| tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs | Updates timeout staging to the new unified map fetch path. |
| src/RustPlusBot.Features.Connections/Supervisor/ServerMapWindowCache.cs | New per-connected-window cache for map snapshot + dimensions (with world-size retry). |
| src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs | Routes map queries through the window cache and guards render/query seams against map-fetch exceptions. |
| src/RustPlusBot.Features.Connections/Listening/ServerMapSnapshot.cs | New snapshot model for geometry/monuments/image + MapGeometry. |
| src/RustPlusBot.Features.Connections/Listening/RustPlusSocketSource.cs | Implements unified GetServerMapAsync() and removes split map accessors. |
| src/RustPlusBot.Features.Connections/Listening/IRustServerConnection.cs | Replaces three map methods with GetServerMapAsync() contract. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+243
to
+247
| @@ -244,7 +244,7 @@ public async Task Connect_MonumentsTimeout_DoesNotKillLoop_AndStillReconnects() | |||
| { | |||
| var source = new FakeRustSocketSource(); | |||
| source.EnqueueConnect(SocketConnectOutcome.Connected); | |||
| source.TimeoutOnMonumentsOnce(); // the marker poll's rig fetch times out on the FIRST connection only | |||
| source.TimeoutOnMapOnce(); // the marker poll's rig fetch times out on the FIRST connection only | |||
Owner
Author
There was a problem hiding this comment.
Agreed, and the name understated it in both directions: the staged fault sinks the whole map read — geometry, monuments and the image — so the window degrades to no rigs, no grid references and no base map, not just rig detection.
Renamed to Connect_MapFetchTimeout_DoesNotKillLoop_AndStillReconnects and reworded the summary and the inline comment to match in 299e9e6.
`TimeoutOnMapOnce` now sinks the whole map read — geometry, monuments and the image — not just the monument list, so the test's name and summary understated its blast radius and made timeout coverage hard to find by searching. Addresses Copilot review feedback on #87. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HandyS11
added a commit
that referenced
this pull request
Sep 8, 2026
…88) * fix: verify the #info RustMaps render against the server's own world The #info map is resolved from `(worldSize, seed)` alone, and that pair does not identify a Rust world. WEREWOLF GAMING reports size 3700 seed 1900693728 over Rust+ while running a pre-generated level — its `GetInfo.Map` reads `procedural__3700_FHv7dBVBBUOBMxTGE8eiuw`, not `Procedural Map`, so `server.seed`/`server.worldsize` are leftover config values — and RustMaps generated a completely different island, which the bot posted as that server's map for the whole wipe. Measured against the live server: 0 of 8 major monuments (ferry terminal, military tunnels, power plant, airfield, water treatment, excavator, dome, junkyard) land anywhere near their RustMaps counterparts, and only 11% of the 63 monuments it reports have ANY RustMaps monument within 50 m. A server that stays up across a map-gen change drifts the same way. The ground truth was already in memory. `GetMap` returns the server's own monuments and map image, cached per connected window since #87, so `RustMapsMapMatcher` checks each ready render against the monuments its requesting servers actually report before that render is ever shown. It is a position fingerprint, not a name comparison: Rust+ tokens map many-to-one onto `MonumentType` (both harbors collapse to one, swamps and labs match by prefix), so comparing types would report false mismatches. The only wrinkle is the origin — Rust+ counts from the map corner, RustMaps from the world centre. The verdicts sit far apart (identical maps agree to the metre; this one scored 11%), so the thresholds are not delicate. The verdict is per (key, server), not per key: two servers can share a (size, seed) while only one of them runs that world. Undecidable — server offline, no monuments yet — is not a verdict; the render stays withheld and the next tick tries again, so a failed fetch can never latch the wrong map in. A mismatched server gets the map Rust+ itself serves, attached to the message and shown through `attachment://`. That is the first upload the reconciler handles, and it re-renders every pass, so the file is named after a hash of its content: an unchanged name means "already posted, leave it", a changed one means the map itself moved on and only a repost can carry the new file. Discord folds an attachment that an embed references INTO that embed — it rewrites the embed's image URL to the CDN one and returns an EMPTY attachments array. Reading only that array reports "no attachment" for exactly the messages that have one, so the first live run re-uploaded 680 KB every reconcile. `LiveMessage.From` therefore recovers the name from the embed's CDN URL, minus the rotating signature query string. The fake gateway now mirrors that folding; echoing the payload back is what let the first version look correct in tests and churn in production. Committing a RustMaps fixture also woke `RustMapsParityTests`, dormant since it was written for want of one. It failed: it derived grid rows from the south edge while the projection derives them from the north, which disagree whenever the world size is not a whole multiple of the cell size (3700 is not). It now checks the projection against `MapGrid.LabelFor` — the labels the bot actually quotes to players. `tools/RustPlusBot.MapParity` had the same centred-vs-corner bug and was drawing its crosshairs off the icons it claims to land on. Verified on the live bot: the mismatch is detected 20s after connect, the #info map is replaced by an image byte-identical to the one Rust+ serves (sha256 0bcdf958d810…), and it then survives four minutes of reconciles with no repost, no edit and no rate-limit warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: keep editing attachment-backed messages, and thread cancellation Copilot review on #88. `UpToDate` skipped the post/edit step entirely once a live message already carried the payload's upload, which is too broad: the upload is the only part an edit cannot carry. A guild switching culture would keep its map embed in the old language for as long as the image itself did not change — the whole wipe, since the image is static per map. Editing is safe because the edit never mentions attachments and Discord keeps them. Verified against the API rather than assumed: posting an embed that references its upload through `attachment://` and then PATCHing only `embeds` leaves the attachment in place, the embed's CDN image URL unchanged and still fetchable. So the flag goes entirely — a matching file name now just means "edit in place", and only a changed or dropped upload still forces the delete-and-repost that carries the new file. `GetLiveMessageAsync` ignored its cancellation token, so reconcile work could not unwind promptly on shutdown; `PostMessageAsync` and `EditMessageAsync` had the same gap. All three now thread it through `RequestOptions.CancelToken` via one helper, which the already-correct delete path reuses. The attachment tests asserted "never edited", which was the wrong expectation rather than a real guarantee — they now assert "never re-uploaded", and a new test pins the reported scenario: retitle around an unchanged upload, and the message must be edited in place (one post, one edit, new title live) instead of reposted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.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
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.
Fixes #86.
The defect
IRustServerQuery.GetMonumentsAsyncisGetMapon the socket, which returns the entire map JPEG alongside the monument list.MapComposercalls it on every compose andMapHostedServicecomposes once perMapRefreshInterval, so a ~683 KB download fired every 30s, forever, to read a static list.Measured on a live bot after #85 (
develop@ ff7ef7f): 10 bursts over a 309s window, one per 30.9s, 7,016,744 B total — 77 MB/h, 99.6% of all Rust+ inbound traffic, 1.85 GB/day per server. The list was already in memory and thrown away:ConnectionSupervisor.GetRigPositionsAsyncfetches it once per connected window, keeps the two oil rigs and drops the rest.The fix
GetMapanswers geometry, monuments and the image in one response, so the three per-purpose wrappers onIRustServerConnectioneach paid for the whole JPEG while wanting a third of it. They collapse into oneGetServerMapAsyncreturning aServerMapSnapshot, and the three are deleted — leaving them is what invites this regression back.ServerMapWindowCacheresolves that snapshot once per connected window and serves every reader from it, replacing theDimensionsHolder#85 added. Resolution is single-flight: on connect the marker poll and the map service's initial compose both arrive before either resolves, and each miss is a full map download. A failed fetch is not cached, and each reader fetches under its own token, so one reader cancelling never poisons another.Correctness rests on the window boundary, same invariant as #85: the map is fixed for a wipe, and the window is torn down and re-resolved on reconnect — exactly when a new map can appear.
The world size is resolved separately, on purpose
MapDimensionsneedsMapSize, which comes fromGetInfo, notGetMap. Folding it into the map fetch would let a transientGetInfofailure — a rate limit on the connect burst, likely given the heavy call it immediately follows — latch "no dimensions" for the whole window and leave #map dark until the next reconnect.ServerMapSnapshottherefore carries aMapGeometry, and the cheap half is resolved separately and retried per read, costing no map download.For the same reason
PollMarkersAsyncnow reads dimensions inside its loop. It previously captured one value before the loop, so a failed connect-time resolve dropped grid references and rig detection for the entire connection — a pre-existing latch this change sits directly on top of.Impact
Connected-window cost also drops from two
GetMapround trips to one (GetMapImageAsyncforBaseMapCache+ the monuments fetch for rigs), which is the follow-up floated in the issue.Trade-off worth a second opinion
A window now holds its JPEG until it disconnects, whether or not the guild uses #map — one bounded ~683 KB array per connected server, released on disconnect. The alternative is fetching the image separately when #map first asks, costing a second full map download per window. I took retention and documented it on the class; happy to switch if the LOH pressure is the bigger worry.
Tests
Both regression tests were watched failing first:
GetMonuments_ServesRepeatReadsFromTheConnectedWindow_WithoutRefetching— saw 7 map fetches where 2 suffice.ConnectedWindow_IssuesASingleMapFetch_ForDimensionsMonumentsAndImage— saw 4 where 1 suffices.ServerMapWindowCacheTestscovers single-flight collapse, failed fetches not being cached, a cancelled reader leaving the window resolvable, and the world-size retry (mutation-tested: re-introducing the latch fails it).The supervisor's map queries now guard their own degradation — the collapsed socket call throws where the old dimensions and image wrappers swallowed everything, and an escaping exception tears down the consuming render loop for the rest of the process. Their existing tests staged the fault after connect, which the window cache would now serve from cache, so they stage it before connect instead.
Release build clean;
dtk dotnet test RustPlusBot.slnx: 1413 passed, 1 skipped;jb cleanupcodeproduces no diff.🤖 Generated with Claude Code