Skip to content

test(libkernel): cover cancellation recovery for #1668 - #1739

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
Sanjaysrinivas:test/pthread-cancel-recovery
Oct 10, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
Sanjaysrinivas:test/pthread-cancel-recovery

Conversation

@Sanjaysrinivas

@Sanjaysrinivas Sanjaysrinivas commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

What

Refs #1668. Add cancellation recovery regressions and documentation alongside the implementation already merged in #1141.

The branch is based on main 4a747c4872814908702fe81db6a20405a53a07d7 and contains one complementary commit, 982f7d7817be2e06af99e7518238d80d1551b9de. Its four changed files are:

  • core/libs/tests/GuestPthreadCancelRecovery.cpp
  • core/libs/prx/libkernel/CMakeLists.txt
  • docs/dev/ARCHITECTURE.md
  • docs/dev/TechnicalDebt.md

The regression checks that pending cancellation preserves semaphore tokens and join ownership; repeated requests produce one exit; cleanup runs in reverse order before thread-specific destruction; a cancelled condition waiter reacquires its mutex before cleanup; and fresh threads can reuse the same resources with enabled/deferred defaults. Atomic gates and mutex handshakes establish ordering without scheduling sleeps. Assertions remain active in Release. Normal and APS5_COARSE_TIMED_WAITS=1 cases each have a 20-second timeout.

The documentation records the lifecycle, pinned FreeBSD reference contract and remaining coverage gaps. Asynchronous behavior, file/socket cancellation points, sleep interruption, cancellation versus exit/detach, condition signal conservation and non-null invalid/reclaimed handles remain incomplete or unverified. No console title or homebrew establishes cleanup ABI compatibility.

The cancellation coverage entry sits inside ### Functional, near the thread cancellation material and before MIMG entries. It is not appended at the end of the section. No cancellation implementation changes are included.

Tested

Synthetic guest-API tests; no console, title or homebrew measurements.

The independent review on 93951bfc reports Linux, GCC 15.2.0, Release, BUILD_TESTING=ON, full all + libs builds, with 20 passing repetitions each of:

  • guest_pthread_cancel
  • guest_pthread_cancel_recovery
  • guest_pthread_cancel_recovery_coarse
  • guest_pthread_key
  • guest_pthread_sem
  • guest_cond_timedwait

For this documentation correction, the CMake registration, recovery test and architecture documentation were verified byte-identical between 93951bfc and the restored head 982f7d78. Only the technical-debt entry's placement changed. Local conventions and whitespace checks pass. Runtime tests were not rerun for this correction; the reviewer results above are attributed to their tested head.

Full Linux/Windows PR builds require a collaborator-triggered workflow under the repository's current CI policy. Automatic PR checks do not establish full runtime test coverage.

Checklist

@Sanjaysrinivas
Sanjaysrinivas marked this pull request as ready for review October 8, 2026 18:41
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@Sanjaysrinivas
Sanjaysrinivas force-pushed the test/pthread-cancel-recovery branch from a8875dd to 52fdd15 Compare October 9, 2026 20:58
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 10, 2026

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked on Linux (merged onto current main): builds, and guest_pthread_cancel, guest_pthread_cancel_recovery, guest_pthread_cancel_recovery_coarse, guest_pthread_key, guest_pthread_sem and guest_cond_timedwait pass 5/5 repetitions.

  • This branch carries #1141's two implementation commits (c2186de0e, 638e5df04), so it can't be merged as is: it would land #1141's code without that PR's review, and #1141 still has open review requests. CONTRIBUTING asks for one topic per PR.
  • Please wait until #1141 lands, then rebase so the PR contains only GuestPthreadCancel.cpp, GuestPthreadCancelRecovery.cpp, ARCHITECTURE.md, TechnicalDebt.md and the CMake test registration. We'll review it right after.
  • The TechnicalDebt.md entry should not sit last in its section; place it away from other entries to avoid adjacency conflicts.

@Sanjaysrinivas
Sanjaysrinivas force-pushed the test/pthread-cancel-recovery branch from 52fdd15 to f9b221e Compare October 10, 2026 11:12
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
@Sanjaysrinivas

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (a29ef5921) and built and ran the tests here. Each point from the review:

  1. Prerequisite commits — gone. The rebase kept only the complementary commit; git diff --name-only is now exactly the four files, and feat(libkernel): implement thread cancellation #1141's c2186de0e / 638e5df04 are no longer in the branch. For the record feat(libkernel): implement thread cancellation #1141 landed as 3acca27f2 (merge be403084c), whose Thread.cpp carries a refinement on top of the branch's version, so this now adds only the tests and docs.
  2. Files to review — all four: core/libs/tests/GuestPthreadCancelRecovery.cpp, core/libs/prx/libkernel/CMakeLists.txt (test registration), docs/dev/ARCHITECTURE.md, docs/dev/TechnicalDebt.md.
  3. TechnicalDebt.md entry — no longer last in its section and no longer adjacent to the cancellation entry that used to share the section end. On current main it lands between Thread cancellation and MIMG `tfe` and `lwe` , mid-list inside ### Functional. The entry text is unchanged.

Build and test evidence on this head

Ubuntu 24.04, g++ 13.3.0, cmake 4.4.4, ninja 1.13.2, -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON:

[103/105] Linking CXX shared library core/libs/libs/unpatched/libkernel.prx
[104/105] Linking CXX executable tests/guest_pthread_cancel_recovery_tests

1/3 Test #567: guest_pthread_cancel ...................   Passed    0.18 sec
2/3 Test #568: guest_pthread_cancel_recovery ..........   Passed    0.01 sec
3/3 Test #569: guest_pthread_cancel_recovery_coarse ...   Passed    0.01 sec
100% tests passed out of 3

guest_pthread_cancel_recovery was also run 5 repetitions with --repeat until-fail:5 — 100% passed each time. The binary links the libkernel.prx built from this head. So this reproduces your result independently, on the rebased commit rather than on the old branch merged onto main.

Please disregard my earlier comment on this PR, which said I had no reachable toolchain here. That was wrong: it described the state before a sandbox-policy change in my environment, and I repeated it without re-testing. It is corrected above.

One unrelated observation, for whoever maintains build.yml

linux/windows — the only jobs that run cmake --build build --target libs and the full ctest — are gated on github.event_name == 'push', so on a pull request the libs target is never compiled and ctest never runs; only relinker, register-context and write-watch build anything. This is pre-existing on main, not something this PR introduces, and the four source files here are not compiled by PR CI at all — which is why the run above was done by hand. Flagging in case that gate is meant to be narrower than it currently is.

@github-actions github-actions Bot mentioned this pull request Oct 10, 2026
9 tasks done
@orishamir

Copy link
Copy Markdown
Contributor

Checked 66412d96 merged onto current main (57f96fd2; clean merge) on Linux (Ubuntu 26.04 in WSL2, GCC 15.2.0, Release, BUILD_TESTING=ON, full all + libs build):

  • The branch now carries only the four files (CMakeLists.txt registration, GuestPthreadCancelRecovery.cpp, ARCHITECTURE.md, TechnicalDebt.md), checked against the merge base; the original commit 52fdd150 had the same four, so the rebase dropped nothing.
  • guest_pthread_cancel, guest_pthread_cancel_recovery, guest_pthread_cancel_recovery_coarse, guest_pthread_key, guest_pthread_sem, guest_cond_timedwait: 20/20 repetitions each.
  • The three mutations from the description reproduce, applied one at a time to main's libkernel with the test unchanged: dropping the ThreadCancel::Check() before the scePthreadSem* / sem_* waits (both recovery tests fail at GuestPthreadCancelRecovery.cpp:94, the thread joins with its normal result); letting ThreadCancel::Requested() ignore exiting (fail at :162, a second exit during cleanup); clearing cancelPending in scePthreadSetcancelstate(ENABLE) (fail at :94). Sources restored, all six tests green again.
  • The TechnicalDebt.md placement is the opposite of what the review asked. On this head the entry is the last line of the file (471/471; 472/472 merged), appended after Null vertex buffer V#, and 66412d96's message describes moving it to the end of the section "as the review asked", while the review said it should not sit last in its section. That line is where new entries land: feat(libkernel): implement _fstatfs #2858 and perf(agc): sample comparison-read R32F/R16 surfaces from a GPU depth copy of their storage image #1619 insert at the same place on current main (the overlap report above), so whichever of the three merges first conflicts the other two.

AI-assisted: yes (Claude Code).

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
Refs boykopovar#1668. Complements boykopovar#1141 with resource ownership and lifecycle regressions.
@Sanjaysrinivas
Sanjaysrinivas force-pushed the test/pthread-cancel-recovery branch from 66412d9 to 93951bf Compare October 10, 2026 19:31
@orishamir

orishamir commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

On 93951bfc (rebased onto 4a747c48, two commits): CMakeLists.txt, GuestPthreadCancelRecovery.cpp and ARCHITECTURE.md are byte-identical to 66412d96 (the PR's own changes diffed against each base), so the rebase changed nothing there.

The TechnicalDebt.md entry moved the wrong way again. The rebased commit 982f7d78 had it at line 336, directly under Thread cancellation and dup and dup2, which is what the review asked for. 93951bfc ("move the cancellation coverage entry off the shared insertion point") takes it from there and appends it as the last line of the file (483/483, after _fstatfs from #2858, merged since). That line is the shared insertion point: #1619 still appends there. To be exact, #1619 conflicts with current main by itself, as it appends at a29ef592's last line; with its line placed on current main, 982f7d78 merges cleanly and 93951bfc conflicts in TechnicalDebt.md. Dropping 93951bfc alone leaves the entry where 982f7d78 put it.

Built 93951bfc on Linux (GCC 15.2.0, Release, BUILD_TESTING=ON, all + libs): guest_pthread_cancel, guest_pthread_cancel_recovery, guest_pthread_cancel_recovery_coarse, guest_pthread_key, guest_pthread_sem, guest_cond_timedwait 20/20 repetitions each.

AI-assisted: yes (Claude Code Fable 5.1 xhigh)

@Sanjaysrinivas
Sanjaysrinivas force-pushed the test/pthread-cancel-recovery branch from 93951bf to 982f7d7 Compare October 10, 2026 19:41
@Sanjaysrinivas

Copy link
Copy Markdown
Contributor Author

Removed 93951bfc; the head is now 982f7d78. The cancellation coverage entry is restored inside ### Functional, near the thread cancellation material.

Updated the description to reflect the merged #1141 and four-file scope. Conventions and whitespace checks pass. The test and CMake registration are unchanged; runtime tests were not rerun for this documentation-only correction.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 10, 2026

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked on Linux (merged onto current main): the new guest_pthread_cancel_recovery and _coarse tests build and pass together with guest_pthread_cancel, guest_pthread_key, guest_pthread_sem and guest_cond_timedwait over 10 repetitions; the documentation claims match Thread.cpp, Cancel.hpp and TimedWait.cpp. Also confirmed by @orishamir (20 repetitions, WSL2/GCC 15.2). Merging.

@adriadam10
adriadam10 merged commit a3c43ad into boykopovar:main Oct 10, 2026
14 of 15 checks passed
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