Skip to content

Make Mac config upgrades safely resumable - #321

Open
CptPanko wants to merge 3 commits into
omacom:quattrofrom
CptPanko:fix/mac-upgrade-idempotent-config
Open

Make Mac config upgrades safely resumable#321
CptPanko wants to merge 3 commits into
omacom:quattrofrom
CptPanko:fix/mac-upgrade-idempotent-config

Conversation

@CptPanko

@CptPanko CptPanko commented Sep 2, 2026

Copy link
Copy Markdown

Summary

  • reuse one durable, complete pre-upgrade config backup across retries
  • stage and publish the Hyprland and Omarchy config transition safely across interruptions
  • preserve repaired and symlinked active layouts instead of replacing them on retry
  • narrow the upgrade guide to the config-transition guarantees the script now enforces

Testing

  • focused Mac upgrade test: 28 assertions passed, including two concurrent production lock holders on ARM64
  • command metadata and parser-aware syntax checks passed
  • ARM64 ./test/all comparison: 2,672 passing assertions versus 2,656 on untouched base; the same 16 baseline assertions and the same 22 of 233 test files fail on both

Fixes #315

@scottjones

Copy link
Copy Markdown
Collaborator

Reviewed this alongside #319 and #322, since all three rewrite bin/omarchy-upgrade-to-quattro-mac. Flagging a cluster-level problem here and on #322 before going line-by-line, because the answer changes what a detailed review should be checking.

The three PRs merge cleanly, and that is hiding a real conflict.

I trial-merged them: #319+#321, #319+#322 and #321+#322 all merge without conflict, and each merges cleanly into current quattro. That's only because they append to disjoint regions of the file. Semantically they are not disjoint at all — merged, that 390-line script becomes roughly 1400 lines carrying two different resumability designs:

  • This PR adds a durable state directory (~/.local/state/omarchy/upgrade-to-quattro-mac/), flock held on fd 9 for the script's lifetime, and six state/marker files with identity checks. Explicit and durable. It leaves main() untouched.
  • Retire seamless login during Mac upgrade #322 solves the same resumability problem by inferring state from the live system (SDDM config, drop-ins, legacy artifacts), with no state directory and no lock. It is also the only one of the three that changes main().

Neither could account for the other, and the concrete collision is this: after both land, main() is #322's version, which wraps update_bashrc, replace_hyprland_config and run_quattro_setup in if (( ! login_cleanup_only )). In that mode replace_hyprland_config — your rewritten version that publishes the staged config transition — is skipped, while backup_user_config has already taken the lock and written its markers. The upgrade would exit reporting success with the config transition staged but never published.

Neither test suite covers that, because neither of you could know about it. git merge being clean is doing a lot of work to conceal it.

What I'd like: rebase this and #322 onto each other and let them be reviewed as one unit, with a single answer to "how does this script track resumable state." I don't think that's a redesign — your state directory and lock look like the right foundation, and #322's login work could plausibly sit on top of it rather than beside it. But it needs to be one design, decided deliberately, rather than two that happen not to collide textually.

Since they're both yours, you're best placed to reconcile them; happy to take direction on which way you'd rather go.

To be explicit about depth: I reviewed this at the architecture level — the state/lock/marker design and how it interacts with the other two PRs — not line-by-line across all +1120. I'll do a full pass once the resumability question is settled, so I'm reviewing the shape that's actually going to land.

@malik-na

malik-na commented Sep 9, 2026

Copy link
Copy Markdown
Member

Please combine this with #322 before further review. The durable markers should be the single source of truth for both config and login transition state, with one test covering an interrupted cleanup-only retry.

@malik-na

malik-na commented Sep 11, 2026

Copy link
Copy Markdown
Member

Needs work: reconcile this with #322. In the combined flow, cleanup-only mode reports success while skipping required config publication and setup. The existing suites pass; a production-main check with independent file assertions catches the failure. Use one completion model and cover interrupted retries through main. The earlier config blockers remain: validate both trees before adoption, publish atomically on the destination filesystem, and preserve directory metadata.

@scottjones

Copy link
Copy Markdown
Collaborator

Re-reviewed against current quattro. The three blockers above all hold up — I confirmed each one in the code rather than taking them on faith, and the cross-filesystem one has a precise tell worth recording. Separately, some things that I don't think have been raised yet.

The cross-filesystem staging bug is an asymmetry, not an oversight in general. The symlink branch builds its temporary as $target_parent/.omarchy3.$backup_suffix.$kind-publish.partial — correct, same filesystem as the destination. The non-symlink branch uses $HOME/.config.omarchy3.$backup_suffix.omarchy-publish.partial and renames it onto $HOME/.config/omarchy. If ~/.config is its own mount, that mv degrades to copy+unlink, and on retry tree_publication_status == pending && -e $active && -e $backup adopts the partial tree as published. The fix is to stage inside $HOME/.config/, which is exactly what the symlink path already does.

#321 and #322 still merge cleanly, and that is still hiding the conflict. I raised this on 2026-09-05 and re-verified it today, so this is a status report rather than a new finding: both branches still merge cleanly into quattro and cleanly with each other. Merged, bin/omarchy-upgrade-to-quattro-mac is 1135 lines carrying two different resumability designs. main() comes from #322, so replace_hyprland_config — this PR's staged publication — is skipped whenever login_cleanup_only=1, while backup_user_config has already taken the lock and written its markers. The upgrade exits reporting success with the config transition staged but never published.

Both test suites pass green on the merged tree. Neither branch has moved since the reconciliation request (this one's head is 2026-09-03, #322's is 2026-09-01), so the request is still outstanding.

config-transition-complete is write-only. publish_config_transition is called at both sites and the marker is read only to validate itself. Nothing ever branches on it. Of the six state files this PR introduces, that one currently does nothing.

The stage directory is never cleaned up. $HOME/.config.omarchy3.<identity>.quattro-config.partial holds a full copy of both config trees and is still there after a successful upgrade, alongside the .bak. Given that #315 is specifically about the upgrade leaving multi-hundred-megabyte copies behind, adding a new permanent one cuts against the point of the PR.

bin/omarchy-upgrade-to-quattro-mac:144:

  # staging, and publication. Use a fixed descriptor for the macOS Bash 3
  # runtime; the standalone upgrade keeps descriptor 9 open until it exits.

This script refuses to run on anything but aarch64 Arch Linux. There is no macOS Bash 3 runtime anywhere near it. Small thing, but in a 469-line addition to a one-way upgrade path, a comment that describes a system that isn't there makes the rest harder to extend trust to.

The lock descriptor leaks into every child process. exec 9>"$config_transition_lock" sets no close-on-exec, and it opens in step one — before install_quattro_packages. Every yay, pacman and sudo child inherits the held lock. Anything that outlives the script (a pacman hook that starts a daemon, for instance) keeps holding it, and every later run then fails with "Another Quattro config transition is already running" with nothing obvious to point at. Either close the descriptor around the long steps or narrow the lock to backup and publication.

On altitude. This is 469 lines and six state files to make one cp -R resumable in a script that runs once per machine on a one-way crossing. The guarantee is worth having, but I'd want to see the case against the smaller version first: one completion marker plus same-directory atomic renames, which is what the symlink branch is already doing correctly. Complexity in this particular script is itself a risk — when it goes wrong, the user is looking at a half-upgraded desktop.

Verified locally: focused suite 28 assertions pass, bin/omarchy commands --check 456 commands, ./test/cli green. The branch is 35 commits behind quattro.

@wesleygrimes wesleygrimes added the defer Parked. Needs a decision, or may die with the fork. Do not close or port yet. label Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

defer Parked. Needs a decision, or may die with the fork. Do not close or port yet.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

omarchy-upgrade-to-quattro-mac is not safe to re-run: each retry re-copies ~/.config and re-replaces ~/.config/hypr

4 participants