Seemingly Random Savefile Corruption

ID:2796549
 
Jun 10 2022, 9:09 pm
(See the best response by Lummox JR.)
Problem description:
My players have reported multiple times that sometimes randomly, they will log in and their name will be 'the mob' and they will have a black screen This makes it clear that their savefiles are being screwed up some how but it is not clear why or how this happens. We don't even know the cause or how to reproduce the issue. However, we do know that it results in extra files in their savefile directory. In addition to their savefile, there is a 000 bad save file and a 000 bad text file.

https://prnt.sc/DJl1x8bSJ5n3
Jun 11 2022, 1:04 am
Without seeing how you are handling your saving this is impossible to help you diagnose. Would you mind sharing a snippet of your save system please?
Jun 11 2022, 10:43 am
 
mob
    proc
        Save()
            if(!client)return
            if(findtext(ckey,"guest"))return
            ip=client.address
            id=client.computer_id
            if(In_Game)
                Head = null
                Body = null
                RHand = null
                LHand = null
                RLeg = null
                LLeg = null
                var/savefile/S = new("Savefiles/Players/[ckey].sav")
                S["lasttime"]<<world.time
                if(client)S["ip"]<<client.address
                S["id"]<<client.computer_id
                S["mob"]<<src
                S["_name"]<<name
                S["_race"]<<Race
                S["_class"]<<Class
                S["_lives"]<<Lives
                S["icon"]<<getFlatIcon(client.mob)
                S["WM"]<<WipeMultiplier
                del S

client
    proc
        Load()
            if(!src||mob.In_Game)return
            if(!fexists("Savefiles/Players/[ckey].sav"))return
            usr=world.LoadPlayer(src)
            mob.In_Game = 1
            for(var/area/Islands/a in world)
                if(a.WANTED.Find(mob.name))
                    a.WANTED.Remove(mob.name)
                    a.AddWanted(mob)
            src.view="24x14"
            if(src)if(mob.HasHaki)
                if(src)winset(src,"Default.label3","is-visible=true")
                if(src)winset(src,"Default.hakibar","is-visible=true")
            for(var/obj/Outpost/O in world)if(src.computer_id==O.RecentOwnerCID&&mob.name==O.RecentOwner&&!O.Owner)
                O.Owner = src
                O.RecentOwner=mob.name
                src << output("You have reclaimed control of [O].","SM")
            mob.Updatelog()
            for(var/obj/Skills/SS in mob.Skills)SS.CD = 0
            if(src)
                var/obj/crhud/Body/bp = new/obj/crhud/Body/HeadHUD
                bp.crhud_add(src)
                bp = new/obj/crhud/Body/TorsoHUD
                bp.crhud_add(src)
                bp = new/obj/crhud/Body/Right_ArmHUD
                bp.crhud_add(src)
                bp = new/obj/crhud/Body/Left_ArmHUD
                bp.crhud_add(src)
                bp = new/obj/crhud/Body/Right_LegHUD
                bp.crhud_add(src)
                bp = new/obj/crhud/Body/Left_LegHUD
                bp.crhud_add(src)
                for(var/obj/crhud/Body/B in src.screen)B.Owner=mob
            mob.HealthBar()
            mob.EnergyBar()
            mob.In_Water()
            mob.Display()
            if(!mob.loc||mob.loc==locate(0,0,0))
                if(src)src<<"<font color=red>Error loading [usr] location... Sending to spawn"
                mob.loc=locate(mob.Spawn_X,mob.Spawn_Y,mob.Spawn_Z)
            if(!mob.x||!mob.y||!mob.z)
                if(src)src<<"<font color=red>[usr] Location Error... Sending to spawn"
                mob.loc=locate(mob.Spawn_X,mob.Spawn_Y,mob.Spawn_Z)
                if(!mob.x||!mob.y||!mob.z)
                    src<<"<font color=red>[usr] Location Error... Sending to Island of Beginnings. If this is happening, it means that your savefile was so corrupted that it couldn't even remember your spawn point. You should probably remake but first think about what happened before you logged out last, and then file a detailed bug report!"
                    mob.loc=locate(510,510,1)
            winset(src,"default.map_child1", "left=map_pane")
            for(var/mob/mo in screen)del mo
            winset(src,"default","is-default=true")
            draw_lighting()
Jun 11 2022, 6:58 pm
There are a few things in here that could be corrupting your save files, but it's important to remember that it's not random. Something consistent is causing it. The randomness is a result of it being something that you just haven't discovered yet. I ran into something similar tracking issues with saving/loading particle effects by mistake in the early 514 beta.

I would suggest first of all, not saving the reference. That means that the line S["mob"]<<src should be flat out removed. This saves a reference to the mob itself, which is useless information when you go to load, and will only serve to bloat your save file. It's a much better idea to save the relevant information and then apply it to a new mob that you create in your load code. Of course, this depends on how you're loading your mob, but I suspect this might be the cause of your issue.

Second, it's a good idea not to save icon information in your saves. This can again bloat the save file. You'll want to remove S["icon"]<<getFlatIcon(client.mob) as well, assuming that's returning some sort of icon object. Depending on what getFlatIcon() does, you might be able to use it to redraw the mob upon loading.

As I mentioned above, make sure that you aren't saving any particle effects, as well as filters. There's no reason to save visual effects. Removing the reference in the save may solve this for you, depending on how you're handling your visual effects.

The Load code that you provided doesn't help, and I don't think it's relevant to your issue. Here are some more articles regarding saving that might be able to point you in the right direction regarding good and bad habits when it comes to saving.

Save & Load Players
Easy Saving
Using var/tmp
Jun 11 2022, 8:11 pm
Best response
The usr abuse in your Load() proc is concerning. Since that's a proc you shouldn't be doing anything with usr there.

This code is a mess:
if(!mob.x||!mob.y||!mob.z)
    if(src)src<<"<font color=red>[usr] Location Error... Sending to spawn"
    mob.loc=locate(mob.Spawn_X,mob.Spawn_Y,mob.Spawn_Z)
    if(!mob.x||!mob.y||!mob.z)
        src<<"<font color=red>[usr] Location Error... Sending to Island of Beginnings. If this is happening, it means that your savefile was so corrupted that it couldn't even remember your spawn point. You should probably remake but first think about what happened before you logged out last, and then file a detailed bug report!"
        mob.loc=locate(510,510,1)

Again there's the problem of usr abuse, but you're also checking vars you don't have to. if(!mob.x || !mob.y || !mob.z) is redundant since if any of those is 0 the others will be to; but you're far better off just checking if(!mob.loc) which is way more succinct.

I suspect a cause of the bad savefile reads is that you've made some changes to the game since those earlier savefiles were written, and some type paths (e.g., for items in a mob's inventory) have changed or no longer exist.
Jun 12 2022, 9:04 am
In response to F0lak
F0lak wrote:

I would suggest first of all, not saving the reference. That means that the line S["mob"]<<src should be flat out removed. This saves a reference to the mob itself, which is useless information when you go to load, and will only serve to bloat your save file. It's a much better idea to save the relevant information and then apply it to a new mob that you create in your load code. Of course, this depends on how you're loading your mob, but I suspect this might be the cause of your issue.
Alright. If saving "mob" isn't a good idea, what I would need instead is an alternative. As far as I know, the primary alternative for savings this way is to save each and every individual variable which seemed less efficient when I saw it. i.e:
F["hp"]<<hp
F["maxhp"]<<maxhp
F["energy"]<<energy
F["maxenergy"]<<maxenergy
etc.

Second, it's a good idea not to save icon information in your saves. This can again bloat the save file. You'll want to remove S["icon"]<<getFlatIcon(client.mob) as well, assuming that's returning some sort of icon object. Depending on what getFlatIcon() does, you might be able to use it to redraw the mob upon loading.
getFlatIcon effectively takes a snapshot of the char as a 2d image that can't really move. Its for showing the char in the UI on the login screen for when they click load. I will look into removing it and seeing if I can get the same result to be generated when they login instead of saving it.
As I mentioned above, make sure that you aren't saving any particle effects, as well as filters. There's no reason to save visual effects. Removing the reference in the save may solve this for you, depending on how you're handling your visual effects.
I don't really have many visual effects in the game to save in the first place so I don't THINK I am. I will double check to be safe though.

Save & Load Players
The reason I would be opposed to this method is because I've looking around the dev help forums alot and I could swear that someone like ter13, or Kaio, or Lummox said that it's never good to call Read() and Write() directly.
Easy Saving
I do not know exactly why they are showing how to save only one variable but I'll just say that I don't understand what's going on in this one. It seems like their only saving one var, if this is the case, then I would have to save each var individually which I don't particularly have a problem with given that it is the most efficient way to go.
Using var/tmp
I already used tmp for all vars that don't need saving. I really like doing it where ever applicable.

Lummox JR wrote:
The usr abuse in your Load() proc is concerning. Since that's a proc you shouldn't be doing anything with usr there.
I have removed all instances of usr that were there. Thanks for the heads up.
but you're also checking vars you don't have to. if(!mob.x || !mob.y || !mob.z) is redundant since if any of those is 0 the others will be to; but you're far better off just checking if(!mob.loc) which is way more succinct.
I have reduced these lines to if(!mob.loc).
Jun 12 2022, 10:31 am
Have you examined the saves that didn't load to see if they had old type paths in them? That's where I'd expect an issue.
Jun 12 2022, 10:48 am
The problem with that is that there's nothing about an issue in my daemon log which usually picks up all errors, and even when I haven't changed any type paths in the game for weeks, the issue still happens. Usually, it will be when the server crashes from whatever makes it do that, and then I bring it up. Players names will be "the mob". But there's two types of players with this problem.

1. Some players get named "the mob" but still have all their stats, skills, and items. They would have had a black screen too but the if(!mob.loc) failsafe usually helps them with that.
2. Other players are named "the mob" and all their stats, and skills become those of a default new mob.
No one has seemed to notice a pattern aside from being online during a server crash. But 90% of the time, being online during the server crash doesn't cause this issue.