Repository navigation
fix(savedata): keep failed host mounts unpublished - #13324
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
The mount path now converts filesystem failures to the existing internal error before publishing an alias, slot, or result, and the supplied Linux evidence exercises those failure paths.
What I ran
Reviewed head 8086a37957a2ad182ae9562e10e24687edc2947e against base f91137142eafd5df646fca6419a1fc4ef61b89d9; the merge preview was clean (tree e0a3bd33285606d39994c8a5f3b154df986e87dd). Environment: Debian 12 x86-64, Python 3.11.2, Git 2.39.5. I did not compile or run C++ tests locally.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13324
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base f91137142eafd5df646fca6419a1fc4ef61b89d9 --head 8086a37957a2ad182ae9562e10e24687edc2947e --json
python3 tools/pr_test_quality.py --root /marc/AnyPS5 --json core/libs/prx/libSceSaveData.native/Export.cpp core/libs/tests/GuestSaveDataMountStatus.cpp docs/dev/debt/libSceSaveData.native.md
git diff --check f91137142eafd5df646fca6419a1fc4ef61b89d9...8086a37957a2ad182ae9562e10e24687edc2947e
python3 tools/check_conventions.py --base f91137142eafd5df646fca6419a1fc4ef61b89d9 --head 8086a37957a2ad182ae9562e10e24687edc2947e --title 'fix(savedata): keep failed host mounts unpublished' --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13324-8086a37957a2/pr-body.md
git merge-tree --write-tree f91137142eafd5df646fca6419a1fc4ef61b89d9 8086a37957a2ad182ae9562e10e24687edc2947eI also checked the full diff, mount failure paths, and test registration. I downloaded the author-supplied test archive and verified its SHA-256 against the PR description; the runs below are still author-provided, not runs I performed.
Results
| Check | Result |
|---|---|
| Diff whitespace and conventions/template | Passed. |
| ABI audit | passed_with_warnings; lexical warnings concern exports outside this PR's changed paths. |
| Test-quality heuristic | One possible-unregistered warning; manual inspection confirmed registration in libSceSaveData.native/CMakeLists.txt. |
| Merge preview | Clean against the PR base. |
| Author-provided Linux qualification | Archive SHA-256 matched eb8db3423918202bceaa39eb8ba550d1bf469d715d9f981c6ddeedfc7dd12621. Its logs report 18/18 baseline and 18/18 candidate passes across six suites, a negative-control failure using the baseline module with the new test, and a permission probe that aborted 3/3 on baseline and passed 3/3 on candidate. This was a module/suite qualification, not a clean whole-repository build. |
| Hosted CI | All listed jobs were queued at final refresh. |
Unedited local check output
The no-build precheck reported a tooling-related failure because its isolated worktree lacks the workspace-local ABI and test-quality scripts; both checks were run manually. The ABI audit produced only lexical warnings for exports outside the changed paths.
Test-quality heuristic:
{"findings": [{"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."]}Conventions and merge preview:
All conventions checks passed.
e0a3bd33285606d39994c8a5f3b154df986e87dd
Negative control
| Assertion | Change to code under test | Result |
|---|---|---|
| Failed mount returns an error without publishing state | Author's archive reports running the new test with the baseline module | The new assertions failed on baseline; candidate tests passed. |
Findings
Blocking
None found in the reviewed source and supplied Linux qualification.
Non-blocking
The archive documents Linux behavior only; firmware-specific error mapping remains explicitly unverified in the PR's debt entry.
Sources
- Mount3 error handling and delayed publication at the reviewed commit.
- Failure-state regression cases at the reviewed commit.
- Recorded firmware error-mapping limitation.
Not verified
I did not independently run or build the C++ tests, run Windows/macOS, test a game, or verify the corresponding PS5 firmware error mapping. Hosted CI remained queued.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
adriadam10
left a comment
There was a problem hiding this comment.
The Windows Build fails on this head: guest_savedata_mount_status aborts with FATAL: C++ exception filesystem_error raised ... libSceSaveData.native.prx+0x1459a has no unwind data (run 41095969559).
core/libs/prx/libSceSaveData.native/Export.cpp:689-693: thetry/catchinsceSaveDataMount3cannot work on Windows, because the PRX has no unwind data and the exception cannot be delivered to the handler. The newCheckHostMountFailurescase (a file blocking_sd) hits it.- Fix: do not throw inside
mount3. Use thestd::error_codeoverloads foris_directory,exists,absoluteandcreate_directories(Export.cpp:651, 663-665) and returnSAVE_DATA_ERROR_INTERNALwhenecis set, then drop thetry/catch. The same rule applies toblocks_path/replace_fileif either can throw.
The rest (sidecar written before publishing alias/slot, cleanup of the new empty directory, trace reading output only on success) looks right; Linux is not the problem here.
|
Addressed in Rebuilt the full module and six SaveData suites on Linux: 18/18 baseline and 18/18 change passes; the unprivileged permission regression passes 3/3 on the change. Updated the description with exact-head logs and a clean merge check against main |
|
Windows rerun completed on exact head Updated the description with full Windows build/test logs and JUnit results, SHA-256 |
|
Checked that the |
JJenkx's #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. #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.
What
A host directory-creation failure currently throws across the guest ABI. A block-count write can also fail after the mount alias, slot and result have already been published. Return the existing
SAVE_DATA_ERROR_INTERNALfor filesystem failures, persist the block-count sidecar through the existing replacement helper before publishing the mount, and remove a newly created empty save directory when size persistence fails. Filesystem probes usestd::error_codeoverloads, including mounted-directory identity checks, so ordinary host failures do not depend on exception unwinding through the Windows PRX. Missing directories retain the normal create/open behavior. Failed calls preserve the caller's result and any existing alias; traces inspect mount output only on success.The regression checks a regular file blocking the save root and a directory blocking size-metadata replacement, including slot/alias/result preservation and recovery. The per-host-error correspondence to PS5 errors remains unverified and is recorded in library debt.
Tested
Linux
7.2.9-arch1-1,c++ (GCC) 16.2.1 20260810. Pinned main1c6b316cf762c7a8d34623907d62bc1cba2217b6versus2606c33532a52416ed993f56814441e7d80f1a96.Rebuilt the complete SaveData.native module and test sources using the existing Release compile/link recipes in independent outputs. Both arms use identical SHA-pinned frozen kernel/libc dependencies; this is module/suite qualification, not a clean whole-repository build.
guest_savedata_search,guest_savedata_delete,guest_savedata_convert,guest_savedata_mount_status,guest_savedata_ps4,guest_savedata_cancel: three runs each on each arm, 18/18 baseline and 18/18 change passes, no whole-test skips.jjenkxprocess against a private root-owned save directory: baseline aborts 3/3 withPermission denied; change passes 3/3, returning INTERNAL while preserving the result and leaving no mounted slot.The unmodified overlap checker scanned all 1,444 open PRs against main
58d5af7daa8087d2ffbdb50ae6e0db3cb6a88e48; it reported no completed positive overlap. It could not compare #10674 because that branch conflicts with main. Its shared diff only changes thesceSaveDataGetProgress(float*)signature and corresponding debt, independent of Mount3. A private source merge with #12748 also passes after avoiding unrelated test include/constant conflicts; no combined runtime qualification is claimed. This branch merges cleanly with checked main58d5af7daa8087d2ffbdb50ae6e0db3cb6a88e48.Reproduction recipes, synthetic probe, full suite logs, negative controls and overlap evidence, SHA-256
eb8db3423918202bceaa39eb8ba550d1bf469d715d9f981c6ddeedfc7dd12621(download verified).Follow-up
2606c33532a52416ed993f56814441e7d80f1a96addresses the Windows CI failure reported in review: the previous catch could not receive a filesystem exception through the PRX. The catch is removed and host probes return errors directly. The existing block-size replacement helper already uses error-code directory creation and native/nonthrowing rename operations;blocks_pathonly constructs the sidecar name. The Linux suite and unprivileged permission results above were rerun against this exact follow-up, with identical counts; conventions pass against fetched main4bc9e779aa7abb2109c65ee3bca19e1b85df3be8, and a private merge-tree check against that main is clean. The Windows rerun completed successfully; see the exact-head comparison below.Follow-up source recipes, synthetic test logs, negative control and clean merge receipt, SHA-256
98a946c01c518e72f0081fabed3036e7f9664354c49b78c147cccc90afb85cf8(download verified). The earlier archive above covers the prior head only.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)Windows validation of the follow-up
On Windows 11 x86-64 with WinLibs GCC 15.2 UCRT r7, built the SaveData module/dependencies and ran the six actual guest SaveData suites in one isolated CMake build:
8086a37957a2ad182ae9562e10e24687edc2947e: 5/6 pass;guest_savedata_mount_statusaborts withfilesystem_errorandlibSceSaveData.native.prx ... has no unwind data, reproducing the review failure.2606c33532a52416ed993f56814441e7d80f1a96: 6/6 pass, zero skips. Search, delete, convert, mount status, PS4 and cancel all execute.Windows build commands, full CTest logs, JUnit results and module hashes, SHA-256
75e001074bd44a5c82564673b1fba0b791532074a6b32ff5beda9149e06c297e(download verified). This is focused module/suite qualification, not a whole-project build or game/GPU test. The first setup attempt stopped before compilation because the donor lacked a dependency; the fresh second attempt checked out all 31 pinned dependencies and produced these results.