diff --git a/internal/services/compositionsvc/compositionservice.go b/internal/services/compositionsvc/compositionservice.go index 2bb9b8aa..6f563fba 100644 --- a/internal/services/compositionsvc/compositionservice.go +++ b/internal/services/compositionsvc/compositionservice.go @@ -58,10 +58,11 @@ type Syncer interface { // new one. See docs/SPEC.md §3's `UX: PROTOTYPE` entry for what this is // testing. type CompositionService struct { - mu sync.Mutex - store settings.Store - user []composition.Workflow - syncer Syncer + mu sync.Mutex + store settings.Store + user []composition.Workflow + syncer Syncer + onDeleted func(id string) } func NewCompositionService(store settings.Store) *CompositionService { @@ -82,6 +83,18 @@ func (c *CompositionService) SetSyncer(s Syncer) { c.syncer = s } +// SetWorkflowDeleted wires the hook DeleteWorkflow fires after a +// successful delete (docs/goals/0250): derived per-workflow state +// held by OTHER services (a trigger-hotkey binding) must be released +// when its workflow goes away, or it leaks in settings keyed by a +// dead id. Same injected-func seam as SetSyncer -- compositionsvc +// never imports the owning service. +// +//wails:ignore +func (c *CompositionService) SetWorkflowDeleted(fn func(id string)) { + c.onDeleted = fn +} + // notifySyncer re-registers every workflow's trigger listener after a // mutation -- a no-op until SetSyncer has run (defensive, not expected // to matter: main.go wires it immediately after construction, before any @@ -369,75 +382,6 @@ func (c *CompositionService) UpdateAttributes(workflowID string, attrs []composi return wf, nil } -// DeleteWorkflow removes a workflow -- seeded or user-composed, both -// live in c.user (see Workflows' doc comment), no built-in special -// case. Blocked while any OTHER workflow's child-workflow node still -// references id (docs/adr/0040 decision 3, same reference-integrity -// rule ConfigureService's own Delete* methods apply to every other -// RefKind, via WorkflowsReferencing). -func (c *CompositionService) DeleteWorkflow(id string) error { - if refs := c.WorkflowsReferencing("workflow", id); len(refs) > 0 { - return fmt.Errorf("workflow %q is still referenced by workflow(s) %s -- remove the reference before deleting it", id, strings.Join(refs, ", ")) - } - - c.mu.Lock() - idx := -1 - for i, wf := range c.user { - if wf.ID == id { - idx = i - break - } - } - if idx == -1 { - c.mu.Unlock() - return fmt.Errorf("no workflow with id %q", id) - } - removed := c.user[idx] - wasBuiltIn := removed.BuiltIn - c.user = append(c.user[:idx], c.user[idx+1:]...) - c.mu.Unlock() - - // A deleted built-in gets a tombstone so top-up seeding (restore) - // never resurrects it -- deletion stays permanent (§2.2). Tombstone - // and removal must succeed together: if the tombstone can't be - // persisted, leaving the in-memory removal in place would mean the - // next restart's top-up seeding silently resurrects a workflow the - // user just deleted (docs/goals/0025 item 2) -- so roll the removal - // back and fail the whole delete instead. - if wasBuiltIn { - if err := seeding.RecordTombstone(c.store, id); err != nil { - c.mu.Lock() - c.insertAtLocked(idx, removed) - c.mu.Unlock() - return fmt.Errorf("tombstone deleted workflow %q: %w", id, err) - } - } - if err := c.persist(); err != nil { - c.mu.Lock() - c.insertAtLocked(idx, removed) - c.mu.Unlock() - return fmt.Errorf("save workflow deletion: %w", err) - } - c.notifySyncer() - dataevent.Emit("workflow", id) // goal 0017: live-sync every open surface - return nil -} - -// insertAtLocked reinserts wf at idx (clamped to the current length) -- -// caller must hold c.mu. Used to undo DeleteWorkflow's removal when the -// tombstone or persist step that must accompany it fails (docs/goals/0025 -// item 2's memory-vs-store consistency rule); the exact index rarely -// matters (nothing depends on workflow order), it's just the least -// surprising place to put it back. -func (c *CompositionService) insertAtLocked(idx int, wf composition.Workflow) { - if idx < 0 || idx > len(c.user) { - idx = len(c.user) - } - c.user = append(c.user, composition.Workflow{}) - copy(c.user[idx+1:], c.user[idx:]) - c.user[idx] = wf -} - func (c *CompositionService) persist() error { c.mu.Lock() user := make([]composition.Workflow, len(c.user)) diff --git a/internal/services/compositionsvc/compositionservice_delete.go b/internal/services/compositionsvc/compositionservice_delete.go new file mode 100644 index 00000000..288ed32c --- /dev/null +++ b/internal/services/compositionsvc/compositionservice_delete.go @@ -0,0 +1,87 @@ +package compositionsvc + +import ( + "fmt" + "strings" + + "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/services/dataevent" + "github.com/alicoding/mill/internal/services/seeding" +) + +// The workflow delete path -- removal, the built-in tombstone, +// rollback on persist failure, and the derived-state release hook +// (docs/goals/0250) -- split from compositionservice.go at the +// 500-line convention. + +// DeleteWorkflow removes a workflow -- seeded or user-composed, both +// live in c.user (see Workflows' doc comment), no built-in special +// case. Blocked while any OTHER workflow's child-workflow node still +// references id (docs/adr/0040 decision 3, same reference-integrity +// rule ConfigureService's own Delete* methods apply to every other +// RefKind, via WorkflowsReferencing). +func (c *CompositionService) DeleteWorkflow(id string) error { + if refs := c.WorkflowsReferencing("workflow", id); len(refs) > 0 { + return fmt.Errorf("workflow %q is still referenced by workflow(s) %s -- remove the reference before deleting it", id, strings.Join(refs, ", ")) + } + + c.mu.Lock() + idx := -1 + for i, wf := range c.user { + if wf.ID == id { + idx = i + break + } + } + if idx == -1 { + c.mu.Unlock() + return fmt.Errorf("no workflow with id %q", id) + } + removed := c.user[idx] + wasBuiltIn := removed.BuiltIn + c.user = append(c.user[:idx], c.user[idx+1:]...) + c.mu.Unlock() + + // A deleted built-in gets a tombstone so top-up seeding (restore) + // never resurrects it -- deletion stays permanent (§2.2). Tombstone + // and removal must succeed together: if the tombstone can't be + // persisted, leaving the in-memory removal in place would mean the + // next restart's top-up seeding silently resurrects a workflow the + // user just deleted (docs/goals/0025 item 2) -- so roll the removal + // back and fail the whole delete instead. + if wasBuiltIn { + if err := seeding.RecordTombstone(c.store, id); err != nil { + c.mu.Lock() + c.insertAtLocked(idx, removed) + c.mu.Unlock() + return fmt.Errorf("tombstone deleted workflow %q: %w", id, err) + } + } + if err := c.persist(); err != nil { + c.mu.Lock() + c.insertAtLocked(idx, removed) + c.mu.Unlock() + return fmt.Errorf("save workflow deletion: %w", err) + } + c.notifySyncer() + if c.onDeleted != nil { + c.onDeleted(id) + } + dataevent.Emit("workflow", id) // goal 0017: live-sync every open surface + return nil +} + +// insertAtLocked reinserts wf at idx (clamped to the current length) -- +// caller must hold c.mu. Used to undo DeleteWorkflow's removal when the +// tombstone or persist step that must accompany it fails (docs/goals/0025 +// item 2's memory-vs-store consistency rule); the exact index rarely +// matters (nothing depends on workflow order), it's just the least +// surprising place to put it back. +func (c *CompositionService) insertAtLocked(idx int, wf composition.Workflow) { + if idx < 0 || idx > len(c.user) { + idx = len(c.user) + } + c.user = append(c.user, composition.Workflow{}) + copy(c.user[idx+1:], c.user[idx:]) + c.user[idx] = wf +} diff --git a/internal/services/triggersvc/triggerhotkeyassignment.go b/internal/services/triggersvc/triggerhotkeyassignment.go index 639895a1..9e2f0d4a 100644 --- a/internal/services/triggersvc/triggerhotkeyassignment.go +++ b/internal/services/triggersvc/triggerhotkeyassignment.go @@ -173,6 +173,38 @@ func (s *TriggerService) DebugAssignHotkey(workflowID string, mods []string, key return label, nil } +// PruneOrphanedHotkeys drops every binding whose workflow id is not +// in validIDs -- the one-shot boot heal for bindings leaked by +// deletes that predate the delete-releases-binding hook +// (docs/goals/0250, the composition-state class). Deliberately NOT +// folded into Sync: Sync can run with transient partial workflow +// lists, and pruning there would silently drop live bindings; this +// runs once from the composition root, after both services have +// loaded their persisted state, with the full id set. +// +//wails:ignore +func (s *TriggerService) PruneOrphanedHotkeys(validIDs []string) { + valid := make(map[string]bool, len(validIDs)) + for _, id := range validIDs { + valid[id] = true + } + s.mu.Lock() + var dropped []string + for id := range s.hkRaw { + if !valid[id] { + dropped = append(dropped, id) + delete(s.hkRaw, id) + } + } + s.mu.Unlock() + if len(dropped) == 0 { + return + } + s.persistHotkeys() + s.logger.Info("orphaned trigger hotkeys pruned", "workflows", dropped) + s.Sync(s.comp.Workflows()) +} + // ListHotkeys returns every workflow ID with an assigned hotkey, mapped // to its human-readable binding label (e.g. "⌘⇧M"). func (s *TriggerService) ListHotkeys() map[string]string { diff --git a/internal/services/triggersvc/triggerhotkeyprune_test.go b/internal/services/triggersvc/triggerhotkeyprune_test.go new file mode 100644 index 00000000..56cc8e62 --- /dev/null +++ b/internal/services/triggersvc/triggerhotkeyprune_test.go @@ -0,0 +1,80 @@ +package triggersvc + +import ( + "log/slog" + "testing" + + "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/services/compositionsvc" + "github.com/alicoding/mill/internal/services/servicetest" +) + +// Regression (docs/goals/0250, composition-state): deleting a workflow +// left its hotkey binding in settings, so a later assign of the same +// combo was refused naming the dead workflow's raw id. The wired +// delete hook must release the binding and free the combo. +func TestDeleteWorkflow_ReleasesHotkeyBinding(t *testing.T) { + store := servicetest.NewFakeStore() + comp := compositionsvc.NewCompositionService(store) + s := NewTriggerService(comp, slog.Default(), store) + comp.SetWorkflowDeleted(s.UnassignHotkey) + + wf, err := comp.CreateWorkflow("Doomed", "", []composition.Node{{ID: "t", NodeTypeID: "trigger-manual"}}, nil) + if err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + if _, err := s.DebugAssignHotkey(wf.ID, []string{"cmd", "shift"}, "K"); err != nil { + t.Fatalf("DebugAssignHotkey: %v", err) + } + + if err := comp.DeleteWorkflow(wf.ID); err != nil { + t.Fatalf("DeleteWorkflow: %v", err) + } + if _, still := s.ListHotkeys()[wf.ID]; still { + t.Fatal("deleted workflow's hotkey binding survived the delete") + } + + // The combo is genuinely free again: another workflow can take it. + if _, err := s.finalizeHotkeyAssignment("survivor", []string{"cmd", "shift"}, "K"); err != nil { + t.Fatalf("reassigning the freed combo: %v", err) + } +} + +// PruneOrphanedHotkeys drops bindings whose workflow no longer exists +// (the boot heal for state leaked before the delete hook existed), +// keeps live ones, and never persists when nothing is orphaned. +func TestPruneOrphanedHotkeys(t *testing.T) { + store := servicetest.NewFakeStore() + comp := compositionsvc.NewCompositionService(store) + s := NewTriggerService(comp, slog.Default(), store) + + if _, err := s.DebugAssignHotkey("ghost-workflow", []string{"cmd"}, "G"); err != nil { + t.Fatalf("DebugAssignHotkey ghost: %v", err) + } + if _, err := s.DebugAssignHotkey("live-workflow", []string{"cmd"}, "L"); err != nil { + t.Fatalf("DebugAssignHotkey live: %v", err) + } + + s.PruneOrphanedHotkeys([]string{"live-workflow"}) + + got := s.ListHotkeys() + if _, ghost := got["ghost-workflow"]; ghost { + t.Fatal("orphaned binding survived the prune") + } + if _, live := got["live-workflow"]; !live { + t.Fatal("live binding was wrongly pruned") + } +} + +func TestPruneOrphanedHotkeys_NoOrphansIsANoOp(t *testing.T) { + store := servicetest.NewFakeStore() + comp := compositionsvc.NewCompositionService(store) + s := NewTriggerService(comp, slog.Default(), store) + + // Nothing bound, nothing orphaned -- the prune must not write the + // bindings key at all (no gratuitous persist on every boot). + s.PruneOrphanedHotkeys([]string{"whatever"}) + if store.Get(HotkeyBindingsKey) != nil { + t.Fatalf("no-op prune persisted: store[%q] = %#v", HotkeyBindingsKey, store.Get(HotkeyBindingsKey)) + } +} diff --git a/internal/services/wiring/wiring.go b/internal/services/wiring/wiring.go index 9ea30900..9250ac0d 100644 --- a/internal/services/wiring/wiring.go +++ b/internal/services/wiring/wiring.go @@ -396,3 +396,20 @@ func WireMillMCPService(mill *mcpsvc.MillMCPService, settingsService *settingssv logger.Info("mill MCP server listening", "addr", addr) } } + +// WireWorkflowLifecycle connects CompositionService's workflow +// lifecycle to TriggerService (docs/goals/0250): the sync seam, the +// delete-releases-hotkey hook, and a one-shot orphan prune healing +// bindings already leaked by deletes that predate the hook -- safe to +// run here because both services have loaded their persisted state by +// wire time. +func WireWorkflowLifecycle(comp *compositionsvc.CompositionService, triggers *triggersvc.TriggerService) { + comp.SetSyncer(triggers) + comp.SetWorkflowDeleted(triggers.UnassignHotkey) + workflows := comp.Workflows() + ids := make([]string, 0, len(workflows)) + for _, wf := range workflows { + ids = append(ids, wf.ID) + } + triggers.PruneOrphanedHotkeys(ids) +} diff --git a/main.go b/main.go index 72277515..2e1432bd 100644 --- a/main.go +++ b/main.go @@ -130,7 +130,7 @@ func main() { compositionService := compositionsvc.NewCompositionService(settingsStore) triggerService := triggersvc.NewTriggerService(compositionService, logger, settingsStore) - compositionService.SetSyncer(triggerService) + wiring.WireWorkflowLifecycle(compositionService, triggerService) // docs/goals/0250-workflow-delete-releases-hotkey.md // MILL_TEST_KEYRING=memory swaps the OS keychain for a process- // local store (e2e servers only): Linux CI has no Secret Service, // so real-keychain semantics -- including "credential absent" --