Race flow: MCOTS handlers, packet reassembly, Sentry observability#2831
Merged
Conversation
…aunch configuration
… add unit tests for default initialization
…fusion UserInfo.deserialize/serialize now use the NPS "lpb" wire format (4-byte length-prefixed string + 4-byte alignment padding) instead of fixed CString(32), matching what the client actually sends (84 bytes, not 100). decompressMessage in internal.ts was declared as accepting ServerPacket but received a MessageNode at runtime. MessageNode.getDataBuffer() returns MessageNodeBody (not Buffer), causing "subarray is not a function" at runtime. Fixed by using MessageNode throughout the inbound path and .data instead of .getDataBuffer(). Also replaces ServerPacket.copy with in-place mutation. Tests added for the compression path, including a regression test that names the original error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Short.serialize() was allocating 4 bytes but only writing 2, producing corrupt output. CString is now a fixed-width null-terminated buffer (size = maxLen) rather than a 4-byte-length-prefixed variable blob, matching the NPS wire format. Also adds Char (1-byte signed int). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fields were write-only; add commId, ipAddress, port, userId, numberOfPlayers, and structSize getters so callers can read back values without accessing private buffers directly. Remove underscore-prefixed private fields from IRunningServerInfo interface — they were never part of the public contract. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
NpsRiffInfo and NpsRiffListMessage implement the correct on-wire layout for Riff/channel info messages (variable-length strings with 4-byte length prefixes, fixed shorts and dwords, 256-byte channel data). BytableChannelData (fixed 256-byte buffer) is added to BytableBuffer and wired into BytableFieldTypes as 'ChannelData'. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rver lookup - Parse all 21 fields of the OpenCommChannel struct (was 7); pass commId as a number rather than a buffer slice throughout - handleSendRiffList switches from shared RiffInfo/RiffInfoListMessage to NpsRiffInfo/NpsRiffListMessage from binary lib - RiffInfo struct corrected to 339 bytes with proper alignment padding; gameServerIsRunning setter takes number instead of boolean - handleGetServerInfo now does a database lookup for game servers (commId > 20) and assigns IP dynamically instead of hardcoding - handleGetUserList returns empty list for commId 2883705 - handleStartGameServer sends joinedChannelMessage first; reads commId as uint32BE; sets packet ID to 0x308 on port 10001 - OpenCommChannelMessage test updated to reflect new field layout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… pit didn't work.
…pdated deps. The code is solid and working.
Inbound StartRaceMsg (70 bytes, pack(1)) and outbound StartRaceResultMsg (40 bytes) classes in packages/transactions, mirroring MCDefs.h:1414/1433. Stub _startRace handler echoes racer ids with okToStart=true, registered at MCOTS opCode 232. TODO(start-race) markers cover validation, fee charging, escrow, and PAP-fetch work for later. Roundtrip-verified against a captured 70-byte MC_RACE_START. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NPS relay (SINGLE family): - Parser for the 16-byte envelope (commId, senderUserId, filterUserId, blob), verified against real captures and Ghidra decompilation of npslib's MsgPack::Compress. - Stub handlers for SEND_BUDDY_LONG (0x93), SEND_SINGLE_LONG (0x95), and SEND_NOT_SINGLE_LONG (0x97). Single-player no-op; multi-player relay marked TODO. MC_RACE_RESULTS (MCOTS opcode 221): - CompletedRaceMessage parser (30-byte fixed prefix plus variable travelMap). - Stub handler logs structured fields and warns on non-zero securityFlags. TODO markers cover validation, anti-cheat, persistence, and prize awards from MCRaces.cpp:4913. Application-layer SPAM packets (peer-to-peer game state): - CarPositionFrame (36 bytes), SpamPositionPacket (type 5), SpamRacerFinishedPacket (type 7), SpamScorePacket (type 10), SpamBigPacketHeader (type 8). Tested against capture bytes. 38 TODO(<topic>) markers across the new files cover the remaining work: real relay, LOGGED variants, LIST/broadcast families, FeStats_Obj parser, BIG_PACKET reassembly, position coordinate decode, and full race-results processing. 44 tests added, all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both opcodes were hitting UNSUPPORTED_MESSAGECODE on port 43300 against real client traffic — they're in-race messages that block the race-start flow on the udp-game-room branch. Damage update (MC_IN_RACE_DAMAGE_UPDATE, 0x00F0): - DamagedPartsMessage parser for the fixed 243-byte struct (msgNo + raceID + noParts + partID[47] + damagePercent[47]). - _inRaceDamageUpdate handler stub: parse, log structured summary, return no responses (matches the legacy fire-and-forget contract). Pre-race CRC (MC_CRC_PRE_RACE_DATA, 0x01B2): - CrcPreRaceDataMessage parser for the 22-byte header (msgNo, checkSum, raceID, trackCRC, sliceInfoCRC, pacejkaCRC); the per-racer TypePreRacePlayerCRC[4] region is kept as an opaque blob with a TODO — observed wire size doesn't match the literal 4*496 byte struct, so per-record decoding is deferred. - _crcPreRaceData replaced with a real parser+log stub for opcode 434. Bonus fix: the existing _crcPreRaceData was registered against opcode 455 (which is actually MC_CRC_PRE_RACE_DATA_TEST_DRIVE per MCDefs.h:559), not 434. Split into: - _crcPreRaceData.ts -> opcode 434 (live race CRC, no reply) - _crcPreRaceDataTestDrive.ts -> opcode 455 (test-drive Ack/Nak path) handlers.ts: _MSG_STRING table updated so 240/434/455 log with the correct names. Legacy messageHandlers[] array (unused) left as-is. Source refs from ~/mco-source: - DamagedPartsMsg struct: MCDefs.h:1545 - InRaceDamageUpdate: MCParts.cpp:545 - CRCPreRaceData struct: MCDefs.h:2398 - MCRaces_CRCPreRaceData: MCRaces.cpp:1275 - TEST_DRIVE handler: MCRaces.cpp:1189 13 TODO(<topic>) markers cover real validation, persistence, anti-cheat flag bookkeeping, and per-record player-CRC decoding. 16 tests added (9 for damage parser, 7 for CRC parser), all green. Captured-packet test fixtures derived from the live traffic recorded on 2026-05-07; raw hex stays in data/captures/ (now gitignored). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
processInput() now throws a dedicated UnsupportedMessageCodeError when no handler is registered for an MCOTS opcode that passed the header check and decryption. Before throwing, it explicitly captures the error to Sentry with structured tags (messageCode, messageName, errorType) and extras (connectionId, sequence, bodyByteLength, decryptedHex) so we can group, prioritize, and reproduce real client traffic from the issue view. Filter: only positive message codes are reported. Non-positive values nearly always come from corrupt buffer reads (signed int16 returning a negative or zero) and would otherwise spam Sentry with noise. Gateway side: mcotsPortRouter's catch in processIncomingPackets now checks `instanceof UnsupportedMessageCodeError` and skips its own generic Sentry.captureException for that case, since processInput has already either reported it or filtered it. All other routing errors still go to Sentry as before. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defensive guard in processIncomingPackets: after parseInitialMessage, verify isValidSignature() and bail with a warn log if false — no decrypt, no dispatch, no Sentry. Header context (signature value, msgLength, flags) is included so the dropped packets can still be diagnosed from logs. Won't fire under the current TOMC-anchored slicer (which by construction extracts packets with a valid signature byte sequence) but covers any future router path that doesn't anchor on TOMC, and keeps the invariant that handlers only ever see signed framing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The mcots router was decrypting truncated packet bodies whenever a TOMC-framed message arrived split across two TCP reads, advancing the server cipher by N bytes while the client had advanced its keystream by N+M. From that point on every decrypt on the connection produced garbage and every message hit UNSUPPORTED_MESSAGECODE — the SERVER-M9 event chain (10217, 25278, 1242, etc.) all trace back to one fragmented MC_CRC_PRE_RACE_DATA packet (sequence 47, ~2017 bytes wire, arrived as 1484 + 533 byte chunks) earlier in the same connection. Replace the old findPackageSignatureIndices + Buffer.subarray slicer (which silently truncated when the declared msgLength extended past the chunk) with a per-connection accumulator that: - appends each TCP chunk to a Map<connectionId, Buffer>; - emits only complete framed packets (msgLength + 2 bytes ≤ buffer); - retains the trailing partial bytes for the next data event; - resyncs past leading junk to the next valid TOMC anchor; - rejects bogus msgLength values (< 9, the header-only minimum); - keeps a 3-byte tail when no TOMC is found, in case the marker itself is split across reads. clearPartialBuffer() is wired into the 'end', 'error', and ECONNRESET handlers so disconnected connections don't leak buffers. 10 new tests in mcotsReassembly.test.ts cover the SERVER-M9 split case, multiple coalesced packets, partial second packet straddling reads, per-connection isolation, leading-junk resync, no-TOMC drop, bogus msgLength resync, TOMC-at-offset-0 non-loop, and disconnect cleanup. All green. Pre-existing socketErrorHandler.test.ts failures (2) are unrelated and predate this change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four more opcodes that were hitting UNSUPPORTED_MESSAGECODE on port 43300 — all part of the race-completion flow that follows the 240/434 work landed earlier today. Each stub deserializes the wire message, logs the structured fields, and returns no responses (matching the legacy fire-and-forget contract on the success path). Real validation, persistence, and side effects are deferred behind TODO(<topic>) markers. MC_UPDATE_BODY_DAMAGE (202 / 0x00CA): - BodyDamageMessage: 12-byte header + variable damage[damageLength]. - _updateBodyDamage: stub. raceID may be 0 (out of race / arcade). - Source: MCDefs.h:1562 (BodyDamage), MCCar.cpp:999 (UpdateBodyDamage). MC_RACER_LEFT_RACE (235 / 0x00EB): - RacerLeftRaceMessage: fixed 10-byte GenericRequest shape; data carries raceID, data2 unused. - _racerLeftRace: stub. The legacy handler runs MCRaces_RacerQuitRace to update racer state and notify the field. - Source: MCDefs.h:632 (GenericRequest), MCRaces.cpp:5236. MC_REPORT_POST_RACE_DAMAGE (241 / 0x00F1): - DamageAndWearMessage: 8-byte header + 8 * noParts records. Each record packs damagePercent into the top byte and wear into the low 24 bits of a single DWORD; the parser unpacks transparently. - _reportPostRaceDamage: stub. Unlike the in-race damage tick (240), the legacy handler *does* reply with RequestFailed/MC_DB_ERROR on stored-proc failure — that path is deferred via TODO. - Source: MCDefs.h:1571/1577, MCParts.cpp:613 (ReportPostRaceDamage). MC_CRC_POST_RACE_DATA (435 / 0x01B3): - CrcPostRaceDataMessage: fixed 58-byte struct (header + TypePostRacePlayerCRC[4]). Much smaller than the pre-race variant (no carCRC, no per-part physics array) — just (playerID, vehicleCRC, modelCRC) per slot. - _crcPostRaceData: stub. The legacy handler compares post-race CRCs against the pre-race values saved by MCRaces_CRCPreRaceData; OR-s MC_CRC_kPrePostCarMismatch / kPrePostCarOpponent into suspiciousEventFlags on mismatches. Comparison logic deferred. - Source: MCDefs.h:2410 (CRCPostRaceData), MCRaces.cpp:1631. handlers.ts: _MSG_STRING table updated so 202/235/241/435 log with the correct names. Registry adds four registrations. 28 new tests across the four parser classes, all green. No new TS errors introduced in any file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The legacy MCO server has no dispatch for MC_RACE_RESULTS (221) — the enum is defined and the name appears in the McDefsMsgs lookup table, but nothing in MCServer.cpp routes it. The actual end-of-race message the client sends is MC_RACER_COMPLETED_RACE (234), dispatched at MCServer.cpp:3486 → MCRaces_RacerCompletedRace (MCRaces.cpp:4913), which casts the body to CompletedRaceMsg. So the existing _raceResults handler — registered at 221 with a CompletedRaceMessage parser — was bound to a never-dispatched opcode, and real race-completion traffic was hitting UNSUPPORTED_MESSAGECODE. Same pattern as the 434 vs 455 fix earlier today: parser was correct, opcode wasn't. Changes: - registry.ts: opCode 221 → 234, name MC_RACE_RESULTS → MC_RACER_COMPLETED_RACE. - CompletedRaceMessage default msgNo 221 → 234, doc comments and serialize-layout test expectation updated. Notes that 221 is vestigial in the legacy server. - File rename: _raceResults.ts → _racerCompletedRace.ts via git mv, function and logger name updated to match. handlers.ts _MSG_STRING table gets the 234 entry. 100/100 transactions tests still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tests had drifted from the source after 95a80f1 ("So close. Just start the race. Do it") refactored the handler to put dynamic data in the structured-log metadata object instead of interpolating it into the message string. ECONNRESET path: assert log.debug("Connection reset by peer", { connectionId }) instead of the old interpolated single-arg form. Default path: assert log.error("Socket error", { connectionId, err: error }) instead of the old interpolated message + { code, errno } shape. Both tests now pass; full gateway suite goes from 193/195 → 195/195. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Hi there 👋 Using this App for a private organization repository requires a paid subscription. You can click If you are a non-profit organization or otherwise can not pay for such a plan, contact me by creating an issue |
|
The author of this PR, drazisil, is not an activated member of this organization on Codecov. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
CI on PR #2831 surfaced three test failures that hadn't been caught locally because we only run subsets at a time. All three were latent on the branch before today's MCOTS work; fixing them gets the branch to a clean 770/770 across 98 files. 1) BytableChannelData default-zero contract restored. The constructor was hard-coding hostID=21, connectedPlayers=1, and userID[0]=21 — values traceable to a specific dev persona used during local testing. They violated the "default = all zeros" test contract, baked a single id into anything that constructs a fresh channelData, and were already added/removed/re-added at least once in the branch history. Removed; callers that need real values must set them explicitly (none of the four call sites depend on the defaults). This also fixes RiffInfo.test.ts > "should initialize with default values", which was a downstream casualty of the same constructor. 2) getLobMiniUserList.test.ts mock fix. The test mocked MiniUserList and GameMessage with arrow functions in mockImplementation(); the production code calls them with `new`, and arrow functions are not constructable. Switched to regular `function ()` expressions so the mocked classes can be `new`-ed. Local full suite now 770/770. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+1
to
+5
| import { CString, sliceBuff } from "rusty-motors-shared"; | ||
| import { serializeString } from "./PersonaMapsMessage.js"; | ||
|
|
||
| /** | ||
| * |
There was a problem hiding this comment.
Bug: A circular import between PersonaRecord.ts and PersonaMapsMessage.ts causes serializeString to be undefined at runtime, leading to a crash when serializing persona records.
Severity: CRITICAL
Suggested Fix
Break the circular dependency. Move the serializeString function and any other shared utilities from PersonaMapsMessage.ts into a new, separate utility file. Both PersonaMapsMessage.ts and PersonaRecord.ts can then import the function from this new file without creating a cycle.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/authentication/src/persona/PersonaRecord.ts#L1-L5
Potential issue: A circular dependency exists where `PersonaMapsMessage.ts` imports
`PersonaRecord`, which in turn imports `serializeString` from `PersonaMapsMessage.ts`.
Due to ESM's module loading order, when `PersonaRecord.ts` is loaded, the
`PersonaMapsMessage` module is only partially initialized, and its `serializeString`
function has not yet been evaluated. This results in `serializeString` being imported as
`undefined`. The subsequent call to `PersonaRecord.serialize()`, which is part of the
core authentication flow, will fail with a `TypeError`, causing a runtime crash.
Also affects:
packages/authentication/src/persona/PersonaMapsMessage.ts:1~5
CI's "Upload coverage to Coveralls" step has been failing with
🚨 ERROR: Couldn't find specified file: ./coverage/lcov.info
The Coveralls action expects ./coverage/lcov.info, but the workflow
runs `npm run coverage` and the script was just `vitest run` — no
--coverage flag, so coverage was never collected. Even with the flag,
Vitest's default reporters are text + html, neither of which write
lcov.info.
Two-line fix:
- package.json: "coverage" script now passes --coverage.
- vitest.config.ts: explicit `coverage: { provider: "v8", reporter:
["text", "lcov", "json"] }`. The `lcov` reporter writes
./coverage/lcov.info; `json` keeps Codecov's auto-detection paths
happy.
Verified locally: `npm run coverage` produces ./coverage/lcov.info
(~231 KB) with full statement/branch/function/line summaries.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
| @@ -165,6 +286,23 @@ async function processIncomingPackets( | |||
| data: packet.toString("hex") | |||
There was a problem hiding this comment.
Bug: Using forEach with an async callback on inPackets can cause unhandled promise rejections if parseInitialMessage fails, leading to a process crash.
Severity: CRITICAL
Suggested Fix
Replace the inPackets.forEach(async ...) with a sequential for...of loop. This allows the use of await on the promise returned by routeInitialMessage and enables a try/catch block to correctly handle any exceptions thrown during packet processing, preventing unhandled rejections.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/gateway/src/mcotsPortRouter.ts#L286
Potential issue: The code uses `inPackets.forEach(async ...)` to process incoming
packets. Since `forEach` does not await the async callbacks, they execute concurrently.
If `parseInitialMessage(packet)` throws an exception, for instance due to a malformed
packet, the error is not caught by the `.catch()` block attached to the
`routeInitialMessage` promise. This results in an unhandled promise rejection, which
will crash the Node.js process. Concurrent execution also introduces a risk of race
conditions when advancing the cipher state during decryption.
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.
Summary
This branch lands ~24 commits across the race-flow path (mostly UDP/MCOTS gateway and transactions) plus today's deeper work on the gateway: fixing a decryptor desync that's been corrupting cipher state for the entire connection whenever a TOMC-framed packet got TCP-fragmented.
Today's commits (top of stack)
a3b44fc1). Per-connection accumulator inmcotsPortRouter. Root cause of SERVER-M9 / SERVER-M10 / etc. — without this, every UNSUPPORTED_MESSAGECODE chain that follows a fragmented packet is symptomatic. 10 new tests including the exact split case from the production logs.4545b03c). Defensive guard in the router; warns + drops, no decrypt, no Sentry.2970cdfc). NewUnsupportedMessageCodeErrorcarriesmessageCode/messageName/connectionId. Sentry capture is filtered tomessageCode > 0(skips garbage from corrupt buffer reads) and includes the decrypted body hex in extras for repro.c61b60a9: 240 (MC_IN_RACE_DAMAGE_UPDATE), 434 (MC_CRC_PRE_RACE_DATA), and a 434↔455 split that fixes a misregistered handler.4a2d510c: 202 (MC_UPDATE_BODY_DAMAGE), 235 (MC_RACER_LEFT_RACE), 241 (MC_REPORT_POST_RACE_DAMAGE), 435 (MC_CRC_POST_RACE_DATA).c5d0b198: opcode 221 → 234 fix (race-completion was bound to a never-dispatched opcode); file rename_raceResults.ts→_racerCompletedRace.ts.fd719783). Stale assertions from a prior structured-logging refactor; now 195/195 in the gateway suite.Earlier commits on the branch
MC_RACE_STARThandler + message classes.OpenCommChannelfull parsing,NpsRiffInfomessages, dynamic server lookup.Test plan
npx vitest run packages/transactions— 100/100 greennpx vitest run packages/gateway— 195/195 green (was 193/195 before the socketErrorHandler test fix)npx vitest run packages/gateway/test/mcotsReassembly.test.ts— 10/10 green; covers the SERVER-M9 split casemessageCode,messageName,errorType, plusdecryptedHexextras for reproNotes
🤖 Generated with Claude Code