From 7c007cd029d9f677d54bf4cc0639b98f2bad2453 Mon Sep 17 00:00:00 2001 From: Young Joon Lee Date: Mon, 9 Nov 2026 21:51:09 +0900 Subject: [PATCH 1/2] chore(sync): drop the hidden --profile alias and name the store in every message dot sync accepted --profile as a deprecated alias of --store for one release after #380. That release is out (v2.80.1 knows --store), so the alias is removed: `dot sync --profile` now fails with "unknown flag" instead of warning on stderr, and the fallback that read the flag, the conflict refusal between the two names, and syncStoreFrom are gone. Everything dot prints and everything it runs on the peer moves to --store too: the owner-mismatch and fence refusals, the NFD fix that peer doctor and peer inventory report, the handover messages, the conflicts hint at the end of `dot peer sync`, the launchd plist and systemd unit the scheduler renders, and the spawn-failed reload line. remotePeerNormalizeCommand now sends --store=peer to the peer; the comment that hedged for a peer whose dot knew only --profile is no longer true, because the peer runs the same release as this Mac. --profile on apply, diff, config, status, init, watchdog setup|notify and container setup is untouched: that flag picks a dotfiles profile and is unrelated to the sync store. Part of #394 --- internal/cli/peer_cmd.go | 7 ++--- internal/cli/sync_cmd.go | 31 ++----------------- internal/cli/sync_cmd_test.go | 16 +++------- internal/cli/sync_names_cmd.go | 2 +- internal/syncer/bootstrap.go | 2 +- internal/syncer/name_normalize.go | 2 +- internal/syncer/owner.go | 4 +-- internal/syncer/peer_doctor_facts.go | 6 ++-- internal/syncer/peer_doctor_facts_test.go | 12 +++---- internal/syncer/peer_handover.go | 8 ++--- internal/syncer/peer_inventory.go | 10 +++--- internal/syncer/peer_test.go | 2 +- internal/syncer/scheduler.go | 2 +- internal/syncer/scheduler_darwin_test.go | 6 ++-- .../syncer/scheduler_spawn_failed_test.go | 2 +- internal/syncer/scheduler_test.go | 8 ++--- .../sync/com.dotfiles.sync.plist.tmpl | 2 +- .../templates/sync/dotfiles-sync.service.tmpl | 2 +- 18 files changed, 45 insertions(+), 79 deletions(-) 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/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..a631ddf8 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`" } 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..a593d72e 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) } } } 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}} From 234343ffcf51d389a6a2d5594a2298f393f983c8 Mon Sep 17 00:00:00 2001 From: Young Joon Lee Date: Mon, 9 Nov 2026 22:03:43 +0900 Subject: [PATCH 2/2] fix(sync): rewrite an installed scheduler unit that still runs --profile A unit stores the whole command line and the service manager replays it on every tick, and neither `dot update` nor `dot apply` rewrites an installed unit. A scheduler installed for a non-default store therefore kept invoking `dot sync push --profile=` after the alias removal of #394 and failed with "unknown flag" on every run, silently. `dot sync resume` is the command that re-arms installed units, so it now rewrites the retired argument to --store before the reload that follows, which is what makes the service manager read the new line. The rewrite changes only the flag: store, mode, interval, --home and log paths stay exactly as the operator installed them, because a fresh render would re-apply setup's current settings and could silently re-time or re-enable a scheduler. `dot sync status` stays read-only and names the installed unit plus the `dot sync --store= pause && ... resume` pair that heals it. 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, so rewriting that file would leave a command that cannot run. Verified: this Mac's com.dotfiles.sync.plist carries no store argument (default store). The peer Mac was unreachable over WARP at review time, so its units are unverified. The migration was exercised against real template renders spelled the old way, for both the launchd plist and the systemd service. Part of #394 --- internal/cli/sync_setup_cmd.go | 8 + internal/cli/sync_status_cmd.go | 24 +++ internal/cli/sync_status_errors_test.go | 94 +++++++++++ internal/syncer/scheduler.go | 86 ++++++++++ internal/syncer/scheduler_test.go | 154 ++++++++++++++++++ .../syncer/sync_scheduler_lifecycle_test.go | 86 ++++++++++ internal/syncer/sync_store_ops.go | 20 ++- 7 files changed, 471 insertions(+), 1 deletion(-) 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/scheduler.go b/internal/syncer/scheduler.go index a631ddf8..21a54738 100644 --- a/internal/syncer/scheduler.go +++ b/internal/syncer/scheduler.go @@ -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_test.go b/internal/syncer/scheduler_test.go index a593d72e..6b3a07c2 100644 --- a/internal/syncer/scheduler_test.go +++ b/internal/syncer/scheduler_test.go @@ -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),