Skip to content

Commit 1410158

Browse files
committed
fix(config): stop writing over a config.json that was never read (#2356)
`MempalaceConfig` read `config.json` behind `Path.exists()` and folded every failure into an empty dict, and each setter then serialized that dict back over the whole file through `open(..., "w")`. A config hand-edited into something that does not parse, one trailing comma or a byte-order mark an editor added, was enough to lose every setting in it: one `mempalace init` in another directory turned a 219-byte config into 25 bytes holding one key, with `palace_path`, `people_map` and `embedding_model` gone and nothing in the output saying so. Files that do not parse also come from the setters themselves. On a 3,617,966-byte config, one `set_hook_setting` call is a window in which the file on disk is not the config: a reader in another thread got 2 reads that would not parse against 3 that would, and ten of ten runs killed inside it left 508,435 to 1,770,851 bytes behind. Reading now separates four states rather than two. `FileNotFoundError` is the only one that establishes there is no config here, because nothing resolved under that name. A file that reads but is not a JSON object is renamed aside before a fresh config is written, with its new name printed, so its bytes stay recoverable. A file that exists and cannot be read at all is not touched: what is in memory then is this session's defaults, and writing those there is how a permission bit or an absent volume becomes a lost config, so the setter says why it declined instead of returning as though it had saved. Writing goes through a temporary file, an fsync and a rename. Eight other places in the package publish a write by a rename, `entity_registry`, `palace_graph`, `hallways`, `repair`, `replica`, `server_registry`, `logstream` and `migrate`; two of them sync the file first and one syncs the directory, and `hallways.py` states the reason for the rename in a comment. The temporary file carries this process's pid rather than a random suffix, so two processes writing the same config never share one. A signal between the write and the rename leaves that file behind, and nothing here removes it. The temporary file is opened `O_NOFOLLOW`, and the directory the rename happened in is fsynced afterwards, which is what makes the rename itself survive a crash and what `EntityRegistry.save` already does. Its mode is set before anything is written rather than after, since a umask that clears the owner's write bit would otherwise leave it at 0400 for as long as it exists. A name the open itself refuses, a directory dropped at it or an orphan this user cannot write, sends the write to a name the directory picks rather than to a write without the rename: the errno of a name this call chose says nothing about the directory, and the errno of a name the directory chose says everything. An orphan this user can write is opened and reused instead, with its link count checked first so a hard link is never truncated through, so the directory is not asked until the rename, and a directory that refuses the rename falls back there. The temporary file holding that write is removed only after the write in place returns, and named if it could not be removed. Two cases keep the previous behaviour deliberately. A config reached through a symlink is written through it, since renaming over the link would replace it with a regular file and leave the real one, in a dotfiles checkout, holding what it held. And a directory that will not take a temporary file gets the write in place, with a message saying the crash-safety is missing: writing to an existing config needs only the file, while a temporary one needs the directory, and losing the setting outright is the worse half of that trade. A UTF-8 byte-order mark stopped counting as a parse failure, since `json.loads` accepts one on bytes; the UTF-16 and UTF-32 marks it also accepts still go to the quarantine. A write that fails is reported rather than swallowed by `except OSError: pass`; the exit status is unchanged, since no setter returns one and cli.py reads none. `save_people_map` writes its own file the same way, and a setter that creates ~/.mempalace restricts it to the owner while leaving an existing one alone. A directory that cannot be made still raises out of the three setters and save_people_map that created it outside any try on develop, and is still reported by set_hook_setting, which did not create it there at all. Thirty-four tests in tests/test_config_unreadable_rewrite.py, twenty-four of which fail against the previous config.py. Thirty-six mutations of the new code are killed by them, among them: an unreadable file counted as absent, the fail-closed exit removed, the quarantine skipped, the quarantine renaming nothing, the write going straight into the file, the swallowed write error restored, the JSON-object check widened, the symlink replaced rather than written through, the BOM treated as a parse failure, the read-only-directory fallback removed, the created directory left world-readable, the directory fsync dropped, the temporary file following a symlink, any write failure falling back to writing in place, a directory that cannot be created raising instead of reporting, the message dropping the reason the read failed, the write in place announced before it is attempted, and the directory helper swallowing why it could not make the directory.
1 parent fae7de0 commit 1410158

3 files changed

Lines changed: 1096 additions & 48 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
1010

1111
### Bug Fixes
1212

13+
- **A `config.json` this process could not read is no longer written over.** `MempalaceConfig` reads the file once and falls back to an empty dict on any failure, and every setter then serializes that dict back over the whole file. One unreadable read was therefore enough to lose every setting the file held: a config hand-edited into something that does not parse, one trailing comma or a byte-order mark an editor added, went from 219 bytes to `{"backend": "chroma"}` at the next `mempalace init --backend`, with `palace_path` back to the default and `people_map` gone. Such files also come from the setters themselves, since one `set_hook_setting` on a 3.6 MB config is a window during which the file on disk is not the config, and ten of ten runs killed inside it left a truncated file behind. The file is now read as bytes, and only `FileNotFoundError` counts as "there is no config here". A file that does not parse is renamed aside, with its new name printed, before a fresh one is written; a file that exists and cannot be read at all is not touched and the setter says so instead of returning as though it had saved. Writes go through a temporary file named after this process and are renamed into place, so an interrupted write leaves the previous config exactly where it was, and a setter that cannot write reports it rather than swallowing the error. `people_map.json` is written the same way. A config reached through a symlink is written through it rather than replaced, so a dotfiles checkout keeps receiving the settings; a directory that will not take a temporary file gets the write in place with a message rather than losing the setting; a UTF-8 byte-order mark is no longer read as a parse failure; and a setter that has to create `~/.mempalace` restricts it to the owner. (#2356)
1314
- **A palace with no database is no longer reported as one that passed its integrity check.** `sqlite_integrity_errors` answers `[]` when `chroma.sqlite3` is absent, and the MCP gate published that as `checked: true, ok: true`. Absence is now decided by `ENOENT` alone, which proves that nothing resolves under the path, and reported as the not-applicable shape #1931 introduced, `checked: false`/`ok: null` plus a reason. Every state that is not proven absent reaches the probe, and a probe that cannot open the file reports `PRAGMA quick_check failed`, which trips the existing `-32002` refusal: a dangling symlink, a database under an unreadable directory, a symlink loop, a name the filesystem rejects, an embedded NUL in the path, and, on POSIX, a palace path whose parent is a file. A palace directory named with a byte that is not valid UTF-8 reached the probe and, up to Python 3.12, raised out of it, which `mempalace mine` and `mempalace repair` never guarded against; it is now reported like every other unreadable path. `/statusz` reads an absent verdict as healthy, so the new `ok: null` does not turn a fresh install red, and non-chroma backends stop reporting themselves unhealthy, which they had done since the #1931 fix. The size-limited startup skip still publishes a clean verdict; the only change there is that it no longer inherits the previous probe's absence reason. (#2290)
1415

1516
---

0 commit comments

Comments
 (0)