diff --git a/cmd/codeaf/chatv3_standing.go b/cmd/codeaf/chatv3_standing.go index 9d0aa1c36..6cb4a1e85 100644 --- a/cmd/codeaf/chatv3_standing.go +++ b/cmd/codeaf/chatv3_standing.go @@ -460,8 +460,7 @@ func v3StandingSeam(seam *session.Standing) tui3.StandingSeam { // named as an interface so a test can hand it a definition pointing at a dead // path without going anywhere near this machine's launchd. type backgroundTimer interface { - Drift() (standing.WatchDrift, error) - Install(ctx context.Context) error + Repair(ctx context.Context) (standing.WatchDrift, error) } // repairBackgroundChecks puts a drifted timer back, and says one line about it @@ -489,11 +488,11 @@ func repairBackgroundChecks(watch backgroundTimer, wanted bool) string { if watch == nil || !wanted { return "" } - drift, err := watch.Drift() - if err != nil || !drift.Present || !drift.Stale { + drift, err := watch.Repair(context.Background()) + if !drift.Present { return "" } - if err := watch.Install(context.Background()); err != nil { + if err != nil { return "could not put the background check back: " + err.Error() } if drift.Gone && drift.Executable != "" { diff --git a/cmd/codeaf/chatv3_standing_test.go b/cmd/codeaf/chatv3_standing_test.go index 877f44350..c95259ec9 100644 --- a/cmd/codeaf/chatv3_standing_test.go +++ b/cmd/codeaf/chatv3_standing_test.go @@ -62,11 +62,15 @@ type driftedTimer struct { fail error } -func (d *driftedTimer) Drift() (standing.WatchDrift, error) { return d.drift, d.err } - -func (d *driftedTimer) Install(context.Context) error { +func (d *driftedTimer) Repair(context.Context) (standing.WatchDrift, error) { + if d.err != nil { + return standing.WatchDrift{}, d.err + } + if !d.drift.Present || !d.drift.Stale { + return standing.WatchDrift{}, nil + } d.installs++ - return d.fail + return d.drift, d.fail } // A TIMER POINTING AT A PROGRAM THAT MOVED RUNS NOTHING, and nothing on screen diff --git a/cmd/codeaf/do_engine_test.go b/cmd/codeaf/do_engine_test.go index 90df20d0d..f3384da34 100644 --- a/cmd/codeaf/do_engine_test.go +++ b/cmd/codeaf/do_engine_test.go @@ -149,6 +149,23 @@ func beltRunEnv(t *testing.T) string { return home } +// beltModelPins preserves the fixture's resource settings while changing the +// seats a test intends to exercise. Replacing the profile wholesale would +// silently re-enable the real machine gate on a busy test host (#1525). +func beltModelPins(t *testing.T, profileDir string, pins map[string]string) { + t.Helper() + settings := config.NewSettings(config.SettingsOptions{ProfileDir: profileDir}) + for key, value := range pins { + row, ok := settings.Row(key) + if !ok { + t.Fatalf("missing setting %s", key) + } + if err := row.Apply(value); err != nil { + t.Fatal(err) + } + } +} + // beltPlandbDoor is the real plandb CLI behind the resolver's override, built // once for the package. THE LOOP ENDS IN THE STORE: a worker's task is done // when `plandb done` marks it so and no other way, so a scripted worker that @@ -376,16 +393,10 @@ func TestDoOnTheRunEngineSeatsEveryLaunchOnTheDoorsModels(t *testing.T) { if err := os.MkdirAll(profileDir, 0o700); err != nil { t.Fatal(err) } - rows, err := json.Marshal(map[string]string{ + beltModelPins(t, profileDir, map[string]string{ config.KeyTierWorkerModel: "vendor/profile-worker", config.KeyTierMastermindModel: "vendor/profile-thinking", }) - if err != nil { - t.Fatal(err) - } - if err := os.WriteFile(config.BudgetConfigPath(profileDir), rows, 0o600); err != nil { - t.Fatal(err) - } workspace := beltRepoWorkspace(t) const ( @@ -433,7 +444,7 @@ func TestDoOnTheRunEngineSeatsEveryLaunchOnTheDoorsModels(t *testing.T) { } var stdout, stderr strings.Builder - err = doErrand(doRequest{ + err := doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, timeout: 60 * time.Second, slots: bound(1), model: workModel, planModel: planModel, checkModel: planModel, stdout: &stdout, stderr: &stderr, newBeltCompleter: newBelt, @@ -668,18 +679,12 @@ func TestDoOnTheRunEngineSeatsACheckOnTheCheckModel(t *testing.T) { if err := os.MkdirAll(profileDir, 0o700); err != nil { t.Fatal(err) } - rows, err := json.Marshal(map[string]string{ + beltModelPins(t, profileDir, map[string]string{ config.KeyTierLowModel: "vendor/profile-small", config.KeyTierWorkerModel: "vendor/profile-worker", config.KeyTierHighModel: "vendor/profile-careful", config.KeyTierMastermindModel: "vendor/profile-thinking", }) - if err != nil { - t.Fatal(err) - } - if err := os.WriteFile(config.BudgetConfigPath(profileDir), rows, 0o600); err != nil { - t.Fatal(err) - } workspace := beltRepoWorkspace(t) const ( @@ -722,7 +727,7 @@ func TestDoOnTheRunEngineSeatsACheckOnTheCheckModel(t *testing.T) { } var stdout, stderr strings.Builder - err = doErrand(doRequest{ + err := doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, timeout: 60 * time.Second, slots: bound(1), model: workModel, planModel: planModel, checkModel: checkModel, @@ -767,17 +772,11 @@ func TestDoOnTheRunEngineSeatsAnUnpinnedCheckOnTheCrewsChecker(t *testing.T) { if err := os.MkdirAll(profileDir, 0o700); err != nil { t.Fatal(err) } - rows, err := json.Marshal(map[string]string{ + beltModelPins(t, profileDir, map[string]string{ config.KeyTierWorkerModel: "vendor/profile-worker", config.KeyTierHighModel: "vendor/profile-careful", config.KeyTierMastermindModel: "vendor/profile-thinking", }) - if err != nil { - t.Fatal(err) - } - if err := os.WriteFile(config.BudgetConfigPath(profileDir), rows, 0o600); err != nil { - t.Fatal(err) - } workspace := beltRepoWorkspace(t) // The same one-seat shape as the two tests above. @@ -813,7 +812,7 @@ func TestDoOnTheRunEngineSeatsAnUnpinnedCheckOnTheCrewsChecker(t *testing.T) { } var stdout, stderr strings.Builder - err = doErrand(doRequest{ + err := doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, timeout: 60 * time.Second, slots: bound(1), stdout: &stdout, stderr: &stderr, newBeltCompleter: newBelt, diff --git a/docs/changes/unreleased/1622-standing-timer-ownership.md b/docs/changes/unreleased/1622-standing-timer-ownership.md new file mode 100644 index 000000000..a1567e274 --- /dev/null +++ b/docs/changes/unreleased/1622-standing-timer-ownership.md @@ -0,0 +1,8 @@ +--- +kind: fixed +title: First standing approval preserves the shared timer's owner +pr: 1622 +surface: [engine] +invalidates: + - "Approving the first standing item in a new profile could take another profile's OS timer despite launch-time ownership protection. Implicit setup and repair now check ownership under the same interprocess lock as explicit settings changes. Another owner is retained and a blocked setup is reported honestly." +--- diff --git a/internal/manual/chat/keeping-an-eye.md b/internal/manual/chat/keeping-an-eye.md index faf713c29..3f5ac2498 100644 --- a/internal/manual/chat/keeping-an-eye.md +++ b/internal/manual/chat/keeping-an-eye.md @@ -701,6 +701,17 @@ program — and a launch speaks only for its own pair. else's: it neither claims it nor rewrites it, and its `/status` says nothing is checking that home. +Approving the first standing item follows the same ownership rule. It installs a +missing timer or repairs this home's stale timer, but never takes another home's +or live build's timer. If another home owns it, the item is saved and CodeAF says +that the background check could not be installed and the existing timer was left +unchanged. The confirmation also says the item was saved but needs a codeaf window +open for this home while no background timer checks it. A non-waking permission +rule does not need a timer. This home is not being checked in the background; use the background +checks row in `/settings` only when you deliberately want to move the shared timer. +Ownership checks and changes are serialized across processes, including that +explicit settings action. + ## Do reminders work over --host — yes, on the far machine Yes, and this is the one ambient thing a connection does not take away. Over diff --git a/internal/session/standing_test.go b/internal/session/standing_test.go index 127d53bdf..d5e162425 100644 --- a/internal/session/standing_test.go +++ b/internal/session/standing_test.go @@ -724,12 +724,18 @@ func TestStandingListSpeaksThePersonsWords(t *testing.T) { // fakeWatch is this machine's scheduler, stood in for. Nothing in these tests // goes near launchd. type fakeWatch struct { + ensures int installs int uninstalls int fail error installed bool } +func (w *fakeWatch) Ensure(ctx context.Context) error { + w.ensures++ + return w.Install(ctx) +} + func (w *fakeWatch) Install(context.Context) error { w.installs++ if w.fail != nil { @@ -791,8 +797,8 @@ func TestTheFirstThingThatStandsTurnsBackgroundChecksOnAndSaysSo(t *testing.T) { watch := &fakeWatch{} events := standRatify(t, store, watch, t.TempDir()) - if watch.installs != 1 { - t.Fatalf("the timer was installed %d times, want exactly 1", watch.installs) + if watch.installs != 1 || watch.ensures != 1 { + t.Fatalf("implicit ensures=%d, installs=%d; want one ensure", watch.ensures, watch.installs) } if line := backgroundLine(events); line != standingBackgroundLine { t.Fatalf("the line said %q, want %q", line, standingBackgroundLine) @@ -2002,3 +2008,34 @@ func standingNextUpdate(t *testing.T, lane <-chan Event) Event { } } } + +func TestFirstStandingApprovalPreservesAnotherProfilesTimer(t *testing.T) { + store := newFakeStanding(t) + watch := &fakeWatch{fail: standing.ErrWatchOwned} + events := standRatify(t, store, watch, t.TempDir()) + if watch.ensures != 1 || watch.installed { + t.Fatalf("implicit ownership refusal: ensures=%d installed=%v", watch.ensures, watch.installed) + } + line := backgroundLine(events) + if !strings.HasPrefix(line, standingBackgroundFailed) || !strings.Contains(line, "existing timer was left unchanged") { + t.Fatalf("ownership refusal was not told honestly: %q", line) + } + if len(store.created) != 1 { + t.Fatal("timer refusal must not discard the approved standing item") + } + if output := toolOutput(t, events, "stand"); !strings.Contains(output, "Background checks are not installed") || !strings.Contains(output, "item was saved") { + t.Fatalf("model receipt hid background unavailability: %q", output) + } + // The first-setup marker suppresses repeated UI notices, not truthful receipts. + events = standRatify(t, store, watch, t.TempDir()) + if output := toolOutput(t, events, "stand"); !strings.Contains(output, "Background checks are not installed") { + t.Fatalf("later receipt claimed background execution: %q", output) + } +} + +func TestNonWakingRuleDoesNotRequireBackgroundTimer(t *testing.T) { + agent := &Agent{config: Config{Standing: &Standing{Watch: &fakeWatch{}}}} + if note := agent.standingBackgroundLimitation(standing.Item{When: standing.When{Kind: standing.WhenHold}}); note != "" { + t.Fatalf("permission rule was said to need a timer: %q", note) + } +} diff --git a/internal/session/steer_test.go b/internal/session/steer_test.go index a58cbd191..cb237da74 100644 --- a/internal/session/steer_test.go +++ b/internal/session/steer_test.go @@ -867,13 +867,15 @@ func TestASteerAdoptsAnOldBashAndItsExitArrivesLater(t *testing.T) { func TestASteerWaitsForAYoungBash(t *testing.T) { t.Parallel() + release := filepath.Join(t.TempDir(), "release-young-bash") completer := &scriptedCompleter{steps: []step{ - func(context.Context, []ai.Message) (*ai.Response, error) { - return toolResponse("young-bash", "bash", `{"command":"sleep 0.5; echo young-finished"}`), nil - }, + bashCall("young-bash", "while [ ! -f "+shellQuoted(release)+" ]; do sleep 0.01; done; echo young-finished"), func(context.Context, []ai.Message) (*ai.Response, error) { return textResponse("done"), nil }, }} agent, _ := newTestAgent(t, completer, nil) + // The command stays observable until released; its age is a separate + // input, not a race against a half-second sleep on a busy test machine. + advanceSteerAge(agent, 0) turn := mustSubmit(t, agent, "run the quick check") waitFor(t, "young foreground bash to start", func() bool { return len(agent.inFlightBash.snapshot()) == 1 }) agent.mu.Lock() @@ -882,18 +884,23 @@ func TestASteerWaitsForAYoungBash(t *testing.T) { if generation != nil { t.Fatal("a young bash still had a model generation to cut") } - began := time.Now() steered := mustSteer(t, agent, "then read the result") - collect(t, turn) - events := collect(t, steered) - if elapsed := time.Since(began); elapsed < 200*time.Millisecond { - t.Fatalf("young bash landed in %s, want the batch to finish first", elapsed) + select { + case event := <-steered: + if got := steerLanding([]Event{event}); got != "waiting for the running step" { + t.Fatalf("steer landing while bash is held = %q", got) + } + case <-time.After(10 * time.Second): + t.Fatal("the steer was not accepted while the bash was held") } if list := agent.jobs.list(); list != "No background jobs." { t.Fatalf("young bash became a job: %q", list) } - if got := steerLanding(events); got != "waiting for the running step" { - t.Fatalf("steer landing = %q", got) + writeFile(t, release, "finish now") + collect(t, turn) + collect(t, steered) + if got := roleText(completer.request(1), "tool"); !strings.Contains(got, "young-finished") { + t.Fatalf("the resumed turn did not receive the foreground result: %q", got) } } diff --git a/internal/session/task_phase_test.go b/internal/session/task_phase_test.go index 67422ef07..e9e7580d5 100644 --- a/internal/session/task_phase_test.go +++ b/internal/session/task_phase_test.go @@ -1,7 +1,6 @@ package session import ( - "context" "strings" "testing" "time" @@ -330,7 +329,7 @@ func TestTheHandoverSaysItIsBriefingAWorkerWhileItWritesTheBrief(t *testing.T) { ran := make(ranNodes, 2) stubbedGraph(agent, func(node *TaskNode) { ran <- node }) - events, err := agent.Submit(context.Background(), "work through the four things I listed and report back") + events, err := agent.Submit(watchedContext(agent), "work through the four things I listed and report back") if err != nil { t.Fatalf("Submit: %v", err) } diff --git a/internal/session/task_quick_test.go b/internal/session/task_quick_test.go index e605370a1..7c8ebc7ab 100644 --- a/internal/session/task_quick_test.go +++ b/internal/session/task_quick_test.go @@ -350,6 +350,9 @@ func TestTwoQuickTasksClaimingOneFileRunOneAfterTheOther(t *testing.T) { agent, graph, _ := quickAgent(t, completer) events := mustSubmit(t, agent, "rewrite both sections of notes.md") + // Admission precedes journaling the tool receipt. Finish the parent turn + // while both workers are held before inspecting what the model was told. + collect(t, events) // WHICH OF THE TWO WINS IS THE SCHEDULER'S, and the law is about the pair // rather than about either one: exactly one of them is working, and the @@ -386,7 +389,6 @@ func TestTwoQuickTasksClaimingOneFileRunOneAfterTheOther(t *testing.T) { } close(release) - collect(t, events) waitDoneNode(t, running) waitDoneNode(t, waiting) if state := waiting.stateNow(); state != TaskDone { diff --git a/internal/session/task_run_belt_test.go b/internal/session/task_run_belt_test.go index cac5f03a7..8a3396dc4 100644 --- a/internal/session/task_run_belt_test.go +++ b/internal/session/task_run_belt_test.go @@ -164,11 +164,22 @@ func registerBeltRunEngine(t *testing.T, engine RunEngine) { // directory's removal, and lost it under load as "directory not empty". func endBeltRun(t *testing.T, agent *Agent, double *beltRunDouble) { t.Helper() + agent.beltMu.Lock() + run := agent.beltRun + agent.beltMu.Unlock() close(double.release) - beltRunWaitFor(t, "the run to end", func() bool { - agent.beltMu.Lock() - defer agent.beltMu.Unlock() - return agent.beltRun == nil + if run == nil { + return + } + // releaseBeltRun clears the agent's pointer before it closes the store. + // The run's completion channel covers that final write/close as well. + beltRunWaitFor(t, "the run and its store to close", func() bool { + select { + case <-run.over: + return true + default: + return false + } }) } diff --git a/internal/session/task_run_orphan_test.go b/internal/session/task_run_orphan_test.go index 6073a1da9..0bf2bdfa8 100644 --- a/internal/session/task_run_orphan_test.go +++ b/internal/session/task_run_orphan_test.go @@ -210,8 +210,9 @@ func TestClosingUnderAProgramsRunEndsItInItsStoreFirst(t *testing.T) { t.Fatalf("the second run is on root %q with brief %q", spec.Store.RootID(), spec.Store.Task(spec.Store.RootID()).Description) } endBeltRun(t, again, second) - close(first.release) - <-first.finished + // Start returning is not the end of the owner's bookkeeping. Join the + // closed agent's run before the fixture removes its store and workspace. + endBeltRun(t, agent, first) } // THE RESTORE ROAD: a conversation read back from disk whose run row comes diff --git a/internal/session/tools_standing.go b/internal/session/tools_standing.go index fc91dfe96..12c9b8895 100644 --- a/internal/session/tools_standing.go +++ b/internal/session/tools_standing.go @@ -105,7 +105,7 @@ const standingPastGrace = 30 * time.Second const standingWatchOffer = "watch-offer.json" // standingWatchAnswer is that marker's whole content. It is journaled BEFORE -// [standing.Watch.Install] is called, so a person whose launchd would not take +// [standing.Watch.Ensure] is called, so a person whose launchd would not take // the file is somebody this build knows it has already spoken to — rather than // somebody it tells again tomorrow. // @@ -520,19 +520,35 @@ func (a *Agent) standPropose(ctx context.Context, parsed standArguments) (string } created = a.standingFileTheExchange(store, created) a.emitStandingUpdate("stood", created, "") - // AND THE FIRST THING THAT EVER STANDS TURNS THE BACKGROUND CHECKS ON. It - // is said to the person and not to the model: the line goes on the screen - // as its own dim row, and the model's whole reply is still the one sentence - // about what now stands ([standingRatifiedLine]). + // Implicit setup reports to the surface once. The tool result also carries + // current availability: saving an item is not a promise that this home + // owns the shared timer, even after the first-setup notice was already sent. a.standingBackgroundOn(store, created) line := fmt.Sprintf("set up %s: %s", created.ID, created.Words) if when := strings.TrimSpace(notice.WhenWords); when != "" { line += "\nit wakes: " + when } line += "\n" + standingRatifiedLine + line += a.standingBackgroundLimitation(created) return line, false, nil } +// Waking items need a truthful timer status in every approval receipt, not just +// the first-setup UI notice. A saved permission rule never needs a timer. +func (a *Agent) standingBackgroundLimitation(item standing.Item) string { + if item.When.Kind == standing.WhenHold || a.config.Standing == nil || a.config.Standing.Watch == nil { + return "" + } + status, err := a.config.Standing.Watch.Status() + if err != nil { + return "\nBackground check status could not be confirmed. Say that the item was saved but do not promise it runs after the window closes." + } + if !status.Installed { + return "\nBackground checks are not installed for this home. Say that the item was saved, but scheduled work needs a codeaf window open for this home; do not promise it runs with the window closed. Do not take another profile's timer or suggest the item failed to save." + } + return "" +} + // standingRatifiedLine is what a model is told the instant something stands, // and it is the WHOLE of what it may say next. // @@ -1213,8 +1229,8 @@ func (a *Agent) emitStandingNews(update string, item standing.Item, text string) // ── background checks, on by default, said once ───────────────────────────── -// standingBackgroundOn installs this machine's timer the first time anything -// ever stands, and says the one dim line about it. +// standingBackgroundOn ensures background checks the first time anything stands, +// without taking another profile's timer, and says the result in one dim line. // // NOBODY IS ASKED, AND IT HAPPENS ONCE, EVER. There used to be a question here // — keep checking when no window is open? — and it had one sensible answer: @@ -1242,7 +1258,7 @@ func (a *Agent) standingBackgroundOn(store standingStore, item standing.Item) { return } standingRememberWatch(store.Root(), true) - if err := a.config.Standing.Watch.Install(context.Background()); err != nil { + if err := a.config.Standing.Watch.Ensure(context.Background()); err != nil { // SAID HONESTLY AND NOT SWALLOWED. The person is about to walk away from // a machine they think is watching something for them. a.emitStandingUpdate(standingBackgroundUpdate, item, diff --git a/internal/session/tools_tasks_test.go b/internal/session/tools_tasks_test.go index 7db3f0d70..af133390a 100644 --- a/internal/session/tools_tasks_test.go +++ b/internal/session/tools_tasks_test.go @@ -572,7 +572,9 @@ func TestHandingAYourCallToTheModelAndItsResolveAreOneRoad(t *testing.T) { // what leaves a your-call landing in the PERSON'S hands: with nobody watching // the model holds every one of them by policy ([Agent.settlePolicy]) and the // card this test is about is never drawn. - agent, _ := newTestAgent(t, &scriptedCompleter{}, func(config *Config) { + // Keep the woken turn open until the test resolves the decision; a finished + // turn correctly hands unresolved work back to the person. + agent, _ := newTestAgent(t, &holdingCompleter{}, func(config *Config) { config.Place, config.AskConsent = Place{Dir: mine}, true }) graph := stubbedGraph(agent, func(node *TaskNode) { diff --git a/internal/standing/standing.go b/internal/standing/standing.go index 02292e71f..2fcb2aa8e 100644 --- a/internal/standing/standing.go +++ b/internal/standing/standing.go @@ -1075,6 +1075,7 @@ type WatchStatus struct { // `codeaf tick` every [Interval]. The core lane builds it on internal/watchdog's // shape with its own unit names, so it can coexist with v1's. type Watch interface { + Ensure(ctx context.Context) error Install(ctx context.Context) error Uninstall(ctx context.Context) error Status() (WatchStatus, error) diff --git a/internal/standing/watch.go b/internal/standing/watch.go index 63299b918..c8faa264b 100644 --- a/internal/standing/watch.go +++ b/internal/standing/watch.go @@ -44,6 +44,7 @@ import ( "time" "github.com/Agent-Field/codeaf/internal/env" + "github.com/Agent-Field/codeaf/internal/filelock" "github.com/Agent-Field/codeaf/internal/home" ) @@ -183,12 +184,64 @@ func NewWatch(options WatchOptions) (*Timer, error) { }, nil } -// Install writes the exact definition and asks this user's operating system to -// use it now. Installing over an existing one repairs drift and is safe. +// ErrWatchOwned means implicit setup would take the login's shared timer away +// from another profile or a live build. Only explicit settings may do that. +var ErrWatchOwned = errors.New("background checks belong to another profile or running build; the existing timer was left unchanged") + +// Ensure provides background checks without taking another owner's timer. +// It is the implicit first-approval path; Install is an explicit takeover. +func (w *Timer) Ensure(ctx context.Context) error { + return w.underLock(ctx, func() error { + seen, err := w.read() + if err != nil { + return err + } + if seen.drift.Present { + if !seen.ours { + return ErrWatchOwned + } + if !seen.drift.Stale { + return nil // This profile is already checked, possibly by another build. + } + if w.otherLiveProgram(seen) { + return ErrWatchOwned + } + } + return w.install(ctx) + }) +} + +// Repair only restores an existing, repairable timer belonging to this profile. +// The returned drift names the repair attempted (successful when err is nil), never an earlier reading +// that could race an explicit off or another profile's install. +func (w *Timer) Repair(ctx context.Context) (WatchDrift, error) { + var repaired WatchDrift + err := w.underLock(ctx, func() error { + seen, err := w.read() + if err != nil { + return err + } + if !seen.drift.Present || !seen.drift.Stale || !seen.ours || w.otherLiveProgram(seen) { + return nil + } + repaired = seen.drift + return w.install(ctx) + }) + return repaired, err +} + +func (w *Timer) otherLiveProgram(seen reading) bool { + return seen.drift.Executable != "" && seen.drift.Executable != w.executable && !seen.drift.Gone +} + +// Install is the explicit settings action: write this profile/program pair, +// including taking over an existing timer. Implicit callers must use Ensure +// or Repair, which apply ownership under the same interprocess lock. func (w *Timer) Install(ctx context.Context) error { - if w == nil { - return errors.New("standing: no timer") - } + return w.underLock(ctx, func() error { return w.install(ctx) }) +} + +func (w *Timer) install(ctx context.Context) error { switch w.platform { case "darwin": return w.installDarwin(ctx) @@ -198,19 +251,61 @@ func (w *Timer) Install(ctx context.Context) error { return fmt.Errorf("standing: keeping watch is not available on %s", w.platform) } -// Uninstall stops the timer and removes its definition. A definition that is -// not there is already uninstalled, and says so without touching the host. +// Uninstall is the explicit settings action that stops and removes the timer. func (w *Timer) Uninstall(ctx context.Context) error { + return w.underLock(ctx, func() error { + switch w.platform { + case "darwin": + return w.uninstallDarwin(ctx) + case "linux": + return w.uninstallLinux(ctx) + } + return fmt.Errorf("standing: keeping watch is not available on %s", w.platform) + }) +} + +// One lock per OS timer, not per CODEAF_HOME: ownership checks and all writes +// must serialize across profiles and builds. The lock file is never removed, +// so a waiter cannot keep a lock on an unlinked inode while another replaces it. +func (w *Timer) underLock(ctx context.Context, change func() error) error { if w == nil { return errors.New("standing: no timer") } - switch w.platform { - case "darwin": - return w.uninstallDarwin(ctx) - case "linux": - return w.uninstallLinux(ctx) + if w.platform != "linux" && w.platform != "darwin" { + return fmt.Errorf("standing: keeping watch is not available on %s", w.platform) + } + if err := ctx.Err(); err != nil { + return err + } + path := w.primaryPath() + ".lock" + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return err + } + lock, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return err + } + defer lock.Close() + retry := time.NewTicker(25 * time.Millisecond) + defer retry.Stop() + for { + if err := ctx.Err(); err != nil { + return err + } + err := filelock.Lock(lock, true, true) + if err == nil { + defer func() { _ = filelock.Unlock(lock) }() + return change() + } + if !filelock.IsBusy(err) { + return err + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-retry.C: + } } - return fmt.Errorf("standing: keeping watch is not available on %s", w.platform) } // Status derives everything: installation from the definition's own bytes, the diff --git a/internal/standing/watch_ownership_test.go b/internal/standing/watch_ownership_test.go new file mode 100644 index 000000000..9a57593f3 --- /dev/null +++ b/internal/standing/watch_ownership_test.go @@ -0,0 +1,224 @@ +package standing + +import ( + "context" + "errors" + "os" + "path/filepath" + "reflect" + "sync" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/filelock" +) + +func timerDefinitions(t *testing.T, w *Timer) map[string]string { + t.Helper() + paths := []string{w.primaryPath()} + if w.platform == "linux" { + paths = append(paths, w.linuxServicePath()) + } + out := map[string]string{} + for _, path := range paths { + b, err := os.ReadFile(path) + if err != nil && !os.IsNotExist(err) { + t.Fatal(err) + } + out[path] = string(b) + } + return out +} + +func TestImplicitTimerSetupRespectsOwnership(t *testing.T) { + for _, platform := range []string{"linux", "darwin"} { + for _, scenario := range []string{"other-home", "other-home-gone", "same-home-live", "same-home-live-drift", "same-home-gone"} { + t.Run(platform+"/"+scenario, func(t *testing.T) { + runner := &recordingRunner{} + owner := newTimer(t, platform, t.TempDir(), "", runner, time.Now()) + if err := owner.Install(context.Background()); err != nil { + t.Fatal(err) + } + contender := *owner + contender.executable = realProgram(t) + if scenario == "other-home" || scenario == "other-home-gone" { + contender.stateRoot = filepath.Join(t.TempDir(), "state") + } + if scenario == "other-home-gone" || scenario == "same-home-gone" { + if err := os.Remove(owner.executable); err != nil { + t.Fatal(err) + } + } + if scenario == "same-home-live-drift" { + f, err := os.OpenFile(owner.primaryPath(), os.O_APPEND|os.O_WRONLY, 0) + if err != nil { + t.Fatal(err) + } + _, err = f.WriteString("\n") + _ = f.Close() + if err != nil { + t.Fatal(err) + } + } + before := timerDefinitions(t, owner) + runner.calls = nil + err := contender.Ensure(context.Background()) + denied := scenario == "other-home" || scenario == "other-home-gone" || scenario == "same-home-live-drift" + if denied && !errors.Is(err, ErrWatchOwned) { + t.Fatalf("want ownership refusal, got %v", err) + } + if !denied && err != nil { + t.Fatal(err) + } + if scenario == "same-home-gone" { + status, err := contender.Status() + if err != nil || !status.Installed || len(runner.calls) == 0 { + t.Fatalf("own stale timer not repaired: %+v %v", status, err) + } + } else { + if len(runner.calls) != 0 || !reflect.DeepEqual(before, timerDefinitions(t, owner)) { + t.Fatal("implicit setup changed another owner or touched its scheduler") + } + } + // Explicit settings still deliberately take ownership. + if err := contender.Install(context.Background()); err != nil { + t.Fatal(err) + } + status, err := contender.Status() + if err != nil || !status.Installed { + t.Fatalf("explicit takeover failed: %+v %v", status, err) + } + }) + } + } +} + +func TestImplicitTimerSetupInstallsOnlyWhenAskedAndRepairKeepsAbsence(t *testing.T) { + for _, platform := range []string{"linux", "darwin"} { + t.Run(platform, func(t *testing.T) { + runner := &recordingRunner{} + timer := newTimer(t, platform, t.TempDir(), "", runner, time.Now()) + drift, err := timer.Repair(context.Background()) + if err != nil || drift.Present || len(runner.calls) != 0 { + t.Fatalf("repair installed an absent timer: %+v %v", drift, err) + } + if err := timer.Ensure(context.Background()); err != nil { + t.Fatal(err) + } + if len(runner.calls) == 0 { + t.Fatal("missing timer was not installed") + } + runner.calls = nil + if err := timer.Ensure(context.Background()); err != nil { + t.Fatal(err) + } + if len(runner.calls) != 0 { + t.Fatal("healthy timer was reinstalled") + } + }) + } +} + +func TestEveryTimerMutationSharesTheCancellableOwnerLock(t *testing.T) { + for _, platform := range []string{"linux", "darwin"} { + t.Run(platform, func(t *testing.T) { + runner := &recordingRunner{} + timer := newTimer(t, platform, t.TempDir(), "", runner, time.Now()) + if err := timer.Install(context.Background()); err != nil { + t.Fatal(err) + } + before := timerDefinitions(t, timer) + runner.calls = nil + lock, err := os.OpenFile(timer.primaryPath()+".lock", os.O_RDWR, 0) + if err != nil { + t.Fatal(err) + } + defer lock.Close() + if err := filelock.Lock(lock, true, false); err != nil { + t.Fatal(err) + } + defer filelock.Unlock(lock) + operations := map[string]func(context.Context) error{"ensure": timer.Ensure, "install": timer.Install, "uninstall": timer.Uninstall, "repair": func(ctx context.Context) error { _, err := timer.Repair(ctx); return err }} + for name, operation := range operations { + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond) + err := operation(ctx) + cancel() + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("%s bypassed lock: %v", name, err) + } + } + if len(runner.calls) != 0 || !reflect.DeepEqual(before, timerDefinitions(t, timer)) { + t.Fatal("blocked writer touched scheduler or definitions") + } + }) + } +} + +type heldInstallRunner struct { + entered, release chan struct{} + once sync.Once +} + +func (r *heldInstallRunner) Run(ctx context.Context, _ string, _ ...string) error { + r.once.Do(func() { close(r.entered) }) + select { + case <-r.release: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} + +func TestConcurrentProfilesCannotBothImplicitlyClaimTimer(t *testing.T) { + for _, platform := range []string{"linux", "darwin"} { + t.Run(platform, func(t *testing.T) { + runner := &heldInstallRunner{entered: make(chan struct{}), release: make(chan struct{})} + var release sync.Once + defer release.Do(func() { close(runner.release) }) + first := newTimer(t, platform, t.TempDir(), "", runner, time.Now()) + second := *first + second.stateRoot = filepath.Join(t.TempDir(), "second") + second.executable = realProgram(t) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + a, b := make(chan error, 1), make(chan error, 1) + go func() { a <- first.Ensure(ctx) }() + select { + case <-runner.entered: + case <-ctx.Done(): + t.Fatal("first installer did not start") + } + go func() { b <- second.Ensure(ctx) }() + select { + case err := <-b: + t.Fatalf("second installer bypassed ownership lock: %v", err) + case <-time.After(40 * time.Millisecond): + } + release.Do(func() { close(runner.release) }) + if err := <-a; err != nil { + t.Fatal(err) + } + if err := <-b; !errors.Is(err, ErrWatchOwned) { + t.Fatalf("second profile stole timer: %v", err) + } + status, err := first.Status() + if err != nil || !status.Installed { + t.Fatalf("first owner lost timer: %+v %v", status, err) + } + }) + } +} + +func TestUnsupportedTimerOperationsDoNotCreateHostFiles(t *testing.T) { + dir := t.TempDir() + timer := &Timer{platform: "unsupported", homeDir: dir} + for _, operation := range []func(context.Context) error{timer.Ensure, timer.Install, timer.Uninstall, func(ctx context.Context) error { _, err := timer.Repair(ctx); return err }} { + if err := operation(context.Background()); err == nil { + t.Fatal("unsupported timer accepted") + } + } + entries, err := os.ReadDir(dir) + if err != nil || len(entries) != 0 { + t.Fatalf("unsupported operations touched host: %v %v", entries, err) + } +}