Repository navigation
fix(io): stage writes through existing output symlinks - #12520
oneandonlydean merged 1 commit into
Conversation
|
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.
|
rooklz
left a comment
There was a problem hiding this comment.
Independent re-run on a second machine and a second OS family. Head 0974e1378e, base 7fea87746b. git merge-tree against current main 2199ee0d47 is clean.
macOS 26.2 arm64, AppleClang, relinker-only Release
- Full ctest: 393/393 pass.
symlink_file_writer_binaryandsymlink_file_writer_textpass. - With main's
FileWriter.cppswapped in, both tests are RED (absolute failed write changed the previous target).
Linux, first run (body says Linux was not run)
- Environment: Ubuntu 24.04 amd64 container, GCC 13,
-fsanitize=address,undefined, standaloneSymlinkFileWriterTests.cpp+FileWriter.cpp+NativePath.cpp. - Head: binary and text both rc 0, with no sanitizer reports.
- Main: both rc 1, with the same message as on macOS.
Mutants of the new block (each run on both modes)
- Killed:
- Read the link once (
parent / read_symlink) instead ofcanonical: fails in the chained case. - Drop the re-
symlink_statusafter resolving: fails in the absolute case.
- Read the link once (
- Equivalent:
statusinstead ofsymlink_statusaftercanonical.canonicalhas already resolved every link, so both calls give the same result. - Survivors (non-blocking):
weakly_canonicalinstead ofcanonical. For a dangling link, this stages a new file at the missing target instead of falling back to the direct write that the body documents ("Dangling links retain direct writes"). The dangling fixture only checks the final link and target, so both behaviours pass. A failure-limit probe on the dangling link would pin it: a direct write leaves a partial file, while staging leaves none.- Dropping
if (error) return false;.canonicalthen returns an empty path, socreateTemporarywrites the payload into./.anyps5-XXXXXXin the current directory before the rename to""fails and the code falls back. Nothing visible remains afterwards, so no test can see it. The guard is necessary; this is only a note that it is not pinned.
Edge probes, head vs main (identical results)
- A link to a writable file inside a read-only directory: staging fails, the code falls back to an in-place write, the file gets
newdata, and the link is kept. - A self-loop link:
canonicalerrors, thenCannot open output file: …/loop.
Gates
check_conventions --base upstream/mainis clean.pr_overlap --branchmatches the body: the only issues arecore/relinker/CMakeLists.txtuncomparables (5021/6713/9793/10861, all pre-existing main conflicts).
LGTM.
|
Correction to my review: the |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
Resolving existing symlinks before the existing staged replacement path prevents failed writes from partially truncating their regular-file targets, and the exact-head fixture passes on Linux here.
What I ran
Reviewed head 0974e1378e1fe2d8046b025f0da367176917c1ab against PR base 7fea87746bde8e14a1d3341af85377c5b53f89ad; it also merges cleanly against current main e4da19aff06e25ae16b90f460dff22e8ac1eb786 (merge tree b45e75d054a65ed76dcd572de514825ffef5f204). Environment: Debian 12, GCC 12.2.0. I compiled the exact PR FileWriter.cpp, NativePath.cpp, and symlink test source as a focused standalone target; the include headers were unchanged by the PR.
git fetch upstream main --quiet
git fetch upstream refs/pull/12520/head:refs/codex/review-12520 --quiet
git merge-tree --write-tree upstream/main refs/codex/review-12520
git diff --check upstream/main..refs/codex/review-12520
git show refs/codex/review-12520:core/relinker/io/src/FileWriter.cpp > /tmp/anyps5-pr12520-review/FileWriter.cpp
git show refs/codex/review-12520:core/relinker/io/src/NativePath.cpp > /tmp/anyps5-pr12520-review/NativePath.cpp
git show refs/codex/review-12520:core/relinker/io/tests/SymlinkFileWriterTests.cpp > /tmp/anyps5-pr12520-review/SymlinkFileWriterTests.cpp
c++ -std=c++20 -O2 -I core/relinker/io/include -I core/relinker/domain/include /tmp/anyps5-pr12520-review/SymlinkFileWriterTests.cpp /tmp/anyps5-pr12520-review/FileWriter.cpp /tmp/anyps5-pr12520-review/NativePath.cpp -o /tmp/anyps5-pr12520-review/symlink_file_writer_tests
/tmp/anyps5-pr12520-review/symlink_file_writer_tests binary
/tmp/anyps5-pr12520-review/symlink_file_writer_tests text
# Negative control: replace only FileWriter.cpp with current main, then build and run the same binary fixture.
git show upstream/main:core/relinker/io/src/FileWriter.cpp > /tmp/anyps5-pr12520-review/FileWriter.main.cpp
c++ -std=c++20 -O2 -I core/relinker/io/include -I core/relinker/domain/include /tmp/anyps5-pr12520-review/SymlinkFileWriterTests.cpp /tmp/anyps5-pr12520-review/FileWriter.main.cpp /tmp/anyps5-pr12520-review/NativePath.cpp -o /tmp/anyps5-pr12520-review/symlink_file_writer_tests_main
/tmp/anyps5-pr12520-review/symlink_file_writer_tests_main binaryResults
| Check | Result |
|---|---|
Merge with current main |
Clean; merge tree b45e75d054a65ed76dcd572de514825ffef5f204 |
git diff --check |
Passed (exit 0; no output) |
| Exact-head binary fixture | Passed (exit 0) |
| Exact-head text fixture | Passed (exit 0) |
Negative control using current-main FileWriter.cpp |
Failed as expected: absolute failed write changed the previous target (exit 1) |
| Hosted conventions, intake, labels, eligibility | Passed |
| Hosted Ubuntu and Windows relinker, relinker ASAN | Passed |
| Hosted macOS arm64 and x86_64 relinker jobs | Pending at review time |
| Hosted product Linux and Windows jobs | Skipped |
The PR description reports 393/393 macOS relinker CTests passing. An independent review reports additional macOS and Linux sanitizer runs; I did not reproduce those runs locally.
Unedited focused test output
The PR binary and text fixture commands both exited 0 and printed no output.
Negative-control output:
absolute failed write changed the previous target
negative_control_exit=1
Hosted checks:
relinker (macos-latest, clang++) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122975693955
conventions pass 14s https://github.com/boykopovar/AnyPS5/actions/runs/40732960482/job/122973872364
intake pass 20s https://github.com/boykopovar/AnyPS5/actions/runs/40732960200/job/122973870083
labels pass 11s https://github.com/boykopovar/AnyPS5/actions/runs/40732960463/job/122973870937
relinker (ubuntu-latest, g++) pass 5m20s https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122975694092
relinker (windows-latest, g++) pass 8m16s https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122975693587
relinker-asan pass 2m42s https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122975693648
relinker (macos-latest, clang++, x86_64) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122975693885
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122975696561
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122975696733
windows skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122975696999
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122975696356
eligibility pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/40732959805/job/122973866547
Negative control
| Assertion | Change to code under test | Result |
|---|---|---|
| An incomplete write through an existing symlink must preserve the previous regular-file target | Replaced only FileWriter.cpp with current-main code; kept the PR fixture unchanged |
Fixture failed with absolute failed write changed the previous target; the PR implementation passes |
Findings
Blocking: None found.
Non-blocking: The direct-write fallback for dangling links and special files retains the existing partial-write behavior, and concurrent filesystem mutation is not covered. The PR description states these limits. The independent review’s correction about weakly_canonical is consistent with the tested dangling-link behavior; I found no remaining blocker in that path.
Sources
- Updated file writer at the reviewed head
- Symlink failure and replacement fixtures at the reviewed head
- Test registration at the reviewed head
- Contribution rules at the reviewed base
Not verified
I did not run macOS or Windows locally, exercise actual disk-full conditions, or stress concurrent symlink replacement. The PR author and an independent reviewer report macOS coverage; the independent reviewer also reports Linux sanitizer coverage.
To get this to a good state
No changes are needed for this reviewed diff. If the head changes, I’ll rerun the focused binary/text fixtures and negative control on the new head.
What
A failed write through an output symlink could truncate the previous target. Resolve existing symlinks before applying the existing POSIX staging-and-replacement path, retaining absolute, relative and chained links. Windows is unchanged.
This intentionally extends #5018's regular-file replacement semantics to existing regular symlink targets: mode bits are retained, but the target gets a new inode; other hard links and open descriptors keep the previous bytes, and ownership, ACLs and extended attributes may change. Dangling links and special files retain direct writes. The existing fallback when staging or rename fails remains, so this does not guarantee preservation for every failure or provide multi-file transactions or crash durability.
Tested
macOS 27.2, Apple M5, AppleClang 21, native arm64 Release relinker-only build; synthetic fixtures, no game files.
Independent review found no P0–P2 findings. The overlap scan covered 1,362 open PRs; manual CMake comparisons for ten PRs with pre-existing main conflicts confirmed this registration adds no shared-file conflicts.
Linux and Windows were not run locally. The file-size limit simulates partial writes; actual disk-full and concurrent filesystem mutation were not tested.
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)