From eb8932a319ee35d368666155d85ecdc593a5e6ea Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 10:28:40 +0800 Subject: [PATCH] fix: refresh mutations and defer busy quit --- internal/app/app.go | 12 ++++ internal/app/coordination_test.go | 47 ++++++++++++ internal/app/storage_test.go | 4 +- internal/tui/model.go | 35 +++++++-- internal/tui/target_test.go | 115 +++++++++++++++++++++++------- 5 files changed, 182 insertions(+), 31 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 5acf69a..c0f2183 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -153,6 +153,18 @@ func (a *Application) dispatch(intent Intent, publish func(Result)) Result { } defer unlock() } + // A different process may have changed the persisted Active Profile since + // this Application was constructed. Reload only after the global mutation + // lock is held, before selecting a Profile or invoking a Target. Load is + // deliberately separate from Store.Mutate so no Profile-store lock is nested. + fresh, err := a.store.Load() + if err != nil { + return Result{Snapshot: a.Snapshot(), Err: fmt.Errorf("reload Profile state for mutation: %w", err)} + } + a.state = fresh + if intent.Kind == SetTargetDesired || intent.Kind == ActivateProfile { + a.refreshInspections() + } if intent.Kind == SetTargetDesired { return a.setTargetDesired(intent) } diff --git a/internal/app/coordination_test.go b/internal/app/coordination_test.go index 5bae975..0b6ad10 100644 --- a/internal/app/coordination_test.go +++ b/internal/app/coordination_test.go @@ -10,6 +10,7 @@ import ( "proxyctl/internal/app" "proxyctl/internal/dockerclient" + "proxyctl/internal/operationlock" "proxyctl/internal/profile" "proxyctl/internal/shell" "proxyctl/internal/target" @@ -56,6 +57,52 @@ func (t *coordinatedTarget) SetDesired(_ context.Context, enabled bool, active * return target.OperationResult{Inspection: target.Inspection{State: t.state, Evidence: "matches " + active.Name}} } +func TestSecondApplicationUsesPersistedActiveProfileChangedByFirstApplication(t *testing.T) { + root := t.TempDir() + storePath := filepath.Join(root, "profiles.json") + bootstrap, err := app.New(profile.NewFileStore(storePath)) + if err != nil { + t.Fatal(err) + } + for _, name := range []string{"old", "new"} { + result := bootstrap.Dispatch(app.Intent{Kind: app.CreateProfile, Profile: profile.Profile{Name: name, Mode: profile.SingleMode, SingleURL: "http://" + name + ".example:8080"}}) + if result.Err != nil { + t.Fatalf("create %s Profile: %v", name, result.Err) + } + } + if result := bootstrap.Dispatch(app.Intent{Kind: app.ActivateProfile, Name: "old"}); result.Err != nil { + t.Fatalf("activate old Profile: %v", result.Err) + } + + firstTarget := &coordinatedTarget{id: "shell", state: target.Disabled} + secondTarget := &coordinatedTarget{id: "shell", state: target.Disabled} + first, err := app.NewWithTargetsAndLock(profile.NewFileStore(storePath), []target.Target{firstTarget}, operationlock.New(filepath.Join(root, "operation.lock"))) + if err != nil { + t.Fatal(err) + } + second, err := app.NewWithTargetsAndLock(profile.NewFileStore(storePath), []target.Target{secondTarget}, operationlock.New(filepath.Join(root, "operation.lock"))) + if err != nil { + t.Fatal(err) + } + if got := second.Snapshot().ActiveProfile; got != "old" { + t.Fatalf("second Application starting Active Profile = %q, want old", got) + } + + if result := first.Dispatch(app.Intent{Kind: app.ActivateProfile, Name: "new"}); result.Err != nil { + t.Fatalf("first Application activates new Profile: %v", result.Err) + } + result := second.Dispatch(app.Intent{Kind: app.SetTargetDesired, TargetID: "shell", Enabled: true}) + if result.Err != nil { + t.Fatalf("second Application enables Target: %v", result.Err) + } + if secondTarget.appliedName != "new" { + t.Fatalf("second Application applied stale Profile %q, want freshly persisted Active Profile new", secondTarget.appliedName) + } + if result.Snapshot.ActiveProfile != "new" { + t.Fatalf("second Application snapshot Active Profile = %q, want new", result.Snapshot.ActiveProfile) + } +} + func TestActiveProfileSwitchCoordinatesEnabledTargetsWithoutRollback(t *testing.T) { first := &coordinatedTarget{id: "first", state: target.Disabled} second := &coordinatedTarget{id: "second", state: target.Disabled, failName: "new"} diff --git a/internal/app/storage_test.go b/internal/app/storage_test.go index 246102d..192aec4 100644 --- a/internal/app/storage_test.go +++ b/internal/app/storage_test.go @@ -104,8 +104,8 @@ func TestApplicationReloadsStateAfterCommittedDurabilityError(t *testing.T) { if !errors.As(result.Err, &committed) { t.Fatalf("mutation error = %v, want committed durability error", result.Err) } - if store.loadCalls != 2 { - t.Fatalf("Load calls = %d, want startup plus post-commit reload", store.loadCalls) + if store.loadCalls != 3 { + t.Fatalf("Load calls = %d, want startup, pre-mutation, and post-commit reloads", store.loadCalls) } if len(result.Snapshot.Profiles) != 1 || result.Snapshot.Profiles[0].Name != "committed" { t.Fatalf("Application snapshot after committed error = %#v", result.Snapshot) diff --git a/internal/tui/model.go b/internal/tui/model.go index 9d9704c..beb2d76 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -41,6 +41,7 @@ type Model struct { refreshing bool profileTransition bool progressTarget string + quitRequested bool } type targetOperationDone struct { @@ -76,10 +77,18 @@ func (m Model) Init() tea.Cmd { return nil } // operation start checks it, so a completion can only belong to the one current // operation rather than racing another operation for shared model state. func (m Model) busy() bool { - return m.applying || m.refreshing || m.profileTransition || m.pendingActivation.Kind != 0 + return m.operationInFlight() || m.pendingActivation.Kind != 0 +} + +func (m Model) operationInFlight() bool { + return m.applying || m.refreshing || m.profileTransition } func (m Model) Update(message tea.Msg) (tea.Model, tea.Cmd) { + if key, ok := message.(tea.KeyMsg); ok && m.operationInFlight() && isQuitKey(key) { + m.quitRequested = true + return m, nil + } if completed, ok := message.(refreshDone); ok { if !m.refreshing { return m, nil @@ -99,7 +108,7 @@ func (m Model) Update(message tea.Msg) (tea.Model, tea.Cmd) { } else if m.selected >= targetCount { m.selected = targetCount - 1 } - return m, nil + return m, m.deferredQuit() } if progress, ok := message.(profileOperationProgress); ok { if !m.profileTransition { @@ -120,7 +129,7 @@ func (m Model) Update(message tea.Msg) (tea.Model, tea.Cmd) { intent := m.pendingActivation m.profileConfirm = &intent m.status = progress.result.Confirmation - return m, nil + return m, m.deferredQuit() } if progress.result.Err != nil { m.status = "\x1b[31mError: " + progress.result.Err.Error() + "\x1b[0m" @@ -132,7 +141,7 @@ func (m Model) Update(message tea.Msg) (tea.Model, tea.Cmd) { m.profileTransition = false m.progressTarget = "" m.pendingActivation = app.Intent{} - return m, nil + return m, m.deferredQuit() } return m, waitForProfileProgress(progress.updates) } @@ -156,7 +165,7 @@ func (m Model) Update(message tea.Msg) (tea.Model, tea.Cmd) { m.targetConfirm = nil m.status = completed.result.Notice } - return m, nil + return m, m.deferredQuit() } key, ok := message.(tea.KeyMsg) if !ok { @@ -179,6 +188,22 @@ func (m Model) Update(message tea.Msg) (tea.Model, tea.Cmd) { } } +func isQuitKey(key tea.KeyMsg) bool { + switch key.String() { + case "q", "esc", "ctrl+c": + return true + default: + return false + } +} + +func (m Model) deferredQuit() tea.Cmd { + if m.quitRequested { + return tea.Quit + } + return nil +} + func (m Model) updateMain(key tea.KeyMsg) (tea.Model, tea.Cmd) { if m.targetConfirm != nil && !m.busy() { switch key.String() { diff --git a/internal/tui/target_test.go b/internal/tui/target_test.go index 55ccafb..4fcda9c 100644 --- a/internal/tui/target_test.go +++ b/internal/tui/target_test.go @@ -147,13 +147,11 @@ func TestBusyActivationBlocksPageEntryAndEveryNewOperation(t *testing.T) { t.Fatalf("busy activation accepted %q: page=%v command=%v intents=%#v", key, model.page, blocked != nil, module.intents) } } - for _, key := range []tea.KeyMsg{keyMessage("q"), {Type: tea.KeyEsc}} { - _, quit := model.Update(key) - if quit == nil { - t.Fatalf("busy activation changed quit behavior for %q", key.String()) - } - if _, ok := quit().(tea.QuitMsg); !ok || len(module.intents) != 1 { - t.Fatalf("busy activation quit %q triggered an operation: intents=%#v", key.String(), module.intents) + for _, key := range []tea.KeyMsg{keyMessage("q"), {Type: tea.KeyEsc}, {Type: tea.KeyCtrlC}} { + updated, quit := model.Update(key) + deferred := updated.(Model) + if quit != nil || !deferred.quitRequested || len(module.intents) != 1 { + t.Fatalf("busy activation did not defer quit %q: command=%v requested=%v intents=%#v", key.String(), quit != nil, deferred.quitRequested, module.intents) } } @@ -187,13 +185,11 @@ func TestBusyRefreshBlocksTogglePageEntryRefreshAndActivation(t *testing.T) { t.Fatalf("busy refresh accepted %q: page=%v command=%v intents=%#v", key, model.page, blocked != nil, module.intents) } } - for _, key := range []tea.KeyMsg{keyMessage("q"), {Type: tea.KeyEsc}} { - _, quit := model.Update(key) - if quit == nil { - t.Fatalf("busy refresh changed quit behavior for %q", key.String()) - } - if _, ok := quit().(tea.QuitMsg); !ok || len(module.intents) != 0 { - t.Fatalf("busy refresh quit %q triggered an operation: intents=%#v", key.String(), module.intents) + for _, key := range []tea.KeyMsg{keyMessage("q"), {Type: tea.KeyEsc}, {Type: tea.KeyCtrlC}} { + updated, quit := model.Update(key) + deferred := updated.(Model) + if quit != nil || !deferred.quitRequested || len(module.intents) != 0 { + t.Fatalf("busy refresh did not defer quit %q: command=%v requested=%v intents=%#v", key.String(), quit != nil, deferred.quitRequested, module.intents) } } @@ -234,17 +230,54 @@ func TestPendingActivationRemainsBusyUntilConfirmationIsResolved(t *testing.T) { } } +func TestProfileConfirmationQuitKeysCancelWithoutLeavingBusyState(t *testing.T) { + for _, cancelKey := range []tea.KeyMsg{keyMessage("q"), {Type: tea.KeyEsc}} { + t.Run(cancelKey.String(), func(t *testing.T) { + initial := app.Snapshot{Profiles: []profile.Profile{{Name: "office"}}} + updates := make(chan app.Result, 2) + updates <- app.Result{Snapshot: initial, Confirmation: "confirm transition"} + module := &progressiveTargetModule{targetModule: targetModule{snapshot: initial}, updates: updates} + model := updateKey(NewModel(module), "p") + updated, command := model.Update(tea.KeyMsg{Type: tea.KeyEnter}) + model = updated.(Model) + updated, command = model.Update(command()) + model = updated.(Model) + if command != nil || model.profileConfirm == nil || !model.busy() { + t.Fatal("activation confirmation was not presented") + } + + updated, command = model.Update(cancelKey) + model = updated.(Model) + if command != nil || model.profileConfirm != nil || model.busy() || model.quitRequested { + t.Fatalf("confirmation cancel %q left the model blocked: %#v", cancelKey.String(), model) + } + + updates <- app.Result{Snapshot: initial, Confirmation: "confirm transition again"} + updated, command = model.Update(tea.KeyMsg{Type: tea.KeyEnter}) + if command == nil { + t.Fatal("confirmation cancellation prevented a later activation") + } + }) + } +} + func TestSingleOperationGuardRejectsMismatchedCompletionAndPreventsOverlap(t *testing.T) { initial := app.Snapshot{Targets: []app.TargetSnapshot{{ID: "shell", Description: target.Description{Name: "Shell"}, Available: true, State: target.Disabled}}} - module := &targetModule{snapshot: initial, result: app.Result{Snapshot: initial}} + refreshed := app.Snapshot{ActiveProfile: "fresh", Targets: initial.Targets} + module := &targetModule{snapshot: initial, result: app.Result{Snapshot: refreshed}} model := NewModel(module) updated, refreshCommand := model.Update(keyMessage("r")) model = updated.(Model) - - updated, command := model.Update(targetOperationDone{result: app.Result{Snapshot: app.Snapshot{}}}) + updated, command := model.Update(keyMessage("q")) model = updated.(Model) - if command != nil || !model.refreshing || !model.busy() { - t.Fatal("completion from a non-current operation cleared refresh state") + if command != nil || !model.quitRequested { + t.Fatal("quit was not deferred during refresh") + } + + updated, command = model.Update(targetOperationDone{result: app.Result{Snapshot: app.Snapshot{ActiveProfile: "stale"}}}) + model = updated.(Model) + if command != nil || !model.refreshing || !model.busy() || model.snapshot.ActiveProfile != "" { + t.Fatal("completion from a non-current operation cleared or corrupted refresh state") } updated, command = model.Update(tea.KeyMsg{Type: tea.KeySpace}) model = updated.(Model) @@ -255,12 +288,46 @@ func TestSingleOperationGuardRejectsMismatchedCompletionAndPreventsOverlap(t *te refreshCompletion := refreshCommand() updated, command = model.Update(refreshCompletion) model = updated.(Model) - if command != nil || model.busy() { - t.Fatal("current refresh completion did not clear busy state") + if command == nil || model.busy() || model.snapshot.ActiveProfile != "fresh" { + t.Fatal("current refresh completion did not apply fresh state and trigger deferred quit") } - updated, command = model.Update(tea.KeyMsg{Type: tea.KeySpace}) - if command == nil { - t.Fatal("new operation was not allowed after the prior completion was consumed") + if _, ok := command().(tea.QuitMsg); !ok { + t.Fatal("current completion did not return tea.Quit") + } +} + +func TestBusyQuitWaitsForLongRunningTargetMutationCompletion(t *testing.T) { + for _, quitKey := range []tea.KeyMsg{keyMessage("q"), {Type: tea.KeyEsc}, {Type: tea.KeyCtrlC}} { + t.Run(quitKey.String(), func(t *testing.T) { + initial := app.Snapshot{ActiveProfile: "office", Targets: []app.TargetSnapshot{{ID: "shell", Description: target.Description{Name: "Shell"}, Available: true, State: target.Disabled}}} + completed := app.Snapshot{ActiveProfile: "office", Targets: []app.TargetSnapshot{{ID: "shell", Description: target.Description{Name: "Shell"}, Available: true, State: target.Enabled}}} + module := &targetModule{snapshot: initial, result: app.Result{Snapshot: completed, Notice: "Shell Target is now Enabled"}} + model := NewModel(module) + updated, operation := model.Update(tea.KeyMsg{Type: tea.KeySpace}) + model = updated.(Model) + if operation == nil || len(module.intents) != 0 { + t.Fatal("Target mutation did not start asynchronously") + } + + updated, quit := model.Update(quitKey) + model = updated.(Model) + if quit != nil || !model.quitRequested || len(module.intents) != 0 { + t.Fatalf("quit was not deferred before mutation completion: command=%v requested=%v intents=%d", quit != nil, model.quitRequested, len(module.intents)) + } + + completion := operation() + if len(module.intents) != 1 { + t.Fatal("deferred quit abandoned the in-flight mutation") + } + updated, quit = model.Update(completion) + model = updated.(Model) + if quit == nil || model.busy() || model.snapshot.Targets[0].State != target.Enabled { + t.Fatalf("mutation completion was not consumed before deferred quit: busy=%v snapshot=%#v", model.busy(), model.snapshot) + } + if _, ok := quit().(tea.QuitMsg); !ok { + t.Fatal("completion did not return tea.Quit") + } + }) } }