Write config files atomically, and start with defaults when one is corrupt

[notluquis]

References

• First half of Unexpected token ' #824, and the SyntaxError half of An error pops up: A javaScript error occured in the main process #431
• Split out of Warn when a config file could not be read, and ignore values of the wrong type #1101, which keeps the second half (value checking, the user-facing notice) and sits on top of this
• Keep the config keys this build has no setting for #1118 touches the same read() and save() and conflicts with this (measured with git merge-tree); landing this first is the cheaper order. Config reads and writes: four things #1101 leaves open #1114 and A config value the reader rejects is deleted from the file at the next save #1116 carry what neither attempts

Code changes

The app was writing the corrupt files it then could not start from. All three config writes went straight at the destination with fs.writeFileSync, which truncates first and writes second, and one caller is the will-quit handler, the moment the OS is most likely to kill the process.

Writing is now a mechanism helper under a policy one, following Ensuring data reaches disk: temporary in the same directory, fsync, rename, fsync the directory. That last step is the one VS Code, write-file-atomic and atomically all skip, and it is what makes the new name survive a power cut. realpath first so a config symlinked into a dotfiles repo keeps both its link and its atomicity. openSync with 'wx', because 'w' follows a symlink planted at <config>.<pid>.tmp and a pid is guessable, which under sudo jlab truncates whatever it names; fchmod/fchown through the descriptor for the same reason. 0600 when there is nothing to carry, since app-data.json holds recentRemoteURLs and those carry a token. It returns false and never throws, because will-quit calls it between preventDefault() and _quit().

Ownership is carried from the containing directory when there is no file to copy from, so a config file that appears for the first time under sudo matches the directory it lands in rather than being created root:root and locking every later unprivileged run out of it. This covers the case where the user data directory already exists and only the file is missing, which is what happens when a config is deleted or when a new one is written for the first time. It does not cover a genuinely first-ever run under sudo: getUserDataDir creates that directory itself, so it is root-owned before the writer sees it and the fallback reads root:root. Carrying the owner onto the directory belongs with that function and is listed under Remaining.

Reading no longer throws. Both read() methods parsed with no guard from constructors that run during module import, so the throw landed before app.whenReady with nothing to soften it. An unreadable file is now left in place, marked, and writes to it refused for the run: VS Code's rule rather than electron-store's clear-and-continue, because troubleshoot.md tells users to hand-edit settings.json.

Not treated as corruption: a byte order mark (Notepad, Out-File), and empty or NUL-padded files, which is #881 and also what the old non-atomic write left behind.

User-facing changes

• A corrupt config no longer stops the app starting. Telling the user is Warn when a config file could not be read, and ignore values of the wrong type #1101; here it reaches the log.
• jlab config set/unset, jlab env set-* and jlab env update-registry report a refused write instead of printing success over it, and exit non-zero, so jlab config set … && deploy.sh does not run the deploy over a write that never happened. jlab env create is the exception and reports without a status, because the function it goes through is also reached from the GUI, where an exit would take the running app down. That split is in Remaining.
• File permissions survive a save. A config created for the first time is 0600 where it holds a secret, which is the app's own directory; a project's .jupyter/desktop-settings.json keeps the umask default master used, since a project directory shared between two accounts is a real place for it.
• With a config the app could not read, a setting changed from the GUI is applied to the window and then dropped, with a line in the log and nothing on screen. The eleven jlab config/jlab env paths report it and exit non-zero; the twelve GUI callers in app.ts, sessionwindow.ts and welcomeview.ts discard the boolean. This is the deliberate cost of starting at all instead of throwing during import, and it is the state most people who hit Unexpected token ' #824 will land in, so it is a call worth making explicitly: the shared way to surface it belongs with the flag-on-the-object change on Config reads and writes: four things #1101 leaves open #1114, not in a branch this size. Warn when a config file could not be read, and ignore values of the wrong type #1101's dialog is not it either, since that fires once at startup rather than per save.

Backwards-incompatible changes

With an unreadable config, settings changed that session are not persisted until it is repaired. Before they were persisted by overwriting the file with defaults, which is the data loss this exists to stop.

A config directory that is not writable now fails the save, where master's in-place write succeeded (measured both sides, uid 501, 0555 directory holding a 0644 file). Creating the sibling temporary needs directory write permission. VS Code, write-file-atomic and atomically all fail in this case too rather than degrade, so this follows them, but on this branch alone the failure only reaches the log; the notice is #1101.

A hard-linked config is detached. rename replaces the directory entry, so the target inode is dropped: a settings.json hard-linked into a dotfiles repo starts at nlink 2, and after one save the config is nlink 1 with the new content while the other name is frozen at the old one, silently. Measured on both sides; master's in-place writeFileSync kept nlink 2 and updated both. Detecting it would mean either refusing the write or giving up the atomicity this exists for, so it is written down rather than handled. Symlinks are handled, including chains.

Shape

writeJsonConfigFile had reached ninety-five lines carrying fourteen concerns. Counted the same way, write-file-atomic carries seven in two hundred and sixty-nine, and all seven are mechanism: it holds no refusal policy, does no JSON, and takes the mode and owner as arguments rather than deciding them.

Six of the fourteen were the first kind and eight the second, so they are now two functions along that line. writeJsonConfigFile decides, at fifty-eight lines: which files are refused, how a symlink resolves, whether a new file is 0600 or takes the umask, the JSON, the parent directory, the ownership carry-over. writeFileAtomicSync publishes, at fifty-four: the unpredictable temporary, the wx open, fchmod, fsync, the close ordering, rename, the directory sync, and removing what it created. It throws once it has cleaned up, so the caller still decides what a failed save means and nothing observable changed.

What made the split worth doing rather than another pass: the review rounds were getting quieter, three then two then two then one, while the function grew twenty-seven per cent, so the count was not the thing to read. The pass before last returned three findings and all three were interactions between things earlier passes had added.

All 648 existing tests pass untouched, which is what says the behaviour is the same. Mutating the wx flag, the rethrow and the temporary cleanup each turns tests red. Mutating the close ordering did not, so there is now a test that the descriptor is not closed twice when the close itself fails.

Manual testing

macOS, fourteen corrupt shapes through the real Electron runtime: all fourteen start, none leaves a temporary, the file is unchanged. Six kill the process on master, and what master does is a throw during load rather than a slow start:

App threw an error during load
SyntaxError: Unexpected end of JSON input

which is the title of #881. Both read paths are covered and they are not the same one: #881 names app-data.json at zero bytes, #824 shows settings.json.

Also run on windows-latest: config-fs.test.ts runs 12 of 20 there, skipping the symlink, mode and umask cases, so Windows covers the write and the recovery and none of the ownership work. Since #1112 merged the unit job runs on all three platforms here as well.

Every new branch mutation-checked: each turns exactly one test red, source restores byte for byte. That includes the directory-owner fallback, whose test the old code could not fail, because the existing root test stubs a file that already exists and so never reached the branch.

Four rounds of fixes came out of a code review over this branch, each reproduced here before being changed: the workspace data loss above, the two torn shapes that were being marked, and the reporting path in cli.ts that no test could reach because the module mocks would have thrown before the first assertion.

Not done: none of the Review guidance checks were run on Windows or Linux. The ownership behaviour is covered by unit tests with getuid stubbed rather than by a real root run.

Remaining

macOS fsync	flushes to the drive, not through its cache; Apple points at F_FULLFSYNC, which Node does not expose
quit is slower	will-quit saves both files, and each now does a file fsync and a directory fsync synchronously, so four blocking syncs where master had two page-cache writes. Visible on a spinning disk or a network home, and on macOS it is latency paid without the durability, per the row above. Accepted: the alternative is the truncated file #824 is made of
Windows rename	libuv uses MoveFileExW; Microsoft recommends ReplaceFile, which preserves ACLs and fails less against a process holding the target
stray temporaries	a process killed between open and rename leaves <name>.<pid>.tmp. A sweep would let a second instance delete the first's in-flight file. #1114
read-only config file	rename needs the directory, not the file, so a 0444 settings.json is now replaced where writeFileSync failed. #1114
refusal keyed by path	the invariant belongs to the object that read the file. #1114
ownership of the user data directory	getUserDataDir creates it, so a first-ever run under sudo leaves the directory root-owned before any config is written and the file's own fallback has nothing user-owned to copy. #1114
no route back from a marked file	resetConfigFile and getUnreadableConfigFiles have no production caller here; the notice that uses them is #1101, so on this branch alone a marked file stays marked for the run. #1114
a marked global splits the session	WorkspaceSettings.read() calls super.read(), so every new session window re-reads settings.json. Once it is marked, those constructions keep their defaults and a new window launches on the default theme and pythonPath while the singleton still holds the real values. The write side is guarded, the read side is not, and this is where master threw instead. #1114. The reproducible case is a file that was fine at import and is edited into invalid JSON while the app runs: the next window's super.read() fails, marks the path, and that window alone falls back to defaults while the userSettings singleton still holds the real ones, so two windows in one session disagree about the theme and the Python path. Corruption already present at startup does not show it, because the singleton got defaults too
jlab env create exits 0 on a refused write	it goes through createPythonEnvironment, which app.ts also imports, and the exit is switched off for that reason. Separating the two entry points is the fix. #1114
success on a write that did not happen	WorkspaceSettings.save() returns true when the merge produced nothing and no file exists, so jlab config set --project theme system prints success and exits 0 while the value stays pinned to whatever the global holds. master's behaviour; the contract that makes it wrong is this branch's. #1114
a transient read error is permanent	one EIO or ESTALE on a network home marks the path exactly as a parse failure does, with no retry. #1114
the GUI drops the refusal	save() returns a boolean and the eleven callers in cli.ts check it, which is the non-zero exit above; the twelve in app.ts, sessionwindow.ts and welcomeview.ts discard it, so a Zen toggle against a marked settings.json applies to the window and is gone at quit with nothing said. Reachable only because this branch lets the app start on that file at all, and not answered by #1101, whose dialog fires once at startup rather than per save. Detail and the shape of the fix are on #1114, with the flag-on-the-object change it belongs to
a long stderr run truncates the message	process.exit drops a write still queued on a pipe. Measured against this repo's electron 42 with stderr on a pipe nobody drains: short output and 60 KB deliver the message and exit 1, at 200 KB and above the message is gone while the status survives. A deferred exit from the write callback was tried and reverted, because under Electron main.ts calls app.quit() in the same then and the deferred form exits 0 five runs out of five, giving up the status too. The realistic jlab config set path is well under the threshold; jlab env update-registry is the one that could reach it, since new Registry() puts electron-log output on stderr first
root still walks a planted directory link when creating parents	carryOwnershipOntoPath now opens with O_NOFOLLOW, but the mkdirSync(parent, { recursive: true }) above it resolves components normally, so under sudo a link planted before the call has the tree built through it. Closing that needs mkdirat walking one component at a time against directory descriptors, which Node does not expose, so it is written down rather than half-done

AI usage

• YES: Some or all of the content of this PR was generated by AI.
• : The human author has carefully reviewed this PR and run this code (keep this PR "draft" until the answer is YES)
• AI tools and models used: Claude Code, Opus 5

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.