bugfix(pathfind): Survive open and closed list cells that lost their PathfindCellInfo - #1
bugfix(pathfind): Survive open and closed list cells that lost their PathfindCellInfo#1wh1ter0se69 wants to merge 1 commit into
Conversation
8995a54 to
a8d85a7
Compare
wh1ter0se69
changed the title
|
Replaced the fix after review feedback from Mauller. The first version removed the
What survived review: the root-cause chain, the reachability analysis (one caller, The correction guards the two readers instead, so nothing observable changes until the frame retail actually faults. Locating those precisely mattered — my original probes checked only the list heads and fired zero times, because TheSuperHackers#2799's fault is at a non-head cell mid-walk. Instrumenting every list-link dereference found it: the guard fires exactly once per replay, at 6238 / 9532 / 12659, and those are the frames the unmodified build crashes at. Result: two of the three replays now complete with no CRC mismatch at all, and the third's only mismatch is at frame 9600, after its fault at 9532. Two corrections to my earlier write-up, both confirmed: only the closed list is guaranteed non-empty at the |
|
Correcting my own characterisation of TheSuperHackers#2934, which was unfair. I wrote that its stated root cause "does not hold up". That overstated it — the observation is literally true and I was denying the premise rather than the inference.
My earlier audit missed it because I scanned for returns between What still holds is the inference, not the premise. But it isn't harmless: the cell is permanently invisible to pathfinding and its info is never reclaimed, since PR body updated accordingly, and |
…PathfindCellInfo Retail strands cells on the open and closed lists when the PathfindCellInfo pool runs dry, and PathfindCell::releaseInfo then refuses to reclaim those infos while m_open or m_closed are still set. A later search can reach such a cell through a list link and publish it as a list head, after which the cell it names has a null m_info. The next reader dereferences it. Guard the two readers rather than the writer. The retail insertion sort now stops when the walk reaches a cell without info and discards the unusable remainder, and putOnClosedList discards a head that has lost its info. Both then fail over the same way the existing detections in releaseOpenList and releaseClosedList do, by setting s_useFixedPathfinding and s_forceCleanCells, so the cells are cleaned up by forceCleanCells at the safe consumption point in cleanOpenAndClosedLists rather than mid-search. This is deliberately a fix at the point of use. The writer side is Pathfinder::checkPathCost returning on allocateInfo failure without unwinding its lists, which was already corrected for the fixed pathfinding path in TheSuperHackers#994; retail compatible mode intentionally keeps the retail behaviour there, so the only correct action in that mode is to survive the consequence. Repairing the writer instead changes simulation state from the moment the pool is exhausted, hundreds of frames before retail crashes, and would desync against an unpatched peer. The added conditions only change behaviour once a cell reachable from a list has a null m_info, which is the point at which retail faults, so nothing observable to the simulation changes before then. Measured against the three repro replays, all three now run to completion with no CRC mismatch at any checkpoint. Addresses the crashes reported in issues 2799 (forwardInsertionSortRetailCompatible) and 2637 (putOnClosedList). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a8d85a7 to
b9d99d4
Compare
|
Ran your branch through the same harness, Mauller —
Your fix is simpler and I'd concede that outright — you check the next link before stepping and sever it, which repairs the dangling pointer at source and means a bad head can't be produced by a later The interesting part is that the guards are not what separates the two results. Deleting only the two failover lines from mine reproduces your numbers exactly on all three replays — so the null-handling is equivalent and the recovery action is the entire difference. Adding My guess at the mechanism, offered as a hypothesis rather than a finding: the repros were recorded on a build that already performed that failover, so failing over the same way at the same frame stays in step with the recording, while severing a pointer and continuing does not. You'd know better than I would whether that's how they were captured. Worth being clear about what this does not show: after the fault there is no retail behaviour to match, since retail crashes there. It's evidence the recovery matches the one the project already uses, not evidence of retail 1.04 CRC compatibility — that still needs the VC6 replay check, which I can't run (no VC6 here, and the fork can't reach the CI game data). |
1 participant
Addresses #2799 and #2637.
What is actually broken
Retail leaves cells linked into the open and closed lists after their
PathfindCellInfois gone.PathfindCell::releaseInfothen refuses to reclaim those infos for as long asm_openorm_closedare set (AIPathfind.cpp:1437, "better leak than crash. jba."), so the flags stay true on cells that are on no live list.A later search reaches such a cell through a list link.
removeFromOpenList/removeFromClosedListinfer "am I the head?" from the cell's ownm_prevOpenand publishlist.m_head = getNextOpen()(1886,1997), so a cell from the abandoned chain becomes the head of the live list. Once the live search releases that chain, the survivors still reference the freed infos, andgetNextOpen()— which ism_info->m_nextOpen->m_cell— names a cell whosem_infois now null.The next reader dereferences it. That is both reported crashes:
forwardInsertionSortRetailCompatible, walking the open list — Game crashes in pathfinding code in function PathfindCell::forwardInsertionSortRetailCompatible TheSuperHackers/GeneralsGameCode#2799putOnClosedList, at the closed list head — Replay crashes on pathfinding TheSuperHackers/GeneralsGameCode#2637The writer that strands the cells is
Pathfinder::checkPathCost, which returns onallocateInfofailure without unwinding its lists. That is already corrected for the fixed pathfinding path by #994; retail compatible mode deliberately keeps the retail behaviour there.Why this fixes the readers and not the writer
RETAIL_COMPATIBLE_PATHFINDINGdefaults to1with no build override (GameDefines.h:99-101),s_useFixedPathfindinginitialises tofalse(AIPathfind.cpp:1113) and is re-zeroed on every map load (4102-4105), andcheckPathCostis on the deterministic simulation path. So retail mode is what every default build runs, and its contract is to reproduce retail bit-exactly including the bug.Repairing the writer changes simulation state from the moment the pool is exhausted, which is hundreds of frames before retail actually faults, and desyncs against an unpatched peer. Guarding the readers changes nothing until a cell reachable from a list has a null
m_info— which is precisely the instant retail dies, and therefore the earliest point at which there is no retail behaviour left to match.The change
currentCell->hasInfo()in its walk, and on stopping at a cell without info discards the unusable remainder instead of dereferencing it. Appending now distinguishes "ran off the end" from "the head itself was unusable".putOnClosedListdiscards a head that has lost its info.s_forceCleanCells, so the infos are reclaimed byforceCleanCells()at the existing safe consumption point incleanOpenAndClosedLists, not mid-search.Both then fail over exactly the way the existing detections in
releaseOpenList/releaseClosedListdo — settings_useFixedPathfindingands_forceCleanCells, soforceCleanCells()runs at the safe consumption point incleanOpenAndClosedListsrather than mid-search.An earlier revision set only
s_forceCleanCells, reasoning that flipping the mode mid-search could route the rest of that search intoreverseInsertionSortwith anm_tailthe retail sort never maintains. Measurement settled it against that reasoning — see the table below. Whether them_tailquestion is real is a pre-existing property of the other detections, not something this change introduces.Measurements
Windows 11, VS2022 BuildTools 17.14, preset
win32-vcpkgRelease, basedc6c595e8, headless, the three repro replays on the!!!pathfinding_testingmap.Before, on unmodified
main, all three crash with0xC0000005— at frames 6220, 9520 and 12640 by a logging-only frame marker.The guard fires exactly once per replay, at the frame retail faults, and the sites match the two reports:
After, all three play to the end with no CRC mismatch at any checkpoint.
Four variants through the identical harness, same base
c090d2882, same script, only the patch differing:main0xC0000005Mauller/fix-retail-pathfinder-crashes)Removing only the two failover lines from this PR reproduces Mauller's results exactly, so the null guards are equivalent and the recovery action is the whole difference. Adding
s_useFixedPathfindingalongsides_forceCleanCells— i.e. the idiom already used byreleaseOpenList/releaseClosedList— is what makes all three replays match their recording end to end.The likely reason, stated as a hypothesis rather than a finding: these repros were recorded on a build that already had that failover, so a build which fails over the same way at the same frame stays in step, while one that only severs a pointer does not. It is worth being precise about what this does and does not show — after the fault there is no retail behaviour left to match, since retail crashes there. It is evidence that the recovery matches the one the project already uses, not evidence of retail 1.04 CRC compatibility.
For contrast, the writer-side version this replaces produced mismatches at frames 5900, 7900 and 11600 — 300 to 1000+ frames before the fault. That gap is what made it wrong.
Runs were gated on the build's own exit code plus a confirmed relink. The replay runner stops at the first CRC mismatch, which for the earlier version landed before the crash frame, so the harness was locally patched — identically on every build compared here — to play through. Numbers above were reproduced on a build with no diagnostic code compiled in.
Not addressed
checkPathCoststill strands cells in retail mode. That is intentional per the above; it is already fixed for the fixed pathfinding path. If it is wanted for retail too, it belongs behind the failover rather than in the normal path, and is a separate change.Pathfinder::patchPathat10660-10671(fixed mode) and10676-10684(both modes), andPathfinder::findAttackPathat10880and10883, which return betweenstartPathfindandm_openList.reset(parentCell). All of them leave a cell withm_openset andm_costSoFar == 0, so the cost checks at6237/6329/7418/8664always reject it. The effect is a permanently unusable cell and a leakedPathfindCellInfo, not this crash. Separate change, and the leak is relevant to pool pressure.GeneralsReplays/GeneralsZH/1.04mismatches at frame 110 on the unmodified baseline in this environment too, becauseTESTING.mdrequires a VC6 build and this was MSVC 2022. The three repro replays above are same-build comparisons, which is what makes them meaningful here. A VC6 replay check is still owed.On the closed AI PR
#2934 guards the same reader sites, which is the right shape. Its stated mechanism is:
The first half is correct, and an earlier revision of this PR wrongly implied it was not.
startPathfinddoes setm_info->m_open = TRUEin retail mode without touchingm_openList(1309-1316), and there is a real path that leaves a cell in exactly that state:Pathfinder::findAttackPathcallsstartPathfind(nullptr)at10871, then returns at10880(!goalCell) or10883(goalCell->allocateInfofailed) — both beforem_openList.reset(parentCell)at10889. On those exits the start cell is left open, unlinked, and still holding aPathfindCellInfo.The second half is what does not follow.
startPathfindalso setsm_costSoFar = 0(1304), and every route to a remover is gated on cost:examineCellsCallback:6237,examineNeighboringCells:6329,groundCellsCallback:7418,checkPathCost:8664.newCostSoFarisfrom->getCostSoFar() + 0.5f*COST_ORTHOGONALwithCOST_ORTHOGONAL = 10(6214,2061), so it is never below 5, and a zero-cost cell failsgetCostSoFar() <= newCostSoFarevery time. Nothing writes a non-zero cost whilem_openis still set, because doing so requires passing those same gates first. So such a cell never reachesremoveFromOpenList/removeFromClosedListand cannot produce these crashes.What it does do is leak: the cell becomes permanently invisible to pathfinding, and
releaseInfowill not reclaim its info whilem_openis set. That is one more contributor to the pool pressure which is the precondition for the crash this PR fixes. So TheSuperHackers#2934 looks to have found a real defect by a mechanism that is not the one that faults — worth its own issue rather than dismissal.AI usage disclosure
Per the AI code generation guidelines: this was produced with LLM assistance (Claude) — the root cause investigation, the instrumentation used to locate the faulting sites, the diff, and a draft of this description. The shipped diff is 29 added and 2 removed lines in one file, 11 of the additions being comment. Every measurement above was executed and observed on real builds rather than asserted, and the first version of this PR was wrong in exactly the way a reviewer would expect an LLM-authored change to be wrong, which is why the correction and its reasoning are stated here in full rather than silently force-pushed.