Repository navigation
Conversation
… cell Loading a cell queues its references' saved changes on BGSSaveLoadGame without a lock, while the background QueuedPromoteReferencesTasks drain the same queue. MoveActor (and the teleport command and map overlay) loaded cells without the pause the game uses for its own cell loads, so a drain could run concurrently or later on freed references (crash at SkyrimSE 35567+0x111, call [rdx+0x160]). Pause the promote tasks around the load and apply the queued changes before resuming, as the game does. Verified in Ghidra (SkyrimSE 1.6.1170): - 403330 is BGSSaveLoadGame (0x348 bytes, flags at +0x340). +0x318 is a BSTArray of 0x28-byte BGSLoadFormData. Each entry holds a raw form pointer at +0x10 with no refcount. - 35567 drains the queue (InitLoadGame/FinishLoadGame) under its own spinlock and sets flags |= 8 while it runs. 35569 is the only push and does not take that lock. Nothing removes an entry when its form is freed. - 20460 TESWorldSpace::LoadCell -> TESObjectCELL::InitItemImpl (18881/18882) -> 35569 pushes and does not drain. - 13333/13334 pause/resume the promote tasks (15335/15336). Pause cancels queued tasks and waits for running ones. Vanilla cell loads (coc/cow 22875/22876, 36641, 40437, 40745, grid update 13288) hold it. Both functions ignore `this`. - The 2026-10-21 crash was on a QueuedPromoteLocationReferencesTask thread, 17 ms after a burst of NotifyActorTeleport -> MoveActor. The 2026-10-18 crash was the main-thread grid drain. Verified on 1.7.104 by comparing raw bytes of the unpacked exes (equal apart from rel32 displacements): the BGSSaveLoadGame ctor/size/+0x340 flags, 35567's flag updates and +0x318/+0x328 accesses, 35569's prologue and kLoading test, and 13333/13334 resolving to 15335/15336 on 400326. Assumed: draining right after loading an unattached cell is safe, as vanilla 15328 does the same from a worker thread. On 1.7.104, 20460 still reaching InitItemImpl was not checked. Not built or tested in game yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…into fix/loadcell-pause-promote-tasks
Collaborator
|
Checked this against my IDB and 4 other crash logs, this is wrong - both the alleged cause and the fix. Closing, will do myself. One small request: please title your PRs appropriately |
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
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.
Remote actor teleports (and /tp, and the overlay teleport button) load exterior cells through TESWorldSpace::LoadCell without the promote-task pause the game uses around its own cell loads. That corrupts an unlocked engine queue, and the game crashes later at SkyrimSE+0603251 (35567+0x111), often on fast travel, a load door or respawn. This PR adds the same pause and drain vanilla uses. One function changed: Code/client/Games/Skyrim/Forms/TESWorldSpace.cpp.
Summary
TESWorldSpace::LoadCell (20460) is our way of loading an exterior cell that isn't loaded yet. Its callers are:
CharacterService::MoveActor, used by NotifyActorTeleport when an owner relinquishes an actor that left its cell;
CommandService::OnTeleportCommandResponse (/tp to a player);
OverlayService::OnNotifyTeleport (the teleport button).
We call 20460 without the guard the game puts around its own cell loads. That can leave dangling form pointers in an engine queue, or race on it. The result is a crash later, often far from the original teleport, during:
a background QueuedPromote(Location)ReferencesTask, or
the next cell transition on the main thread (fast travel, load door, respawn, walking across a cell border).
The crash signature is:
EXCEPTION_ACCESS_VIOLATION at SkyrimSE.exe+0603251 (ID 35567+0x111) call [rdx+0x160]
RCX/RBX = form pointer read from the queue, RDX = garbage "vtable" (e.g. 0x3A0000602, 0xA0C80302)
This PR wraps LoadCell the way the game wraps its own cell loads:
pause the promote tasks;
load the cell;
apply the queued changes;
resume the promote tasks.
The change is one function in Code/client/Games/Skyrim/Forms/TESWorldSpace.cpp and covers all three callers.
The engine mechanism
The details below were verified in Ghidra on SE 1.6.1170, then checked again on 1.7.104 (next section).
The queue. 403330 is BGSSaveLoadGame (0x348 bytes, flags at +0x340; layout matches CommonLibSSE).
+0x318 is a BSTArray<BGSLoadFormData*> of references whose saved changes have been read (LoadGame) but whose InitLoadGame/FinishLoadGame is deferred.
Each 0x28-byte entry stores the form as a raw TESForm* at +0x10 with no refcount taken.
Queued forms get flag 0x200000 ("init pending").
Draining: 35567. It takes a spinlock (0x14317abb8) and sets flags |= 8 (kInitingForms). Then, for each entry:
form->AsReference() (vtbl+0x160). This is the crash site.
InitLoadGame (+0x80) and FinishLoadGame (+0x88).
It frees the entry.
Finally it clears the array.
Pushing: 35569, the only writer. It's reached through:
TESObjectCELL::InitItemImpl, vtable slot 0x13. The chain is 18881 → 18882 → 35572 / 19283 → 35569.
LoadCell (20460) calls that slot when it creates a cell. It doesn't drain afterwards.
35569 does not take the drain's lock, and nothing removes an entry when its form is destroyed.
I found that by checking every instruction that touches +0x318/+0x328 in the 0x1405f8000–0x140630000 range of the function list:
ctor (0x1406028f0)
35563 (clear)
35565 (dtor)
35567 (drain)
35569 (push)
35600 (load game)
How vanilla stays safe. 13333 / 13334 pause and resume the promote tasks through 15335 / 15336 on 400326:
Pausing cancels queued promote tasks (75638).
It also waits for running ones (75639 spins while the task state is 3 or 4). These are the tasks that drain the queue on worker threads.
The task manager only submits new promote tasks while the pause count is 0 (15321).
Vanilla callers of 20460 hold this pause:
console coc/cow (22875 / 22876)
36641
40437
40745
The grid update 13288 pauses on its own. Grid loading (13430) also sets kDeferInitForms (0x10) and drains straight after loading. The promote task (15328) drains straight after pushing. Vanilla therefore never leaves entries queued past the scope that pushed them, and never pushes while a promote task can be draining.
What our call breaks. We call 20460 from network handlers on the main thread with neither the pause nor a drain. That gives two failure modes:
Concurrent push and drain. A promote task drains on a worker thread while our LoadCell appends to the same BSTArray without the lock. The drain's free-entries / clear and our append interleave, leaving stale entry or form pointers.
Stale entries. Entries pushed by our load stay queued until some later drain. If a queued reference is freed first, the next drain dereferences freed memory. One way that can happen: non-persistent references of an unattached exterior cell are freed when the cell is unloaded. (I didn't trace the cell-unload path in Ghidra.)
The fix closes both:
The pause stops and waits out every drainer while we push. The drain's own spinlock still covers any other drainer.
The immediate drain means none of our entries outlive the call.
The drain is skipped during a save load (flag 2), when the game drains itself. It's also skipped while a drain is already running (flag 8), to avoid re-entering the non-recursive spinlock on the same thread.
Version coverage: 1.6.1170 and 1.7.104
All addresses go through Address Library IDs. The only hardcoded layout is BGSSaveLoadGame::flags at +0x340 and bits 2 and 8.
On 1.7.104, I checked an unpacked SkyrimSE.exe (FileVersion 1.7.104.0):
Raw bytes match 1.6.1170, apart from rel32 displacements, in:
the ctor and creator (35562 / 35564: 0x348 allocation, +0x318 array, +0x340 flags = 1, +0x344 = 0xFF);
35567's flag updates and +0x318 / +0x328 accesses;
35569's prologue and kLoading test;
13333 / 13334, which ignore RCX and jump to 15335 / 15336 on 400326.
In Ghidra:
20460 calls vtable slot 0x13 of TESObjectCELL (RTTI-checked vtable), which is 18881;
18882 pushes through 35569 / 35572 / 19283.
Is this Skyrim Together–specific, a modding problem, or vanilla?
The evidence points to Skyrim Together, with load-order size only affecting how often it happens:
The trigger is our unguarded cell load. Every vanilla 20460 caller I checked holds the pause or runs inside grid or load code that drains. Not all of the grid/load callers were checked individually.
No public crash logs match. We searched for this function's crash offset (35567+0x111) in every version we could map: 1.6.1170 +0603251, 1.7.104 +0615C01, 1.6.1130 +06035C1, 1.6.640 +05ADAB1, 1.6.353 +059C101. Nothing came up.
QueuedPromoteLocationReferencesTask does appear in unrelated vanilla and modded crash logs, but never at this site.
There are no issues here mentioning it.
This is absence of evidence, not proof.
Mods make it more frequent, but don't cause it. The queue only gets entries for references that have saved changes (35569 returns early otherwise). Long saves, script-heavy load orders and quests that promote location references all raise the odds. None of them call 20460 bare.
ST's own form deletion is ruled out:
Temporary actors and dropped items are deleted through ObjectReference.Delete. Runtime-created references have no pending saved-change entry, so they never enter this queue. (The early return is verified; that FF references never have pending saved changes is assumed.)
ST never reads or writes +0x318. Its only use of 403330 is TESForm::GetChangeFlags, which reads +0x330.
Why it can happen anywhere, and look like a fast-travel or respawn crash
NotifyActorTeleport goes to every other player whenever an owner relinquishes an actor that moved cells, wherever those players are. Each client then loads that actor's exterior cell, which can be far away.
The crash then lands on whichever drain runs next:
2026-10-21: a QueuedPromoteLocationReferencesTask worker (BGSLocation "Morvunskar" on the stack), 17 ms after a burst of Successfully teleported actor ... into cells 912A, B4BB and B4DC.
2026-10-18: the main-thread grid update (13288 → 13430 → 35567). That's the same drain fast travel, load doors and respawn go through. The stack holds a Mudcrab adjacent to two Mudcrabs we had teleported a few minutes earlier.
Death respawn (PlayerCharacter::RespawnPlayer) doesn't call LoadCell: GetCellFromCoordinates (13718) with aSpawnCell = false only looks up loaded cells. It does run the vanilla cell-transition drain, though. So fast travel and respawn were where a bad entry left by an earlier teleport blew up. This PR stops those entries from being created.
History
The unguarded path has existed since bb6a450 ("handle lazy loaded cells for moving actors", 2022, 0.9.2).
The teleport command and overlay button came in 147fa7c / 297d028 (2022).
MoveActor, OnNotifyActorTeleport and the server's relinquish → NotifyActorTeleport are identical in v1.8.3 (4917189) and dev. So this isn't a regression; it got likelier as ownership handoffs across cells became more common.
What's verified and what's assumed
Verified (Ghidra 1.6.1170 and 1.7.104 / byte compare):
the queue layout;
the single unlocked push;
no removal on free;
LoadCell → InitItemImpl → push with no drain;
what the pause / resume does, and that it ignores this;
which vanilla callers hold the pause;
the flag bits and offsets used.
Assumed:
Draining right after loading an unattached cell is safe. Vanilla 15328 does exactly this for references it loads into unattached cells.
The exact event that frees a queued reference in failure mode 2.
Pausing can stall the main thread for as long as a running promote task takes to finish, the same as vanilla coc. I don't expect that to be noticeable.
Testing
Client builds (releasedbg, skyrim_version=1.6.1170) on top of current dev. Maybe just coc everywhere.