Tolerate missing juliaupself.json - #1557
Conversation
IanButterworth
left a comment
There was a problem hiding this comment.
Codex findings:
-
P1 — Create juliaupself.json atomically. src/config_file.rs:555 creates/truncates the destination before serializing. Because the launcher reads configuration without locking, it can observe an empty/
partial file and fail. A crash or write error also leaves a corrupt file that this PR does not tolerate. Use the existing temp-file, flush/sync, atomic-rename pattern and then reopen it. -
P2 — Missing config can leave the background updater after uninstall. src/config_file.rs:546 defaults background_selfupdate_interval to None. During self uninstall, that equals the requested disabled
state, so src/command_config_backgroundselfupdate.rs:29 skips uninstall_background_selfupdate(). If the deleted config previously enabled the cron/scheduled task, uninstall leaves it behind. Cleanup
should be unconditional when uninstalling or when configuration state is unknown. -
P2 — The changed behavior has no regression coverage. The config tests are excluded when selfupdate is enabled at src/config_file.rs:788, while the self-update fixture explicitly creates the file at
tests/command_selfupdate_test.rs:146. Add tests for lock-free reads with a missing self-config and atomic mutable bootstrap.
CI also currently fails rustfmt at src/config_file.rs:567; cargo fmt resolves that.
read_config_db and load_mut_config_db previously hard-bailed on any error opening juliaupself.json, including NotFound. This is inconsistent with the main config (juliaup.json), which returns JuliaupConfig::default() on NotFound. The asymmetry meant that a missing juliaupself.json would: - prevent Julia from launching (launcher -> load_config_db_lockfree -> bail) - prevent juliaup from running any mutating command (load_mut_config_db -> bail) - prevent juliaup self uninstall from running (trapped users) This is also the single biggest risk for any future install-layout change, and is independently a fragility bug. Fix: - read_config_db: treat NotFound as JuliaupSelfConfig::default() - load_mut_config_db: on NotFound, create the file with default contents and proceed normally (bootstraps the self-config) - Add Default derive to JuliaupSelfConfig
9e1b45a to
5a97039
Compare
|
Should have addressed P1 (copied the normal juliaup.self mechanism also for the selfconfig file), not sure what's the suggestion for P2 though. I find that wording a bit confusing. |
read_config_db and load_mut_config_db previously hard-bailed on any error opening juliaupself.json, including NotFound. This is inconsistent with the main config (juliaup.json), which returns JuliaupConfig::default() on NotFound.
The asymmetry meant that a missing juliaupself.json would:
This is also the single biggest risk for any future install-layout change, and is independently a fragility bug.
Fix: