diff --git a/apps/backend/internal/backendapp/ownershiplock/lock.go b/apps/backend/internal/backendapp/ownershiplock/lock.go index e203e12b9f..a47c713608 100644 --- a/apps/backend/internal/backendapp/ownershiplock/lock.go +++ b/apps/backend/internal/backendapp/ownershiplock/lock.go @@ -14,10 +14,15 @@ var ErrConflict = errors.New("runtime state is already owned") // ConflictError identifies the target that prevented startup. type ConflictError struct { Target Target + Owner *OwnerRecord } func (e *ConflictError) Error() string { - return fmt.Sprintf("%s target %q is already owned by another backend", e.Target.Kind, e.Target.ResourcePath) + message := fmt.Sprintf("%s target %q is already owned by another backend", e.Target.Kind, e.Target.ResourcePath) + if e.Owner != nil { + message += " (" + e.Owner.String() + ")" + } + return message } func (e *ConflictError) Unwrap() error { return ErrConflict } @@ -55,10 +60,11 @@ func Acquire(targets []Target) (*Owner, error) { _ = file.Close() _ = owner.Close() if errors.Is(err, ErrConflict) { - return nil, &ConflictError{Target: target} + return nil, &ConflictError{Target: target, Owner: readOwner(target.LockPath)} } return nil, fmt.Errorf("acquire %s ownership lock %q: %w", target.Kind, target.LockPath, err) } + _ = writeOwnerFn(file) owner.held = append(owner.held, heldLock{target: target, file: file}) } return owner, nil @@ -68,7 +74,7 @@ func openLockFile(path string) (*os.File, error) { if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return nil, err } - return os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600) + return openLockFilePlatform(path) } // Close releases every held OS lock in reverse acquisition order. The first diff --git a/apps/backend/internal/backendapp/ownershiplock/lock_test.go b/apps/backend/internal/backendapp/ownershiplock/lock_test.go index 3a9d1c5885..262a94599c 100644 --- a/apps/backend/internal/backendapp/ownershiplock/lock_test.go +++ b/apps/backend/internal/backendapp/ownershiplock/lock_test.go @@ -2,8 +2,11 @@ package ownershiplock import ( "bufio" + "bytes" + "encoding/json" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -86,6 +89,32 @@ func TestOwnershipLockReleasesAfterForcedProcessExit(t *testing.T) { } } +func TestOwnershipLockRejectsSymlinkedLockPath(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + link := filepath.Join(dir, "lock") + want := []byte("must remain unchanged") + if err := os.WriteFile(target, want, 0o600); err != nil { + t.Fatalf("write target: %v", err) + } + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + file, err := openLockFile(link) + if err == nil { + _ = file.Close() + t.Fatal("openLockFile succeeded for symlinked lock path") + } + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("read target: %v", err) + } + if !bytes.Equal(got, want) { + t.Fatalf("symlink target changed to %q, want %q", got, want) + } +} + func TestOwnershipLockAllowsIndependentHomes(t *testing.T) { firstHome := t.TempDir() firstTargets, err := Targets(firstHome, "sqlite", "") @@ -223,4 +252,126 @@ func TestOwnershipLockConflictErrorIncludesTarget(t *testing.T) { if conflict.Target.ResourcePath != targets[0].ResourcePath { t.Fatalf("conflict target = %q, want canonical home path %q", conflict.Target.ResourcePath, targets[0].ResourcePath) } + if conflict.Owner == nil { + t.Fatal("conflict owner metadata is nil") + } + if conflict.Owner.PID != int64(os.Getpid()) { + t.Fatalf("conflict owner PID = %d, want %d", conflict.Owner.PID, os.Getpid()) + } + if !strings.Contains(conflict.Error(), fmt.Sprintf("pid %d", os.Getpid())) { + t.Fatalf("conflict error = %q, want owner PID", conflict.Error()) + } +} + +func TestOwnershipLockConflictFallsBackWithoutUsableOwner(t *testing.T) { + oldLiveness := ownerLivenessFn + ownerLivenessFn = func(int64) (bool, bool) { return false, true } + t.Cleanup(func() { ownerLivenessFn = oldLiveness }) + + tests := []struct { + name string + data []byte + }{ + {name: "empty", data: ownerMetadataFixture("")}, + {name: "invalid", data: ownerMetadataFixture("not json")}, + {name: "dead", data: ownerMetadataBytes(mustOwnerJSON(t, OwnerRecord{PID: 987654321, Executable: "dead-kandev", StartedAt: "2026-08-19T09:14:35Z"}))}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + home := t.TempDir() + targets, err := Targets(home, "sqlite", "") + if err != nil { + t.Fatalf("Targets: %v", err) + } + owner, err := Acquire(targets) + if err != nil { + t.Fatalf("Acquire primary owner: %v", err) + } + t.Cleanup(func() { _ = owner.Close() }) + if err := writeOwnerMetadataFixture(owner.held[0].file, test.data); err != nil { + t.Fatalf("write conflict metadata: %v", err) + } + + _, err = Acquire(targets) + var conflict *ConflictError + if !errors.As(err, &conflict) { + t.Fatalf("second Acquire error = %v, want ConflictError", err) + } + want := fmt.Sprintf("%s target %q is already owned by another backend", TargetHome, targets[0].ResourcePath) + if conflict.Error() != want { + t.Fatalf("conflict error = %q, want %q", conflict.Error(), want) + } + if conflict.Owner != nil { + t.Fatalf("conflict owner = %+v, want nil", conflict.Owner) + } + }) + } +} + +func writeOwnerMetadataFixture(file *os.File, data []byte) error { + if err := file.Truncate(0); err != nil { + return err + } + n, err := file.WriteAt(data, 0) + if err != nil { + return err + } + if n != len(data) { + return io.ErrShortWrite + } + return nil +} + +func TestOwnershipLockConflictKeepsUnknownOwner(t *testing.T) { + oldLiveness := ownerLivenessFn + ownerLivenessFn = func(int64) (bool, bool) { return false, false } + t.Cleanup(func() { ownerLivenessFn = oldLiveness }) + + home := t.TempDir() + targets, err := Targets(home, "sqlite", "") + if err != nil { + t.Fatalf("Targets: %v", err) + } + owner, err := Acquire(targets) + if err != nil { + t.Fatalf("Acquire primary owner: %v", err) + } + t.Cleanup(func() { _ = owner.Close() }) + + _, err = Acquire(targets) + var conflict *ConflictError + if !errors.As(err, &conflict) { + t.Fatalf("second Acquire error = %v, want ConflictError", err) + } + if conflict.Owner == nil { + t.Fatal("unknown liveness dropped owner metadata") + } +} + +func TestOwnershipLockMetadataWriteFailureDoesNotBlockAcquire(t *testing.T) { + oldWriteOwner := writeOwnerFn + writeOwnerFn = func(*os.File) error { return errors.New("metadata unavailable") } + t.Cleanup(func() { writeOwnerFn = oldWriteOwner }) + + home := t.TempDir() + targets, err := Targets(home, "sqlite", "") + if err != nil { + t.Fatalf("Targets: %v", err) + } + owner, err := Acquire(targets) + if err != nil { + t.Fatalf("Acquire with metadata failure: %v", err) + } + if err := owner.Close(); err != nil { + t.Fatalf("Close: %v", err) + } +} + +func mustOwnerJSON(t *testing.T, owner OwnerRecord) []byte { + t.Helper() + data, err := json.Marshal(owner) + if err != nil { + t.Fatalf("marshal owner: %v", err) + } + return data } diff --git a/apps/backend/internal/backendapp/ownershiplock/lock_unix.go b/apps/backend/internal/backendapp/ownershiplock/lock_unix.go index 3994b80d51..0bfd359ff5 100644 --- a/apps/backend/internal/backendapp/ownershiplock/lock_unix.go +++ b/apps/backend/internal/backendapp/ownershiplock/lock_unix.go @@ -4,11 +4,34 @@ package ownershiplock import ( "errors" + "fmt" "os" "golang.org/x/sys/unix" ) +func openLockFilePlatform(path string) (*os.File, error) { + fd, err := unix.Open(path, unix.O_RDWR|unix.O_CREAT|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0o600) + if err != nil { + return nil, err + } + file := os.NewFile(uintptr(fd), path) + if file == nil { + _ = unix.Close(fd) + return nil, fmt.Errorf("create ownership lock file handle for %q", path) + } + info, err := file.Stat() + if err != nil { + _ = file.Close() + return nil, err + } + if !info.Mode().IsRegular() { + _ = file.Close() + return nil, fmt.Errorf("ownership lock path %q is not a regular file", path) + } + return file, nil +} + func lockFile(file *os.File) error { err := unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB) if err == nil { diff --git a/apps/backend/internal/backendapp/ownershiplock/lock_windows.go b/apps/backend/internal/backendapp/ownershiplock/lock_windows.go index 6b19ae4dca..d9494b273b 100644 --- a/apps/backend/internal/backendapp/ownershiplock/lock_windows.go +++ b/apps/backend/internal/backendapp/ownershiplock/lock_windows.go @@ -4,11 +4,51 @@ package ownershiplock import ( "errors" + "fmt" "os" "golang.org/x/sys/windows" ) +func openLockFilePlatform(path string) (*os.File, error) { + handle, err := windows.CreateFile( + windows.StringToUTF16Ptr(path), + windows.GENERIC_READ|windows.GENERIC_WRITE, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_ALWAYS, + windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, + 0, + ) + if err != nil { + return nil, err + } + file := os.NewFile(uintptr(handle), path) + if file == nil { + _ = windows.CloseHandle(handle) + return nil, fmt.Errorf("create ownership lock file handle for %q", path) + } + fileType, err := windows.GetFileType(handle) + if err != nil { + _ = file.Close() + return nil, err + } + var info windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(handle, &info); err != nil { + _ = file.Close() + return nil, err + } + if info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { + _ = file.Close() + return nil, fmt.Errorf("ownership lock path %q is a reparse point", path) + } + if info.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0 || fileType != windows.FILE_TYPE_DISK { + _ = file.Close() + return nil, fmt.Errorf("ownership lock path %q is not a regular file", path) + } + return file, nil +} + func lockFile(file *os.File) error { err := windows.LockFileEx( windows.Handle(file.Fd()), diff --git a/apps/backend/internal/backendapp/ownershiplock/owner.go b/apps/backend/internal/backendapp/ownershiplock/owner.go new file mode 100644 index 0000000000..822f381a97 --- /dev/null +++ b/apps/backend/internal/backendapp/ownershiplock/owner.go @@ -0,0 +1,98 @@ +package ownershiplock + +import ( + "encoding/json" + "fmt" + "io" + "os" + "time" + + "github.com/kandev/kandev/internal/common/proclive" +) + +// OwnerRecord is advisory diagnostic metadata about the process that acquired +// a lock. It is never consulted to decide ownership, staleness, or takeover. +type OwnerRecord struct { + PID int64 `json:"pid"` + Executable string `json:"executable"` + StartedAt string `json:"started_at"` +} + +var ( + writeOwnerFn = writeOwner + ownerLivenessFn = proclive.Alive +) + +const ( + // Windows locks the first byte of the file. Keep metadata after that byte + // so a conflicting process can read it through a separate file handle. + ownerMetadataOffset int64 = 1 + maxOwnerMetadataLength int64 = 4096 +) + +func writeOwner(file *os.File) error { + executable, _ := os.Executable() + record := OwnerRecord{ + PID: int64(os.Getpid()), + Executable: executable, + StartedAt: time.Now().UTC().Format(time.RFC3339Nano), + } + data, err := json.Marshal(record) + if err != nil { + return err + } + if err := file.Truncate(0); err != nil { + return err + } + n, err := file.WriteAt(data, ownerMetadataOffset) + if err != nil { + return err + } + if n != len(data) { + return io.ErrShortWrite + } + return nil +} + +func readOwner(path string) *OwnerRecord { + file, err := os.Open(path) + if err != nil { + return nil + } + defer func() { _ = file.Close() }() + info, err := file.Stat() + if err != nil { + return nil + } + metadataLength := info.Size() - ownerMetadataOffset + if metadataLength <= 0 || metadataLength > maxOwnerMetadataLength { + return nil + } + data := make([]byte, int(metadataLength)) + if _, err := file.ReadAt(data, ownerMetadataOffset); err != nil { + return nil + } + var record OwnerRecord + if err := json.Unmarshal(data, &record); err != nil || record.PID <= 0 { + return nil + } + alive, known := ownerLivenessFn(record.PID) + if known && !alive { + return nil + } + return &record +} + +func (r *OwnerRecord) String() string { + if r == nil { + return "" + } + detail := fmt.Sprintf("pid %d", r.PID) + if r.Executable != "" { + detail += ", " + r.Executable + } + if r.StartedAt != "" { + detail += ", started " + r.StartedAt + } + return detail +} diff --git a/apps/backend/internal/backendapp/ownershiplock/owner_test.go b/apps/backend/internal/backendapp/ownershiplock/owner_test.go new file mode 100644 index 0000000000..b68b4fbff2 --- /dev/null +++ b/apps/backend/internal/backendapp/ownershiplock/owner_test.go @@ -0,0 +1,118 @@ +package ownershiplock + +import ( + "os" + "path/filepath" + "testing" +) + +func TestWriteOwnerRoundTrips(t *testing.T) { + path := filepath.Join(t.TempDir(), "lock") + file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600) + if err != nil { + t.Fatalf("open lock file: %v", err) + } + t.Cleanup(func() { _ = file.Close() }) + + if err := writeOwner(file); err != nil { + t.Fatalf("writeOwner: %v", err) + } + owner := readOwner(path) + if owner == nil { + t.Fatal("readOwner returned nil") + return + } + if owner.PID != int64(os.Getpid()) { + t.Fatalf("owner PID = %d, want %d", owner.PID, os.Getpid()) + } + if owner.Executable == "" { + t.Fatal("owner executable is empty") + } + if owner.StartedAt == "" { + t.Fatal("owner start time is empty") + } +} + +func TestReadOwnerWhileLockIsHeld(t *testing.T) { + path := filepath.Join(t.TempDir(), "lock") + file, err := openLockFile(path) + if err != nil { + t.Fatalf("open lock file: %v", err) + } + t.Cleanup(func() { + _ = unlockFile(file) + _ = file.Close() + }) + if err := lockFile(file); err != nil { + t.Fatalf("lockFile: %v", err) + } + if err := writeOwner(file); err != nil { + t.Fatalf("writeOwner: %v", err) + } + + owner := readOwner(path) + if owner == nil { + t.Fatal("readOwner returned nil while lock was held") + return + } + if owner.PID != int64(os.Getpid()) { + t.Fatalf("owner PID = %d, want %d", owner.PID, os.Getpid()) + } +} + +func TestWriteOwnerTruncatesPreviousRecord(t *testing.T) { + path := filepath.Join(t.TempDir(), "lock") + file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600) + if err != nil { + t.Fatalf("open lock file: %v", err) + } + t.Cleanup(func() { _ = file.Close() }) + if _, err := file.WriteString(`{"pid":123456789,"executable":"` + string(make([]byte, 4096)) + `"}`); err != nil { + t.Fatalf("write previous record: %v", err) + } + + if err := writeOwner(file); err != nil { + t.Fatalf("writeOwner: %v", err) + } + owner := readOwner(path) + if owner == nil { + t.Fatal("readOwner returned nil after truncating previous metadata") + return + } + if owner.PID != int64(os.Getpid()) { + t.Fatalf("owner PID = %d, want %d", owner.PID, os.Getpid()) + } +} + +func TestReadOwnerRejectsInvalidMetadata(t *testing.T) { + tests := []struct { + name string + data string + }{ + {name: "missing", data: ""}, + {name: "invalid json", data: "not json"}, + {name: "missing pid", data: `{"executable":"kandev"}`}, + {name: "non-positive pid", data: `{"pid":0}`}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "lock") + if test.name != "missing" { + if err := os.WriteFile(path, ownerMetadataFixture(test.data), 0o600); err != nil { + t.Fatalf("write metadata: %v", err) + } + } + if owner := readOwner(path); owner != nil { + t.Fatalf("readOwner = %+v, want nil", owner) + } + }) + } +} + +func ownerMetadataFixture(data string) []byte { + return ownerMetadataBytes([]byte(data)) +} + +func ownerMetadataBytes(data []byte) []byte { + return append([]byte{0}, data...) +} diff --git a/apps/backend/internal/common/proclive/proclive.go b/apps/backend/internal/common/proclive/proclive.go new file mode 100644 index 0000000000..995f855f80 --- /dev/null +++ b/apps/backend/internal/common/proclive/proclive.go @@ -0,0 +1,5 @@ +// Package proclive provides platform-specific process liveness probes. +// +// A false known value means the platform cannot determine liveness. Callers +// must treat that result as "do not act". +package proclive diff --git a/apps/backend/internal/system/storage/tempartifacts/process_alive_unix.go b/apps/backend/internal/common/proclive/proclive_unix.go similarity index 53% rename from apps/backend/internal/system/storage/tempartifacts/process_alive_unix.go rename to apps/backend/internal/common/proclive/proclive_unix.go index 0cabefc159..61401c538b 100644 --- a/apps/backend/internal/system/storage/tempartifacts/process_alive_unix.go +++ b/apps/backend/internal/common/proclive/proclive_unix.go @@ -1,10 +1,12 @@ //go:build !windows -package tempartifacts +package proclive import "syscall" -func processAlive(pid int64) (alive, known bool) { +// Alive reports whether pid is alive. Unix signal-zero checks distinguish a +// dead process from a process that exists but cannot be signaled by the caller. +func Alive(pid int64) (alive, known bool) { if pid <= 0 { return false, true } diff --git a/apps/backend/internal/common/proclive/proclive_unix_test.go b/apps/backend/internal/common/proclive/proclive_unix_test.go new file mode 100644 index 0000000000..4225153f79 --- /dev/null +++ b/apps/backend/internal/common/proclive/proclive_unix_test.go @@ -0,0 +1,46 @@ +//go:build !windows + +package proclive + +import ( + "os" + "os/exec" + "testing" +) + +func TestAliveReportsCurrentProcess(t *testing.T) { + alive, known := Alive(int64(os.Getpid())) + if !alive || !known { + t.Fatalf("Alive(self) = (%t, %t), want (true, true)", alive, known) + } +} + +func TestAliveReportsReapedProcessAsDead(t *testing.T) { + child := exec.Command("sh", "-c", "exit 0") + if err := child.Start(); err != nil { + t.Fatalf("start child: %v", err) + } + t.Cleanup(func() { + if child.ProcessState == nil { + _ = child.Process.Kill() + _ = child.Wait() + } + }) + if err := child.Wait(); err != nil { + t.Fatalf("wait child: %v", err) + } + + alive, known := Alive(int64(child.Process.Pid)) + if alive || !known { + t.Fatalf("Alive(reaped child) = (%t, %t), want (false, true)", alive, known) + } +} + +func TestAliveReportsNonPositivePIDAsDeadAndKnown(t *testing.T) { + for _, pid := range []int64{0, -1} { + alive, known := Alive(pid) + if alive || !known { + t.Errorf("Alive(%d) = (%t, %t), want (false, true)", pid, alive, known) + } + } +} diff --git a/apps/backend/internal/common/proclive/proclive_windows.go b/apps/backend/internal/common/proclive/proclive_windows.go new file mode 100644 index 0000000000..c136aa685d --- /dev/null +++ b/apps/backend/internal/common/proclive/proclive_windows.go @@ -0,0 +1,8 @@ +//go:build windows + +package proclive + +// Alive cannot portably determine process liveness on Windows. +func Alive(int64) (alive, known bool) { + return false, false +} diff --git a/apps/backend/internal/common/proclive/proclive_windows_test.go b/apps/backend/internal/common/proclive/proclive_windows_test.go new file mode 100644 index 0000000000..c510abe3e2 --- /dev/null +++ b/apps/backend/internal/common/proclive/proclive_windows_test.go @@ -0,0 +1,15 @@ +//go:build windows + +package proclive + +import ( + "os" + "testing" +) + +func TestAliveReportsUnknownOnWindows(t *testing.T) { + alive, known := Alive(int64(os.Getpid())) + if alive || known { + t.Fatalf("Alive(self) = (%t, %t), want (false, false)", alive, known) + } +} diff --git a/apps/backend/internal/launcher/constants.go b/apps/backend/internal/launcher/constants.go index 96b1e7218f..ca368a831f 100644 --- a/apps/backend/internal/launcher/constants.go +++ b/apps/backend/internal/launcher/constants.go @@ -10,6 +10,7 @@ const ( defaultBackendPort = 38429 defaultWebPort = 37429 defaultAgentctlPort = 39429 + parentPIDEnv = "KANDEV_LAUNCHER_PARENT_PID" healthTimeoutReleaseMS = 45000 healthTimeoutDevMS = 600000 diff --git a/apps/backend/internal/launcher/parentwatch.go b/apps/backend/internal/launcher/parentwatch.go new file mode 100644 index 0000000000..49b2e52d8d --- /dev/null +++ b/apps/backend/internal/launcher/parentwatch.go @@ -0,0 +1,104 @@ +package launcher + +import ( + "os" + "strconv" + "strings" + "sync" + "time" + + "github.com/kandev/kandev/internal/common/proclive" +) + +const parentWatchPollInterval = 2 * time.Second + +type parentWatchdog struct { + parentPID int64 + interval time.Duration + probe func(int64) (alive, known bool) + shutdown func(string) + exit func(int) + + mu sync.Mutex + started bool + stopped bool + stopCh chan struct{} + wg sync.WaitGroup +} + +func newParentWatchdog(parentPID int64, shutdown func(string), exit func(int)) *parentWatchdog { + return &parentWatchdog{ + parentPID: parentPID, + interval: parentWatchPollInterval, + probe: proclive.Alive, + shutdown: shutdown, + exit: exit, + } +} + +func newParentWatchdogFromEnv(shutdown func(string), exit func(int)) *parentWatchdog { + return newParentWatchdog(parentPIDFromEnv(), shutdown, exit) +} + +func parentPIDFromEnv() int64 { + raw := strings.TrimSpace(os.Getenv(parentPIDEnv)) + pid, err := strconv.ParseInt(raw, 10, 64) + if err != nil || pid <= 0 { + return 0 + } + return pid +} + +func (w *parentWatchdog) start() { + w.mu.Lock() + if w.parentPID <= 0 || w.started || w.stopped { + w.mu.Unlock() + return + } + w.started = true + w.stopCh = make(chan struct{}) + w.wg.Add(1) + w.mu.Unlock() + + go w.run() +} + +func (w *parentWatchdog) stop() { + w.mu.Lock() + if w.stopped { + w.mu.Unlock() + return + } + w.stopped = true + started := w.started + if started { + close(w.stopCh) + } + w.mu.Unlock() + if started { + w.wg.Wait() + } +} + +func (w *parentWatchdog) run() { + defer w.wg.Done() + ticker := time.NewTicker(w.interval) + defer ticker.Stop() + + for { + select { + case <-w.stopCh: + return + case <-ticker.C: + alive, known := w.probe(w.parentPID) + if !known || alive { + continue + } + launcherInfof("parent process exited; shutting down supervised processes (parent_pid=%d)", w.parentPID) + shutdownDebugf("launcher parent watchdog detected exit parent_pid=%d launcher_pid=%d", w.parentPID, os.Getpid()) + w.shutdown("parent process exit") + w.exit(0) + return + } + } +} diff --git a/apps/backend/internal/launcher/parentwatch_test.go b/apps/backend/internal/launcher/parentwatch_test.go new file mode 100644 index 0000000000..e0553c025b --- /dev/null +++ b/apps/backend/internal/launcher/parentwatch_test.go @@ -0,0 +1,100 @@ +package launcher + +import ( + "sync/atomic" + "testing" + "testing/synctest" + "time" +) + +func TestParentWatchdogShutsDownWhenParentDies(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + var alive atomic.Bool + alive.Store(true) + var shutdowns atomic.Int32 + var exits atomic.Int32 + watchdog := newParentWatchdog(1234, func(string) { + shutdowns.Add(1) + }, func(code int) { + if code == 0 { + exits.Add(1) + } + }) + watchdog.interval = time.Second + watchdog.probe = func(int64) (bool, bool) { + return alive.Load(), true + } + t.Cleanup(watchdog.stop) + + watchdog.start() + time.Sleep(time.Second) + synctest.Wait() + if shutdowns.Load() != 0 { + t.Fatal("watchdog shut down while parent was alive") + } + + alive.Store(false) + time.Sleep(time.Second) + synctest.Wait() + if shutdowns.Load() != 1 { + t.Fatalf("shutdown count = %d, want 1", shutdowns.Load()) + } + if exits.Load() != 1 { + t.Fatalf("exit count = %d, want 1", exits.Load()) + } + }) +} + +func TestParentWatchdogIgnoresUnknownLiveness(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + var shutdowns atomic.Int32 + var exits atomic.Int32 + watchdog := newParentWatchdog(1234, func(string) { + shutdowns.Add(1) + }, func(int) { + exits.Add(1) + }) + watchdog.interval = time.Second + watchdog.probe = func(int64) (bool, bool) { + return false, false + } + t.Cleanup(watchdog.stop) + + watchdog.start() + time.Sleep(3 * time.Second) + synctest.Wait() + if shutdowns.Load() != 0 || exits.Load() != 0 { + t.Fatalf("unknown liveness caused shutdown=%d exit=%d", shutdowns.Load(), exits.Load()) + } + }) +} + +func TestParentWatchdogStopIsIdempotent(t *testing.T) { + watchdog := newParentWatchdog(1234, func(string) { + t.Fatal("unexpected shutdown") + }, func(int) { + t.Fatal("unexpected exit") + }) + watchdog.probe = func(int64) (bool, bool) { return true, true } + watchdog.start() + watchdog.stop() + watchdog.stop() +} + +func TestParentPIDFromEnvRejectsInvalidValues(t *testing.T) { + for _, value := range []string{"", "0", "-1", "not-a-pid"} { + t.Run(value, func(t *testing.T) { + t.Setenv(parentPIDEnv, value) + if got := parentPIDFromEnv(); got != 0 { + t.Fatalf("parentPIDFromEnv() = %d for %q, want 0", got, value) + } + }) + } +} + +func TestParentPIDFromEnvAcceptsPositivePID(t *testing.T) { + t.Setenv(parentPIDEnv, "1234") + if got := parentPIDFromEnv(); got != 1234 { + t.Fatalf("parentPIDFromEnv() = %d, want 1234", got) + } +} diff --git a/apps/backend/internal/launcher/parentwatch_unix_test.go b/apps/backend/internal/launcher/parentwatch_unix_test.go new file mode 100644 index 0000000000..ffb03c632c --- /dev/null +++ b/apps/backend/internal/launcher/parentwatch_unix_test.go @@ -0,0 +1,57 @@ +//go:build !windows + +package launcher + +import ( + "os/exec" + "testing" + "time" +) + +func TestParentWatchdogDetectsReapedProcess(t *testing.T) { + child := exec.Command("sleep", "10") + if err := child.Start(); err != nil { + t.Fatalf("start child: %v", err) + } + t.Cleanup(func() { + if child.ProcessState == nil { + _ = child.Process.Kill() + } + _ = child.Wait() + }) + + shutdown := make(chan string, 1) + exited := make(chan int, 1) + watchdog := newParentWatchdog(int64(child.Process.Pid), func(reason string) { + shutdown <- reason + }, func(code int) { + exited <- code + }) + watchdog.interval = 5 * time.Millisecond + watchdog.start() + t.Cleanup(watchdog.stop) + + if err := child.Process.Kill(); err != nil { + t.Fatalf("kill child: %v", err) + } + if err := child.Wait(); err == nil { + t.Fatal("child exited successfully after kill") + } + + select { + case reason := <-shutdown: + if reason != "parent process exit" { + t.Fatalf("shutdown reason = %q, want parent process exit", reason) + } + case <-time.After(time.Second): + t.Fatal("watchdog did not detect the reaped process") + } + select { + case code := <-exited: + if code != 0 { + t.Fatalf("exit code = %d, want 0", code) + } + case <-time.After(time.Second): + t.Fatal("watchdog did not request launcher exit") + } +} diff --git a/apps/backend/internal/launcher/start.go b/apps/backend/internal/launcher/start.go index b57f1017e0..c4b2760d41 100644 --- a/apps/backend/internal/launcher/start.go +++ b/apps/backend/internal/launcher/start.go @@ -18,6 +18,11 @@ var ( attachSignalsFn = func(supervisor *processSupervisor) { supervisor.attachSignals() } + startParentWatchFn = func(supervisor *processSupervisor) *parentWatchdog { + watchdog := newParentWatchdogFromEnv(supervisor.shutdown, launcherExit) + watchdog.start() + return watchdog + } ) func runStart(ctx context.Context, opts Options) int { @@ -92,6 +97,10 @@ func runManagedApp(ctx context.Context, cfg managedAppConfig) int { supervisor := newSupervisorFn() attachSignalsFn(supervisor) shutdownDebugf("runManagedApp signal handler attached") + parentWatchdog := startParentWatchFn(supervisor) + if parentWatchdog != nil { + defer parentWatchdog.stop() + } healthToken, err := launchHealthToken() if err != nil { fmt.Fprintln(os.Stderr, "[kandev] "+err.Error()) diff --git a/apps/backend/internal/launcher/start_test.go b/apps/backend/internal/launcher/start_test.go index 8c965cd228..8cce7be3d5 100644 --- a/apps/backend/internal/launcher/start_test.go +++ b/apps/backend/internal/launcher/start_test.go @@ -168,11 +168,13 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { oldNewSupervisor := newSupervisorFn oldLaunchBackend := launchBackendFn oldAttachSignals := attachSignalsFn + oldStartParentWatch := startParentWatchFn oldWaitForHealth := waitForHealthFn t.Cleanup(func() { newSupervisorFn = oldNewSupervisor launchBackendFn = oldLaunchBackend attachSignalsFn = oldAttachSignals + startParentWatchFn = oldStartParentWatch waitForHealthFn = oldWaitForHealth }) @@ -201,6 +203,10 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { attachSignalsFn = func(_ *processSupervisor) { events = append(events, "attach-signals") } + startParentWatchFn = func(_ *processSupervisor) *parentWatchdog { + events = append(events, "start-parent-watch") + return newParentWatchdog(0, nil, nil) + } waitForHealthFn = func(_ context.Context, _ string, _ childState, _ time.Duration, expectedToken string, _ func()) error { waitedHealthToken = expectedToken events = append(events, "wait-health") @@ -222,7 +228,7 @@ func TestRunManagedAppAttachesSignalsBeforeBackendLaunch(t *testing.T) { if code != 0 { t.Fatalf("runManagedApp() = %d, want 0", code) } - want := []string{"new-supervisor", "attach-signals", "launch-backend", "wait-health"} + want := []string{"new-supervisor", "attach-signals", "start-parent-watch", "launch-backend", "wait-health"} if !reflect.DeepEqual(events, want) { t.Fatalf("events = %v, want %v", events, want) } diff --git a/apps/backend/internal/system/storage/tempartifacts/process_alive_windows.go b/apps/backend/internal/system/storage/tempartifacts/process_alive_windows.go deleted file mode 100644 index 7859255f7c..0000000000 --- a/apps/backend/internal/system/storage/tempartifacts/process_alive_windows.go +++ /dev/null @@ -1,9 +0,0 @@ -//go:build windows - -package tempartifacts - -func processAlive(pid int64) (alive, known bool) { - // Windows does not expose a portable, permission-safe signal-zero probe. - // Unknown liveness is treated as protected by reconciliation. - return false, false -} diff --git a/apps/backend/internal/system/storage/tempartifacts/registry.go b/apps/backend/internal/system/storage/tempartifacts/registry.go index 16446b617b..c188fcfb3d 100644 --- a/apps/backend/internal/system/storage/tempartifacts/registry.go +++ b/apps/backend/internal/system/storage/tempartifacts/registry.go @@ -15,6 +15,7 @@ import ( "github.com/google/uuid" + "github.com/kandev/kandev/internal/common/proclive" "github.com/kandev/kandev/internal/system/storage" ) @@ -206,7 +207,7 @@ func (r *Registry) Reconcile(ctx context.Context) error { } continue } - alive, known := processAlive(artifact.OwnerPID) + alive, known := proclive.Alive(artifact.OwnerPID) if !known || alive { continue } diff --git a/apps/backend/internal/system/storage/tempartifacts/registry_test.go b/apps/backend/internal/system/storage/tempartifacts/registry_test.go index 89e1fcffe8..be0695a145 100644 --- a/apps/backend/internal/system/storage/tempartifacts/registry_test.go +++ b/apps/backend/internal/system/storage/tempartifacts/registry_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/kandev/kandev/internal/common/proclive" "github.com/kandev/kandev/internal/system/storage" ) @@ -116,7 +117,7 @@ func TestRegistryReconcilesPreviousRunsAndDeadOwners(t *testing.T) { t.Fatalf("create current-run artifact: %v", err) } deadPID := int64(os.Getpid()) + 1_000_000 - alive, known := processAlive(deadPID) + alive, known := proclive.Alive(deadPID) canCheckDead := known && !alive if canCheckDead { store.artifacts[filepath.Join(root, "kandev-host-utility-dead")] = storage.TemporaryArtifact{ diff --git a/apps/desktop/src-tauri/src/backend.rs b/apps/desktop/src-tauri/src/backend.rs index e86e0eea48..2445cb82d8 100644 --- a/apps/desktop/src-tauri/src/backend.rs +++ b/apps/desktop/src-tauri/src/backend.rs @@ -24,6 +24,7 @@ const DEFAULT_DESKTOP_PORT: u16 = 38430; const DESKTOP_PORT_ENV: &str = "KANDEV_DESKTOP_PORT"; const DESKTOP_HEALTH_TOKEN_ENV: &str = "KANDEV_DESKTOP_HEALTH_TOKEN"; const DESKTOP_NATIVE_NOTIFICATIONS_ENV: &str = "KANDEV_DESKTOP_NATIVE_NOTIFICATIONS"; +const LAUNCHER_PARENT_PID_ENV: &str = "KANDEV_LAUNCHER_PARENT_PID"; const DESKTOP_HEALTH_TOKEN_HEADER: &str = "x-kandev-desktop-health-token"; const STARTUP_OUTPUT_LIMIT: usize = 12 * 1024; const HEALTH_READY_SETTLE: Duration = Duration::from_millis(100); @@ -269,6 +270,7 @@ fn launch_and_wait(app: &AppHandle, state: &BackendState) -> Result Result) { + env.insert( + OsString::from(LAUNCHER_PARENT_PID_ENV), + OsString::from(std::process::id().to_string()), + ); +} + #[cfg(feature = "desktop-runtime")] fn resolve_runtime_dir(app: &AppHandle) -> Result { if let Some(dir) = env::var_os("KANDEV_DESKTOP_RUNTIME_DIR") { @@ -742,6 +751,10 @@ mod tests { OsString::from(DESKTOP_HEALTH_TOKEN_ENV), OsString::from("health-token"), ); + inherited.insert( + OsString::from(LAUNCHER_PARENT_PID_ENV), + OsString::from("12345"), + ); inherited.insert(OsString::from("PATH"), OsString::from("/existing/bin")); let spec = build_backend_command( @@ -780,6 +793,10 @@ mod tests { spec.env.get(OsStr::new(DESKTOP_HEALTH_TOKEN_ENV)), Some(&OsString::from("health-token")) ); + assert_eq!( + spec.env.get(OsStr::new(LAUNCHER_PARENT_PID_ENV)), + Some(&OsString::from("12345")) + ); assert_eq!( spec.env .get(OsStr::new("KANDEV_DESKTOP_NATIVE_NOTIFICATIONS")), @@ -787,6 +804,18 @@ mod tests { ); } + #[test] + fn launcher_parent_environment_uses_shell_pid() { + let mut inherited = BTreeMap::new(); + + add_launcher_parent_pid(&mut inherited); + + assert_eq!( + inherited.get(OsStr::new(LAUNCHER_PARENT_PID_ENV)), + Some(&OsString::from(std::process::id().to_string())) + ); + } + #[test] fn desktop_environment_preserves_path_and_adds_gui_paths() { let runtime_dir = Path::new("/opt/kandev"); diff --git a/docs/plans/desktop-orphaned-backend-lock/plan.md b/docs/plans/desktop-orphaned-backend-lock/plan.md new file mode 100644 index 0000000000..fca684c6f3 --- /dev/null +++ b/docs/plans/desktop-orphaned-backend-lock/plan.md @@ -0,0 +1,342 @@ +--- +spec: docs/specs/desktop-tauri-app/spec.md +created: 2026-08-19 +status: completed +--- + +# Implementation Plan: Orphaned desktop backend holds the Kandev home lock + +## Overview + +Quitting the macOS desktop app can leave its launcher and backend running. The +orphaned backend keeps the `flock` on `~/.kandev/.kandev-backend.lock`, so every +later start, desktop or CLI, is rejected with a message that names only the home +path and gives the operator nothing to act on. This plan fixes both halves: the +launcher learns to exit when the shell that spawned it dies, and the ownership +lock learns to say who is holding it. + +Order is bottom-up. A shared process-liveness helper lands first because both +halves need it, then the two independent behaviors, then the desktop change that +activates the watchdog. Two specs are amended by this repair: +[desktop-tauri-app](../../specs/desktop-tauri-app/spec.md) owns the launcher +lifetime guarantee, and +[port-collision-safety](../../specs/port-collision-safety/spec.md) owns the +runtime-state ownership contract. + +## Root cause + +Confirmed on a live machine, not inferred. + +The desktop shell owns backend shutdown through one path: `shutdown_and_exit` in +`apps/desktop/src-tauri/src/main.rs` (reached from `WindowEvent::CloseRequested` +and `MenuAction::Quit`) calls `BackendState::stop`, which sends `SIGTERM` to the +launcher and escalates to `SIGKILL` after five seconds. That path is correct and +is not what failed. The defect is that it is the *only* path: nothing binds the +launcher's lifetime to the shell, so when the Tauri process ends without running +it, the launcher survives. + +Observed state after a quit: launcher PID 51228 +(`Kandev.app/Contents/Resources/kandev/bin/kandev --headless --port 38430`) alive +with **PPID 1**, having been reparented to `launchd` when its Tauri parent died; +backend PID 51229 alive as its child; `lsof` showing 51229 holding +`/Users/cfl12/.kandev/.kandev-backend.lock`; and no `Kandev.app/Contents/MacOS` +process at all. `SIGTERM` to 51228 shut the tree down cleanly and freed the lock, +confirming the launcher's own signal handling was never the problem. + +Two consequences compound into the reported dead end: + +1. `apps/backend/internal/launcher/` has no parent-death detection. Neither + `runManagedApp` (`start.go`) nor `processSupervisor` (`process.go`) observes + the parent, so an orphaned launcher runs until it is killed by hand. +2. `ownershiplock` never writes to the lock file it holds, so the conflict + message in `apps/backend/internal/backendapp/main.go` can only name the path. + The file is zero bytes, which also makes it look deletable; deleting it does + nothing, because the `flock` is the ownership, not the file. + +Smallest reliable reproduction: start the desktop app, `kill -9` the Tauri shell +process (simulating a crash or force quit), then run `make start-debug`. It fails +with `home target "..." is already owned by another backend`. + +Regression-test level: Go unit and real-subprocess tests in +`internal/launcher` and `internal/backendapp/ownershiplock`, plus a Rust unit +test in `apps/desktop/src-tauri`. No E2E; see Tests. + +--- + +## Contracts introduced + +Both halves are new cross-component contracts. Naming them here so the task files +do not each re-invent them. + +### `KANDEV_LAUNCHER_PARENT_PID` + +The spawning process writes its own PID into this environment variable. A +launcher that sees a valid positive PID binds its lifetime to that process and +shuts its supervised tree down once the process is gone. Unset, empty, `0`, or +unparseable means no watchdog, which keeps every current invocation, including +`make dev`, plain `kandev start` in a terminal, `nohup`, and service-manager +installs, behaving exactly as it does today. + +Opt-in is deliberate. Deriving the parent from `os.Getppid()` drift would also +kill deliberately detached CLI launches (`nohup kandev start &`), and under +`launchd`/`systemd` the initial parent is already PID 1, so there is no stable +signal to compare against. The spawner is the only party that knows it owns the +launcher's lifetime, so the spawner declares it. + +This variable is launcher-scoped and must **not** be added to the +`allowedSupervisorEnv` allowlist in `apps/backend/internal/launcher/supervisor.go`. +That allowlist is the environment replayed into a restarted *backend* child; the +watchdog belongs to the launcher process, which survives supervisor restarts. + +### Lock owner metadata + +After acquiring a lock, the owner writes one line of JSON into the lock file it +already holds: + +```json +{"pid":51229,"executable":"/Applications/Kandev.app/.../bin/kandev","started_at":"2026-08-19T09:14:35.123456789Z"} +``` + +Written with `Truncate(0)` then `WriteAt(1)`, leaving the first byte reserved for +the Windows lock range so a conflicting process can read the metadata through a +separate handle while the lock is held. A concurrent reader sees either empty +or a valid prefix, never a stale tail from a longer previous record. It is +advisory only: never read to decide ownership, staleness, or takeover, and a +write failure never fails acquisition. The lock sidecar is opened with +platform no-follow semantics and must be a regular file; symlink or reparse +point paths are rejected before any write. + +--- + +## Backend + +### Shared process liveness (`internal/common/proclive`, new) + +`apps/backend/internal/system/storage/tempartifacts/process_alive_unix.go` and +`process_alive_windows.go` already implement exactly the probe both halves of +this fix need, including the `(alive, known bool)` shape that distinguishes "the +process is gone" from "this platform cannot tell". Two more copies would violate +the repo's rule against duplicating cross-package helpers, so promote it to +`internal/common/proclive` and migrate `tempartifacts` onto it. + +The signature stays `Alive(pid int64) (alive, known bool)`. Keeping `int64` +preserves `tempartifacts`' existing `artifact.OwnerPID` call site byte for byte +and avoids a 32-bit truncation question at the boundary; the two new callers pass +`int64(pid)`. + +Behavior is unchanged from the current implementation: Unix uses signal 0, +treating `EPERM` as alive and `ESRCH` as gone; Windows returns `(false, false)`, +meaning unknown. Both new callers must therefore be correct when liveness is +unknown, and both are: the watchdog only acts on a positive `(false, true)`, and +the lock reader only suppresses owner detail on a positive `(false, true)`. + +### Launcher parent watchdog (`internal/launcher/parentwatch.go`, new) + +A `parentWatchdog` owning one goroutine, following the package's existing +lifecycle conventions: `start` registers on a `sync.WaitGroup`, `stop` closes a +channel and waits for drain, and the poll loop uses `time.NewTicker` inside a +`select` that also watches the stop channel, never `time.Sleep`. + +On a poll where the watched PID is positively gone, it mirrors what +`attachSignals` already does for a signal: emit an operator-visible +`launcherInfof` line and a `shutdownDebugf` line, call +`supervisor.shutdown(reason)`, then `launcherExit(0)`. Reusing that exact +sequence means the watchdog inherits the tested graceful-shutdown path +(`managedProcessShutdownGrace`, process-group cleanup) rather than introducing a +second way to stop the tree. + +Poll interval 2 seconds, as a package constant next to the existing shutdown +timing constants. Injectable seams for tests: the liveness probe function and the +ticker interval. + +Wiring: `runManagedApp` in `apps/backend/internal/launcher/start.go`, immediately +after `attachSignalsFn(supervisor)`, through a package-level `var` in the same +style as the existing `attachSignalsFn` indirection so tests can substitute it. +`runManagedApp` covers `start`, `run`, and `service`; the desktop spawns +`kandev --headless --port `, which is `start`. `dev.go` is intentionally not +wired: `make dev` is a foreground developer command whose parent is `make`, and +no spawner sets the variable for it. + +The env var name belongs in `apps/backend/internal/launcher/constants.go` +alongside the other launcher constants. + +### Ownership-lock owner diagnostics (`internal/backendapp/ownershiplock/`) + +`owner.go` (new) holds the record struct plus `writeOwner(*os.File)` and +`readOwner(path string)`. `lock.go` gains a `writeOwner` call after `lockFile` +succeeds, ignoring its error, and a `readOwner` call on the `ErrConflict` branch +to populate a new `Owner *OwnerRecord` field on `ConflictError`. + +`ConflictError.Error()` extends its existing sentence rather than replacing it, +so the message keeps naming the path and the existing +`use a separate KANDEV_HOME_DIR` guidance appended in `main.go` still reads +correctly: + +```text +home target "/Users/cfl12/.kandev" is already owned by another backend (pid 51229, /Applications/Kandev.app/.../bin/kandev, started 2026-08-19T09:14:35Z) +``` + +Owner detail is omitted, falling back to today's exact wording, when the file is +empty or unparseable, or when `proclive.Alive` positively reports the recorded +PID as gone. It is included when liveness is unknown, which is what makes the +detail useful on Windows. + +No change is needed in `apps/backend/internal/backendapp/main.go`: it already +prints `%v` of the error, so the enriched text reaches stderr, and from there +reaches the desktop startup-error dialog through the launcher output capture in +`apps/desktop/src-tauri/src/backend.rs`. + +--- + +## Desktop + +### `apps/desktop/src-tauri/src/backend.rs` + +`launch_and_wait` already inserts `KANDEV_DESKTOP_HEALTH_TOKEN` into +`inherited_env` before building the spec. Insert `KANDEV_LAUNCHER_PARENT_PID` +set to `std::process::id()` the same way, one line, next to the existing token +insert. `desktop_environment` passes unrecognized inherited variables through +untouched, which the existing `CUSTOM_ENV` assertion in +`command_spec_uses_headless_launcher_and_loopback_env` already pins. + +No Rust logic changes anywhere else. `BackendState::stop` remains the primary +shutdown path for a normal quit; the watchdog is the backstop for the abnormal +exits `stop` cannot cover. + +--- + +## Frontend + +None. This repair has no SPA surface: no component, store slice, API client, or +user-facing copy changes, so no i18n work either. The one operator-visible string +is backend stderr, which stays English per the backend i18n scope in +`apps/backend/AGENTS.md`. + +--- + +## Tests + +| What | File | How | +|---|---|---| +| `Alive` reports self alive, reports a reaped PID gone, treats non-positive PIDs as gone-and-known | `internal/common/proclive/proclive_unix_test.go` | Separate Unix tests; reap a real short-lived `os/exec` child for the dead case | +| `Alive` reports unknown on Windows | `internal/common/proclive/proclive_windows_test.go` | Windows-only test asserts `(false, false)` for the current process | +| `tempartifacts` keeps its current reconciliation behavior after migrating | `internal/system/storage/tempartifacts/registry_test.go` | Existing tests must pass unchanged | +| Watchdog shuts the tree down when the watched PID dies | `internal/launcher/parentwatch_test.go` | `testing/synctest` with an injected liveness probe flipping to `(false, true)`; assert `supervisor.shutdown` and `launcherExit(0)` via the package's existing `launcherExit` seam | +| Watchdog stays quiet while the parent lives, and while liveness is unknown | `internal/launcher/parentwatch_test.go` | Same harness with probes returning `(true, true)` and `(false, false)`; assert no shutdown after several ticks | +| Unset/empty/`0`/unparseable env means no watchdog goroutine | `internal/launcher/parentwatch_test.go` | Table-driven over env values; assert `start` returns a no-op and `stop` is safe | +| `stop` drains the goroutine and is idempotent | `internal/launcher/parentwatch_test.go` | Call `stop` twice; assert no panic and no goroutine left | +| Watchdog fires against a real killed process | `internal/launcher/parentwatch_unix_test.go` | Spawn a real child, kill and reap it, assert the probe path observes it gone | +| Conflict names the owning PID and executable | `internal/backendapp/ownershiplock/lock_test.go` | Acquire in-process, attempt a second acquire, assert `ConflictError.Owner` and the rendered message | +| Empty, unparseable, and known-dead metadata fall back to the path-only message | `internal/backendapp/ownershiplock/lock_test.go` | Table-driven over lock-file contents with an injected liveness probe | +| Owner-metadata write failure does not fail acquisition | `internal/backendapp/ownershiplock/lock_test.go` | Force the write to fail; assert `Acquire` still returns an owner | +| Metadata write truncates a longer previous record | `internal/backendapp/ownershiplock/owner_test.go` | Pre-fill the file with a longer record, re-write, assert no trailing bytes | +| Existing startup-conflict behavior is unchanged | `internal/backendapp/main_test.go` | Existing `TestBackendStartupConflictStopsBeforeSharedStateInitialization` must pass unchanged | +| Desktop spec carries the parent PID to the launcher | `apps/desktop/src-tauri/src/backend.rs` (`mod tests`) | Extend the existing command-spec test to assert the variable survives into `BackendCommandSpec.env` | + +Commands, each runnable from the repo root: + +```bash +cd apps/backend && go test ./internal/common/proclive/... ./internal/launcher/... ./internal/backendapp/... ./internal/system/storage/tempartifacts/... +cd apps/desktop/src-tauri && cargo test --features desktop-runtime +``` + +Lint gate for the changed Go packages: + +```bash +cd apps/backend && golangci-lint run ./internal/common/proclive/... ./internal/launcher/... ./internal/backendapp/... --new-from-rev=origin/main --timeout=5m +``` + +--- + +## E2E Tests + +None, deliberately. The spec changes have no SPA surface, so there is no +Playwright scenario to write. The desktop smoke harness +(`pnpm --filter @kandev/desktop e2e`) runs under Xvfb on Linux and would have to +abnormally kill the shell and then assert on host process state and a host lock +file, which is exactly the coupling that harness avoids. The real-subprocess Go +test in `internal/launcher` covers the mechanism, and the Rust unit test covers +the wiring that activates it. + +--- + +## Verification Results + +- `cd apps/backend && go test -race ./internal/common/proclive/... ./internal/launcher/... ./internal/backendapp/... ./internal/system/storage/tempartifacts/...` + passed for all six affected packages. +- `cd apps/backend && gofmt -l internal/common/proclive internal/launcher internal/backendapp/ownershiplock internal/system/storage/tempartifacts` + produced no output. +- `cd apps/backend && golangci-lint run ./internal/common/proclive/... ./internal/launcher/... ./internal/backendapp/... ./internal/system/storage/tempartifacts/... --new-from-rev=origin/main --timeout=5m` + passed with 0 issues. +- `GOOS=windows GOARCH=amd64 go test -c` passed for `proclive`, `launcher`, and + `backendapp/ownershiplock`; temporary Windows test binaries were removed. +- `cd apps/desktop/src-tauri && cargo fmt --check && cargo test --features desktop-runtime` + passed: 57 tests, 0 failures. +- macOS parent-death process-boundary verification passed with a newly built + launcher and the installed runtime bundle: after SIGKILLing and reaping the + declared parent shell, the launcher logged `parent process exited`, completed + graceful shutdown, exited, and the temporary runtime lock had no holder. +- `git diff --check` passed. Generated Tauri schema output from the test build + was removed; no generated artifact remains in the diff. + +--- + +## Implementation Waves And Parallel Candidates + +```text +Wave 1: +- [x] [task-01-proclive-shared-helper](task-01-proclive-shared-helper.md) + +Wave 2 (parallel candidates, user authorization required): +- [x] [task-02-launcher-parent-watchdog](task-02-launcher-parent-watchdog.md) +- [x] [task-04-lock-owner-diagnostics](task-04-lock-owner-diagnostics.md) + +Wave 3: +- [x] [task-03-desktop-parent-pid](task-03-desktop-parent-pid.md) +``` + +Task 01 is a shared dependency of both wave-2 tasks and must land first. Tasks 02 +and 04 touch disjoint packages (`internal/launcher` and +`internal/backendapp/ownershiplock`), share no schema, migration, generated +contract, lockfile, or package config, and are genuinely parallel-safe. Task 03 +is last because it activates the watchdog task 02 introduces, and the product +should not advertise the binding before the launcher honors it. + +Waves are a review aid. The default remains sequential execution in the primary +conversation; they do not authorize subagents. + +## Risks + +- **Contradicts a rejected ADR alternative.** ADR + [2026-08-09-exclusive-runtime-state-ownership](../../decisions/2026-08-09-exclusive-runtime-state-ownership.md) + rejected "Use a PID file" because stale PIDs and PID reuse do not provide + atomic ownership, and `port-collision-safety` listed PID identity matching as + out of scope. Both objections are about using a PID to *decide* ownership. This + plan keeps `flock` as the sole ownership proof and uses the PID only as text in + an error message, and the spec amendment narrows the out-of-scope line to say + so explicitly. Reviewers should confirm that framing is acceptable before the + code lands; if it is not, task 04 drops and task 02 still fixes the reported + dead end. +- **PID reuse weakens the watchdog silently.** If the shell dies and its PID is + reused before the next poll, the watchdog sees a live PID and never fires. The + failure mode is exactly today's behavior, so it degrades rather than breaks. A + handle-based or pipe-EOF mechanism would close it and is the natural follow-up + if this proves insufficient in practice. +- **Windows gets the watchdog as a no-op**, because `proclive.Alive` cannot + determine liveness there. Windows also has no reparent-to-PID-1 semantics, and + the reported bug is macOS, so this is scoped rather than solved. The lock + diagnostics do work on Windows, since unknown liveness still reports the + recorded values. +- **A 2 second poll leaves a window** where a restarted desktop still sees the + old lock. The new error message names the owning PID, which makes that window + self-explanatory instead of mysterious. + +## Out of scope + +- Changing `flock` for a different ownership primitive, or any takeover, + lock-breaking, or stale-lock-reclaim behavior. +- Wiring the watchdog into `dev.go`, or changing `make dev` process supervision. +- Any UI affordance for detecting or killing a conflicting backend from the + desktop startup-error screen. +- Changing `BackendState::stop`, the health-token contract, or the desktop + updater restart path. diff --git a/docs/plans/desktop-orphaned-backend-lock/task-01-proclive-shared-helper.md b/docs/plans/desktop-orphaned-backend-lock/task-01-proclive-shared-helper.md new file mode 100644 index 0000000000..f529bb3c88 --- /dev/null +++ b/docs/plans/desktop-orphaned-backend-lock/task-01-proclive-shared-helper.md @@ -0,0 +1,124 @@ +--- +id: "01-proclive-shared-helper" +title: "Shared process-liveness helper" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/desktop-tauri-app/spec.md" +parallelism: sequential +--- + +# Task 01: Shared process-liveness helper + +Promote the existing private `processAlive` probe out of +`internal/system/storage/tempartifacts` into `internal/common/proclive` so the +launcher watchdog (task 02) and the ownership-lock diagnostics (task 04) consume +one implementation instead of adding a second and third copy. + +This is a pure move plus re-export. Do not change the probe's behavior. + +## Inputs + +- Plan section "Shared process liveness (`internal/common/proclive`, new)". +- Existing implementation to move: + `apps/backend/internal/system/storage/tempartifacts/process_alive_unix.go` + and `process_alive_windows.go`. +- Its only current call site: `tempartifacts/registry.go:209`. +- `apps/backend/AGENTS.md`, "Cross-tier shared code belongs in `internal/common/`", + which is why this lands as a shared package rather than being duplicated. + +## Implementation + +Create `apps/backend/internal/common/proclive/` with: + +- `proclive.go` holding only the package doc comment. State that `known=false` + means the platform cannot determine liveness and that callers must treat that + as "do not act", since both consumers depend on that reading. +- `proclive_unix.go` (`//go:build !windows`) and `proclive_windows.go` + (`//go:build windows`), each exporting: + +```go +func Alive(pid int64) (alive, known bool) +``` + +Move the two existing bodies verbatim, changing only the package clause and the +identifier from `processAlive` to `Alive`. Keep the `int64` parameter: it matches +`tempartifacts`' `artifact.OwnerPID` exactly and avoids a 32-bit truncation +question at the boundary. Keep the Windows comment explaining why it returns +unknown. + +Then delete both `tempartifacts/process_alive_*.go` files and update +`registry.go:209` to call `proclive.Alive(artifact.OwnerPID)`. Update +`registry_test.go:119` the same way. Do not change any surrounding +reconciliation logic. + +## Acceptance + +- `internal/common/proclive` exports `Alive(pid int64) (alive, known bool)` with + Unix and Windows implementations, and both `tempartifacts/process_alive_*.go` + files are gone. +- `tempartifacts` calls `proclive.Alive` and its existing tests pass unchanged. +- No behavior change: signal 0 on Unix with `EPERM` treated as alive and `ESRCH` + as gone, `(false, false)` on Windows. + +## Verification + +```bash +cd apps/backend && go test ./internal/common/proclive/... ./internal/system/storage/tempartifacts/... +``` + +```bash +cd apps/backend && gofmt -l internal/common/proclive internal/system/storage/tempartifacts +``` + +The `gofmt` invocation must print nothing; the pre-commit hook fails the commit +otherwise. + +## Files likely touched + +- `apps/backend/internal/common/proclive/proclive.go` (new) +- `apps/backend/internal/common/proclive/proclive_unix.go` (new) +- `apps/backend/internal/common/proclive/proclive_windows.go` (new) +- `apps/backend/internal/common/proclive/proclive_test.go` (new) +- `apps/backend/internal/system/storage/tempartifacts/process_alive_unix.go` (delete) +- `apps/backend/internal/system/storage/tempartifacts/process_alive_windows.go` (delete) +- `apps/backend/internal/system/storage/tempartifacts/registry.go` +- `apps/backend/internal/system/storage/tempartifacts/registry_test.go` + +## Dependencies + +None. + +## Tests to write + +In `proclive_test.go`, table-driven: + +- The current process (`int64(os.Getpid())`) reports `(true, true)` on Unix. +- A reaped child reports `(false, true)` on Unix. Start a real short-lived + process with `os/exec`, `Wait` for it so the PID is fully released, then probe. + Guard this case with `//go:build !windows` or a `runtime.GOOS` skip, since + Windows returns unknown by design. +- `0` and a negative PID report `(false, true)` on Unix without touching the + platform call. + +## Output contract + +Report the moved files, the migrated call sites, the exact commands run with +their outcomes, and confirmation that no `tempartifacts` behavior changed. Update +this file's `status` and `## Results`, and the matching checkbox in `plan.md`. + +## Results + +- Added `internal/common/proclive` with Unix signal-zero and Windows unknown + implementations, plus Unix tests for the current process, a reaped child, and + non-positive PIDs. +- Migrated `tempartifacts` production and test call sites to `proclive.Alive`; + removed the duplicated platform files. +- `cd apps/backend && go test ./internal/common/proclive/... ./internal/system/storage/tempartifacts/...` + passed (`ok` for both packages). +- `gofmt -w internal/common/proclive internal/system/storage/tempartifacts` + completed successfully. +- Cleanup: the subprocess used by the liveness test is reaped with `Wait`; + no temporary capture files or external resources remain. +- Security/trust and external side-effect boundaries: none. diff --git a/docs/plans/desktop-orphaned-backend-lock/task-02-launcher-parent-watchdog.md b/docs/plans/desktop-orphaned-backend-lock/task-02-launcher-parent-watchdog.md new file mode 100644 index 0000000000..2c8bd996d3 --- /dev/null +++ b/docs/plans/desktop-orphaned-backend-lock/task-02-launcher-parent-watchdog.md @@ -0,0 +1,170 @@ +--- +id: "02-launcher-parent-watchdog" +title: "Launcher parent-death watchdog" +status: done +wave: 2 +depends_on: ["01-proclive-shared-helper"] +plan: "plan.md" +spec: "../../specs/desktop-tauri-app/spec.md" +parallelism: parallel-safe +--- + +# Task 02: Launcher parent-death watchdog + +Bind the launcher's lifetime to the process that spawned it, so a shell that dies +without running its own shutdown path cannot leave the launcher and backend +holding the Kandev home lock forever. + +Parallel-safe with task 04: this task touches only `internal/launcher`, task 04 +touches only `internal/backendapp/ownershiplock`, and they share no schema, +migration, generated contract, lockfile, or package config. Both depend on task +01. + +## Inputs + +- Plan sections "`KANDEV_LAUNCHER_PARENT_PID`" and "Launcher parent watchdog". +- Spec: `docs/specs/desktop-tauri-app/spec.md`, the abnormal-termination bullet + under "Failure modes" and its matching scenario under "Scenarios". +- Pattern to mirror: `processSupervisor.attachSignals` in + `apps/backend/internal/launcher/process.go:141`. The watchdog performs the same + terminal sequence as a received signal, so reuse that shape rather than + inventing a second way to stop the tree. +- Test seams already in the package: `launcherExit` (`process.go:27`) and + `launcherStatusOutput` (`process.go:26`); the `attachSignalsFn` indirection in + `start.go:18`. +- `apps/backend/AGENTS.md`, "Goroutine ownership and leak testing", for the + required `start`/`stop` shape. + +## Implementation + +Add the env var name to `apps/backend/internal/launcher/constants.go`: + +```go +// parentPIDEnv lets a spawning process bind this launcher's lifetime to its +// own. Desktop shells set it so an abnormal shell exit cannot strand a backend +// holding the Kandev home's runtime-state lock. +parentPIDEnv = "KANDEV_LAUNCHER_PARENT_PID" +``` + +Add `parentWatchPollInterval = 2 * time.Second` next to the existing shutdown +timing constants in `process.go`, or in the new file; keep it a package constant +either way. + +Create `apps/backend/internal/launcher/parentwatch.go` with a `parentWatchdog` +type owning exactly one goroutine: + +- Construct from the env var. A missing, empty, non-numeric, zero, or negative + value yields a watchdog whose `start` and `stop` are no-ops, so every current + invocation keeps today's behavior. +- `start()` launches the poll goroutine and registers it on a `sync.WaitGroup`. +- `stop()` closes a stop channel and waits for drain. Idempotent: safe to call + twice. +- The loop uses `time.NewTicker(interval)` inside a `select` that also watches + the stop channel. Do not use `time.Sleep`. +- On a tick, call the liveness probe. Act **only** on `(alive=false, known=true)`. + `(false, false)` means the platform cannot tell and must be ignored, which is + what makes this a no-op on Windows rather than a process killer. +- When the parent is positively gone, emit both an operator-visible + `launcherInfof` line naming the watched PID and a `shutdownDebugf` line, then + call `supervisor.shutdown(reason)` followed by `launcherExit(0)`. + +Expose two injectable seams as struct fields or package `var`s so tests do not +depend on real time or real processes: the liveness probe (defaulting to +`proclive.Alive`) and the poll interval. + +Wire it in `runManagedApp` (`apps/backend/internal/launcher/start.go:86`), +immediately after `attachSignalsFn(supervisor)`, through a new package-level +`var startParentWatchFn = ...` in the same style as the existing +`attachSignalsFn`, so tests can substitute it. Stop the watchdog before +`runManagedApp` returns. + +Do **not** wire `dev.go`, and do **not** add `KANDEV_LAUNCHER_PARENT_PID` to +`allowedSupervisorEnv` in `supervisor.go`. That allowlist is replayed into a +restarted backend child; the watchdog belongs to the launcher process, which +outlives supervisor restarts. Adding it there would be a real bug. + +## Acceptance + +- With `KANDEV_LAUNCHER_PARENT_PID` set to a live PID, the launcher runs exactly + as it does today; when that PID is positively gone, it runs the standard + graceful shutdown and exits 0. +- With the variable unset, empty, `0`, negative, or unparseable, no goroutine + starts and behavior is byte-for-byte unchanged. +- An unknown-liveness probe result never triggers shutdown. + +## Verification + +```bash +cd apps/backend && go test ./internal/launcher/... +``` + +```bash +cd apps/backend && gofmt -l internal/launcher && golangci-lint run ./internal/launcher/... --new-from-rev=origin/main --timeout=5m +``` + +## Files likely touched + +- `apps/backend/internal/launcher/parentwatch.go` (new) +- `apps/backend/internal/launcher/parentwatch_test.go` (new) +- `apps/backend/internal/launcher/parentwatch_unix_test.go` (new) +- `apps/backend/internal/launcher/constants.go` +- `apps/backend/internal/launcher/start.go` + +Note `apps/backend/AGENTS.md`'s 800-effective-line file limit, which applies to +test files: keep the new tests in the new files above rather than appending to +`process_test.go` or `start_test.go`. + +## Dependencies + +Task 01 (`proclive.Alive`). + +## Tests to write + +In `parentwatch_test.go`, using `testing/synctest` so ticker waits resolve +instantly, with an injected probe and the `launcherExit` seam captured: + +- Parent dies: probe flips to `(false, true)`; assert the supervisor shut down + and `launcherExit` was called with `0`. +- Parent lives: probe stays `(true, true)` across several ticks; assert no + shutdown and no exit. +- Liveness unknown: probe returns `(false, false)` across several ticks; assert + no shutdown and no exit. This is the Windows path and the most important + negative case. +- Env parsing, table-driven over unset, `""`, `"0"`, `"-1"`, `"abc"`, and a + valid PID; assert a watchdog is created only for the last, and that `start` + and `stop` on the others are safe no-ops. +- `stop` is idempotent and drains: call it twice, assert no panic and that the + goroutine returned. + +In `parentwatch_unix_test.go` (`//go:build !windows`), one real-process test: +spawn a child, kill and `Wait` it so the PID is released, then assert the +watchdog's real (non-injected) probe path observes it gone. This is what proves +the mechanism against a real OS rather than a fake. + +## Output contract + +Report the new files, the wiring point in `start.go`, exact commands with +outcomes, and explicit confirmation that `allowedSupervisorEnv` and `dev.go` were +left untouched. Update this file's `status` and `## Results`, and the matching +checkbox in `plan.md`. + +## Results + +- Added `parentWatchdog` with explicit start/stop ownership, a ticker-driven + liveness loop, positive-death-only shutdown, and injectable liveness/timing + seams. +- Added `KANDEV_LAUNCHER_PARENT_PID` parsing and wired the watchdog into + `runManagedApp`; `dev.go` and `allowedSupervisorEnv` remain untouched. +- Added fake-time tests for parent death, live parents, unknown liveness, invalid + environment values, idempotent stop, and a real Unix subprocess test that + kills and reaps the watched process. +- `cd apps/backend && go test -run 'TestParentWatchdog|TestParentPIDFromEnv|TestRunManagedAppAttachesSignalsBeforeBackendLaunch' ./internal/launcher/...` + passed. +- `cd apps/backend && go test ./internal/launcher/...` passed for both launcher + packages. +- `gofmt -w` completed for all changed launcher Go files. +- Cleanup: the real subprocess test kills and reaps its child, and every + watchdog registers a cleanup stop; no background watchdog remains. +- Security/trust and external side-effect boundaries: the PID is an explicit + lifetime declaration from the desktop spawner; unknown liveness never causes + shutdown. diff --git a/docs/plans/desktop-orphaned-backend-lock/task-03-desktop-parent-pid.md b/docs/plans/desktop-orphaned-backend-lock/task-03-desktop-parent-pid.md new file mode 100644 index 0000000000..ac1745b59a --- /dev/null +++ b/docs/plans/desktop-orphaned-backend-lock/task-03-desktop-parent-pid.md @@ -0,0 +1,138 @@ +--- +id: "03-desktop-parent-pid" +title: "Desktop publishes its parent PID to the launcher" +status: done +wave: 3 +depends_on: ["02-launcher-parent-watchdog"] +plan: "plan.md" +spec: "../../specs/desktop-tauri-app/spec.md" +parallelism: sequential +--- + +# Task 03: Desktop publishes its parent PID to the launcher + +Activate the watchdog from task 02 by having the Tauri shell declare its own +process ID to the launcher it spawns. This is the change that actually fixes the +reported bug; tasks 01 and 02 build the mechanism, this one turns it on. + +Sequential and last: the launcher must already honor +`KANDEV_LAUNCHER_PARENT_PID` before the desktop advertises it. + +## Inputs + +- Plan sections "`KANDEV_LAUNCHER_PARENT_PID`" and "Desktop". +- Spec: `docs/specs/desktop-tauri-app/spec.md`, the abnormal-termination bullet + under "Failure modes" and its matching scenario under "Scenarios". +- The exact pattern to copy: `launch_and_wait` in + `apps/desktop/src-tauri/src/backend.rs:263`, which already inserts + `DESKTOP_HEALTH_TOKEN_ENV` into `inherited_env` before building the spec. +- `apps/desktop/AGENTS.md` for the desktop test command and the rule that the + shell stays a thin wrapper. + +## Implementation + +In `apps/desktop/src-tauri/src/backend.rs`, add the constant beside the existing +desktop env constants near line 24: + +```rust +const LAUNCHER_PARENT_PID_ENV: &str = "KANDEV_LAUNCHER_PARENT_PID"; +``` + +In `launch_and_wait`, next to the existing health-token insert, add: + +```rust +inherited_env.insert( + OsString::from(LAUNCHER_PARENT_PID_ENV), + OsString::from(std::process::id().to_string()), +); +``` + +That is the whole production change. `desktop_environment` passes unrecognized +inherited variables through untouched, which the existing `CUSTOM_ENV` +assertion in `command_spec_uses_headless_launcher_and_loopback_env` already +pins, so no plumbing change is needed in `backend_command_spec`. + +Do not change `BackendState::stop`, `terminate_child`, `shutdown_and_exit`, or +any `RunEvent` handling. Those remain the primary shutdown path for a normal +quit. The watchdog is strictly a backstop for the abnormal exits that path +cannot cover, and weakening it would trade one bug for another. + +## Acceptance + +- A launcher spawned by the desktop shell receives + `KANDEV_LAUNCHER_PARENT_PID` set to the shell's own PID. +- Normal quit behavior is unchanged: the shell still terminates its child + directly and does not wait for the watchdog. +- No other desktop behavior changes. + +## Verification + +```bash +cd apps/desktop/src-tauri && cargo test --features desktop-runtime +``` + +Manual confirmation of the actual reported bug, on macOS, after tasks 01 to 03 +have landed and the desktop runtime has been rebuilt: + +1. Start the desktop app and confirm it reaches the SPA. +2. Find the Tauri shell process with + `pgrep -f "Kandev.app/Contents/MacOS"` and `kill -9` it, simulating the crash + or force quit that `BackendState::stop` cannot cover. +3. Within roughly the poll interval, confirm + `pgrep -f "Kandev.app.*bin/kandev"` returns nothing and + `lsof ~/.kandev/.kandev-backend.lock` reports no holder. +4. Confirm `make start-debug` then starts normally. + +Record the observed result of step 3, including how long it took, in +`## Results`. + +## Files likely touched + +- `apps/desktop/src-tauri/src/backend.rs` + +## Dependencies + +Task 02. The launcher must honor the variable before the desktop sets it. + +## Tests to write + +Extend the existing `mod tests` in `backend.rs`: assert that a +`KANDEV_LAUNCHER_PARENT_PID` entry present in `inherited_env` survives into +`BackendCommandSpec.env` with its value intact. Follow +`command_spec_uses_headless_launcher_and_loopback_env`, which already proves the +pass-through shape with `CUSTOM_ENV`. + +Assert on the pass-through rather than on `std::process::id()`, so the test does +not depend on the PID of the test binary. + +## Output contract + +Report the changed file, the exact `cargo test` outcome, and the observed result +of the manual macOS orphan check including timing. If the manual check cannot be +run on the executing machine, say so explicitly rather than implying it passed. +Update this file's `status` and `## Results`, and the matching checkbox in +`plan.md`. + +## Results + +- Added `LAUNCHER_PARENT_PID_ENV` and pass the Tauri shell PID into the + launcher environment during `launch_and_wait`. +- Added a focused Rust test for the PID environment helper and extended the + command-spec pass-through coverage. +- The first RED run, + `cargo test --features desktop-runtime launcher_parent_environment_uses_shell_pid`, + failed at compile time because `add_launcher_parent_pid` did not yet exist. +- `cd apps/desktop/src-tauri && cargo fmt --check && cargo test --features desktop-runtime` + passed: 57 tests, 0 failures; updater verifier and doc-test targets also + passed with zero tests. +- macOS process-boundary verification passed using a newly built launcher and + the installed runtime bundle: a shell parent declared its PID, the launcher + reached `/health`, the shell was SIGKILLed and reaped, the launcher exited + within the watchdog window, and `lsof` found no holder for the temporary + runtime lock. The launcher log recorded `parent process exited` and a + graceful shutdown. +- Cleanup: the temporary runtime home, database, logs, launcher, backend, and + subprocess parent were removed or reaped by the verification command. +- Security/trust and external side-effect boundaries: the PID is only passed + from the spawning desktop shell to its child launcher; no frontend or network + contract changed. diff --git a/docs/plans/desktop-orphaned-backend-lock/task-04-lock-owner-diagnostics.md b/docs/plans/desktop-orphaned-backend-lock/task-04-lock-owner-diagnostics.md new file mode 100644 index 0000000000..771f1b40c7 --- /dev/null +++ b/docs/plans/desktop-orphaned-backend-lock/task-04-lock-owner-diagnostics.md @@ -0,0 +1,195 @@ +--- +id: "04-lock-owner-diagnostics" +title: "Ownership-lock owner diagnostics" +status: done +wave: 2 +depends_on: ["01-proclive-shared-helper"] +plan: "plan.md" +spec: "../../specs/port-collision-safety/spec.md" +parallelism: parallel-safe +--- + +# Task 04: Ownership-lock owner diagnostics + +Make a runtime-state ownership conflict name the process holding the lock, so an +operator with no leftover terminal can identify what to stop instead of being +told only that the home is busy. + +Parallel-safe with task 02: this task touches only +`internal/backendapp/ownershiplock`, task 02 touches only `internal/launcher`. +Both depend on task 01. + +**Read the boundary before writing code.** ADR +[2026-08-09-exclusive-runtime-state-ownership](../../decisions/2026-08-09-exclusive-runtime-state-ownership.md) +rejected "Use a PID file" as an ownership mechanism, and +`docs/specs/port-collision-safety/spec.md` lists PID identity matching as out of +scope. This task stays inside that boundary: the `flock` remains the sole +ownership proof, and the recorded PID is text in an error message. If you find +yourself reading owner metadata to decide whether a lock is held, stale, or +takeable, stop. That is the rejected design. + +## Inputs + +- Plan sections "Lock owner metadata" and "Ownership-lock owner diagnostics". +- Spec: `docs/specs/port-collision-safety/spec.md`, the "Exclusive runtime-state + ownership" paragraphs on advisory owner metadata, its narrowed out-of-scope + bullet, and scenarios 6 to 8 under "Concurrent backend startup". +- Current code: `apps/backend/internal/backendapp/ownershiplock/lock.go`, + especially `Acquire` (line 40), `openLockFile` (line 67), and `ConflictError` + (line 15). +- Consumer of the message: + `apps/backend/internal/backendapp/main.go:220`. It prints `%v`, so no change + is required there; confirm that rather than editing it. + +## Implementation + +Create `apps/backend/internal/backendapp/ownershiplock/owner.go` with: + +```go +// OwnerRecord is advisory diagnostic metadata about the process that acquired a +// lock. It is never consulted to decide ownership, staleness, or takeover: the +// operating-system lock is the only proof. See ADR 2026-08-09. +type OwnerRecord struct { + PID int64 `json:"pid"` + Executable string `json:"executable"` + StartedAt string `json:"started_at"` // RFC3339Nano +} +``` + +plus `writeOwner(file *os.File) error` and `readOwner(path string) *OwnerRecord`. + +`writeOwner` builds the record from `os.Getpid()`, `os.Executable()` (empty +string on error, not a failure), and `time.Now().UTC().Format(time.RFC3339Nano)`, +then marshals to a single line. Write it with `file.Truncate(0)` followed by +`file.WriteAt(data, ownerMetadataOffset)` on the handle already held. The first +byte is reserved for the Windows lock range, so a conflicting process can read +the metadata through a separate handle while the lock is held. Truncating first +is what guarantees a concurrent reader sees either nothing or a valid prefix, +never a stale tail from a longer previous record. + +`readOwner` opens the path separately for reading, reads after the reserved +byte, unmarshals, and returns `nil` on any error, on an empty file, or on a +non-positive PID. It must never block and never take a lock. + +In `lock.go`: + +- Open each sidecar with platform no-follow semantics and reject a symlink, + reparse point, or non-regular file before returning the handle. The opened + handle, rather than a second path lookup, is the regular-file proof used for + the later metadata write. +- After `lockFile(file)` succeeds in `Acquire`, call `writeOwner(file)` and + discard the error. A metadata write failure must not fail acquisition; the + process owns the lock either way. +- On the `errors.Is(err, ErrConflict)` branch, call `readOwner(target.LockPath)` + and put the result on a new `Owner *OwnerRecord` field of `ConflictError`. +- Suppress the detail when it cannot be trusted: if `proclive.Alive(rec.PID)` + returns `(false, true)`, meaning positively gone, set `Owner` to `nil`. Keep it + when liveness is unknown, `(false, false)`, which is what makes this useful on + Windows. + +Extend `ConflictError.Error()` rather than replacing it, so the existing sentence +and the `use a separate KANDEV_HOME_DIR` guidance appended by `main.go` still +read correctly: + +```text +home target "/Users/cfl12/.kandev" is already owned by another backend (pid 51229, /path/to/kandev, started 2026-08-19T09:14:35Z) +``` + +With no trustworthy owner, the message must be byte-identical to today's. + +Omit an empty executable from the parenthetical instead of rendering an empty +string. + +## Acceptance + +- A second `Acquire` against a held lock returns a `*ConflictError` whose `Owner` + carries the first process's PID and executable, and whose `Error()` includes + them. +- Empty, unparseable, or known-dead metadata produces exactly today's message. +- A failed metadata write still yields a successful `Acquire`. +- A symlinked lock path is rejected without modifying its target. +- Owner metadata remains readable while the lock is held on Windows. +- No code path reads `OwnerRecord` to decide whether a lock may be acquired. + +## Verification + +```bash +cd apps/backend && go test ./internal/backendapp/... +``` + +```bash +cd apps/backend && gofmt -l internal/backendapp && golangci-lint run ./internal/backendapp/... --new-from-rev=origin/main --timeout=5m +``` + +## Files likely touched + +- `apps/backend/internal/backendapp/ownershiplock/owner.go` (new) +- `apps/backend/internal/backendapp/ownershiplock/owner_test.go` (new) +- `apps/backend/internal/backendapp/ownershiplock/lock.go` +- `apps/backend/internal/backendapp/ownershiplock/lock_test.go` + +## Dependencies + +Task 01 (`proclive.Alive`). + +## Tests to write + +In `lock_test.go`, extending the existing conflict coverage: + +- Acquire a lock, attempt a second acquire on the same target, assert the + returned `*ConflictError` has a non-nil `Owner` with this process's PID and + that `Error()` contains both the path and the PID. +- Table-driven fallback: pre-write lock-file contents of `""`, `"not json"`, + `"{}"`, and a valid record whose PID an injected probe reports as + `(false, true)`; assert `Owner` is nil and the message equals the pre-change + wording in every case. +- Unknown liveness, `(false, false)`, keeps the owner detail. +- Metadata write failure still returns a usable owner from `Acquire`. +- Create a symlinked sidecar and assert opening it fails without changing the + target bytes. + +In `owner_test.go`: + +- Round-trip `writeOwner` then `readOwner`. +- Truncation: pre-fill the file with a longer valid record, write a shorter one, + read back and assert no trailing bytes survive. +- Hold the OS lock and read owner metadata through a second handle. +- `readOwner` on a missing file, a directory, and a non-positive PID returns + `nil` without error. + +Inject the liveness probe through a package-level `var` so tests do not depend on +real PID lifetimes. `internal/backendapp/main_test.go`'s existing +`TestBackendStartupConflictStopsBeforeSharedStateInitialization` must pass +unchanged; it is the regression guard that this task did not disturb the +fail-closed startup ordering. + +## Output contract + +Report the new files, the changed `ConflictError` shape, exact commands with +outcomes, a sample of the enriched message, and explicit confirmation that no +acquisition decision reads owner metadata. Update this file's `status` and +`## Results`, and the matching checkbox in `plan.md`. + +## Results + +- Added advisory `OwnerRecord` JSON metadata with PID, executable, and RFC3339Nano + start time; acquisition ignores metadata write failures. +- Enriched `ConflictError` with owner details only when metadata is parseable and + liveness is not positively known to be dead. Path-only wording remains the + fallback. +- Added regression coverage for owner rendering, truncation, invalid/dead + metadata, unknown liveness, metadata-write failure, and existing startup + conflict behavior. +- `cd apps/backend && go test -run 'TestOwnershipLockConflict|TestOwnershipLockMetadataWriteFailure' ./internal/backendapp/ownershiplock/...` + passed. +- `cd apps/backend && go test ./internal/backendapp/...` passed for backendapp + and ownershiplock. +- `gofmt -w` completed for all changed ownership-lock Go files. +- Cleanup: all in-process owners are closed with test cleanup or explicit close; + no lock handles remain. +- Security/trust and external side-effect boundaries: owner metadata never + participates in acquisition, staleness, or takeover decisions; the OS lock + remains authoritative. +- Fixup hardening: platform no-follow sidecar opens reject symlinks/reparse + points before metadata writes, and the reserved lock byte keeps Windows + conflict diagnostics readable while the lock is held. diff --git a/docs/specs/desktop-tauri-app/spec.md b/docs/specs/desktop-tauri-app/spec.md index ee789a5757..54ce4e9ca1 100644 --- a/docs/specs/desktop-tauri-app/spec.md +++ b/docs/specs/desktop-tauri-app/spec.md @@ -1,7 +1,7 @@ --- status: shipped created: 2026-06-23 -updated: 2026-08-11 +updated: 2026-08-19 owner: tbd --- @@ -219,6 +219,17 @@ display before being shown. rejected without stopping the running application. - A missing packaged launcher/helper, backend bind failure, failed authenticated health check, or startup timeout produces a visible startup error and terminates the owned process tree. +- On platforms with a reliable process-liveness probe, if the shell process ends without running + its own shutdown path, because it crashed, was force quit, or was killed by the operating system, + the owned launcher and backend must still exit on their own. Shell-initiated cleanup covers quit, + startup failure, and update restart, but it cannot cover its own abnormal termination, so the + launcher is additionally bound to the lifetime of the shell that spawned it. The shell publishes + its process identity to the launcher at spawn time, and the launcher stops its supervised tree + once that process is positively known to be gone. This is a hard requirement rather than + best-effort cleanup on those platforms: a launcher that outlives its shell keeps the Kandev + home's exclusive runtime-state lock and blocks every later desktop and CLI start until the + operator finds and kills it by hand. Platforms without a reliable probe retain their existing + normal-shutdown guarantee and do not infer parent death from an unknown liveness result. - A GUI environment that cannot find an agent CLI surfaces the existing setup-health guidance. - If backend shutdown fails during update restart, installation does not force an unclean restart; the user sees an actionable error and can quit normally. @@ -261,6 +272,11 @@ display before being shown. Settings page opens in the main window. - **GIVEN** the user clicks the macOS red close control, **WHEN** shutdown completes, **THEN** both the app and its owned backend exit. +- **GIVEN** a desktop platform with a reliable process-liveness probe and a shell that ends without + running its shutdown path, **WHEN** the launcher observes that the shell process it was spawned + by is positively gone, **THEN** the launcher shuts down its backend, releases the Kandev home + runtime-state lock, and exits, so the next desktop or CLI start acquires ownership without the + operator killing a leftover process. - **GIVEN** a signed newer desktop version exists, **WHEN** an automatic check finds it, **THEN** Kandev prompts in the existing Updates page before any download, install, or restart. - **GIVEN** a Linux AppImage installation, **WHEN** the user confirms a valid update, **THEN** the diff --git a/docs/specs/port-collision-safety/spec.md b/docs/specs/port-collision-safety/spec.md index 63e0e6e57f..26d5727f6a 100644 --- a/docs/specs/port-collision-safety/spec.md +++ b/docs/specs/port-collision-safety/spec.md @@ -1,7 +1,7 @@ --- status: building created: 2026-08-07 -updated: 2026-08-11 +updated: 2026-08-19 --- # Port collision and backend ownership safety @@ -130,6 +130,26 @@ database path and tells the operator to use a separate `KANDEV_HOME_DIR` for an instance. The rejected process must not initialize file logging, create a database backup, apply a migration, reconcile a session, launch agentctl, or start an HTTP server. +That message also names the current owner when the owner is knowable. After a backend acquires a +lock it records advisory owner metadata, its process ID, executable path, and start time, into the +lock file it already holds, replacing any previous content. A process rejected for a conflict reads +that metadata and includes it in its stderr message, so an operator who has no leftover terminal can +identify the process to stop instead of being told only that the home is busy. + +This metadata is diagnostics, not ownership. The operating-system lock remains the sole proof: the +metadata is never consulted to decide whether a lock is held, whether it is stale, or whether it may +be taken over, and acquisition never waits on it. Failing to write it does not fail acquisition, and +absent, empty, or unparseable metadata falls back to the message that names only the path. Because a +crashed owner leaves its last-written metadata in place, a reader that can positively determine the +recorded process is no longer running omits the owner detail rather than naming a dead process; a +platform that cannot determine liveness reports the recorded values as-is. + +The lock sidecar is opened with platform no-follow semantics and must resolve to a regular file before +the backend can write metadata. A symlink or platform reparse point at the lock path is rejected +without modifying its target. The first byte of the sidecar is reserved for the Windows lock range, +so a conflicting process can read the advisory metadata through a separate handle while the lock is +held. + The backend fails closed when it cannot create, open, or acquire a required lock. Launcher port preflight and health-token ownership remain useful readiness checks, but they are not persistent state ownership checks. @@ -201,13 +221,25 @@ not part of this contract. against that home, then the direct command fails without changing task or session state. 5. Given two distinct homes, databases, and ports, when two local backends start, then both can run as independent instances. +6. Given a backend owns a Kandev home and has recorded its owner metadata, when a second backend + starts against the same home, then the rejection names the owning process ID and executable in + addition to the conflicting home path. +7. Given a lock file still holds metadata from an owner that has since died, when a second backend + is rejected by whichever process holds the lock now, then the message omits owner detail it + cannot confirm and still names the conflicting home path. +8. Given a backend cannot write its owner metadata, when it has already acquired the lock, then it + starts normally and a later conflicting process falls back to the path-only message. +9. Given a lock sidecar path is a symlink or platform reparse point, when the backend starts, then + it rejects the lock before writing and leaves the link target unchanged. ## Out of scope - Changing default ports, fallback ranges, or automatic-port selection policy. - Choosing a different port when the user explicitly requested one. -- PID or process-tree identity matching; the launcher wrapper chain makes that unreliable for - dev mode. +- PID or process-tree identity matching as an ownership, staleness, or takeover decision; the + launcher wrapper chain makes that unreliable for dev mode, and a recorded PID can be stale or + reused. Advisory owner metadata written into the lock file is diagnostic text only and is + explicitly not such a decision input. - Changing the health response body, the backend health status semantics, or the desktop WebView flow. - Renaming KANDEV_DESKTOP_HEALTH_TOKEN to a neutral variable; that would be a separate contract