fix: deleting a workflow releases its trigger-hotkey binding (goal 0250)

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))

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
+}

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 {

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))
+	}
+}

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)
+}

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" --