diff --git a/internal/cli/peer_cmd.go b/internal/cli/peer_cmd.go index 8e553c9e..7be20405 100644 --- a/internal/cli/peer_cmd.go +++ b/internal/cli/peer_cmd.go @@ -81,9 +81,8 @@ the new owner and removes its scheduler, so two coordinators never run.`, } // peerBootstrapOptions translates the peer command tree's flags into the -// engine's bootstrap options. The profile is fixed here, which is why -// `--profile` is deliberately not read: `dot peer` only ever addresses the -// peer store. +// engine's bootstrap options. The profile is fixed here, which is why no +// flag selects it: `dot peer` only ever addresses the peer store. func peerBootstrapOptions(cmd *cobra.Command) syncer.BootstrapOptions { dryRun, _ := cmd.Flags().GetBool("dry-run") verbose, _ := cmd.Flags().GetBool("verbose") @@ -331,7 +330,7 @@ on a laptop.`, p.Line("%s %d deletion(s) from here under %s on the peer", verb, res.QuarantinedOnPeer, dir) } p.Line("Conflicting edits keep both versions; list them with:") - p.Line(" dot sync conflicts --profile=%s", PeerProfile) + p.Line(" dot sync conflicts --store=%s", PeerProfile) return nil }, } diff --git a/internal/cli/sync_cmd.go b/internal/cli/sync_cmd.go index 2b1916d0..3abf2ee0 100644 --- a/internal/cli/sync_cmd.go +++ b/internal/cli/sync_cmd.go @@ -96,25 +96,9 @@ Run without a subcommand to print this help.`, return cmd.Help() }, SilenceUsage: true, - // --store and the deprecated --profile name the same thing; two - // different values must not silently pick one (#380). - PersistentPreRunE: func(c *cobra.Command, _ []string) error { - if c.Flags().Changed("store") && c.Flags().Changed("profile") { - store, _ := c.Flags().GetString("store") - profile, _ := c.Flags().GetString("profile") - if store != profile { - return fmt.Errorf("--store %q and --profile %q name different stores; drop --profile, which is the deprecated name of --store", store, profile) - } - } - return nil - }, } cmd.PersistentFlags().String("store", syncer.DefaultProfile, "sync store under /.dotfiles//; \"sync\" is the cloud mirror, \"peer\" the other Mac") - cmd.PersistentFlags().String("profile", "", "Deprecated: use --store") - // Hidden, not MarkDeprecated: cobra writes that notice to stdout, which - // would corrupt --json output; syncStoreFrom warns on stderr instead. - _ = cmd.PersistentFlags().MarkHidden("profile") cmd.PersistentFlags().BoolP("verbose", "V", false, "Show rsync progress output") cmd.AddCommand( newSyncPullCmd(), @@ -153,28 +137,17 @@ Run without a subcommand to print this help.`, return cmd } -// syncStoreFrom is --store, or the deprecated --profile when that was given. -func syncStoreFrom(cmd *cobra.Command) string { - if cmd.Flags().Changed("profile") { - fmt.Fprintln(cmd.ErrOrStderr(), "warning: dot sync --profile is deprecated; use --store (it will be removed after one release)") - v, _ := cmd.Flags().GetString("profile") - return v - } - v, _ := cmd.Flags().GetString("store") - return v -} - // syncBootstrapOptions translates the sync command tree's flags into the // engine's bootstrap options. Flag translation is cli's retained role; state // loading, profile resolution and runner construction are the engine's // (syncer.Bootstrap, D-07). func syncBootstrapOptions(cmd *cobra.Command, readOnly bool) syncer.BootstrapOptions { dryRun, _ := cmd.Flags().GetBool("dry-run") - profile := syncStoreFrom(cmd) + store, _ := cmd.Flags().GetString("store") verbose, _ := cmd.Flags().GetBool("verbose") filterMode, _ := cmd.Flags().GetString("filter-mode") return syncer.BootstrapOptions{ - Profile: profile, + Profile: store, ReadOnly: readOnly, DryRun: dryRun, Verbose: verbose, diff --git a/internal/cli/sync_cmd_test.go b/internal/cli/sync_cmd_test.go index 7d28621e..b86c6b1a 100644 --- a/internal/cli/sync_cmd_test.go +++ b/internal/cli/sync_cmd_test.go @@ -361,28 +361,22 @@ func TestSyncStatusSeparatesLiveConflictsFromBackups(t *testing.T) { } } -// --store names the sync store; --profile still works for one release, warns, -// and is gone from every command that is not dot sync (#380). -func TestSyncStoreFlagAndDeprecatedProfile(t *testing.T) { +// --store names the sync store; the deprecated --profile alias is gone (#394). +func TestSyncStoreFlag(t *testing.T) { f := newSyncCLIFixture(t) _ = f - out, errOut, err := runDotForTest("sync", "status", "--profile", "sync") - if err != nil || !strings.Contains(errOut, "use --store") || strings.Contains(out, "deprecated") { - t.Fatalf("deprecated --profile: err=%v stderr=%q stdout=%q", err, errOut, out) - } if _, _, err := runDotForTest("sync", "status", "--store", "sync"); err != nil { t.Fatalf("--store: %v", err) } + if _, _, err := runDotForTest("sync", "status", "--profile", "sync"); err == nil || !strings.Contains(err.Error(), "unknown flag") { + t.Fatalf("sync status --profile: %v", err) + } if _, _, err := runDotForTest("peer", "status", "--profile", "x"); err == nil || !strings.Contains(err.Error(), "unknown flag") { t.Fatalf("peer status --profile: %v", err) } if _, _, err := runDotForTest("sync", "status", "--mode", "clean"); err == nil || !strings.Contains(err.Error(), "unknown flag") { t.Fatalf("sync status --mode: %v", err) } - // Two different stores through both names is refused, not silently resolved. - if _, _, err := runDotForTest("sync", "status", "--store", "sync", "--profile", "peer"); err == nil || !strings.Contains(err.Error(), "name different stores") { - t.Fatalf("conflicting --store/--profile: %v", err) - } // Only the commands that read --profile take it. for _, args := range [][]string{{"watchdog", "status", "--profile", "x"}, {"container", "status", "--profile", "x"}} { if _, _, err := runDotForTest(args...); err == nil || !strings.Contains(err.Error(), "unknown flag") { diff --git a/internal/cli/sync_names_cmd.go b/internal/cli/sync_names_cmd.go index 86e01e12..312cf334 100644 --- a/internal/cli/sync_names_cmd.go +++ b/internal/cli/sync_names_cmd.go @@ -13,7 +13,7 @@ import ( const nfdCLIPlanLimit = 20 // newSyncNamesCmd owns workspace filename maintenance below `dot sync names`. -// The parent sync command supplies the inherited --profile, --dry-run, and +// The parent sync command supplies the inherited --store, --dry-run, and // --yes flags; keeping those flags inherited avoids a second source of truth. func newSyncNamesCmd() *cobra.Command { cmd := &cobra.Command{ diff --git a/internal/cli/sync_setup_cmd.go b/internal/cli/sync_setup_cmd.go index bb513edb..b18ecf04 100644 --- a/internal/cli/sync_setup_cmd.go +++ b/internal/cli/sync_setup_cmd.go @@ -249,11 +249,19 @@ func runSyncResume(cmd *cobra.Command, _ []string) error { if err != nil { return err } + dryRun, _ := cmd.Flags().GetBool("dry-run") if res.WasPaused { p.Line("✓ sync resumed.") } else { p.Line("sync was not paused.") } + for _, store := range res.MigratedStores { + if dryRun { + p.Line(" ~ would rewrite the installed scheduler unit to call `dot sync --store=%s`", store) + continue + } + p.Line("✓ rewrote the installed scheduler unit to call `dot sync --store=%s`", store) + } if res.SchedulerOff { p.Line("scheduler remains off — run `dot sync setup --push-interval=DUR` or `--pull-interval=DUR` to enable.") return nil diff --git a/internal/cli/sync_status_cmd.go b/internal/cli/sync_status_cmd.go index 63843790..82d01ecc 100644 --- a/internal/cli/sync_status_cmd.go +++ b/internal/cli/sync_status_cmd.go @@ -109,6 +109,7 @@ func runSyncStatus(cmd *cobra.Command, _ []string) error { } else { p.KV("Pull scheduler", "(off — `dot sync setup --pull-interval=DUR` to enable)") } + reportStaleStoreUnits(p, sched) if st.Propagation.Delete { p.KV("Max delete", fmt.Sprintf("%d", st.MaxDelete)) } @@ -134,6 +135,29 @@ func runSyncStatus(cmd *cobra.Command, _ []string) error { return nil } +// reportStaleStoreUnits names an installed unit that still calls +// `dot sync --profile=`. This release rejects that flag, and neither +// `dot update` nor `dot apply` rewrites an installed unit, so the scheduled run +// fails on every tick while status would otherwise report it as running (#394). +// Status stays read-only: it prints the reload pair that rewrites the unit, +// which is the same idiom the spawn-failed recovery already names. +func reportStaleStoreUnits(p *Printer, sched *syncer.Scheduler) { + for _, unit := range []struct { + kind syncer.SchedulerKind + label string + }{ + {syncer.SchedulerKindPush, "push"}, + {syncer.SchedulerKindIntake, "pull"}, + } { + store, stale := sched.StaleStoreUnit(unit.kind) + if !stale { + continue + } + command := fmt.Sprintf("dot sync --store=%s pause && dot sync --store=%s resume", store, store) + p.Warn("the installed %s unit still calls `dot sync --profile=%s`, which this release no longer accepts, so its scheduled runs fail; reload it with `%s`", unit.label, store, command) + } +} + // printConflictState shows live conflicts apart from retained backups (#264): // live conflicts block a clean push, backups are copies that need nothing // until they are pruned. diff --git a/internal/cli/sync_status_errors_test.go b/internal/cli/sync_status_errors_test.go index 9fdbff99..8f5996a1 100644 --- a/internal/cli/sync_status_errors_test.go +++ b/internal/cli/sync_status_errors_test.go @@ -50,6 +50,100 @@ func TestStatusCommandsReportRefusedConflictDirectory(t *testing.T) { } } +// installStaleStoreUnit writes the unit this platform installs for a +// non-default store, still calling the retired `--profile=` argument, and +// returns its path. +func installStaleStoreUnit(t *testing.T, home, store string) string { + t.Helper() + files := []struct{ path, body string }{ + { + path: filepath.Join(home, "Library", "LaunchAgents", "com.dotfiles."+store+".plist"), + body: "--profile=" + store + "\n", + }, + } + if runtime.GOOS != "darwin" { + files = []struct{ path, body string }{ + { + path: filepath.Join(home, ".config", "systemd", "user", "dotfiles-"+store+".service"), + body: "ExecStart=%h/bin/dot sync push --profile=" + store + "\n", + }, + { + path: filepath.Join(home, ".config", "systemd", "user", "dotfiles-"+store+".timer"), + body: "[Timer]\nOnUnitActiveSec=15m\n", + }, + } + } + for _, file := range files { + writeCLITestFile(t, file.path, file.body) + } + return files[0].path +} + +// stubSchedulerManager puts a service manager on PATH that reports the job as +// neither running nor loaded and accepts every lifecycle action. +func stubSchedulerManager(t *testing.T) { + t.Helper() + bin := t.TempDir() + body := "#!/bin/sh\n[ \"$1\" = print ] && exit 1\nexit 0\n" + for _, name := range []string{"launchctl", "systemctl"} { + if err := os.WriteFile(filepath.Join(bin, name), []byte(body), 0o755); err != nil { + t.Fatal(err) + } + } + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +// A unit installed before #394 keeps invoking `dot sync push --profile=` +// and fails with "unknown flag" on every tick, and nothing rewrites an +// installed unit. `dot sync status` is where an operator looks, so it names the +// unit and the reload pair; `dot sync resume` is the command that re-arms +// units, so it rewrites the argument before reloading. +func TestSyncReportsAUnitInstalledWithTheRetiredStoreFlag(t *testing.T) { + home, root := goldenSyncFixture(t) + stubSchedulerManager(t) + // A custom store is where `dot sync setup --store=` may install a + // unit; `dot peer setup` owns the peer store's plist instead. + writeCLITestFile(t, filepath.Join(root, ".dotfiles", "research", "config.yaml"), + "target: local:"+filepath.Join(home, "gdrive-workspace", "work")+"\npropagation:\n create: true\n update: true\n delete: false\ninterval: 600\n") + unit := installStaleStoreUnit(t, home, "research") + before, err := os.ReadFile(unit) + if err != nil { + t.Fatal(err) + } + + out, stderr, err := runDotForTest("sync", "status", "--store=research") + if err != nil { + t.Fatalf("status: %v, %s", err, stderr) + } + for _, wanted := range []string{"still calls `dot sync --profile=research`", "dot sync --store=research pause && dot sync --store=research resume"} { + if !strings.Contains(out+stderr, wanted) { + t.Fatalf("status missing %q: %s%s", wanted, out, stderr) + } + } + if after, err := os.ReadFile(unit); err != nil || string(after) != string(before) { + t.Fatalf("status rewrote the unit: %v", err) + } + + out, stderr, err = runDotForTest("sync", "resume", "--store=research") + if err != nil { + t.Fatalf("resume: %v, %s", err, stderr) + } + if !strings.Contains(out+stderr, "rewrote the installed scheduler unit to call `dot sync --store=research`") { + t.Fatalf("resume did not report the rewrite: %s%s", out, stderr) + } + rewritten, err := os.ReadFile(unit) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(rewritten), "--profile=") || !strings.Contains(string(rewritten), "--store=research") { + t.Fatalf("unit still runs the retired flag:\n%s", rewritten) + } + out, stderr, err = runDotForTest("sync", "status", "--store=research") + if err != nil || strings.Contains(out+stderr, "still calls") { + t.Fatalf("status kept warning after the rewrite: %v, %s%s", err, out, stderr) + } +} + func TestSyncStatusReportsSpawnFailed(t *testing.T) { if runtime.GOOS != "darwin" { t.Skip("launchd status is macOS-specific") diff --git a/internal/syncer/bootstrap.go b/internal/syncer/bootstrap.go index 2463e721..9f9bc715 100644 --- a/internal/syncer/bootstrap.go +++ b/internal/syncer/bootstrap.go @@ -19,7 +19,7 @@ import ( // the same struct without inventing a flag set. type BootstrapOptions struct { // Profile names the sync store to resolve. Empty means the default - // profile, matching the `--profile` flag's own default. + // profile, matching the `--store` flag's own default. Profile string // ReadOnly resolves the profile without creating the local store, diff --git a/internal/syncer/name_normalize.go b/internal/syncer/name_normalize.go index 96bea3b8..94b171bc 100644 --- a/internal/syncer/name_normalize.go +++ b/internal/syncer/name_normalize.go @@ -390,7 +390,7 @@ func nfdPushRefusal(marked bool, renames int, profile string) error { return nil } return fmt.Errorf( - "found %d selected filename(s) requiring NFD migration; run `dot sync names normalize --profile=%s --dry-run`, then rerun with `--yes`", + "found %d selected filename(s) requiring NFD migration; run `dot sync names normalize --store=%s --dry-run`, then rerun with `--yes`", renames, profile) } diff --git a/internal/syncer/owner.go b/internal/syncer/owner.go index 9b39ad12..524ca9e0 100644 --- a/internal/syncer/owner.go +++ b/internal/syncer/owner.go @@ -196,8 +196,8 @@ func (e *OwnerMismatchError) Error() string { "profile %q is owned by %q but this machine answers to %q; refusing to write.\n"+ " Two writers on one target corrupt it: each machine keeps its own baseline and\n"+ " delete propagation would make the runs undo each other.\n"+ - " To move ownership here: dot sync owner --profile=%s --set-self\n"+ - " To read instead of write: dot sync pull --profile=%s", + " To move ownership here: dot sync owner --store=%s --set-self\n"+ + " To read instead of write: dot sync pull --store=%s", e.Profile, e.Owner, e.Host, e.Profile, e.Profile) } diff --git a/internal/syncer/peer_doctor_facts.go b/internal/syncer/peer_doctor_facts.go index 042ea4eb..fac7218b 100644 --- a/internal/syncer/peer_doctor_facts.go +++ b/internal/syncer/peer_doctor_facts.go @@ -315,7 +315,7 @@ func answersTo(names []string, name string) bool { // its next preflight to refuse. TestDoctorNFDVerdictMatchesTheSync runs the // real syncs for each case. func nfdVerdict(s, o *PeerSideFacts, host, other string) (level, detail, fix string) { - fix = "on " + host + ": dot sync names normalize --profile=peer --yes" + fix = "on " + host + ": dot sync names normalize --store=peer --yes" var quoted []string for _, name := range s.NonNFDSample { quoted = append(quoted, fmt.Sprintf("%q", name)) @@ -375,7 +375,7 @@ func topologyFix(local, peer *PeerSideFacts, here, there string) string { if strings.ContainsAny(target, " \t'\"$\\`;&|()<>*?[]{}!#~") { target = shellQuote(target) } - fixes = append(fixes, fmt.Sprintf("on %s: dot sync target --profile=peer %s", s.host, target)) + fixes = append(fixes, fmt.Sprintf("on %s: dot sync target --store=peer %s", s.host, target)) } return strings.Join(fixes, "; ") } @@ -610,7 +610,7 @@ func evaluatePeerSides(local, peer *PeerSideFacts, here, there string) []DoctorC if peer.NFDMarked { marked, other = there, here } - add("nfd", DoctorWarn, "the NFD marker is set on "+marked+" only", "on "+other+": dot sync names normalize --profile=peer --yes") + add("nfd", DoctorWarn, "the NFD marker is set on "+marked+" only", "on "+other+": dot sync names normalize --store=peer --yes") } // The fence checks topology first, on whichever Mac syncs, and dot peer diff --git a/internal/syncer/peer_doctor_facts_test.go b/internal/syncer/peer_doctor_facts_test.go index 8631aacf..e35f67ec 100644 --- a/internal/syncer/peer_doctor_facts_test.go +++ b/internal/syncer/peer_doctor_facts_test.go @@ -53,8 +53,8 @@ func TestEvaluatePeerSides(t *testing.T) { checks = evaluatePeerSides(local, peer, "m5x26", "m3x23") for _, want := range []struct{ name, level, fix string }{ {"roles", DoctorFail, "dot peer adopt --owner --epoch 3"}, - {"nfd", DoctorFail, "on m3x23: dot sync names normalize --profile=peer"}, - {"nfd", DoctorWarn, "on m3x23: dot sync names normalize --profile=peer"}, + {"nfd", DoctorFail, "on m3x23: dot sync names normalize --store=peer"}, + {"nfd", DoctorWarn, "on m3x23: dot sync names normalize --store=peer"}, {"rsync", DoctorFail, "on m3x23: brew install rsync"}, {"config", DoctorWarn, ""}, } { @@ -72,7 +72,7 @@ func TestEvaluatePeerSides(t *testing.T) { local, peer = doctorFacts() peer.NFDMarked, peer.NonNFD = true, 3 checks = evaluatePeerSides(local, peer, "m5x26", "m3x23") - if c := checkFor(checks, "nfd", DoctorFail); c == nil || c.Fix != "on m3x23: dot sync names normalize --profile=peer --yes" { + if c := checkFor(checks, "nfd", DoctorFail); c == nil || c.Fix != "on m3x23: dot sync names normalize --store=peer --yes" { t.Errorf("the other Mac's names under an unmarked coordinator: %+v", c) } @@ -111,7 +111,7 @@ func TestEvaluatePeerSides(t *testing.T) { mutate func(local, peer *PeerSideFacts) fix string }{ - {"unmarked coordinator with names", func(l, _ *PeerSideFacts) { l.NonNFD = 16 }, "on m5x26: dot sync names normalize --profile=peer --yes"}, + {"unmarked coordinator with names", func(l, _ *PeerSideFacts) { l.NonNFD = 16 }, "on m5x26: dot sync names normalize --store=peer --yes"}, {"coordinator cannot plan", func(l, _ *PeerSideFacts) { l.NonNFDError = "permission denied" }, ""}, {"marked coordinator, peer cannot plan", func(l, p *PeerSideFacts) { l.NFDMarked, p.NFDMarked, p.NonNFDError = true, true, "collision" }, ""}, } { @@ -130,7 +130,7 @@ func TestEvaluatePeerSides(t *testing.T) { peer.WorkspacePath, peer.TargetPath, peer.TargetHost = "/Users/b/work", "/Users/a/elsewhere", "m5x26.ts.net" // The fix names the Mac that is wrong and the target, and leaves the // owner and epoch alone (dot peer init would claim the owner). - if c := checkFor(evaluatePeerSides(local, peer, "m5x26", "m3x23"), "topology", DoctorFail); c == nil || !strings.Contains(c.Detail, "does not point back") || c.Fix != "on m3x23: dot sync target --profile=peer ssh:m5x26.ts.net:/Users/a/work" { + if c := checkFor(evaluatePeerSides(local, peer, "m5x26", "m3x23"), "topology", DoctorFail); c == nil || !strings.Contains(c.Detail, "does not point back") || c.Fix != "on m3x23: dot sync target --store=peer ssh:m5x26.ts.net:/Users/a/work" { t.Errorf("topology mismatch: %+v", c) } // Also with both Macs coordinating after a takeover: the fence refuses @@ -459,7 +459,7 @@ func TestPeerInventoryNFDErrorNamesHostAndCommand(t *testing.T) { t.Fatal(err) } _, err := PeerDiff(context.Background(), PeerDiffOptions{Config: cfg, Probe: peerScheduleRunner(false)}) - if err == nil || !strings.Contains(err.Error(), "on fake-peer; run there: dot sync names normalize --profile=peer") { + if err == nil || !strings.Contains(err.Error(), "on fake-peer; run there: dot sync names normalize --store=peer") { t.Fatalf("err = %v", err) } } diff --git a/internal/syncer/peer_handover.go b/internal/syncer/peer_handover.go index 30264050..bb4dfe32 100644 --- a/internal/syncer/peer_handover.go +++ b/internal/syncer/peer_handover.go @@ -228,7 +228,7 @@ func noPeerOwnerFix(local, remote fenceSide) string { func noLocalOwnerFix(remote fenceSide) string { switch { case strings.TrimSpace(remote.Owner) == "": - return "set one with `dot sync owner --profile=peer --set `" + return "set one with `dot sync owner --store=peer --set `" } return fmt.Sprintf("to keep %s coordinating, record it here: dot peer adopt --owner %s --epoch %d", shellQuote(remote.Owner), shellQuote(remote.Owner), remote.Epoch) } @@ -264,12 +264,12 @@ func fenceDecision(local, remote fenceSide) (demote bool, err error) { // cannot admit a second coordinator. if remote.CanPush { return false, fmt.Errorf( - "peer fence: equal owner epochs (%d) and the peer also passes its own owner guard (its owner %q, local %q); both sides refuse — set one coordinator with `dot sync owner --profile=peer --set ` on both machines", + "peer fence: equal owner epochs (%d) and the peer also passes its own owner guard (its owner %q, local %q); both sides refuse — set one coordinator with `dot sync owner --store=peer --set ` on both machines", local.Epoch, remote.Owner, local.Owner) } if !sameOwner(local.Owner, local.Aliases, remote.Owner, remote.Aliases) { return false, fmt.Errorf( - "peer fence: equal owner epochs (%d) with different owners (local %q, remote %q); both sides refuse — pick one coordinator and set it with `dot sync owner --profile=peer --set `", + "peer fence: equal owner epochs (%d) with different owners (local %q, remote %q); both sides refuse — pick one coordinator and set it with `dot sync owner --store=peer --set `", local.Epoch, local.Owner, remote.Owner) } return false, nil @@ -710,7 +710,7 @@ func PeerHandover(ctx context.Context, opts PeerHandoverOptions) (*PeerHandoverR result.NewOwner = remoteOwner if _, err := PeerAdopt(cfg, PeerAdoptOptions{Owner: remoteOwner, Epoch: epoch}); err != nil { return nil, fmt.Errorf( - "peer handover: the peer adopted %q (epoch %d) but the local write failed: %w. The peer is now coordinator; set this machine's owner with `dot sync owner --profile=peer --set %s`, run `dot peer setup --off` here, then `dot peer sync` and `dot peer setup` on %s", + "peer handover: the peer adopted %q (epoch %d) but the local write failed: %w. The peer is now coordinator; set this machine's owner with `dot sync owner --store=peer --set %s`, run `dot peer setup --off` here, then `dot peer sync` and `dot peer setup` on %s", remoteOwner, epoch, err, remoteOwner, cfg.Target.Host) } step("owner is %q (epoch %d) on both machines", remoteOwner, epoch) diff --git a/internal/syncer/peer_inventory.go b/internal/syncer/peer_inventory.go index ebf2a110..a4ac37e9 100644 --- a/internal/syncer/peer_inventory.go +++ b/internal/syncer/peer_inventory.go @@ -57,7 +57,7 @@ func peerRemoteInventory(ctx context.Context, runner *exec.Runner, cfg *Config, snap, err := parsePeerRemoteInventory(res.Stdout, time.UTC, baseline, requireNFD) if errors.Is(err, errPeerNameNotNFD) { // The name lives on the peer, so the fix runs there (#182). - return nil, fmt.Errorf("%w on %s; run there: dot sync names normalize --profile=peer --yes (dot peer doctor counts them)", err, cfg.Target.Host) + return nil, fmt.Errorf("%w on %s; run there: dot sync names normalize --store=peer --yes (dot peer doctor counts them)", err, cfg.Target.Host) } return snap, err } @@ -285,9 +285,9 @@ func remotePeerStatusCommand(dot string) string { const PeerStatusUnscannedEnv = "DOT_PEER_STATUS_UNSCANNED" func remotePeerNormalizeCommand(dot, excludes string) string { - // The peer may run an older dot that knows only --profile; every release - // since 2.80 accepts it as a deprecated alias of --store (#380). - return PeerNormalizeExcludesEnv + "=" + shellQuote(excludes) + " exec " + shellQuote(dot) + " sync names normalize --profile=peer --yes" + // --store needs a dot from 2.80 or later; the peer runs the same release + // as this Mac, and --profile is gone as an alias (#394). + return PeerNormalizeExcludesEnv + "=" + shellQuote(excludes) + " exec " + shellQuote(dot) + " sync names normalize --store=peer --yes" } // remotePeerStatus is the narrow view of a remote `dot peer status --json` @@ -450,7 +450,7 @@ func ownerMatchError(local, remote fenceSide) error { } if remote.CanPush { return fmt.Errorf( - "peer coordinator check: the peer also passes its own owner guard (its owner %q, local %q); two coordinators would write to each other. Set one owner on both machines with `dot sync owner --profile=peer --set `", + "peer coordinator check: the peer also passes its own owner guard (its owner %q, local %q); two coordinators would write to each other. Set one owner on both machines with `dot sync owner --store=peer --set `", remote.Owner, local.Owner) } if NormalizeHostname(local.Owner) == "" || !sameOwner(local.Owner, local.Aliases, remote.Owner, remote.Aliases) { diff --git a/internal/syncer/peer_test.go b/internal/syncer/peer_test.go index 2d37a619..e22693ab 100644 --- a/internal/syncer/peer_test.go +++ b/internal/syncer/peer_test.go @@ -428,7 +428,7 @@ func TestSchedulerUnitsAreProfileScoped(t *testing.T) { t.Errorf("peer label = %s, want it to name the profile", peer) } if got := profileArg(DefaultProfile); got != "" { - t.Errorf("default profile must render no --profile arg, got %q", got) + t.Errorf("default profile must render no --store arg, got %q", got) } if got := profileArg("peer"); got != "peer" { t.Errorf("profileArg(peer) = %q", got) diff --git a/internal/syncer/scheduler.go b/internal/syncer/scheduler.go index f9b62b19..21a54738 100644 --- a/internal/syncer/scheduler.go +++ b/internal/syncer/scheduler.go @@ -122,7 +122,7 @@ func (s SchedulerState) StringForProfile(profile string) string { if s != SchedulerSpawnFailed || profile == "" || profile == DefaultProfile { return s.String() } - command := "dot sync --profile=" + shellQuote(profile) + command := "dot sync --store=" + shellQuote(profile) return "spawn failed; reload the job with `" + command + " pause && " + command + " resume`" } @@ -219,6 +219,92 @@ func profileArg(profile string) string { return profile } +// retiredStoreFlag is the scheduler store argument removed in #394; storeFlag +// is the spelling units render today. A unit stores the whole command line and +// launchd and systemd replay it on every tick, while `dot update` and +// `dot apply` never rewrite an installed unit. One rendered before the removal +// therefore keeps invoking `dot sync push --profile=` and fails with +// "unknown flag" on every run, silently, until it is rewritten here. +const ( + retiredStoreFlag = "--profile=" + storeFlag = "--store=" +) + +// retiredStoreIn returns the store a rendered unit names with the retired +// argument, or "" when it names none. The value ends where a rendered plist +// (--profile=peer) or a systemd ExecStart (--profile=peer +// "--home=…") ends it. +func retiredStoreIn(content string) string { + idx := strings.Index(content, retiredStoreFlag) + if idx < 0 { + return "" + } + value := content[idx+len(retiredStoreFlag):] + if end := strings.IndexAny(value, " \t\r\n\"'<>"); end >= 0 { + value = value[:end] + } + return value +} + +// installedUnitPath returns the unit file holding this platform's scheduler +// for the kind: the launchd plist on macOS, the systemd service elsewhere. +// Reading whichever file exists keeps the migration platform-neutral. +func (s *Scheduler) installedUnitPath(kind SchedulerKind) string { + for _, path := range []string{s.Paths.PlistFor(kind), s.Paths.SystemdServiceFor(kind)} { + if path != "" && s.Runner.FileExists(path) { + return path + } + } + return "" +} + +// StaleStoreUnit reports the store an installed unit names with the retired +// --profile argument. Read-only, so status can name the fix without writing. +// +// The peer store is exempt: com.dotfiles.peer.plist belongs to `dot peer setup` +// (it runs `dot peer sync` and renders no store argument), and `dot sync +// --store=peer` is refused outright, so rewriting that file would leave a +// command that cannot run. `dot peer setup` re-renders it. +func (s *Scheduler) StaleStoreUnit(kind SchedulerKind) (string, bool) { + if s.Paths.schedulerProfile() == PeerProfile { + return "", false + } + path := s.installedUnitPath(kind) + if path == "" { + return "", false + } + content, err := s.Runner.ReadFile(path) + if err != nil { + return "", false + } + store := retiredStoreIn(string(content)) + return store, store != "" +} + +// MigrateStaleStoreUnit rewrites the retired argument to --store in place and +// changes nothing else: store, mode, interval, --home and log paths stay as +// the operator installed them, so a migration cannot silently re-time or +// re-enable a scheduler. The caller reloads the unit, because loading is what +// makes the service manager read the new command line. +func (s *Scheduler) MigrateStaleStoreUnit(kind SchedulerKind) (string, error) { + store, stale := s.StaleStoreUnit(kind) + if !stale { + return "", nil + } + path := s.installedUnitPath(kind) + content, err := s.Runner.ReadFile(path) + if err != nil { + return store, fmt.Errorf("reading installed unit %s: %w", path, err) + } + // The unit holds exactly one store argument, inside the ProgramArguments + // entry the plist or systemd template rendered. + migrated := strings.ReplaceAll(string(content), retiredStoreFlag, storeFlag) + if err := s.Runner.WriteFileAtomic(path, []byte(migrated), 0644); err != nil { + return store, fmt.Errorf("rewriting installed unit %s: %w", path, err) + } + return store, nil +} + type Scheduler struct { Runner *exec.Runner Paths *Paths diff --git a/internal/syncer/scheduler_darwin_test.go b/internal/syncer/scheduler_darwin_test.go index 05de1895..d026322c 100644 --- a/internal/syncer/scheduler_darwin_test.go +++ b/internal/syncer/scheduler_darwin_test.go @@ -202,8 +202,8 @@ func TestSchedulerInstallKind_ProfiledPlist(t *testing.T) { if label != paths.LaunchdLabelFor(kind) { t.Fatalf("persisted Label = %q, want %q", label, paths.LaunchdLabelFor(kind)) } - if got := matchingProfileArguments(args); len(got) != 1 || got[0] != "--profile=research" { - t.Fatalf("profile arguments = %q, want exactly --profile=research", got) + if got := matchingProfileArguments(args); len(got) != 1 || got[0] != "--store=research" { + t.Fatalf("profile arguments = %q, want exactly --store=research", got) } }) } @@ -651,7 +651,7 @@ func plistLabelAndProgramArguments(t *testing.T, body []byte) (string, []string) func matchingProfileArguments(args []string) []string { var found []string for _, arg := range args { - if strings.HasPrefix(arg, "--profile=") { + if strings.HasPrefix(arg, "--store=") { found = append(found, arg) } } diff --git a/internal/syncer/scheduler_spawn_failed_test.go b/internal/syncer/scheduler_spawn_failed_test.go index 0b135d89..82389d1f 100644 --- a/internal/syncer/scheduler_spawn_failed_test.go +++ b/internal/syncer/scheduler_spawn_failed_test.go @@ -67,7 +67,7 @@ func TestLaunchdLastExitCode(t *testing.T) { func TestSpawnFailedRecoveryUsesActiveProfile(t *testing.T) { got := SchedulerSpawnFailed.StringForProfile("backup") - if !strings.Contains(got, "dot sync --profile='backup' pause && dot sync --profile='backup' resume") { + if !strings.Contains(got, "dot sync --store='backup' pause && dot sync --store='backup' resume") { t.Fatalf("recovery = %q", got) } if got := SchedulerRunning.StringForProfile("backup"); got != "running" { diff --git a/internal/syncer/scheduler_test.go b/internal/syncer/scheduler_test.go index 71b55569..6b3a07c2 100644 --- a/internal/syncer/scheduler_test.go +++ b/internal/syncer/scheduler_test.go @@ -241,11 +241,11 @@ func TestResolveScheduler_ProfilePathAndRenderedUnitAgree(t *testing.T) { if err != nil { t.Fatalf("render %s systemd service: %v", kind.Action(), err) } - if !strings.Contains(string(service), "--profile=research-2.v1 \"--home="+home) { + if !strings.Contains(string(service), "--store=research-2.v1 \"--home="+home) { t.Errorf("%s systemd ExecStart does not carry the same profile before --home:\n%s", kind.Action(), service) } args := renderedPlistProgramArguments(t, data) - if !containsString(args, "--profile=research-2.v1") { + if !containsString(args, "--store=research-2.v1") { t.Errorf("%s plist profile argument = %q, want research-2.v1", kind.Action(), args) } } @@ -288,8 +288,8 @@ func TestResolveScheduler_ProfilePathAndRenderedUnitAgree(t *testing.T) { if err != nil { t.Fatalf("render default %s systemd service: %v", kind.Action(), err) } - if strings.Contains(string(service), "--profile=") { - t.Errorf("default %s service unexpectedly renders --profile:\n%s", kind.Action(), service) + if strings.Contains(string(service), "--store=") { + t.Errorf("default %s service unexpectedly renders --store:\n%s", kind.Action(), service) } } } @@ -620,3 +620,157 @@ func TestResolvedPathsIncludeSchedulerArtifacts(t *testing.T) { t.Errorf("SystemdTimer tail wrong: %s", paths.SystemdTimer) } } + +// A unit installed before #394 stores the whole command line with +// `dot sync push --profile=`, a flag this release rejects. Nothing +// rewrites an installed unit (neither `dot update` nor `dot apply` touches it) +// and launchd and systemd replay the stored line, so the scheduled run fails +// with "unknown flag" on every tick. The fixtures are real renders of the +// current templates with the flag spelled the old way, which is byte-identical +// to what 2.80.1 installed. +func TestSchedulerMigratesUnitsInstalledWithTheRetiredStoreFlag(t *testing.T) { + for _, tc := range []struct { + name string + tmpl string + unit func(*Paths, SchedulerKind) string + }{ + {"launchd plist", "sync/com.dotfiles.sync.plist.tmpl", (*Paths).PlistFor}, + {"systemd service", "sync/dotfiles-sync.service.tmpl", (*Paths).SystemdServiceFor}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := resolvedSchedulerConfig(t, t.TempDir(), "research") + scheduler, paths, err := ResolveScheduler(cfg, peerScheduleRunner(false)) + if err != nil { + t.Fatalf("ResolveScheduler: %v", err) + } + data := scheduler.templateDataFor(SchedulerKindPush) + if err := preparePlistTemplateData(&data); err != nil { + t.Fatal(err) + } + current, err := scheduler.Engine.Render(tc.tmpl, data) + if err != nil { + t.Fatalf("render %s: %v", tc.tmpl, err) + } + unit := tc.unit(paths, SchedulerKindPush) + if err := os.MkdirAll(filepath.Dir(unit), 0o755); err != nil { + t.Fatal(err) + } + installed := strings.ReplaceAll(string(current), storeFlag+"research", retiredStoreFlag+"research") + if installed == string(current) { + t.Fatalf("fixture did not render a store argument:\n%s", current) + } + if err := os.WriteFile(unit, []byte(installed), 0o644); err != nil { + t.Fatal(err) + } + + if store, stale := scheduler.StaleStoreUnit(SchedulerKindPush); store != "research" || !stale { + t.Fatalf("StaleStoreUnit = %q, %v; want research, true", store, stale) + } + store, err := scheduler.MigrateStaleStoreUnit(SchedulerKindPush) + if err != nil || store != "research" { + t.Fatalf("MigrateStaleStoreUnit = %q, %v; want research", store, err) + } + migrated, err := os.ReadFile(unit) + if err != nil { + t.Fatal(err) + } + // Only the flag changed: the store, mode, interval, --home and log + // paths are exactly what a fresh render writes. + if string(migrated) != string(current) { + t.Fatalf("migrated unit =\n%s\nwant the fresh render\n%s", migrated, current) + } + if _, stale := scheduler.StaleStoreUnit(SchedulerKindPush); stale { + t.Fatal("the migrated unit still names the retired flag") + } + if store, err := scheduler.MigrateStaleStoreUnit(SchedulerKindPush); err != nil || store != "" { + t.Fatalf("second migration = %q, %v; want a no-op", store, err) + } + }) + } +} + +// A unit for the default store renders no store argument at all, so the +// migration must leave it byte-identical: rewriting a working scheduler is not +// part of the fix. +func TestSchedulerLeavesUnitsWithoutAStoreArgumentAlone(t *testing.T) { + cfg := resolvedSchedulerConfig(t, t.TempDir(), DefaultProfile) + scheduler, paths, err := ResolveScheduler(cfg, peerScheduleRunner(false)) + if err != nil { + t.Fatalf("ResolveScheduler: %v", err) + } + data := scheduler.templateDataFor(SchedulerKindPush) + if err := preparePlistTemplateData(&data); err != nil { + t.Fatal(err) + } + current, err := scheduler.Engine.Render("sync/com.dotfiles.sync.plist.tmpl", data) + if err != nil { + t.Fatalf("render plist: %v", err) + } + unit := paths.PlistFor(SchedulerKindPush) + if err := os.MkdirAll(filepath.Dir(unit), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(unit, current, 0o644); err != nil { + t.Fatal(err) + } + + if store, stale := scheduler.StaleStoreUnit(SchedulerKindPush); stale { + t.Fatalf("default-store unit read as stale (store %q)", store) + } + if store, err := scheduler.MigrateStaleStoreUnit(SchedulerKindPush); err != nil || store != "" { + t.Fatalf("MigrateStaleStoreUnit = %q, %v; want a no-op", store, err) + } + body, err := os.ReadFile(unit) + if err != nil { + t.Fatal(err) + } + if string(body) != string(current) { + t.Fatalf("a unit without a store argument was rewritten:\n%s", body) + } +} + +// com.dotfiles.peer.plist belongs to `dot peer setup`, which renders +// `dot peer sync` with no store argument, and `dot sync --store=peer` is +// refused, so the peer store is outside the migration: rewriting that file +// would leave a command that cannot run. +func TestSchedulerLeavesThePeerSchedulerUnitAlone(t *testing.T) { + cfg := resolvedSchedulerConfig(t, t.TempDir(), PeerProfile) + scheduler, paths, err := ResolveScheduler(cfg, peerScheduleRunner(false)) + if err != nil { + t.Fatalf("ResolveScheduler: %v", err) + } + unit := paths.PlistFor(SchedulerKindPush) + if err := os.MkdirAll(filepath.Dir(unit), 0o755); err != nil { + t.Fatal(err) + } + stale := "" + retiredStoreFlag + "peer\n" + if err := os.WriteFile(unit, []byte(stale), 0o644); err != nil { + t.Fatal(err) + } + + if store, isStale := scheduler.StaleStoreUnit(SchedulerKindPush); isStale { + t.Fatalf("the peer scheduler unit read as stale (store %q)", store) + } + if store, err := scheduler.MigrateStaleStoreUnit(SchedulerKindPush); err != nil || store != "" { + t.Fatalf("MigrateStaleStoreUnit = %q, %v; want a no-op", store, err) + } + if body, err := os.ReadFile(unit); err != nil || string(body) != stale { + t.Fatalf("the peer scheduler unit was rewritten: %v\n%s", err, body) + } +} + +// The store value ends at the argument boundary each unit format draws, so the +// fix names exactly the store the operator installed. +func TestRetiredStoreInReadsArgumentBoundaries(t *testing.T) { + for _, tc := range []struct{ content, want string }{ + {"--profile=peer", "peer"}, + {`ExecStart=/opt/homebrew/bin/dot sync push --mode=clean --profile=research-2 "--home=/Users/a work"`, "research-2"}, + {" --profile=backup\n", "backup"}, + {"dot sync push --store=peer", ""}, + {"", ""}, + } { + if got := retiredStoreIn(tc.content); got != tc.want { + t.Errorf("retiredStoreIn(%q) = %q, want %q", tc.content, got, tc.want) + } + } +} diff --git a/internal/syncer/sync_scheduler_lifecycle_test.go b/internal/syncer/sync_scheduler_lifecycle_test.go index 0600de7f..d6a58268 100644 --- a/internal/syncer/sync_scheduler_lifecycle_test.go +++ b/internal/syncer/sync_scheduler_lifecycle_test.go @@ -228,3 +228,89 @@ func schedulerLifecycleActionCount(commands []string) int { } return count } + +// `dot sync resume` is the command that re-arms installed units, so it is where +// a unit installed before #394 gets its retired `--profile=` rewritten: the +// service manager reads the command line at load time, so a rewrite without the +// reload that follows here would leave the job broken. +func TestSyncResumeMigratesAUnitInstalledWithTheRetiredStoreFlag(t *testing.T) { + root := t.TempDir() + t.Setenv("HOME", root) + binDir := filepath.Join(root, "bin") + if err := os.MkdirAll(binDir, 0o755); err != nil { + t.Fatal(err) + } + record := filepath.Join(root, "scheduler-args") + writeStub(t, filepath.Join(binDir, "launchctl"), "#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$DOTFILES_TEST_SCHEDULER_ARGS\"\nexit 0\n") + writeStub(t, filepath.Join(binDir, "systemctl"), "#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$DOTFILES_TEST_SCHEDULER_ARGS\"\necho active\nexit 0\n") + t.Setenv("PATH", binDir) + t.Setenv("DOTFILES_TEST_SCHEDULER_ARGS", record) + + paths, err := resolvePathsForHomeProfile(root, "research") + if err != nil { + t.Fatalf("resolvePathsForHomeProfile: %v", err) + } + // Install the unit this platform's scheduler reads: the launchd plist on + // macOS, the systemd service elsewhere (its timer carries the state query + // and the reload). + unit := paths.PlistFor(SchedulerKindPush) + stale := "" + retiredStoreFlag + "research\n" + if runtime.GOOS != "darwin" { + unit = paths.SystemdServiceFor(SchedulerKindPush) + stale = "ExecStart=%s sync push " + retiredStoreFlag + "research\n" + } + if err := os.MkdirAll(filepath.Dir(unit), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(unit, []byte(stale), 0o644); err != nil { + t.Fatal(err) + } + if runtime.GOOS != "darwin" { + timer := paths.SystemdTimerFor(SchedulerKindPush) + if err := os.MkdirAll(filepath.Dir(timer), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(timer, []byte("[Timer]\nOnUnitActiveSec=15m\n"), 0o644); err != nil { + t.Fatal(err) + } + } + localPaths := ResolveLocalPaths(filepath.Join(root, "workspace")) + if err := SaveLocalConfig(localPaths, &LocalConfig{Propagation: DefaultPropagationPolicy()}); err != nil { + t.Fatal(err) + } + cfg := &Config{ + Profile: "research", + LocalPath: filepath.Join(root, "workspace"), + LocalPaths: localPaths, + SystemPaths: paths, + Interval: 900, + Paused: true, + } + result, err := SyncResume(context.Background(), cfg, exec.NewRunner(false, slog.New(slog.NewTextHandler(io.Discard, nil)))) + if err != nil { + t.Fatalf("SyncResume: %v", err) + } + if len(result.MigratedStores) != 1 || result.MigratedStores[0] != "research" { + t.Fatalf("migrated stores = %q, want [research]", result.MigratedStores) + } + if !result.SchedulerResumed { + t.Fatal("the installed unit was not re-armed after the migration") + } + body, err := os.ReadFile(unit) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(body), retiredStoreFlag) || !strings.Contains(string(body), storeFlag+"research") { + t.Fatalf("%s still runs the retired flag:\n%s", unit, body) + } + // The migration itself talks to no service manager: the only calls are the + // state query and the reload the resume already made. + got := readSchedulerLifecycleCommands(t, record) + want := []string{ + schedulerStateCommand(paths, SchedulerKindPush), + schedulerLifecycleCommand(paths, SchedulerKindPush, "resume"), + } + if strings.Join(got, "\n") != strings.Join(want, "\n") { + t.Fatalf("service-manager argv = %q, want %q", got, want) + } +} diff --git a/internal/syncer/sync_store_ops.go b/internal/syncer/sync_store_ops.go index 0846da94..1ad95bcd 100644 --- a/internal/syncer/sync_store_ops.go +++ b/internal/syncer/sync_store_ops.go @@ -727,7 +727,11 @@ type ResumeResult struct { WasPaused bool SchedulerOff bool // no interval configured, so nothing to re-arm SchedulerResumed bool - SchedulerErr error + // MigratedStores names the stores whose installed unit still called + // `dot sync --profile=`, which this release rejects. The unit was rewritten + // to --store before it was re-armed (#394). + MigratedStores []string + SchedulerErr error } // SyncResume clears the paused gate and reattaches an installed scheduler. @@ -747,6 +751,20 @@ func SyncResume(ctx context.Context, cfg *Config, runner *exec.Runner) (*ResumeR // The state save succeeded; the scheduler is best-effort. return res, nil } + // A unit installed before #394 still runs `dot sync --profile=`, and + // this release rejects that flag, so its scheduled runs fail until the unit + // is rewritten. Resuming is the command that re-arms installed units, so it + // is where the rewrite belongs: the load below then starts a working job. + for _, kind := range [2]SchedulerKind{SchedulerKindPush, SchedulerKindIntake} { + store, err := sched.MigrateStaleStoreUnit(kind) + if err != nil { + res.SchedulerErr = err + return res, nil + } + if store != "" { + res.MigratedStores = append(res.MigratedStores, store) + } + } states := [2]SchedulerState{ sched.StateKind(ctx, SchedulerKindPush), sched.StateKind(ctx, SchedulerKindIntake), diff --git a/internal/template/templates/sync/com.dotfiles.sync.plist.tmpl b/internal/template/templates/sync/com.dotfiles.sync.plist.tmpl index 8fb17df9..b485fe3a 100644 --- a/internal/template/templates/sync/com.dotfiles.sync.plist.tmpl +++ b/internal/template/templates/sync/com.dotfiles.sync.plist.tmpl @@ -12,7 +12,7 @@ {{.Action}} --mode={{.Mode}} {{- if .Profile }} - --profile={{.Profile}} + --store={{.Profile}} {{- end }} {{- if .Home }} {{.PlistHomeArg}} diff --git a/internal/template/templates/sync/dotfiles-sync.service.tmpl b/internal/template/templates/sync/dotfiles-sync.service.tmpl index 9d07a0bb..5bb76845 100644 --- a/internal/template/templates/sync/dotfiles-sync.service.tmpl +++ b/internal/template/templates/sync/dotfiles-sync.service.tmpl @@ -3,4 +3,4 @@ Description={{.Description}} [Service] Type=oneshot -ExecStart={{.DotfilesPath}} sync {{.Action}} --mode={{.Mode}}{{if .Profile}} --profile={{.Profile}}{{end}}{{if .Home}} {{.SystemdHomeArg}}{{end}} +ExecStart={{.DotfilesPath}} sync {{.Action}} --mode={{.Mode}}{{if .Profile}} --store={{.Profile}}{{end}}{{if .Home}} {{.SystemdHomeArg}}{{end}}