Repository navigation
feat(libSceSaveData): deliver backup completions to a registered event callback - #13551
oneandonlydean merged 21 commits into
Conversation
…taRestoreBackupData and sceSaveDataUmountWithBackup
…e read it sceSaveDataCheckBackupData and sceSaveDataRestoreBackupData now read a backup through one check that runs before anything in the live save changes. A backup directory without a save inside, a missing .param, a .param that is not exactly one SaveDataParam, or a blocks entry that is not a file returns SAVE_DATA_ERROR_INTERNAL, whether or not the caller asked for param. Before, the check threw on a wrong-sized .param, and the restore removed the live save's files before finding the .param missing.
…back a failed swap sceSaveDataRestoreBackupData used to remove the live save's files and copy the backup over them, so a copy that failed half way left neither. It now copies the backup, its .param and its blocks file to _sd_restore_tmp/<dir>, checks that the copy has every entry at its size, and flushes it. Then it moves the live save, .param and blocks file to _sd_restore_old/<dir>, moves the staged copy into place and removes the old one. A failed move puts back everything already moved, so the live save stays as it was. The staging roots sit next to _sd, not in it, where sceSaveDataDirNameSearch would list them. SaveDataSetFileStepHook_nid_no_patch sets a function that is called before each copy and rename of the backup and restore paths with a step name and the path written; when it returns false that step fails. It does nothing unless a test sets it.
…ls, and flush before removing it backupSave moved the previous backup to _sd_backup_old and then renamed the new copy into place; when that rename failed, the save had no backup until the next one. A failed rename now moves the previous backup back into _sd_backup and removes the staged copy. Every staged file is flushed before the swap (FlushFileBuffers on Windows, fsync on POSIX, as libc does), and on POSIX the directories on both sides of each rename are flushed too, all before the previous backup is removed.
Before any copy or delete, the backup and the restore check the roots they use (_sd, _sd_backup and their staging and old directories) and every entry under them for the save: each must be a plain directory or file, not a symbolic link, or on Windows a reparse point such as a junction, and must stay under its root after weakly_canonical. Anything else makes the restore return SAVE_DATA_ERROR_INTERNAL, and the backup report it in its event, with nothing changed. sceSaveDataCheckBackupData checks the backup the same way. The copies walk the tree themselves and refuse a link instead of following it.
Checks behaviour 5fed277 already has: a second sceSaveDataUmountWithBackup replaces the first backup; a backup copy that fails still unmounts and returns 0, with SAVE_DATA_ERROR_INTERNAL in its event and the previous backup kept; another title's titleId is SAVE_DATA_ERROR_NOT_FOUND for the check and the restore; ".." and "a/b" are SAVE_DATA_ERROR_PARAMETER for the check, the restore and the mount a backup needs, and as mount points sceSaveDataUmountWithBackup reports them SAVE_DATA_ERROR_NOT_MOUNTED, as it does for any unknown mount point. The sizes and field offsets of SaveDataCheckBackupData and SaveDataRestoreBackupData are checked with static_assert.
…save Three entries in the libSceSaveData.native debt file: a failed restore leaves the live save as it was, and where a crash during a restore or a backup leaves the save and its backup, since neither is recovered at the next start; and that on Windows directory entries cannot be flushed, so crash safety there rests on NTFS journaling and is not shown.
ntamas94 ran sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup on a PS4 Pro with firmware 10.71's system libSceSaveData.sprx. The console agrees with this library on every case it covers. Two of its rows go in the debt file: another title's titleId returns NOT_FOUND, and sceSaveDataDelete drops the save's backup as this library does, which the entry called unknown. The run also shows the check returning PARAMETER for a param buffer filled with 0xAA, and 0 for a zeroed or null one. Which bytes the console checks was not measured, so the check is not changed; a new entry states the measured cases and that the byte set is unknown.
# Conflicts: # core/libs/tests/GuestSaveDataMountStatus.cpp # docs/dev/debt/libSceSaveData.native.md
# Conflicts: # core/libs/tests/GuestSaveDataMountStatus.cpp # docs/dev/debt/libSceSaveData.native.md
…t callback sceSaveDataRegisterEventCallback and sceSaveDataUnregisterEventCallback were parameterless declarations that threw. They now follow shadPS4 boykopovar#5226: registering before initialization is NOT_INITIALIZED, a null callback is PARAMETER, a second registration replaces the callback and its userdata, and unregistering needs no initialization. Every backup completion that queues an event for sceSaveDataGetEventResult, type 2 from sceSaveDataBackup and type 1 from sceSaveDataUmountWithBackup, is queued for polling first and then handed to the callback registered at that moment. A library thread delivers it: sceSaveDataInitialize3 starts it with scePthreadCreate, as libSceUsbStorage starts its monitor, so the callback runs on a guest thread, and the last sceSaveDataTerminate joins it. Unregistering waits for a running callback and drops completions not yet delivered, and a terminate from inside the callback returns BUSY instead of joining its own thread, as shadPS4 does. guest_savedata_event_callback covers the errors, both event types, the delivering thread, unregistration, replacement, a terminate from inside the callback and an idle terminate followed by a restart.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The event callback registration can race with final termination, leaving a callback registered after shutdown and allowing it to be reused after reinitialization without a new registration.
What I ran
Reviewed head 629477b800f75f29aac25fb104616d189e8a265b against PR base 4bc9e779aa7abb2109c65ee3bca19e1b85df3be8; the individual merge preview against that base was clean. The declared order-only dependency chain is #10674 → #12748 → #13551; #12748 and #13551 preview cleanly against current main, while #10674 currently conflicts with it. Environment: Linux x86-64, Python 3.11.2, GCC 12.2.0.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13551
python3 tools/pr_stack_check.py 13551 --repo boykopovar/AnyPS5 --output-dir /tmp/stack-13551-new
git diff --check 4bc9e779aa7abb2109c65ee3bca19e1b85df3be8...629477b800f75f29aac25fb104616d189e8a265b
git diff --check refs/pr/12748...refs/review/13551
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base refs/review/main-latest --head refs/review/13551 --json
python3 tools/pr_test_quality.py --root /tmp/anyps5-13551-head.0eti4f --json core/libs/tests/GuestSaveDataEventCallback.cpp core/libs/tests/GuestSaveDataMountStatus.cpp
gh pr view 13551 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff and conventions | Precheck passed diff and convention checks; it reported the repository's existing long technical-debt-line warning. Both direct diff checks exited 0 with no output. |
| Dependency/mergeability | #13551 and #12748 preview cleanly against current main; #10674 has a conflict and is declared order-only by #12748. The precheck could not invoke its isolated stack helper, so I reran the stack check directly. |
| Export ABI lexical audit | passed_with_warnings: 15 heuristic findings. Seven refer to save-data exports from the current base absent from the PR branch tree, but the merge preview is clean; one is the new internal worker function in an anonymous namespace. The audit is not a C++ parser or binary ABI test. |
| Test-quality heuristic | Two possible-unregistered-cpp-test warnings are false positives: CMake explicitly builds both files and registers CTest entries. |
| Hosted CI | At refresh, eligibility, conventions, portability, intake, labels, and report were queued. The PR description reports prior repeated Linux/Windows runs, but I did not reproduce them for this head. |
| Local build/tests | Not run; the initial review is no-build, and no exact-head test binary was available. |
Unedited output from focused checks
The convention check emitted this warning and exit summary:
docs/dev/debt/libSceSaveData.native.md:21: warning: [debt-length] Technical debt entry over 1000 characters: state the gap first (what differs from the console, what is unverified, what throws) and keep only the behaviour it needs: 5415 characters
1 findings, 0 errors; GitHub shows at most 10 error annotations, the job summary lists all
The ABI-audit summary command emitted:
status: passed_with_warnings
symbols: 119
findings: 15
kinds: {'public-declaration-not-found': 1, 'definition-not-found': 7, 'export-removed': 7}
{
"findings": [
{
"path": "core/libs/tests/GuestSaveDataEventCallback.cpp",
"line": 1,
"kind": "possible-unregistered-cpp-test",
"message": "Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."
},
{
"path": "core/libs/tests/GuestSaveDataMountStatus.cpp",
"line": 1,
"kind": "possible-unregistered-cpp-test",
"message": "Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."
}
],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}The diff checks exited 0 and emitted no output. The precheck's isolated checkout did not contain the workspace-local ABI, stack, or test-quality helper scripts; I ran those helpers from the workspace against the exact fetched head.
Findings
Blocking
- In
sceSaveDataRegisterEventCallback, theg_initializationscheck happens before taking the callback locks and is not synchronized with the finalsceSaveDataTerminatetransition. If registration reads a nonzero count, then final termination sets it to zero andDetachEventDeliveryclears the callback, registration can subsequently store the callback and return success with no worker. A laterInitialize3starts a worker without clearing that callback, so events may call it after reinitialization even though it was unregistered by termination. Serialize the initialized-state check and callback installation with the final-termination transition, and add a regression test that races registration with final termination and verifies no callback survives a subsequent initialization without registering again.
Non-blocking
The declared order-only dependency chain includes #10674, whose branch currently conflicts with main; this is an integration-order issue, not a request to rebase the dependent PR before its dependency is resolved.
Sources
- Registration and initialization check at the reviewed head
- Final termination and callback detach at the reviewed head
- Callback test registration in CMake at the reviewed head
Not verified
I did not compile the change or run its C++ tests locally. Current hosted checks were queued at the final refresh; PR-description test results are author-reported. I did not verify console behavior or the dependency's eventual merge order.
To get this to a good state
Synchronize registration with the final termination lifecycle and add the concurrent regression case. I will rerun the same no-build checks on the new head; build-dependent CI evidence should also be checked before approval.
…final terminate sceSaveDataRegisterEventCallback read g_initializations before it took any lock. A registration that read a nonzero count could then store its callback after the final sceSaveDataTerminate had set the count to zero and cleared the callback, return OK with no delivery thread, and have the next Initialize3 deliver events to a callback the terminate had dropped. Registration now checks the count and stores the callback under g_slots_mutex, the lock the final terminate holds while it reaches zero and detaches delivery. It takes the calling lock first, the order the delivery thread already uses when a callback calls back into SaveData, so a terminate from inside the callback is still BUSY and the terminate joins the delivery thread outside both locks. The new case races registration against the final terminate on two guest threads for 200 iterations, holding the delivery thread in a callback on every other one so a registration that passed its check waits across the whole terminate. Each must end NOT_INITIALIZED, or OK with the callback cleared, and a backup after Initialize3 must not reach the raced callback.
|
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.
|
Progress report (a4eca2a → 4de3c59)📈 System libraries: 26.05% (+0.01%, +6 implemented functions) ✅ 6 implemented
Changed libraries (implemented exports)
|
|
@EagleBearerYS reviewed at 629477b (own commit on top of #12748). The delivery reads right:
Two requests:
|
…istration sceSaveDataInitialize3 started the callback delivery thread for every title that initializes save data, though nearly none register a callback, and could throw there if scePthreadCreate failed. The thread now starts at the first successful sceSaveDataRegisterEventCallback of an initialization, under the calling, g_slots_mutex and callbacks.mutex locks that registration already takes, so a title that never registers gets no thread and sceSaveDataInitialize3 is again only the count. A replacing registration, one from inside a callback and one after an unregister reuse the running thread; a refused one starts nothing. The last sceSaveDataTerminate still takes the worker out under g_slots_mutex and joins it outside the lock, and only when one was started, so a session that registers between that terminate's detach and its join starts its own thread. The tests run two sessions with no registration, and on Linux count the process's threads to check that Initialize3 and a refused registration start none, that the first registration starts one and later ones reuse it, and that the final terminate joins it.
|
@MarcZX10 @oneandonlydean thanks, both reviews were right. The head is now The registration race (MarcZX10's blocking finding, oneandonlydean's point 2):
The delivery thread starts at the first registration (oneandonlydean's point 1):
Could you both take another look? |
JJenkx's boykopovar#13324 gives isMountedDirectory an std::error_code parameter. The merge passes one in checkBackupData and restoreBackupData and returns SAVE_DATA_ERROR_INTERNAL on a host error, as main's callers do. GuestSaveDataMountStatus.cpp: CheckBackups writes param.json where ResolvePath_nid_no_patch puts /app0/sce_sys/param.json, not under the working directory. boykopovar#13324's CheckHostMountFailures now resolves the first guest path from its mount-host-failures subdirectory, so /app0 maps there, and checkBackupData didn't find the title's param.json.
# Conflicts: # core/libs/prx/libSceSaveData.native/Export.cpp
# Conflicts: # docs/dev/debt/libSceSaveData.native.md
# Conflicts: # docs/dev/debt/libSceSaveData.native.md The pending declarations line keeps main's sceSaveDataGetProgress split and drops the three names this branch implements (sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData, sceSaveDataUmountWithBackup).
# Conflicts: # docs/dev/debt/libSceSaveData.native.md
What
sceSaveDataRegisterEventCallbackandsceSaveDataUnregisterEventCallbackwere parameterless declarations that threw throughNotImplemented_nid_no_patch. They now take shadPS4's signatures and follow its #5226 ("save_data: deliver backup completion callbacks"), as on itsmain1d8004a22f. That covers registration, unregistration, and delivering each backup completion to the registered callback from a thread the library starts.This PR is stacked on #12748, which adds the type-1 event of
sceSaveDataUmountWithBackup.sceSaveDataRegisterEventCallbacksavedata.cppL1345-L1358SAVE_DATA_ERROR_NOT_INITIALIZED. A null callback:SAVE_DATA_ERROR_PARAMETER. Otherwise it stores the callback and userdata. A second registration replaces both.sceSaveDataUnregisterEventCallbacksavedata.cppL1763-L1768Delivery, as in
DispatchBackupEvent(L315-L339) and the backup thread (save_backup.cppL140-L205).sceSaveDataGetEventResultalso delivers the same event to the callback registered at that point. That's type 2 fromsceSaveDataBackupand type 1 from feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748'ssceSaveDataUmountWithBackup.sceSaveDataConvert's type-5 event isn't a backup completion, and shadPS4 doesn't deliver it either.The thread: shadPS4 runs the callback on an emulated kernel thread, so that it has a guest TLS and stack (L82).
scePthreadCreateat the first successfulsceSaveDataRegisterEventCallbackof an initialization, aslibSceUsbStoragedoes for its monitor callbacks. A title that never registers gets no thread, andsceSaveDataInitialize3only counts, as onmain.Registration against the last terminate: Register takes
callbacks.calling, theng_slots_mutex, before it checks the initialization count and stores the callback. The last terminate holdsg_slots_mutexfrom its check through the detach. So a registration racing the last terminate either completes first and is cleared by it, or getsNOT_INITIALIZED. Callback reentrancy is kept, and the join stays outside the lock.Terminate:
SAVE_DATA_ERROR_BUSY, before any other check, as in shadPS4'ssceSaveDataTerminate(L1704-L1726). It never joins the delivery thread from that thread.Unregister also drops completions not yet delivered, so no callback runs after it returns.
Not measured on a console. The debt entry lists what is unknown there:
It also says PS5's signatures are assumed to be PS4's.
Tested
guest_savedata_event_callback(new) runs in its own process. Every wait is bounded at 5 s on a condition variable, with no sleeps.sceSaveDataInitialize3givesNOT_INITIALIZED;PARAMETER.sceSaveDataBackup: the callback gets exactly one event:NOT_FOUND.sceSaveDataUmountWithBackup: exactly one type-1 event, with the same checks.BUSY;NOT_INITIALIZED;NotImplemented_nid_no_patchand the rest of this change kept (run):guest_savedata_event_callbackfails on both runners, aborting at its first case with "sceSaveDataRegisterEventCallback not implemented".guest_savedata*tests, 1 failed, 0 skipped.guest_savedata*tests each, all 10 runs passed, 0 skipped.--repeat until-fail:20: 20 of 20 passed;ctest -R 'savedata|save_data'once: 14 tests, 14 passed. At 629477b everyguest_savedata*test started and stopped the delivery thread with its initializations; since0d0abe09c1only a registration starts it.check_test_registration.pypasses.check_conventions.pypasses with one warning,[debt-length]on the pending-declarations line, which feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748 already has at a longer length (5,607 characters there, 5,415 here).-fsyntax-only: no new diagnostic.bc6b4d76f9, the registration race (MarcZX10's review, oneandonlydean's point 2):CheckRegisterRacingFinalTerminate: 200 iterations of a registration against the final terminate on two guest threads.calling, so the registration waits there while the last terminate runs. The other 100 are a plain race.Fencecallback, and asserts that the raced callback ran 0 times.--repeat until-fail:50on Linux, and ×10 on GitHub's runners (run): 13guest_savedata*tests each on Linux and Windows, all passing, 0 skipped.0d0abe09c1, the delivery thread at the first registration (oneandonlydean's point 1):CheckSessionsWithoutRegistration: two initialize/terminate sessions with no registration, each call returning 0.CheckDeliveryThreadStartsAtRegistration: it counts the process's threads in/proc/self/task, asGuestPthreadStack.cppdoes. Initialize3 and a refused registration add none, the first registration adds one, later ones reuse it, and the final terminate joins it.bc6b4d76f9's code the new cases fail 10 of 10 runs, since the thread count there is 2 where the change gives 1.guest_savedata*tests each on Linux and Windows, all passing, 0 skipped.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.main7fcab08 and with it JJenkx's fix(savedata): keep failed host mounts unpublished #13324 (run).isMountedDirectorycalls, in Check and Restore, take fix(savedata): keep failed host mounts unpublished #13324's newstd::error_codeparameter and returnSAVE_DATA_ERROR_INTERNALon a host filesystem error instead of throwing, consistent with fix(savedata): keep failed host mounts unpublished #13324.CheckBackupsnow writes param.json at the resolved/app0path, since fix(savedata): keep failed host mounts unpublished #13324's test fixes the guest root in a subdirectory first.^guest_savedata_and^savedata_withguest_savedata_event_callback,guest_savedata_mount_statusamong them.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.Claim check (rerun 2026-10-30 17:49-17:53Z):
Overlap:
git merge-treeof this head:main, clean;maind8b60b4 into feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748. It brings feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748's resolution of the one conflict withmain: inlibSceSaveData.native/Export.cpp's include block, feat(savedata): add sceSaveDataMount2 #8285's<string_view>sits beside this branch's includes, and both are kept. It merged without conflicts, and this PR's own diff is unchanged.guest_savedata_*tests,guest_savedata_event_callbackamong them. Linux: 9 run, 9 pass, 0 skip, 0 fail. Windows: 9 run, 9 pass, 0 skip, 0 fail. The libc and libkernel suites run 208 and 309 tests on Linux and 235 and 330 on Windows, with 0 failed. The suites ran withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.main3c3b3cd (run). The one conflict was the pending-declarations list indocs/dev/debt/libSceSaveData.native.md: MarcZX10's feat(libSceSaveData): declare sceSaveDataGetProgress with its known signature #10674 movessceSaveDataGetProgressout of it into its own entry, and the merge keeps that with this branch's removal of the five calls it and feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748 implement. This PR's diff over feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748 is unchanged. The^guest_savetests, withguest_savedata_event_callbackandguest_savedata_mount_status, pass 10 runs in a row. Linux: 9 run, 9 pass, 0 skip, 0 fail. Windows: 9 run, 9 pass, 0 skip, 0 fail. Suites: libc and libkernel pass on Linux (214 and 316) and on Windows (241 and 337). The suites ran withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.mainb28bb77 into feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748 (run). The one conflict was the pending-declarations list indocs/dev/debt/libSceSaveData.native.md, next tomain'ssceSaveDataGetProgressentry, which xecedemon's docs(libSceSaveData): survey the pending exports across citable reimplementations #15077 extends. The merge keeps that entry with this branch's removal of the five calls it and feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748 implement. This PR's diff over feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748 is unchanged. The^guest_savetests, withguest_savedata_event_callbackandguest_savedata_mount_status, pass 10 runs in a row. Linux: 9 run, 9 pass, 0 skip, 0 fail. Windows: 9 run, 9 pass, 0 skip, 0 fail. Suites: libc and libkernel pass on Linux (217 and 319) and on Windows (244 and 340). The suites ran withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)convert(see feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup #12748), it conflicts insceSaveDataBackup, where this PR builds the backup event and callsDeliverEventafter the scopedg_state_mutexlock. Whichever lands second keeps fix(libSceSaveData.native): validate guest pointers across convert, backup, mount, and icon operations #15042's copied-intitle_idand directory name, with this PR's scoped lock and theDeliverEventcall after it.