Skip to content

fix(libkernel): serialize positioned file transfers on Windows - #4973

Merged
oneandonlydean merged 4 commits into
boykopovar:mainfrom
ducloc080202:fix/cq3-positioned-io
Oct 13, 2026
Merged

oneandonlydean merged 4 commits into
boykopovar:mainfrom
ducloc080202:fix/cq3-positioned-io

Conversation

@ducloc080202

@ducloc080202 ducloc080202 commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

What

Rebases #3861 (open but conflicting since Oct 12) onto current main. Serializes Windows file-position transfers with a mutex per descriptor, shared across duplicated descriptors: positioned transfers (NativePositioned_nid_no_patch), plain transfers (NativeTransfer, NativeRead, NativeWrite) and seeks (NativeLseek) of one descriptor can no longer interleave SetFilePointerEx positioning. Entries persist per descriptor number; a lock is only ever held alone, so no new lock order.

Supersedes #3861; that PR can be closed if this one is accepted.

Tested

  • Windows (MinGW 15.2.0, Release): full build passes; new guest_positioned_race (concurrent pread against read+lseek on one descriptor, verifying both sides) fails on main on the first run and passes 10/10 with the fix; file_position, guest_posix_filesystem, guest_filesystem, guest_file_vector, file_streams, guest_aio all pass (7/7). Linux/macOS rely on CI.
  • Cat Quest III (PPSA15065), Windows: loads all 454 asset bundles to the loading screen with no file-read errors. Linux/macOS rely on CI.

Checklist

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Taking this one over #3861. Three things before it can go in:

  • The branch has a merge commit (e2e4ba2a0); please rebase the original commit onto main instead, keeping its author.
  • Open.cpp:46-48: NativeLseek can still run between the save and the restore in NativePositioned_nid_no_patch (Stdio.cpp:186-197), and the restore then overwrites the seek. Take the same lock there.
  • A Windows test that runs pread against read and lseek on one descriptor from two threads and fails on main.

A lock per descriptor instead of one for all files would be better, as asked on #3861.

@oneandonlydean oneandonlydean self-assigned this Oct 13, 2026
@ducloc080202
ducloc080202 force-pushed the fix/cq3-positioned-io branch from e2e4ba2 to e69306f Compare October 13, 2026 08:37
@ducloc080202

Copy link
Copy Markdown
Contributor Author

Done, pushed as 31bcf98..e69306f (branch rebased, no merge commit): 1) original commit cherry-picked with its author kept; 2) the same per-descriptor lock now covers NativeLseek, so a seek between the positioned save and restore can no longer clobber either side; 3) new guest_positioned_race test (pread vs read+lseek on one descriptor, both sides verified) fails on main first run, passes 10/10 with the fix; 4) one mutex per descriptor number, shared across dup, entries persist so close/reuse cannot strand a waiter. The debt entry for the Windows positioned race now describes the serialized behavior.

@nicolasfalesy

Copy link
Copy Markdown
Contributor

Linux build (Arch, GCC 16.2.1, Release, BUILD_TESTING=ON), head f9881adaa merged alone onto main 4bb2c7f: 592 targets fail to link, everything that links libkernel.prx:

/usr/bin/ld: core/libs/libs/unpatched/libkernel.prx: undefined reference to `File::SharePositionedTransfer(int, int)'

File::SharePositionedTransfer is defined in File/src/Stdio.cpp inside the #ifdef _WIN32 block that starts at line 50, but ShareDescriptor in File/src/DescriptorControl.cpp now calls it on every platform (the call sits before that function's own #ifdef _WIN32). Either guarding the call with #ifdef _WIN32 like the GuestFileLockShare_nid_no_patch line below it, or giving Linux an empty definition, should fix it. The CI linux job is skipped for this PR, so it would not show there.

@xXPBSXx

xXPBSXx commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Windows counter-test (follow-up): Windows 10 (19045), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release, this PR merged onto main 2a2a3dc65. guest_positioned_race passes with the PR; with the PR's source files replaced by main's, same test, it aborts at line 74.

Not tested: Linux, a console, a title run.

AI-assisted (Claude Code).

@p3n1cillin

Copy link
Copy Markdown

Data point from a third Unity title, for scope: Cult of the Lamb (PPSA06465) also faults intermittently on Loading.PreloadManager at start-up on Windows. This PR merged onto main a0f5916a2 doesn't change its rate: in 8 alternated 40 s runs per side, there were 5 faults with the PR and 4 without. So its fault is a different race from the one fixed here; details in #5490. This says nothing against the PR for Hotline Miami or Cat Quest III.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 13, 2026
anshikagupta and others added 4 commits October 14, 2026 11:18
The per-descriptor lock exists only in the Windows transfer path, so ShareDescriptor calls SharePositionedTransfer under _WIN32 like GuestFileLockShare. Linux failed to link libkernel with an undefined reference to it.
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Rebased onto main (bf8dc61) and force-pushed. The only conflict was the include list in File/src/Stdio.cpp, which now keeps both <unordered_map> and <utility>. Your three commits keep their authors.
Added one commit: DescriptorControl.cpp:568 now calls File::SharePositionedTransfer under _WIN32, next to GuestFileLockShare_nid_no_patch. The lock is only defined in the Windows part of Stdio.cpp, so libkernel didn't link on Linux. Windows behaviour is unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants