bugfix(crc): Fix spurious mismatches for disconnected players at low CRC intervals

[Caball009]

• Related to PR bugfix(network): Add command id overflow check to keep network commands in chronological order #2736

This PR introduces a small to fix to avoid spurious mismatches that can occur if the following conditions are met simultaneously:

1. Low CRC intervals; e.g. once per frame instead of once per 100 frames (default value).
2. Runahead decreases, so that multiple frames are executed in one go.
3. Player disconnects during one of these frames.

As players disconnect they stop sending data to other players to speed up the disconnect process. If that coincides with a decrease in runahead, it's possible that a single execution frame contains e.g. 5 CRC messages per player, but fewer for the disconnecting player. The game only checks a player's most recent CRC value. This would lead to a mismatch because the most recent CRC value for the disconnecting player is not up-to-date with the other (connected) players.

You can see it mismatch here as the player disconnects (this was with a special test build):
https://www.youtube.com/live/RM27jEkTGfw?t=2099s
https://www.youtube.com/live/V_l-q9Y-DmA?t=7576s

Here's a replay for the second match. The mismatch happens at frame 3836 because of player index 6 disconnecting from the game: MM NED.zip

I have no idea how to reproduce this in a test environment, unfortunately.

TODO:

• Replicate in Generals.

[greptile-apps]

Greptile Summary

This PR fixes a false-positive CRC mismatch that arises when a player disconnects during a frame where multiple CRC checks fire in a single processCommandList call (e.g., runahead contraction at a 1-frame CRC interval). The disconnecting player stops sending data mid-batch, leaving a stale CRC entry in the map that was previously used as the comparison baseline.

• The old loop unconditionally used m_cachedCRCs.begin()->second as the validator, so a stale entry at the lowest slot index became the reference and triggered false mismatches against every connected player's fresh CRC.
• The new loop skips entries whose player index fails isPlayerConnected, then pins referenceCRC from the first connected entry via hasReferenceCRC — fixing the validator-selection bug noted in the prior review.
• The CachedCRCMap typedef extracted to the header removes repeated std::map<Int, UnsignedInt> spellings throughout the .cpp.

Confidence Score: 5/5

Safe to merge; the new loop correctly skips disconnected players on both the validator and validated sides, and the Generals counterpart gap is an acknowledged TODO.

The rewritten comparison loop correctly addresses the false-positive scenario described in the PR: disconnected players are filtered before the reference CRC is selected, so a stale lowest-slot entry can no longer become the baseline. The pre-existing m_cachedCRCs.size() < numPlayers guard is unchanged and continues to catch the missing-CRC case. The only unfixed path is the Generals (non-ZH) counterpart, which is explicitly listed as a TODO.

No files require special attention. The Generals counterpart (Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp) still carries the old loop and is the natural next target for a follow-up PR.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Include/GameLogic/GameLogic.h	Extracts CachedCRCMap typedef from the inline member declaration, enabling use of the type alias in GameLogic.cpp without repeating the full std::map<Int, UnsignedInt> spelling.
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Rewrites the CRC comparison loop to skip disconnected players; uses a hasReferenceCRC flag so the reference value is always taken from the first connected player, correctly handling the edge case where the lowest-indexed slot belongs to the disconnecting player.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[processCommandList called] --> B{m_shouldValidateCRCs &&\nnot sawCRCMismatch?}
    B -- No --> Z[Done]
    B -- Yes --> C[Count connected players\nnumPlayers]
    C --> D{m_cachedCRCs.size\n< numPlayers?}
    D -- Yes --> E[sawCRCMismatch = TRUE\nNot enough CRCs!]
    D -- No --> F[Iterate m_cachedCRCs]
    F --> G{isPlayerConnected\nit->first ?}
    G -- No disconnecting player --> F
    G -- Yes --> H{hasReferenceCRC?}
    H -- No --> I[Set referenceCRC = crc\nhasReferenceCRC = TRUE]
    I --> F
    H -- Yes --> J{referenceCRC != crc?}
    J -- No --> F
    J -- Yes --> E
    E --> K[TheNetwork::setSawCRCMismatch]

Loading

_{Reviews (5): Last reviewed commit: "bugfix(crc): Fix spurious mismatches for..." | Re-trigger Greptile}

[Mauller]

Looks okay to me, i don't think it can be much simpler since zero can be a valid CRC funnily enough for crc32.

[xezon]

Does it need to be behind a retail compile guard?

[Caball009]

No, I don't think so. Do you see potential issues?

[xezon]

I suggest test with a client that has this change and a client that does not in a Network match - if not already done. My first instinct would be to guard it with RETAIL_COMPATIBLE_NETWORK but I do not know.

[Caball009]

I suggest test with a client that has this change and a client that does not in a Network match - if not already done.

I already did testing and I wasn't able to reproduce this issue in a test environment because of the timing issue with the decrease in the runahead value.

My first instinct would be to guard it with RETAIL_COMPATIBLE_NETWORK but I do not know.

It's an issue with and without retail compatibility. I'm trying to avoid this type of 'fake' mismatches, so putting it behind a macro doesn't make sense to me.

[xezon]

This change assumes that all player disconnects are perfectly in sync. Is this guaranteed?

[xezon]

For a follow up, consider moving the Network test before the above TheNetwork->sawCRCMismatch()

[xezon]

I suggest test with a client that has this change and a client that does not in a Network match - if not already done. My first instinct would be to guard it with RETAIL_COMPATIBLE_NETWORK but I do not know.

[xezon]

Is isPlayerConnected guaranteed to return the same result on every client in the same frame? I am asking because this function is in GameNetwork, not in GameLogic.

Can Client A lose connection to Client B, while Client C is still connected to Clients A, B? Is this scenario possible?

I suggest to test this with multiple clients.

[Caball009]

This change assumes that all player disconnects are perfectly in sync. Is this guaranteed?

Is isPlayerConnected guaranteed to return the same result on every client in the same frame?

I think so, because player disconnects are handled by Network::processFrameSynchronizedNetCommand, which has a relevant description:
"This is where network commands that need to be executed on the same frame should be executed."

[Caball009]

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp

Lines 2654 to 2664 in 091ab3d

	for (Int i=0; i<MAX_SLOTS; ++i)
	{
	if (TheNetwork->isPlayerConnected(i))
	++numPlayers;
	}
	if (m_cachedCRCs.size() < numPlayers)
	{
	DEBUG_CRASH(("Not enough CRCs!"));
	sawCRCMismatch = TRUE;
	}

FWIW this original code also strongly suggests that isPlayerConnected is synchronized across clients.

[Mauller]

In theory, players with the fix should carry on playing and not register a mismatch, so their game will continue.

Retail players may register the mismatch and dropout.

I don't entirely see a problem with this as it just replicates how crashes are handled, As TSH clients typically continue playing but retail players drop out.