Desync Postmortem

[BlackYps]

Let's start with the good news first: Due to relentless work by Nomander, supported through testing by JaggedAppliance, they managed to pinpoint the cause of the desync issues that plagued FAF recently. As I suspected, it was an engine patch, which brings me to the main topic I want to talk about: How do we deal with engine patches?

We are in a peculiar position, because the people doing engine patches and opening Pull Requests for them are frustrated that getting them actually deployed in the game takes so long. On the other hand, these changes are extremely hard to review, because they look like this:
´´´
0x007A7858:
jmp asm__ShouldAddUnit
nop

0x007EF12B:
push esp
mov ecx, esi
call RenderRange__Moho__UserUnit__UserUnit
nop

0x005BF011:
mov ecx, edi
mov edx, eax
call Hooked_SyncVisionRange
jmp 0x005BF02C
´´´
Good luck checking the code for logical errors. And as we can see with the latest debacle, an engine patch can work on the surface. When you test it, it does what it is supposed to do, but it can bite us later when random desyncs pop up because the patch also can corrupt memory under special conditions.
So for me as a team lead, merging in a barely tested engine patch feels like chugging an improvised bomb into my basement for storage. Probably nothing bad happens, but it definitely doesn't feel comfortable.

Recently we got a lot more engine patches by new contributors, which is nice in principle, but it makes the problem even bigger, because with the old contributors I at least know that they have some experience. With the new guys it could be some AI-assisted reverse engineering which can be a good or a bad thing, but I can't tell at all how diligenty they worked on that.

And the thing that annoys me the most is that I as a team lead am supposed to provide a good contribution environment, where people don't feel bogged down by bureaucracy and long approval processes. But if things go south, and nobody knows where the mysterious bugs that have appeared originated from, then I feel like all eyes go to the team lead to fix it. And frankly, I would rather stop storing IEDs in my basement than constantly rebuild the room. In this case, thankfully Nomander took the initiative, but he is also the Balance Team Lead since recently, so once again it is a team lead investing time cleaning up the mess other people made.

I personally think that it is not a good look on FAForever as a whole, if the gaming experience is tainted by random desyncs and crashes. And I think we need to change something about how we apporach this whole engine patch topic. I realize that the position of "just stop it entirely" is too extreme, but just carring on is also unbearable. God, this whole thing makes me angry, not at anyone in particular, but at this whole situation. We can do better than this. We need to do better.

I am looking forward to constructive suggestions.

[Katharsas]

Throw out the engine binary and build from the REed source released a day ago?^^
https://forum.faforever.com/topic/10497/sc-fa-moho-engine-sources/9

However, you probably still want to test changes to the RE engine in some form before releasing them to all players. In an ideal world you could even collect stats about crashes between different engine versions to get something similar to an A/B Test, no idea how hard that would be though.

At the end of the day, people will flock to the REed version at some point because it will at some point become better than assembly-patched original (according to the author it already is but even if not its just a question of time). Im pretty sure FAF should hop onto that train as soon as possible, because the player base will get there one way or another.

Edit:
Of course the RE version should probably be tested with a few thousand more replays for near-perfect sim-compatibility.

[Deribus]

I think engine rebuild and/or decompilation is a separate discussion.

I agree it's unreasonable to expect bulletproof code reviews for binary patches. Best solution I can think of is tracking replays and see if there are upticks in desyncs after specific patches. Desyncs are visible from replay data and replays are publicly accessible.

[Giebmasse]

Rewrites and RE's ofcourse interesting and probably relevant soon.

Until then:
Some years ago we had a "forced" testing period for more significant changes (maybe it was ICE). For instance test thing X for 3 days, more activity on feedback gathering/analytics, then revert and digest results and decide from that. New releases are kinda that, but this would maybe emphasize more knowingly it is a test and data gathering for the decision making later on.

Another avenue which has been used is the fafdevelop, if a binary patch could used that and more users pushed/incentivized to test that and report feedback, or again analytics in place.

These could always be coordinated with promo team to try pushing people to test and report specific things we are on the lookout.

[Brutus5000]

@blackyps Would any of these desync issues appear in a replay out did it only happen in live games? Can we build a CI-pipeline on old repos? @draiget does this for the reverse engineering development somehow.

We now have desync tracking in the lobby server, so this could be used as alarm tracking after patches. But that would basically require more releases / smaller changes per patch.

[Draiget]

@Brutus5000 said:

Can we build a CI-pipeline on old repos? @draiget does this for the reverse engineering development somehow.

I need to finish implementation of headless faf (version for server-side networking without rendering) and it could be used on linux, but today it's just a replay checker that pickups Vault replays without desyncs and simulating game, any desync of valid replays means build has potential issue. This isn't for CI, or it would be very long CI pipeline run, checking 80+ replays will take few hours. I'm also checking every 50 beat checksums by storing log-file.

[IndexLibrorum]

I know Deribus said that the engine rebuild is not so relevant right now, but with the speed those things are now developing, and the speed at which we at FAF decide/execute things, isn't it more reasonable to expect that the engine rebuild will just straight up solve this issue?

With a reconstructed source code, engine patches like these are no longer necessary, aren't they?