fix: replace config.xml atomically instead of truncating it

CLAUDE.md

@@ -562,6 +562,21 @@ named `deleteMultiplePartialMatchIsNotDetected`, pinning the gap as known rather
 **A test that records a weaker behaviour with a name saying so is a defect somebody chose to
 describe** — read it as a lead, not as a decision, and check what its siblings do.
 
+**A file rewritten in place, with nothing to fall back on.** `FileHandler::save()` did
+`ftruncate(0)` and then `fwrite()`, so the file is empty on disk between the two. `config.xml`
+goes through it, and it holds the database credentials, the password salt and the master-password
+hash — a process killed in that window leaves an installation that cannot boot *and cannot be
+repaired through the UI*, because the container is built before `Init` runs and the install route
+refuses once `<installed>` is set. There is no backup to fall back on either:
+`ConfigBackupService::backup()` exists and is called from nowhere in `src/`.
+
+It is now a sibling temp file renamed into place, which is atomic within a filesystem. The lock was
+never what protected readers and could not have been — `XmlFileStorage::load()` hands the *path* to
+`DOMDocument` and `readToString()` reads by path, so neither ever took it. **Ask what a half-written
+file would cost before asking who holds the lock**; and note the two things a rename has to carry
+over by hand: the target's permissions, and a stat cache that would otherwise still describe the
+replaced file.
+
 **A static factory a subclass cannot use.** `SPException` offers `error()`, `info()`, `critical()`,
 `warning()` and `from()`, each doing `new static($message, …)`. Four subclasses fix their own message
 and take `int $type` first instead — `AccountPermissionException`, `UnauthorizedActionException`,

src/Infrastructure/File/FileHandler.php

@@ -44,11 +44,24 @@ final class FileHandler extends SplFileObject implements FileHandlerInterface
 {
     public const  CHUNK_FACTOR = 3;
 
+    /**
+     * @inheritDoc
+     */
+    /**
+     * The mode is kept because `save()` no longer writes through this handle — it replaces the
+     * file by renaming a sibling over it, which the directory's permissions allow whatever this
+     * handle was opened for. Refusing a read-only handle has to be explicit now that the stream
+     * will not refuse it for us.
+     */
+    private readonly string $mode;
+
     /**
      * @inheritDoc
      */
     public function __construct(private readonly string $file, string $mode = 'r')
     {
+        $this->mode = $mode;
+
         parent::__construct($this->file, $mode);
     }
 
@@ -134,19 +147,88 @@ public function read(): iterable
      */
     public function save(string $data): FileHandlerInterface
     {
+        // `r` without `+` is the only read-only family; w, a, x and c all open for writing.
+        if (str_starts_with($this->mode, 'r') && !str_contains($this->mode, '+')) {
+            throw FileException::error(sprintf(__('Unable to read/write the file (%s)'), $this->file));
+        }
+
         $this->lock();
 
-        $this->rewind();
-        $this->ftruncate(0);
+        try {
+            $this->replaceWith($data);
+        } finally {
+            $this->unlock();
+        }
 
-        if ($this->fwrite($data) === false) {
+        return $this;
+    }
+
+    /**
+     * Put `$data` in place of this file's contents, without the file ever being partly written.
+     *
+     * This used to be `ftruncate(0)` followed by `fwrite()`, which has a window in which the file
+     * is empty on disk. `config.xml` goes through here, and it holds the database credentials, the
+     * password salt and the master-password hash — so a process killed in that window (an OOM, a
+     * container stopped mid-save, the host losing power) left an installation that cannot boot and
+     * cannot be recovered through the UI, because the container is built before `Init` runs and the
+     * install route refuses once `<installed>` was set. Nothing takes a backup first:
+     * `ConfigBackupService::backup()` exists and is called from nowhere.
+     *
+     * A sibling temp file renamed into place cannot show a half-written state to anybody, because
+     * `rename()` within a filesystem is atomic. That is what readers need, and the lock was never
+     * going to give it to them: `XmlFileStorage::load()` hands the *path* to `DOMDocument`, and
+     * `readToString()` reads by path too, so neither has ever taken this handle's lock. The lock
+     * stays because it still orders two writers that hold the same open file, and because losing
+     * one of two concurrent saves is a different and much smaller problem than losing the file.
+     *
+     * The temp file is created private and given the target's own permissions before the rename,
+     * so the mode does not change and the contents are never briefly readable at the umask.
+     *
+     * @throws FileException
+     */
+    private function replaceWith(string $data): void
+    {
+        // Per-process, so two of them cannot write into each other's temp file.
+        $temp = sprintf('%s.%d.tmp', $this->file, getmypid());
+
+        $mode = file_exists($this->file)
+            ? (fileperms($this->file) & 0777)
+            : (0666 & ~umask());
+
+        $handle = @fopen($temp, 'wb');
+
+        if ($handle === false) {
             throw FileException::error(sprintf(__('Unable to read/write the file (%s)'), $this->file));
         }
 
-        $this->fflush();
-        $this->unlock();
+        try {
+            @chmod($temp, 0600);
 
-        return $this;
+            if (fwrite($handle, $data) === false) {
+                throw FileException::error(sprintf(__('Unable to read/write the file (%s)'), $this->file));
+            }
+
+            fflush($handle);
+        } finally {
+            fclose($handle);
+        }
+
+        try {
+            @chmod($temp, $mode);
+
+            if (!@rename($temp, $this->file)) {
+                throw FileException::error(sprintf(__('Unable to read/write the file (%s)'), $this->file));
+            }
+        } catch (FileException $e) {
+            @unlink($temp);
+
+            throw $e;
+        }
+
+        // This handle still refers to the file that was just replaced. Nothing reads through it —
+        // every read in this class goes by path — but clearing the stat cache keeps getFileSize()
+        // and getFileTime() answering about what is now on disk.
+        clearstatcache(true, $this->file);
     }
 
     /**

tests/Unit/Infrastructure/File/FileHandlerTest.php

@@ -90,6 +90,124 @@ public function testSavePersistsToDisk(): void
         self::assertSame('saved content', file_get_contents($this->file));
     }
 
+    /**
+     * The file is replaced, not truncated and rewritten.
+     *
+     * `config.xml` goes through `save()`, and it holds the database credentials, the password salt
+     * and the master-password hash. `ftruncate(0)` followed by `fwrite()` has a window in which
+     * that file is empty on disk, and a process killed in it — an OOM, a container stopped
+     * mid-save, the host losing power — leaves an installation that cannot boot and cannot be
+     * recovered through the UI. Nothing takes a backup first.
+     *
+     * The inode is the observable part of the fix: a rename gives the path a different file, where
+     * a truncate keeps the same one. It is also exactly the property that makes the replacement
+     * atomic for a reader, which is what this is for.
+     *
+     * @throws FileException
+     */
+    public function testSaveReplacesTheFileRatherThanTruncatingIt(): void
+    {
+        file_put_contents($this->file, 'the old contents');
+        $before = fileinode($this->file);
+
+        (new FileHandler($this->file, 'c+'))->save('the new contents');
+
+        clearstatcache(true, $this->file);
+
+        self::assertSame('the new contents', file_get_contents($this->file));
+        self::assertNotSame($before, fileinode($this->file), 'the file must be replaced, not truncated');
+    }
+
+    /**
+     * A save that cannot be completed leaves the file exactly as it was.
+     *
+     * This is the failure the change is about, and the only way to reach it deterministically here
+     * is to make the temporary file impossible to create — a directory in its place does that for
+     * root as well, which is what the suite runs as. Before the change the write went straight into
+     * the file, so the same conditions replaced its contents instead of preserving them.
+     *
+     * @throws FileException
+     */
+    public function testASaveThatCannotBeCompletedLeavesTheFileAsItWas(): void
+    {
+        file_put_contents($this->file, 'the old contents');
+
+        // Where save() wants to put its temporary file.
+        mkdir(sprintf('%s.%d.tmp', $this->file, getmypid()));
+
+        $handler = new FileHandler($this->file, 'c+');
+
+        try {
+            @$handler->save('the new contents');
+            self::fail('a save that cannot write its temporary file has to throw');
+        } catch (FileException) {
+            // asserted below: what matters is what is left on disk
+        } finally {
+            @rmdir(sprintf('%s.%d.tmp', $this->file, getmypid()));
+        }
+
+        clearstatcache(true, $this->file);
+
+        self::assertSame('the old contents', file_get_contents($this->file));
+    }
+
+    /**
+     * Replacing the file keeps the permissions it had, rather than giving it the umask's.
+     *
+     * `config/config.xml` is 0644 with its *directory* held at 0750, and that arrangement is
+     * deliberate — a save must not quietly change either half of it.
+     *
+     * @throws FileException
+     */
+    public function testSaveKeepsThePermissionsOfTheFileItReplaces(): void
+    {
+        file_put_contents($this->file, 'contents');
+        chmod($this->file, 0640);
+
+        (new FileHandler($this->file, 'c+'))->save('new contents');
+
+        clearstatcache(true, $this->file);
+
+        self::assertSame(0640, fileperms($this->file) & 0777);
+    }
+
+    /**
+     * And it leaves nothing beside the file it wrote.
+     *
+     * @throws FileException
+     */
+    public function testSaveLeavesNoTemporaryFileBehind(): void
+    {
+        (new FileHandler($this->file, 'c+'))->save('contents');
+
+        self::assertSame([$this->file], glob($this->dir . DIRECTORY_SEPARATOR . '*'));
+    }
+
+    /**
+     * After a save, this handle still describes what is on disk.
+     *
+     * It refers to the file that was just replaced, so this is the thing a rename could plausibly
+     * have broken: `ConfigFile::isExpired()` compares the config cache against
+     * `$this->fileStorage->getFileTime()`, and a stale mtime there would leave the cache looking
+     * current after every save. `SplFileInfo` stats the pathname rather than the open descriptor,
+     * and `save()` clears the stat cache for it, so both answers follow the rename.
+     *
+     * @throws FileException
+     */
+    public function testTheHandleStillDescribesTheFileAfterASave(): void
+    {
+        file_put_contents($this->file, 'old');
+        touch($this->file, time() - 60);
+
+        $handler = new FileHandler($this->file, 'c+');
+        $before = $handler->getFileTime();
+
+        $handler->save('new and longer contents');
+
+        self::assertGreaterThan($before, $handler->getFileTime(), 'the mtime must follow the rename');
+        self::assertSame(strlen('new and longer contents'), $handler->getFileSize());
+    }
+
     /**
      * @throws FileException
      */