Skip to content

fix(libkernel): preserve semaphore wakeups across cancellation - #6241

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
Ondra97:codex/fix-semaphore-canceled-wakeup
Oct 18, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
Ondra97:codex/fix-semaphore-canceled-wakeup

Conversation

@Ondra97

@Ondra97 Ondra97 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

What

Two guest threads can wait on an empty semaphore while one post selects a waiter. If that waiter is canceled after unregistering its cancellation wake but before reacquiring the semaphore mutex, it exits without consuming the token or waking its peer. scePthreadSemGetvalue reports 1 while the survivor remains blocked.

I forward one notification in ThreadCancel::WaitUntil before unlocking and delivering cancellation. The peer rechecks its predicate under the existing mutex and consumes the original token. Token accounting and ordinary posts are unchanged. The helper serves SCE/POSIX semaphore waits and the condition-based join path; the wake contract is described by sem_post.

The regression controls the interleaving through the existing mutexes and uses the actual guest APIs. It covers timed/untimed SCE waits, disabled cancellation and cancellation without a post, accepts spurious wakes through bounded retries, and rescues the survivor after a failed handoff so both threads can be joined.

The rebase preserves the upstream guest_pthread_timed_locks registration and both canceled-wake registrations. The production change and fixture are identical to previous head 5d8189f79e2cc284d8e7e135ac0f69b7cbb66f6e. Related #6091 addresses condition-signal ownership and #6740 addresses semaphore-object lifetime; neither changes this helper. #6103 is already merged into the base. No dependency is needed.

Tested

Base: 66702f1cffd3646f2d674ed093fde0a85a70761f.
Head: 57f353df3e131cd719e3df9d11639b0b29c2cc2c.
Both builds use Release, Ninja and BUILD_TESTING=ON.

  • Windows (native host): Windows 11 Pro 10.0.26300, supported WinLibs MinGW-w64 GCC 15.2.0; AMD Ryzen 9 7900X, 32 GB RAM, NVIDIA GeForce RTX 4070 Ti SUPER. Full build including libraries/NID patching passed with cmake --build build-runtime --parallel 4. All 22 selected runtime tests passed, no skips; 127 patched libraries loaded, none failed. Both Windows wait backends were confirmed in the logs. All 40 additional canceled-wake runs passed.
  • Linux (VirtualBox): Ubuntu 24.04.5 LTS, kernel 7.0.0-38-generic, GCC 13.3.0, VirtualBox 7.2.20r175154 on the Windows host above; 4 vCPUs, 8 GiB guest RAM. Graphics: not exercised. Full build including graphical components and NID patching passed with cmake --build <build> --parallel 2. The final serial set had 21 passes, 1 timeout (precise_wait), 0 skips; 127 patched libraries loaded, none failed. All 40 additional canceled-wake runs passed. The coarse registration uses the same backend as the default registration on Linux.

The initial Linux (VirtualBox) run with -j 2 had a precise_wait timeout and one guest_cond_timedwait abort. Both passed on an isolated serial rerun. Restoring only the base Cancel.hpp reproduced the precise_wait timeout; the condition test passed all three baseline repetitions and the final serial set. precise_wait_tests and libc.prx retained identical SHA-256 hashes across that comparison and do not depend on libkernel. Its configured timeout remains 10 seconds. I did not change code, test expectations or timeouts to obtain a pass. The precise cause of the intermittent timing failures is unresolved; this is not a fully passing Linux test set.

For each negative control below, I kept the final fixture, restored only Cancel.hpp from the base, rebuilt and ran both registrations. The fixed header was restored in finally, its SHA-256 verified, and the full build rerun.

Environment / metric Before (base header) After (fix)
Windows: canceled-wake registrations passing 0/2 2/2
Linux (VirtualBox): canceled-wake registrations passing 0/2 2/2
Enabled cancellation, timed and untimed SCE waits, both environments Survivor blocked; 1 token available Survivor completed; 0 tokens available

Conventions, test registration, whitespace, current-main synchronization and independent read-only patch review passed. Only the three original PR files are changed.

Focused runtime commands

Use -j 2 on Windows and -j 1 for the final Linux (VirtualBox) run:

ctest --test-dir <build> --output-on-failure -j 1 -R '^(patched_libraries|precise_wait|guest_(execute_once|std_execute_once|once|thread_self|pthread_(start|sem|cancel(_recovery(_coarse)?)?|timed_locks|join_self|second_join|key|destructor_lifetime)|semaphore|sem_canceled_wake(_coarse)?|cond_(timedwait|recursive_mutex|unowned_mutex)|libc_cnd_timedwait))$'
ctest --test-dir <build> --output-on-failure -R '^guest_sem_canceled_wake(_coarse)?$' --repeat until-fail:20

The default process environment has APS5_COARSE_TIMED_WAITS unset; CTest sets it to 1 for the coarse registrations.

Limits: no retail title run. The forced cancellation-after-wakeup race is measured through SCE semaphore APIs; existing POSIX cancellation/semaphore and join tests also ran. This does not establish console scheduler ordering or fix the separate sceKernelWaitSema issue #6006. Linux graphics/Vulkan tests: NOT RUN — intentionally excluded from VirtualBox validation. Native/bare-metal Linux was not tested. Native Linux GPU validation: not tested. Full hosted platform builds require collaborator dispatch under CONTRIBUTING.md; skipped jobs are not passes.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes — Codex. I am responsible for the change.

This was referenced Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 4972 core/libs/prx/libkernel/CMakeLists.txt
PR 5021 core/libs/prx/libkernel/CMakeLists.txt
PR 6044 core/libs/prx/libkernel/CMakeLists.txt
PR 6226 core/libs/prx/libkernel/CMakeLists.txt
PR 6908 core/libs/prx/libkernel/CMakeLists.txt
PR 6936 core/libs/prx/libkernel/CMakeLists.txt
PR 6977 core/libs/prx/libkernel/CMakeLists.txt
PR 7192 core/libs/prx/libkernel/CMakeLists.txt
PR 7201 core/libs/prx/libkernel/CMakeLists.txt
PR 7332 core/libs/prx/libkernel/CMakeLists.txt
PR 7450 core/libs/prx/libkernel/CMakeLists.txt

@Dextroid17

Copy link
Copy Markdown
Contributor

I ran this on Linux, since your runs were on Windows. The added condition.NotifyOne() in Cancel.hpp lets the second waiter take the token on Linux too, and your test fails when that line is removed.

Setup: head 5d8189f79 (merge-base with main d92a8dad3), Release, BUILD_TESTING=ON, Ninja, CachyOS (kernel 7.2.9), i9-11900K, GCC 16.2.1. The head, fix-reverted and merged checks each ran ctest -R '^guest_sem_canceled_wake' (2 tests) 3 times, under env -i with only HOME and PATH set.

  • Head: 2 of 2 pass in each of the 3 runs.
  • Fix reverted: with Cancel.hpp as on d92a8dad3 and your test kept, both tests fail 3 of 3 with a canceled awakened waiter stranded an available semaphore token. The cases with cancellation enabled (disabled=0) print survivor_returned=0 unclaimed_tokens=1 for both the timed and untimed wait, which is the stuck waiter and leftover token you describe. With the fix they print survivor_returned=1 unclaimed_tokens=0.
  • Merged with main at 9bf01db66: clean merge, 2 of 2 pass in each of the 3 runs. tools/check_conventions.py passes.
  • 16 copies at once (head only): I started the test binary directly, 16 copies at once and 25 runs each, so 400 runs, under the same env -i plus LD_LIBRARY_PATH. All 400 passed. A second batch with APS5_COARSE_TIMED_WAITS=1 also passed 400 of 400, but that adds nothing on Linux: Coarse() in Time/TimedWait.cpp reads the variable only under _WIN32 and otherwise returns true. For the same reason, guest_sem_canceled_wake_coarse runs the same path as the default test here.

From reading the code: the new NotifyOne also runs when the cancelled thread was not the one woken, or when nothing was posted. In those cases scePthreadCancel (Pthread/src/Thread.cpp:884) has already sent NotifyAll to the condition while the thread was registered, so the new line adds one more wake. That looks harmless, because a woken peer rechecks predicate() under the same lock and waits again. Your cancel-without-post case checks that the token count stays at 0 there. The new line runs only when cancellation is pending, so a timed wait that is not cancelled is unchanged.

Overlap: this PR merges cleanly with #6103 (head 58a1e712c, which you mention) and with #6908 (head 571556754, which wakes POSIX sleep for cancellation and also adds a test to libkernel/CMakeLists.txt). I only ran git merge-tree for those, with no build or tests.

Not checked: Windows (including the fine-grained and coarse wait backends your coarse test targets), macOS, PS5 hardware, and a retail title. My runs only used the SCE semaphore calls in your test. The POSIX sem_wait/sem_timedwait path (WaitFor in Posix/Sem.cpp) uses the same helper, but I only read that code and did not run it.

Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.

AI-assisted: yes

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 17, 2026
@Ondra97
Ondra97 force-pushed the codex/fix-semaphore-canceled-wakeup branch from 5d8189f to 57f353d Compare October 17, 2026 16:48
@Ondra97

Ondra97 commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

Could a maintainer review this PR, especially the cancellation wake handoff in ThreadCancel::WaitUntil and its semaphore/join callers? I rebased it onto main (head 57f353df3e), and it now merges cleanly. Local build/test results and the remaining Linux (VirtualBox) timing-test limitations are documented in the description.

AI-assisted: yes — Codex.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 17, 2026
@adriadam10 adriadam10 self-assigned this Oct 18, 2026
@adriadam10
adriadam10 merged commit 3a14fdb into boykopovar:main Oct 18, 2026
8 of 9 checks passed
@Ondra97
Ondra97 deleted the codex/fix-semaphore-canceled-wakeup branch October 19, 2026 04:24
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.

3 participants