Repository navigation
fix(deltatracker): clear the resized variable's dirty flag, not the next - #74
Conversation
When a variable's serialized size changed, WriteVar advanced the index before clearing isDirty. The resized variable kept its flag and was resent on the next pass, the following variable lost a flag set by FlagDirtyFromBitArray or a lost-send receipt, and resizing the last variable wrote one past the end of the list.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesVariable list tracking
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
Problem
VariableListDeltaTracker::WriteVar, in the branch taken when a variable's serialized size changes, advancednextWriteIndexbefore clearingisDirty:nextWriteIndex++; variableList[nextWriteIndex].isDirty=false;FlagDirtyFromBitArrayset it or a lost-send receipt did. If its value was unchanged it was not resent, which is the update the flag exists to force.List::operator[].Any variable-length type triggers it (
RakString,std::string, containers), so it hits ReplicaManager3 entities that useVariableDeltaSerializer. The MafiaHub Framework is about to mark forced fields dirty this way after a server override (MafiaHub/Framework#325), and a string field resized just before one of them would silently cancel that resend.Change
Clear the flag before advancing the index. The fix is one line.
Testing
New
Tests/Unit/VariableListDeltaTrackerTests.cpp, one case per symptom. All three failed before the fix, the last one on the bounds assert:ResizedValueKeepsTheNextVariableDirtyResizedValueClearsItsOwnDirtyFlagResizingTheLastVariableStaysInBoundsWith the fix, the full
UnitTestsbinary passes (215/215, Windows Debug).Summary by CodeRabbit