Repository navigation
fix(relinker): replace output files instead of rewriting them in place - #5018
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CnRJay
left a comment
There was a problem hiding this comment.
Writing to a sibling file and renaming over the output is the right approach for ETXTBSY and for partial writes. The fallbacks for symlinks and non-regular files are sensible.
One case I'd change: the fixed <output>.tmp name. If a relink is killed between writeFile and moveOver (Ctrl+C or SIGKILL; catch (...) only covers exceptions), the .tmp is left behind. Every later relink of that output then sees an existing .tmp and silently falls back to the in-place write. That brings back exactly the failure this PR fixes: Cannot open output file after the modules have already been rewritten, and SIGBUS for a running process. Nothing tells the user why. A unique name in the same directory avoids that and still never touches an unrelated file. For example, mkstemp on <output>.XXXXXX, then writing through that descriptor, or opening a generated name with O_CREAT | O_EXCL. A unique temporary also has the right mode from the start if it is created with the previous permission bits, rather than created and then changed with permissions().
This also overlaps with #4802. Once outputs are replaced by rename on Linux and macOS, a hard link between two outputs no longer makes one overwrite the other: each rename replaces only its own name. That removes most of the reason for #4802's hard-link identity probing on those hosts. #4802's case-insensitive and same-path checks are still useful.
A relink killed between writing <output>.tmp and renaming it left that file behind, and every later relink of the output then fell back to the in-place write, bringing back the failure on a running build. The temporary file is now created under a new name beside the output with O_CREAT | O_EXCL and the previous permission bits, and written through that descriptor, so a file left by an earlier run never blocks a replacement and is never touched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks, good catch. Fixed in 26c62cc: the temporary file now gets a fresh name beside the output ( On #4802: agreed. With the rename, two hard-linked outputs no longer write through each other on Linux and macOS. Windows still writes outputs in place, so the identity check still matters there. |
|
macOS data point. Apple M2 Max, macOS 27.2 (26B5091g), AppleClang, Ninja Release, relinker-only. Head
Harness (APFS, links
So on macOS Hard links (same on any POSIX host), as listed in the description: an output that is a hard link is now replaced by a new inode, so the other name keeps the old contents ( Written with AI assistance (Claude Code). I ran the builds and tests above on my machine. |
|
Thanks for the macOS run and the harness, especially the Rosetta and |
|
You're right, it is listed under "Behaviour changes for replaced outputs"; I missed it. I've corrected my comment above. |
|
Checked the temp-file-and-rename path in |
What
On Linux, relinking while the previous build is still running fails with
Cannot open output file: …/game.elf(ETXTBSY), but only after the guest modules have been rewritten in place. The running build's loaded modules change under it, and a process that has one mapped gets SIGBUS when the module gets shorter. The output directory is left with new modules beside the previous executable and registries. Separately, a write that fails part way, for example on a full disk, leaves a truncated output.On Linux and macOS,
FileWriternow writes a regular output to a new file beside it and renames it over the output, assavedata::replace_filedoes. The new file gets a name that does not exist yet:<output>.plus six random characters, created withO_CREAT | O_EXCL. It gets the previous file's permission bits and is written through that descriptor. Open files keep their previous contents, and a failed write leaves the previous file whole. An output is still written in place, as before, in these cases:A relink killed between the write and the rename leaves its temporary file behind. That name is never used again, so later relinks are not affected. (The first revision used
<output>.tmp, and a file left there made every later relink fall back to the in-place write; thanks to CnRJay's review.)Behaviour changes for replaced outputs:
Windows is unchanged. It refuses to replace a running executable or a loaded library as well, so only the partial-write case would improve there, and that needs the rename handling #3660 added for Windows readers.
Tested
Ubuntu 24.04, GCC 13.3, relinker-only build with
BUILD_TESTING=ON: 145/145 tests pass on this branch, and 155/155 with it merged intomain016db92.FileWriterTests.cpp(Linux) gains four checks, run for both overloads:sleepruns from the output path with a descriptor open on it. The write succeeds, and the path has the new contents. The write runs underumask 077, so the previous permission bits (0770) must be restored exactly. The descriptor still reads the previous contents, the process keeps running, and no temporary file is left.<output>.tmpby an earlier, killed relink. The output is still replaced, and that file is not touched.RLIMIT_FSIZEfails and keeps the previous file, with no temporary file left.<output>.tmpis left alone.With main's
FileWriter, the first two checks fail withCannot open output file, and the third leaves a truncated file. With this PR's first revision, the second check fails withCannot open output file. Without thefchmodthat restores the previous permission bits, the first check fails.End to end, with the test fixtures (a bundled module and
--registry): a copy ofsleepruns from the output path, and another process maps a longer previous guest module. Relinking again:Other output states give the same result as main, re-run with this revision. They are a fresh output (same contents and modes), an existing 0755 output, a symbolic-link output, a writable output in a read-only directory, an
<output>.tmpsymbolic link (its target is left alone), and a leftover<output>.tmpfile.Not covered:
FileWriteras well.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)🤖 Generated with Claude Code