Batch move_warship intents to stay under MAX_INTENT_SIZE - #4735
Batch move_warship intents to stay under MAX_INTENT_SIZE#4735bighurdan-cell wants to merge 5 commits into
Conversation
Large fleet selections produced a single move_warship intent frame larger than the server's 2000-byte cap, which kicks the client. Split fleet orders client-side into batches that each fit in one intent, and share the MAX_INTENT_SIZE constant between client and server.
The issue lists bounding the array as an alternative to batching; 1000 sits above the 964 ids a single batch can physically hold. The kick reason had no en.json entry, so players saw the raw translation key.
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughLarge warship selections are split into size-bounded intents and sent through a paced queue for remote games. The shared intent-size limit is used by batching and server rate limiting, with tests covering batching, serialization, schema validation, and pacing. The oversized-data kick reason now has an English translation. ChangesWarship intent batching
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ClientTransport
participant BatchingHelper
participant PacedSender
participant ServerRateLimiter
ClientTransport->>BatchingHelper: split selected unit IDs for destination tile
BatchingHelper-->>ClientTransport: return size-bounded batches
ClientTransport->>PacedSender: queue each move_warship intent
PacedSender->>ServerRateLimiter: send batches at the configured interval
ServerRateLimiter-->>PacedSender: accept or kick oversized intents
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/Transport.ts`:
- Around line 653-659: Update the warship move handling around
batchMoveWarshipUnitIds and sendIntent to rate-limit batches to the server’s
INTENTS_PER_SECOND allowance. Queue excess move_warship intents so they are sent
at the permitted interval, preserving each batch exactly once and preventing
dropped batches from being re-sent as duplicates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 962e6577-a53d-4014-a4ff-9441c0896e9a
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/Transport.tssrc/core/Schemas.tssrc/server/ClientMsgRateLimiter.tstests/MoveWarshipIntentBatching.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f4ffabe70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Batches were emitted in one synchronous burst, so an order producing more than ten batches exceeded the server's per-second intent budget and the excess was silently dropped. Send the first batch immediately and the rest at 150ms spacing. Singleplayer uses no spacing since LocalServer has no rate limiter.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/Transport.ts`:
- Around line 665-672: The warship batch callback queued in the loop around
batchMoveWarshipUnitIds must survive reconnects when the socket is unavailable.
Update the PacedSender/sendIntent flow to avoid removing the callback until
sendIntent succeeds, or requeue failed sends at the front, preserving batch
order and retrying the same unitIds after reconnection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4b62c20-8910-4ee6-8a4b-b23f6f408deb
📒 Files selected for processing (3)
src/client/PacedSender.tssrc/client/Transport.tstests/client/PacedSender.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b824042b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
PacedSender dequeued before sending, so a batch whose send landed while the socket was reconnecting was lost permanently. sendIntent now reports whether the message went out, and the batch stays at the front of the queue until it does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72c74d4b95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An open socket does not mean the server has processed our join or rejoin. Worker.ts handles messages asynchronously and discards anything other than join/rejoin until that completes, so a paced batch sent right after reconnect could be dropped while sendIntent reported success. Track whether the server has replied on this connection and gate intent sends on it.
| // Client messages larger than this get the client kicked by ClientMsgRateLimiter. | ||
| export const MAX_INTENT_SIZE = 2000; | ||
|
|
||
| export function batchMoveWarshipUnitIds( |
There was a problem hiding this comment.
this funciton shouldn't be in Schemas.ts
5 participants
Resolves #4718
Supersedes #4732, which I opened before reading the contribution rules properly and which was auto-closed for having no approved issue. Same branch, plus the schema bound and the missing translation entry that #4718 also asks for.
Description:
move_warshipsends every selected unit id in one intent, and past ~274-320 ships (id-width dependent) the frame exceedsMAX_INTENT_SIZE, soClientMsgRateLimiterkicks the player.As proposed in the issue: batches the order in
onMoveWarshipEventinto intents that each fit under the cap, kept maximal so realistic orders stay well inside the 10/s intent budget (1 intent below ~274 ships, 4 at 1,000). Also boundsunitIdsat.max(1000)in the schema. Above the 964 ids a single conforming batch can physically hold, and adds the missingkick_reason.too_much_dataentry toen.jsonso the modal shows text instead of the raw key.MAX_INTENT_SIZEmoved toSchemas.tsso client and server share it.MoveWarshipExecutionapplies each unit id independently and dedupes, so N intents are equivalent to one.Hurdan