From 52be8b8e47141220081d5f953ad45e2ee65dc801 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sun, 9 Aug 2026 21:44:52 +0100 Subject: [PATCH] fix: guard backend runtime-state ownership (#2468) --- .github/workflows/backend-tests.yml | 2 +- apps/backend/Makefile | 2 +- apps/backend/internal/backendapp/main.go | 27 ++- apps/backend/internal/backendapp/main_test.go | 57 +++++ .../internal/backendapp/ownershiplock/lock.go | 100 ++++++++ .../backendapp/ownershiplock/lock_test.go | 226 ++++++++++++++++++ .../backendapp/ownershiplock/lock_unix.go | 25 ++ .../backendapp/ownershiplock/lock_windows.go | 32 +++ .../backendapp/ownershiplock/targets.go | 117 +++++++++ .../backendapp/ownershiplock/targets_test.go | 89 +++++++ .../orchestrator/reconcile_restart_test.go | 69 ++++++ apps/backend/internal/orchestrator/service.go | 18 +- .../task/statussummary/projector_test.go | 53 ++++ ...08-09-exclusive-runtime-state-ownership.md | 68 ++++++ docs/decisions/INDEX.md | 1 + .../backend-runtime-state-ownership/plan.md | 179 ++++++++++++++ .../task-01-runtime-state-ownership.md | 83 +++++++ .../task-02-startup-recovery-publication.md | 75 ++++++ .../task-03-runtime-ownership-docs.md | 66 +++++ docs/public/backend-development.md | 2 + docs/public/configuration.md | 2 + docs/public/contributing.md | 2 + .../platform/bounded-task-status-delivery.md | 11 +- docs/specs/port-collision-safety/spec.md | 58 ++++- .../specs/tasks/interrupted-task-indicator.md | 6 + 25 files changed, 1356 insertions(+), 14 deletions(-) create mode 100644 apps/backend/internal/backendapp/main_test.go create mode 100644 apps/backend/internal/backendapp/ownershiplock/lock.go create mode 100644 apps/backend/internal/backendapp/ownershiplock/lock_test.go create mode 100644 apps/backend/internal/backendapp/ownershiplock/lock_unix.go create mode 100644 apps/backend/internal/backendapp/ownershiplock/lock_windows.go create mode 100644 apps/backend/internal/backendapp/ownershiplock/targets.go create mode 100644 apps/backend/internal/backendapp/ownershiplock/targets_test.go create mode 100644 docs/decisions/2026-08-09-exclusive-runtime-state-ownership.md create mode 100644 docs/plans/backend-runtime-state-ownership/plan.md create mode 100644 docs/plans/backend-runtime-state-ownership/task-01-runtime-state-ownership.md create mode 100644 docs/plans/backend-runtime-state-ownership/task-02-startup-recovery-publication.md create mode 100644 docs/plans/backend-runtime-state-ownership/task-03-runtime-ownership-docs.md diff --git a/.github/workflows/backend-tests.yml b/.github/workflows/backend-tests.yml index 01bed54b2e..9da7cee6e5 100644 --- a/.github/workflows/backend-tests.yml +++ b/.github/workflows/backend-tests.yml @@ -378,7 +378,7 @@ jobs: # a self-hosting Go helper (see testmain_test.go) so they run on # every platform. - name: Test windows-sensitive packages - run: go test -race -v ./internal/agentctl/server/process/... ./internal/agent/runtime/agentctl/launcher/... ./internal/common/ptyexec/... + run: go test -race -v ./internal/agentctl/server/process/... ./internal/agent/runtime/agentctl/launcher/... ./internal/common/ptyexec/... ./internal/backendapp/ownershiplock/... # Keep port-collision coverage targeted: the instance package contains # broader stdio MCP fixture tests that depend on Unix command behavior. diff --git a/apps/backend/Makefile b/apps/backend/Makefile index 919dc67c9a..1e56cf8e62 100644 --- a/apps/backend/Makefile +++ b/apps/backend/Makefile @@ -286,7 +286,7 @@ test: ## runtime.GOOS guards in source. test-windows: @echo "Running Windows-sensitive packages..." - $(CGO_PREFIX) $(GO) test -tags fts5 -v ./internal/agentctl/server/process/... ./internal/agent/runtime/agentctl/launcher/... + $(CGO_PREFIX) $(GO) test -tags fts5 -v ./internal/agentctl/server/process/... ./internal/agent/runtime/agentctl/launcher/... ./internal/backendapp/ownershiplock/... @echo "Running Windows port-collision tests..." $(CGO_PREFIX) $(GO) test -tags fts5 -v ./internal/common/netutil -run TestIsAddrInUse $(CGO_PREFIX) $(GO) test -tags fts5 -v ./internal/agentctl/server/instance -run TestAllocatePortAndListenerRetriesAddressInUse diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index 457d1cd957..2251a86cfe 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -25,6 +25,7 @@ import ( "go.uber.org/zap" // Common packages + "github.com/kandev/kandev/internal/backendapp/ownershiplock" "github.com/kandev/kandev/internal/common/config" "github.com/kandev/kandev/internal/common/logger" @@ -208,7 +209,23 @@ func Run(args []string, build BuildInfo) int { cfg.Logging.Level = parsedFlags.LogLevel } - // 2. Initialize logger + // Acquire runtime-state ownership before any shared-state initialization. + // The lock remains held through logger and service cleanup, so a second + // backend cannot reconcile or migrate the live home before its bind fails. + owner, err := acquireRuntimeStateOwnership(cfg) + if err != nil { + fmt.Fprintf(os.Stderr, + "Failed to acquire backend runtime-state ownership: %v; use a separate KANDEV_HOME_DIR for an intentional second instance\n", + err) + return 1 + } + defer func() { + if closeErr := owner.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "Failed to release backend runtime-state ownership: %v\n", closeErr) + } + }() + + // Initialize logger only after runtime-state ownership is secured. log, err := logger.NewBackendLogger(logger.BackendLoggingConfig{ HomeDir: cfg.ResolvedHomeDir(), Level: cfg.Logging.Level, @@ -252,6 +269,14 @@ func Run(args []string, build BuildInfo) int { return 0 } +func acquireRuntimeStateOwnership(cfg *config.Config) (*ownershiplock.Owner, error) { + targets, err := ownershiplock.Targets(cfg.ResolvedHomeDir(), cfg.Database.Driver, cfg.Database.Path) + if err != nil { + return nil, fmt.Errorf("resolve backend runtime-state ownership: %w", err) + } + return ownershiplock.Acquire(targets) +} + func setBuildInfo(build BuildInfo) { if build.Version != "" { Version = build.Version diff --git a/apps/backend/internal/backendapp/main_test.go b/apps/backend/internal/backendapp/main_test.go new file mode 100644 index 0000000000..2d9d5a19bd --- /dev/null +++ b/apps/backend/internal/backendapp/main_test.go @@ -0,0 +1,57 @@ +package backendapp + +import ( + "bytes" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/kandev/kandev/internal/backendapp/ownershiplock" +) + +func TestBackendStartupConflictStopsBeforeSharedStateInitialization(t *testing.T) { + home := t.TempDir() + targets, err := ownershiplock.Targets(home, "sqlite", "") + if err != nil { + t.Fatalf("Targets: %v", err) + } + owner, err := ownershiplock.Acquire(targets) + if err != nil { + t.Fatalf("Acquire primary owner: %v", err) + } + t.Cleanup(func() { _ = owner.Close() }) + + var stderr bytes.Buffer + cmd := exec.Command(os.Args[0], "-test.run", "^TestBackendStartupConflictHelper$") + cmd.Env = append(os.Environ(), + "KANDEV_BACKEND_OWNERSHIP_HELPER=1", + "KANDEV_HOME_DIR="+home, + ) + cmd.Stderr = &stderr + err = cmd.Run() + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 1 { + t.Fatalf("backend helper error = %v, want exit code 1; stderr=%s", err, stderr.String()) + } + if !strings.Contains(stderr.String(), home) || !strings.Contains(stderr.String(), "separate KANDEV_HOME_DIR") { + t.Fatalf("conflict stderr = %q, want target and isolation guidance", stderr.String()) + } + for _, path := range []string{ + filepath.Join(home, "logs"), + filepath.Join(home, "data", "kandev.db"), + } { + if _, statErr := os.Stat(path); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("shared-state path %q exists or failed unexpectedly: %v", path, statErr) + } + } +} + +func TestBackendStartupConflictHelper(t *testing.T) { + if os.Getenv("KANDEV_BACKEND_OWNERSHIP_HELPER") != "1" { + return + } + os.Exit(Run(nil, BuildInfo{Version: "test"})) +} diff --git a/apps/backend/internal/backendapp/ownershiplock/lock.go b/apps/backend/internal/backendapp/ownershiplock/lock.go new file mode 100644 index 0000000000..e203e12b9f --- /dev/null +++ b/apps/backend/internal/backendapp/ownershiplock/lock.go @@ -0,0 +1,100 @@ +package ownershiplock + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "sync" +) + +// ErrConflict means another process currently owns a required runtime target. +var ErrConflict = errors.New("runtime state is already owned") + +// ConflictError identifies the target that prevented startup. +type ConflictError struct { + Target Target +} + +func (e *ConflictError) Error() string { + return fmt.Sprintf("%s target %q is already owned by another backend", e.Target.Kind, e.Target.ResourcePath) +} + +func (e *ConflictError) Unwrap() error { return ErrConflict } + +type heldLock struct { + target Target + file *os.File +} + +// Owner holds all locks acquired for one backend process. Close is idempotent +// and never removes the sidecar files; the OS lock is the ownership proof. +type Owner struct { + mu sync.Mutex + held []heldLock + closed bool +} + +// Acquire obtains every target without waiting. If any target is unavailable, +// previously acquired targets are released before the error is returned. +func Acquire(targets []Target) (*Owner, error) { + owner := &Owner{} + seen := make(map[string]struct{}, len(targets)) + for _, target := range targets { + if _, exists := seen[target.LockPath]; exists { + continue + } + seen[target.LockPath] = struct{}{} + + file, err := openLockFile(target.LockPath) + if err != nil { + _ = owner.Close() + return nil, fmt.Errorf("open %s ownership lock %q: %w", target.Kind, target.LockPath, err) + } + if err := lockFile(file); err != nil { + _ = file.Close() + _ = owner.Close() + if errors.Is(err, ErrConflict) { + return nil, &ConflictError{Target: target} + } + return nil, fmt.Errorf("acquire %s ownership lock %q: %w", target.Kind, target.LockPath, err) + } + owner.held = append(owner.held, heldLock{target: target, file: file}) + } + return owner, nil +} + +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) +} + +// Close releases every held OS lock in reverse acquisition order. The first +// release or close error is returned, while all handles are still drained. +func (o *Owner) Close() error { + if o == nil { + return nil + } + o.mu.Lock() + if o.closed { + o.mu.Unlock() + return nil + } + o.closed = true + held := o.held + o.held = nil + o.mu.Unlock() + + var firstErr error + for i := len(held) - 1; i >= 0; i-- { + if err := unlockFile(held[i].file); err != nil && firstErr == nil { + firstErr = fmt.Errorf("release %s ownership lock %q: %w", held[i].target.Kind, held[i].target.LockPath, err) + } + if err := held[i].file.Close(); err != nil && firstErr == nil { + firstErr = fmt.Errorf("close %s ownership lock %q: %w", held[i].target.Kind, held[i].target.LockPath, err) + } + } + return firstErr +} diff --git a/apps/backend/internal/backendapp/ownershiplock/lock_test.go b/apps/backend/internal/backendapp/ownershiplock/lock_test.go new file mode 100644 index 0000000000..3a9d1c5885 --- /dev/null +++ b/apps/backend/internal/backendapp/ownershiplock/lock_test.go @@ -0,0 +1,226 @@ +package ownershiplock + +import ( + "bufio" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + if os.Getenv("KANDEV_OWNERSHIP_LOCK_HELPER") == "1" { + os.Exit(m.Run()) + } + goleak.VerifyTestMain(m) +} + +func TestOwnershipLockRejectsSecondProcessAndReleasesAfterNormalExit(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() }) + + conflict := startLockHelper(t, home, "conflict") + if line := readHelperLine(t, conflict); line != "conflict" { + t.Fatalf("helper result = %q, want conflict", line) + } + if err := conflict.cmd.Wait(); err != nil { + t.Fatalf("conflict helper: %v; stderr=%s", err, conflict.stderr.String()) + } + + _ = owner.Close() + released := startLockHelper(t, home, "normal") + if line := readHelperLine(t, released); line != "ready" { + t.Fatalf("normal helper result = %q, want ready", line) + } + if err := released.cmd.Wait(); err != nil { + t.Fatalf("normal helper: %v; stderr=%s", err, released.stderr.String()) + } + + second, err := Acquire(targets) + if err != nil { + t.Fatalf("Acquire after normal exit: %v", err) + } + if err := second.Close(); err != nil { + t.Fatalf("release after normal exit: %v", err) + } +} + +func TestOwnershipLockReleasesAfterForcedProcessExit(t *testing.T) { + home := t.TempDir() + helper := startLockHelper(t, home, "hold") + if line := readHelperLine(t, helper); line != "ready" { + t.Fatalf("holding helper result = %q, want ready", line) + } + + if err := helper.cmd.Process.Kill(); err != nil { + t.Fatalf("kill holding helper: %v", err) + } + if err := helper.cmd.Wait(); err == nil { + t.Fatal("holding helper exited successfully after forced termination") + } + + targets, err := Targets(home, "sqlite", "") + if err != nil { + t.Fatalf("Targets after forced exit: %v", err) + } + owner, err := Acquire(targets) + if err != nil { + t.Fatalf("Acquire after forced exit: %v", err) + } + if err := owner.Close(); err != nil { + t.Fatalf("release after forced exit: %v", err) + } +} + +func TestOwnershipLockAllowsIndependentHomes(t *testing.T) { + firstHome := t.TempDir() + firstTargets, err := Targets(firstHome, "sqlite", "") + if err != nil { + t.Fatalf("first Targets: %v", err) + } + first, err := Acquire(firstTargets) + if err != nil { + t.Fatalf("Acquire first home: %v", err) + } + t.Cleanup(func() { _ = first.Close() }) + + secondHome := t.TempDir() + second := startLockHelper(t, secondHome, "normal") + if line := readHelperLine(t, second); line != "ready" { + t.Fatalf("independent helper result = %q, want ready", line) + } + if err := second.cmd.Wait(); err != nil { + t.Fatalf("independent helper: %v; stderr=%s", err, second.stderr.String()) + } +} + +func TestOwnershipLockHelper(t *testing.T) { + if os.Getenv("KANDEV_OWNERSHIP_LOCK_HELPER") != "1" { + return + } + home := os.Getenv("KANDEV_OWNERSHIP_HOME") + targets, err := Targets(home, "sqlite", "") + if err != nil { + fmt.Fprintf(os.Stderr, "Targets: %v\n", err) + os.Exit(2) + } + owner, err := Acquire(targets) + if err != nil { + if errors.Is(err, ErrConflict) { + _, _ = fmt.Fprintln(os.Stdout, "conflict") + return + } + fmt.Fprintf(os.Stderr, "Acquire: %v\n", err) + os.Exit(2) + } + defer func() { _ = owner.Close() }() + _, _ = fmt.Fprintln(os.Stdout, "ready") + if os.Getenv("KANDEV_OWNERSHIP_MODE") == "hold" { + timer := time.NewTimer(5 * time.Minute) + defer timer.Stop() + <-timer.C + } +} + +type lockHelper struct { + cmd *exec.Cmd + stdout *os.File + stderr *strings.Builder +} + +func startLockHelper(t *testing.T, home, mode string) *lockHelper { + t.Helper() + stdout, stdin, err := os.Pipe() + if err != nil { + t.Fatalf("create helper stdout pipe: %v", err) + } + cmd := exec.Command(os.Args[0], "-test.run", "^TestOwnershipLockHelper$") + cmd.Env = append(os.Environ(), + "KANDEV_OWNERSHIP_LOCK_HELPER=1", + "KANDEV_OWNERSHIP_HOME="+home, + "KANDEV_OWNERSHIP_MODE="+mode, + ) + cmd.Stdout = stdin + stderr := new(strings.Builder) + cmd.Stderr = stderr + if err := cmd.Start(); err != nil { + _ = stdout.Close() + _ = stdin.Close() + t.Fatalf("start lock helper: %v", err) + } + t.Cleanup(func() { + if cmd.ProcessState == nil && cmd.Process != nil { + _ = cmd.Process.Kill() + } + _ = cmd.Wait() + _ = stdout.Close() + _ = stdin.Close() + }) + return &lockHelper{cmd: cmd, stdout: stdout, stderr: stderr} +} + +func readHelperLine(t *testing.T, helper *lockHelper) string { + t.Helper() + lines := make(chan string, 1) + done := make(chan struct{}) + go func() { + defer close(done) + scanner := bufio.NewScanner(helper.stdout) + if scanner.Scan() { + lines <- scanner.Text() + return + } + lines <- "" + }() + timer := time.NewTimer(5 * time.Second) + defer timer.Stop() + select { + case line := <-lines: + <-done + return line + case <-timer.C: + _ = helper.stdout.Close() + <-done + t.Fatal("timed out waiting for lock helper") + return "" + } +} + +func TestOwnershipLockConflictErrorIncludesTarget(t *testing.T) { + home := filepath.Join(t.TempDir(), "home") + targets, err := Targets(home, "sqlite", "") + if err != nil { + t.Fatalf("Targets: %v", err) + } + owner, err := Acquire(targets) + if err != nil { + t.Fatalf("Acquire: %v", err) + } + t.Cleanup(func() { _ = owner.Close() }) + + _, err = Acquire(targets) + if !errors.Is(err, ErrConflict) { + t.Fatalf("second Acquire error = %v, want ErrConflict", err) + } + var conflict *ConflictError + if !errors.As(err, &conflict) { + t.Fatalf("second Acquire error = %T, want ConflictError", err) + } + if conflict.Target.ResourcePath != targets[0].ResourcePath { + t.Fatalf("conflict target = %q, want canonical home path %q", conflict.Target.ResourcePath, targets[0].ResourcePath) + } +} diff --git a/apps/backend/internal/backendapp/ownershiplock/lock_unix.go b/apps/backend/internal/backendapp/ownershiplock/lock_unix.go new file mode 100644 index 0000000000..3994b80d51 --- /dev/null +++ b/apps/backend/internal/backendapp/ownershiplock/lock_unix.go @@ -0,0 +1,25 @@ +//go:build !windows + +package ownershiplock + +import ( + "errors" + "os" + + "golang.org/x/sys/unix" +) + +func lockFile(file *os.File) error { + err := unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB) + if err == nil { + return nil + } + if errors.Is(err, unix.EWOULDBLOCK) || errors.Is(err, unix.EAGAIN) { + return ErrConflict + } + return err +} + +func unlockFile(file *os.File) error { + return unix.Flock(int(file.Fd()), unix.LOCK_UN) +} diff --git a/apps/backend/internal/backendapp/ownershiplock/lock_windows.go b/apps/backend/internal/backendapp/ownershiplock/lock_windows.go new file mode 100644 index 0000000000..6b19ae4dca --- /dev/null +++ b/apps/backend/internal/backendapp/ownershiplock/lock_windows.go @@ -0,0 +1,32 @@ +//go:build windows + +package ownershiplock + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +func lockFile(file *os.File) error { + err := windows.LockFileEx( + windows.Handle(file.Fd()), + windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, + 0, + 1, + 0, + &windows.Overlapped{}, + ) + if err == nil { + return nil + } + if errors.Is(err, windows.ERROR_LOCK_VIOLATION) { + return ErrConflict + } + return err +} + +func unlockFile(file *os.File) error { + return windows.UnlockFileEx(windows.Handle(file.Fd()), 0, 1, 0, &windows.Overlapped{}) +} diff --git a/apps/backend/internal/backendapp/ownershiplock/targets.go b/apps/backend/internal/backendapp/ownershiplock/targets.go new file mode 100644 index 0000000000..841d570512 --- /dev/null +++ b/apps/backend/internal/backendapp/ownershiplock/targets.go @@ -0,0 +1,117 @@ +package ownershiplock + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" +) + +// TargetKind identifies the persistent resource protected by an ownership lock. +type TargetKind string + +const ( + TargetHome TargetKind = "home" + TargetDatabase TargetKind = "database" +) + +// Target describes the canonical resource path and the stable sidecar file used +// to hold its operating-system lock. +type Target struct { + Kind TargetKind + ResourcePath string + LockPath string +} + +// Targets returns the persistent resources a backend must own before startup. +// The Kandev home owns the default SQLite database, so an explicit database +// target is needed only when SQLite points outside that home. +func Targets(homeDir, databaseDriver, databasePath string) ([]Target, error) { + home, err := canonicalPath(homeDir) + if err != nil { + return nil, fmt.Errorf("canonicalize Kandev home %q: %w", homeDir, err) + } + if home == "" { + return nil, errors.New("kandev home cannot be empty") + } + + targets := []Target{{ + Kind: TargetHome, + ResourcePath: home, + LockPath: filepath.Join(home, ".kandev-backend.lock"), + }} + if !strings.EqualFold(databaseDriver, "sqlite") { + return targets, nil + } + if strings.TrimSpace(databasePath) == "" { + // Empty path uses the in-home SQLite database, already covered by the + // home lock. + return targets, nil + } + + database, err := canonicalPath(databasePath) + if err != nil { + return nil, fmt.Errorf("canonicalize SQLite database %q: %w", databasePath, err) + } + if pathWithin(home, database) { + return targets, nil + } + + return append(targets, Target{ + Kind: TargetDatabase, + ResourcePath: database, + LockPath: database + ".lock", + }), nil +} + +func canonicalPath(path string) (string, error) { + path = strings.TrimSpace(path) + if path == "" { + return "", nil + } + abs, err := filepath.Abs(path) + if err != nil { + return "", err + } + abs = filepath.Clean(abs) + if resolved, err := filepath.EvalSymlinks(abs); err == nil { + return filepath.Clean(resolved), nil + } else if !errors.Is(err, os.ErrNotExist) { + return "", err + } + + // The resource itself may not exist yet, but every existing ancestor still + // needs to resolve to the same lock identity as its real path. This also + // normalizes Windows short-name aliases when a database directory is new. + for parent := filepath.Dir(abs); ; parent = filepath.Dir(parent) { + resolvedParent, err := filepath.EvalSymlinks(parent) + if err == nil { + relative, relErr := filepath.Rel(parent, abs) + if relErr != nil { + return "", relErr + } + return filepath.Clean(filepath.Join(resolvedParent, relative)), nil + } + if !errors.Is(err, os.ErrNotExist) { + return "", err + } + next := filepath.Dir(parent) + if next == parent { + return abs, nil + } + } +} + +func pathWithin(base, candidate string) bool { + if runtime.GOOS == "windows" { + base = strings.ToLower(base) + candidate = strings.ToLower(candidate) + } + relative, err := filepath.Rel(base, candidate) + if err != nil { + return false + } + return relative == "." || (relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator))) +} diff --git a/apps/backend/internal/backendapp/ownershiplock/targets_test.go b/apps/backend/internal/backendapp/ownershiplock/targets_test.go new file mode 100644 index 0000000000..adad5895fb --- /dev/null +++ b/apps/backend/internal/backendapp/ownershiplock/targets_test.go @@ -0,0 +1,89 @@ +package ownershiplock + +import ( + "os" + "path/filepath" + "testing" +) + +func TestTargetsDefaultSQLiteUsesHomeOwnershipOnly(t *testing.T) { + root := evalTempDir(t) + separator := string(filepath.Separator) + home := root + separator + "home" + separator + "." + separator + "nested" + separator + ".." + + targets, err := Targets(home, "sqlite", "") + if err != nil { + t.Fatalf("Targets: %v", err) + } + if len(targets) != 1 { + t.Fatalf("targets = %#v, want one home target", targets) + } + if targets[0].Kind != TargetHome { + t.Fatalf("target kind = %q, want %q", targets[0].Kind, TargetHome) + } + if targets[0].ResourcePath != filepath.Join(root, "home") { + t.Fatalf("resource path = %q, want canonical home", targets[0].ResourcePath) + } +} + +func TestTargetsExternalSQLiteAddsCanonicalDatabaseTarget(t *testing.T) { + root := evalTempDir(t) + home := filepath.Join(root, "home") + databaseDir := filepath.Join(root, "database") + if err := os.MkdirAll(databaseDir, 0o700); err != nil { + t.Fatalf("create database dir: %v", err) + } + separator := string(filepath.Separator) + databasePath := databaseDir + separator + ".." + separator + "database" + separator + "kandev.db" + + targets, err := Targets(home, "sqlite", databasePath) + if err != nil { + t.Fatalf("Targets: %v", err) + } + if len(targets) != 2 { + t.Fatalf("targets = %#v, want home and database targets", targets) + } + if targets[1].Kind != TargetDatabase { + t.Fatalf("database target kind = %q, want %q", targets[1].Kind, TargetDatabase) + } + wantDatabase := filepath.Join(databaseDir, "kandev.db") + if targets[1].ResourcePath != wantDatabase { + t.Fatalf("database resource path = %q, want %q", targets[1].ResourcePath, wantDatabase) + } + if targets[1].LockPath != wantDatabase+".lock" { + t.Fatalf("database lock path = %q, want %q", targets[1].LockPath, wantDatabase+".lock") + } +} + +func TestTargetsDoesNotDuplicateDatabaseInsideHome(t *testing.T) { + home := evalTempDir(t) + separator := string(filepath.Separator) + databasePath := home + separator + "data" + separator + ".." + separator + "data" + separator + "kandev.db" + + targets, err := Targets(filepath.Join(home, "."), "sqlite", databasePath) + if err != nil { + t.Fatalf("Targets: %v", err) + } + if len(targets) != 1 { + t.Fatalf("targets = %#v, want one canonical home target", targets) + } +} + +func TestTargetsPostgresLocksOnlyHome(t *testing.T) { + targets, err := Targets(t.TempDir(), "postgres", filepath.Join(t.TempDir(), "external.db")) + if err != nil { + t.Fatalf("Targets: %v", err) + } + if len(targets) != 1 || targets[0].Kind != TargetHome { + t.Fatalf("targets = %#v, want only home target", targets) + } +} + +func evalTempDir(t *testing.T) string { + t.Helper() + dir, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("resolve temp dir: %v", err) + } + return dir +} diff --git a/apps/backend/internal/orchestrator/reconcile_restart_test.go b/apps/backend/internal/orchestrator/reconcile_restart_test.go index cc9f9bf52f..13d0cd3593 100644 --- a/apps/backend/internal/orchestrator/reconcile_restart_test.go +++ b/apps/backend/internal/orchestrator/reconcile_restart_test.go @@ -9,6 +9,7 @@ import ( runtimeapi "github.com/kandev/kandev/internal/agent/runtime" "github.com/kandev/kandev/internal/agentruntime" + "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/task/models" ) @@ -16,6 +17,74 @@ import ( // stop failure never causes a confirmed-dead row to be pruned. var errWorkTransient = errors.New("runtime stop transient failure") +func TestReconcileSessionsOnStartupPublishesSettledSessionState(t *testing.T) { + for _, previousState := range []models.TaskSessionState{ + models.TaskSessionStateStarting, + models.TaskSessionStateRunning, + } { + t.Run(string(previousState), func(t *testing.T) { + repo := setupTestRepo(t) + ctx := context.Background() + seedTaskAndSession(t, repo, "task-startup-event", "session-startup-event", previousState) + if err := repo.SetSessionPrimary(ctx, "session-startup-event"); err != nil { + t.Fatalf("set primary session: %v", err) + } + if err := repo.UpsertExecutorRunning(ctx, &models.ExecutorRunning{ + ID: "session-startup-event", + SessionID: "session-startup-event", + TaskID: "task-startup-event", + Runtime: agentruntime.RuntimeStandalone, + Status: models.ExecutorRunningStatusRunning, + LocalPID: 4242, + CreatedAt: time.Now().UTC(), + UpdatedAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("upsert executor row: %v", err) + } + + eventBus := &recordingEventBus{} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), &mockAgentManager{}) + svc.eventBus = eventBus + svc.reconcileSessionsOnStartup(ctx) + + var stateEvent *recordedEvent + for i := range eventBus.events { + if eventBus.events[i].subject == events.TaskSessionStateChanged { + stateEvent = &eventBus.events[i] + break + } + } + if stateEvent == nil { + t.Fatalf("startup reconciliation published no %s event; events=%+v", events.TaskSessionStateChanged, eventBus.events) + } + + data, ok := stateEvent.event.Data.(map[string]interface{}) + if !ok { + t.Fatalf("session state event data = %T, want map[string]interface{}", stateEvent.event.Data) + } + if got := data["old_state"]; got != string(previousState) { + t.Errorf("old_state = %v, want %q", got, previousState) + } + if got := data["new_state"]; got != string(models.TaskSessionStateWaitingForInput) { + t.Errorf("new_state = %v, want %q", got, models.TaskSessionStateWaitingForInput) + } + if got := data["is_primary"]; got != true { + t.Errorf("is_primary = %v, want true", got) + } + foregroundActivity, present := data["foreground_activity"] + if !present { + t.Fatal("foreground_activity is missing from startup state event") + } + if foregroundActivity != nil { + t.Errorf("foreground_activity = %v, want explicit nil", foregroundActivity) + } + if got := data["active_subagent_count"]; got != 0 { + t.Errorf("active_subagent_count = %v, want 0", got) + } + }) + } +} + // TestReconcileSessionsOnStartupMakesRowsTrue is the restart-reconciliation // integration test for #1597 startup reconciliation (the backlog stops // growing, rows are made true, resumability is never lost). After a restart, diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index 35aea70b33..f5a5d19421 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -1819,7 +1819,7 @@ func (s *Service) reconcileOneSessionOnStartup(ctx context.Context, running *mod return } - s.reconcileActiveSessionOnStartup(ctx, running, sessionID, previousState) + s.reconcileActiveSessionOnStartup(ctx, running, sessionID, previousState, session) } func (s *Service) reconcileActiveSessionOnStartup( @@ -1827,15 +1827,21 @@ func (s *Service) reconcileActiveSessionOnStartup( running *models.ExecutorRunning, sessionID string, previousState models.TaskSessionState, + session *models.TaskSession, ) { // Active states: STARTING, RUNNING, WAITING_FOR_INPUT // Set session to WAITING_FOR_INPUT (idle, ready for lazy resume when user opens it) if previousState != models.TaskSessionStateWaitingForInput { - if err := s.repo.UpdateTaskSessionState(ctx, sessionID, models.TaskSessionStateWaitingForInput, ""); err != nil { - s.logger.Warn("failed to set session to WAITING_FOR_INPUT on startup", - zap.String("session_id", sessionID), - zap.Error(err)) - } + s.updateTaskSessionStateWithHook( + ctx, + running.TaskID, + sessionID, + models.TaskSessionStateWaitingForInput, + "", + false, + nil, + session, + ) } s.abandonOpenTurnsOnStartup(ctx, sessionID, "active session reconciled to waiting") diff --git a/apps/backend/internal/task/statussummary/projector_test.go b/apps/backend/internal/task/statussummary/projector_test.go index be366fbb7f..440a5ff74d 100644 --- a/apps/backend/internal/task/statussummary/projector_test.go +++ b/apps/backend/internal/task/statussummary/projector_test.go @@ -176,6 +176,59 @@ func TestProjectorDoesNotSubscribeToRawStreamsOrStreamingMessageAppends(t *testi _ = ctx } +func TestProjectorRepairsStoredRunningSummaryAfterStartupRecovery(t *testing.T) { + store := newProjectorTestStore() + storedAt := time.Date(2026, 8, 1, 17, 59, 0, 0, time.UTC) + store.rows["task-startup-recovery"] = &StoredTaskStatusSummary{ + TaskID: "task-startup-recovery", + WorkspaceID: "workspace-1", + Summary: TaskStatusSummary{ + Revision: 7, + UpdatedAt: storedAt, + PrimarySession: &PrimarySessionSummary{ID: "session-startup-recovery", State: "RUNNING"}, + ForegroundActivity: "generating", + ActiveSubagentCount: 2, + }, + } + + projector := NewProjector(ProjectorConfig{ + Store: store, + ResolveWorkspace: func(context.Context, string) (string, error) { + return "workspace-1", nil + }, + Now: func() time.Time { return storedAt.Add(time.Minute) }, + }) + + if err := projector.HandleEvent(context.Background(), bus.NewEvent(events.TaskSessionStateChanged, "test", map[string]interface{}{ + "task_id": "task-startup-recovery", + "workspace_id": "workspace-1", + "session_id": "session-startup-recovery", + "new_state": "WAITING_FOR_INPUT", + "is_primary": true, + "foreground_activity": nil, + "active_subagent_count": 0, + })); err != nil { + t.Fatalf("apply startup recovery event: %v", err) + } + + got := store.summary("task-startup-recovery") + if got == nil || got.PrimarySession == nil { + t.Fatalf("repaired summary = %+v, want primary session", got) + } + if got.Revision <= 7 { + t.Fatalf("repaired summary revision = %d, want newer than 7", got.Revision) + } + if got.PrimarySession.State != "WAITING_FOR_INPUT" { + t.Errorf("primary session state = %q, want WAITING_FOR_INPUT", got.PrimarySession.State) + } + if got.ForegroundActivity != "" { + t.Errorf("foreground activity = %q, want empty", got.ForegroundActivity) + } + if got.ActiveSubagentCount != 0 { + t.Errorf("active subagent count = %d, want 0", got.ActiveSubagentCount) + } +} + func TestProjectorDerivesBoundedStatusAcrossSources(t *testing.T) { _, store, eventBus, updates, _ := newProjectorTest(t) const taskID = "task-status-sources" diff --git a/docs/decisions/2026-08-09-exclusive-runtime-state-ownership.md b/docs/decisions/2026-08-09-exclusive-runtime-state-ownership.md new file mode 100644 index 0000000000..e6d485b332 --- /dev/null +++ b/docs/decisions/2026-08-09-exclusive-runtime-state-ownership.md @@ -0,0 +1,68 @@ +# ADR-2026-08-09-exclusive-runtime-state-ownership: Lock Runtime State Before Backend Startup + +**Status:** accepted +**Date:** 2026-08-09 +**Area:** backend, infra + +## Context + +Backend startup changes shared state before it binds the HTTP port. It opens logs, creates database +backups, applies migrations, and reconciles active sessions. Launcher port checks do not protect +direct backend commands or a bind race. + +A development command started a second backend against the live Kandev home. The process changed +session state and then failed to bind the occupied HTTP port. SQLite serialized individual writes, +but it did not protect the ownership of the runtime state. + +## Decision + +Each backend process must acquire non-blocking operating-system advisory locks before shared-state +initialization. The process locks the canonical Kandev home. It also locks a custom SQLite database +when that database is outside the home. + +The backend acquires all required locks after it loads configuration and before it initializes the +backend logger. It holds the locks until backend cleanup is complete. Lock acquisition errors stop +startup before logs, backups, migrations, session recovery, agentctl, or HTTP services start. + +The implementation uses file-handle locks on Unix and Windows. A crash releases these locks. The +implementation keeps lock files after release because removing a locked file can create two lock +identities. + +The lock scope is independent of ports. Intentional local instances use separate Kandev homes, +separate SQLite databases, and separate ports. The launcher supervisor stops the old backend before +it starts the replacement, so a planned restart preserves this ownership rule. + +## Consequences + +A stray direct backend command cannot change a live Kandev database or session state. The failure +appears before the process creates its normal backend log, so the launcher or terminal must show a +clear stderr message. + +The lock adds platform-specific code and native Windows coverage. Custom SQLite paths outside the +home need a second lock target. Postgres backends still lock their local home, but this decision does +not define active-active Postgres operation. + +An operator can find an old lock file after a crash. The file is harmless because the operating +system lock, not the file, proves ownership. + +## Alternatives Considered + +### Bind the HTTP port first + +Rejected. A different port can still open the same home or SQLite database. Port ownership does not +protect logs, secrets, backups, worktrees, or migrations. + +### Keep ownership checks in launchers + +Rejected. Direct `__backend`, `make dev-backend`, service managers, and bind races can bypass a +launcher check. The backend owns persistent initialization and must enforce the boundary. + +### Use a PID file + +Rejected. A crash leaves stale PID data, and operating systems can reuse a PID. A PID file does not +provide atomic ownership. + +### Depend on SQLite write locking + +Rejected. SQLite protects transactions, not the semantic ownership of startup recovery. It also +does not protect other files in the Kandev home. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 3fe2b8c03c..3d0146319f 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -140,3 +140,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-08-workspace-scoped-task-create-workflow-memory | [Remember Task-Create Workflows Per Workspace](2026-08-08-workspace-scoped-task-create-workflow-memory.md) | accepted | backend, frontend | 2026-08-08 | | 2026-08-08-owned-temp-artifact-cleanup | [Clean only registered Kandev temporary artifacts](2026-08-08-owned-temp-artifact-cleanup.md) | accepted | backend, frontend, infra, security | 2026-08-08 | | 2026-08-08-task-owned-worktree-lifetime | [Keep Worktree Ownership at the Task Lifecycle](2026-08-08-task-owned-worktree-lifetime.md) | accepted | backend, frontend | 2026-08-08 | +| 2026-08-09-exclusive-runtime-state-ownership | [Lock Runtime State Before Backend Startup](2026-08-09-exclusive-runtime-state-ownership.md) | accepted | backend, infra | 2026-08-09 | diff --git a/docs/plans/backend-runtime-state-ownership/plan.md b/docs/plans/backend-runtime-state-ownership/plan.md new file mode 100644 index 0000000000..b1eace2206 --- /dev/null +++ b/docs/plans/backend-runtime-state-ownership/plan.md @@ -0,0 +1,179 @@ +--- +spec: docs/specs/port-collision-safety/spec.md +created: 2026-08-09 +status: completed +--- + +# Implementation Plan: Backend Runtime-State Ownership + +## Overview + +This repair prevents a second backend from changing the runtime state of a live Kandev instance. +It also makes startup session recovery publish authoritative state before backend readiness. + +The confirmed reproduction used this command in a task worktree while another backend was active: + +```bash +make -C apps/backend dev +``` + +The second process opened the shared database and reconciled sessions. It then failed to bind the +occupied HTTP port. The first backend later saw an already-settled session, so it did not clear the +stored `generating` task summary. + +This plan implements +[ADR-2026-08-09-exclusive-runtime-state-ownership](../../decisions/2026-08-09-exclusive-runtime-state-ownership.md). +It also updates the status-delivery and interrupted-task contracts. + +## Backend + +### Runtime-state ownership locks + +Add `apps/backend/internal/backendapp/ownershiplock/` with a small cross-platform lock API. The +package owns target selection, canonical paths, lock acquisition, and lock release. + +- `lock.go` defines the owner type, conflict error, target metadata, and all-or-nothing acquisition. +- `lock_unix.go` uses a non-blocking exclusive file-handle lock. +- `lock_windows.go` uses the equivalent non-blocking Windows file lock through + `golang.org/x/sys/windows`, which is already a backend dependency. +- `targets.go` selects the Kandev-home lock. It adds a database lock when SQLite uses a path outside + the canonical home. +- Lock files remain after release. The open file handle owns the lock. + +Update `apps/backend/internal/backendapp/main.go`. Acquire ownership after `config.Load()` and flag +overrides, but before `logger.NewBackendLogger()`. Hold ownership until `Run` completes all service +cleanup. On any acquisition error, write one clear stderr error and return exit code 1. + +The rejected process must not create backend logs, open a database, take a backup, apply a +migration, launch agentctl, reconcile a session, or start an HTTP listener. + +Add the native lock package to `apps/backend/Makefile` and +`.github/workflows/backend-tests.yml` Windows-sensitive package lists. + +### Startup state publication + +Update `apps/backend/internal/orchestrator/service.go`. Route a successful startup transition from +`STARTING` or `RUNNING` to `WAITING_FOR_INPUT` through the existing semantic session-state +publication path. + +The transition must keep these existing startup behaviors: + +- Abandon an orphan turn. +- Move an eligible task from `IN_PROGRESS` to `REVIEW`. +- Add the interrupted-task marker. +- Preserve the executor row and resume data. +- Repair confirmed-dead local process data. + +The semantic state event must contain the authoritative primary-session state and an explicit empty +foreground-activity value. The existing task activity publisher remains the settle fallback for a +`RUNNING` session. A session that is already `WAITING_FOR_INPUT` remains a no-op. + +No status-summary schema or frontend change is necessary. The current projector consumes semantic +session events and explicit task activity values. The current sidebar already shows the interrupted +icon when `generating` is absent. + +## Public Documentation + +Update these public pages: + +- `docs/public/backend-development.md` +- `docs/public/contributing.md` +- `docs/public/configuration.md` + +State that one backend owns a Kandev home at a time. Explain that raw backend commands use the +normal home unless `KANDEV_HOME_DIR` is set. Keep the existing isolated command example. + +Do not document lock-file removal as a recovery step. A stale file does not hold an operating-system +lock. + +## Tests + +### Runtime-state ownership + +- **What:** A second process cannot acquire the same Kandev-home lock. +- **File:** `apps/backend/internal/backendapp/ownershiplock/lock_test.go` +- **How:** Use a subprocess helper and temporary directories. Assert conflict, release after normal + exit, release after forced exit, and independent-home success. + +- **What:** Separate homes cannot open the same external SQLite database. +- **File:** `apps/backend/internal/backendapp/ownershiplock/targets_test.go` +- **How:** Build target sets for default and custom database paths. Assert canonical deduplication + and external-database ownership. + +- **What:** Backend startup stops before shared-state initialization when ownership is unavailable. +- **File:** `apps/backend/internal/backendapp/main_test.go` +- **How:** Hold a temporary-home lock, invoke the backend entry seam, and assert exit code 1. Assert + that no database, backup, backend log, agentctl child, or listener appears. + +### Startup state publication + +- **What:** Startup recovery publishes `RUNNING -> WAITING_FOR_INPUT` through the semantic session + event path. +- **File:** `apps/backend/internal/orchestrator/reconcile_restart_test.go` +- **How:** Seed a running session, task, orphan turn, executor row, and recording event bus. This + regression must fail before the production change because the state event is absent. + +- **What:** A stale `RUNNING` and `generating` summary converges after startup recovery. +- **File:** `apps/backend/internal/task/statussummary/projector_test.go` +- **How:** Seed a stored summary, send the authoritative recovery event, and assert a newer revision + with `WAITING_FOR_INPUT`, empty foreground activity, and zero active subagents. + +- **What:** Existing task review, interruption, resume, and executor-row behavior does not change. +- **File:** `apps/backend/internal/orchestrator/reconcile_restart_test.go` and + `apps/backend/internal/orchestrator/turn_lifecycle_test.go` +- **How:** Extend the current startup-reconciliation cases. Assert the existing database outcomes + with the new event assertions. + +### Public docs + +- **What:** Published docs describe one-home ownership and isolated direct backend commands. +- **Files:** `docs/public/backend-development.md`, `docs/public/contributing.md`, and + `docs/public/configuration.md` +- **How:** Run the public-doc tests and validator. + +## Verification Results + +- Focused ownership tests: `go test -tags fts5 ./internal/backendapp/ownershiplock ./internal/backendapp` — 304 tests passed. +- Startup recovery, task service, and projector tests: `go test -tags fts5 ./internal/orchestrator ./internal/task/service ./internal/task/statussummary` — 2,658 tests passed. +- Public documentation tests: `node --test scripts/validate-public-docs.test.mjs` — 58 tests passed; `node scripts/validate-public-docs.mjs` — 41 pages validated. + +## Implementation Waves And Parallel Candidates + +Wave 1 contains two parallel candidates. User authorization is required before delegation. + +- [x] [Task 01: Enforce runtime-state ownership](task-01-runtime-state-ownership.md) — + `parallel-safe` +- [x] [Task 02: Publish startup recovery state](task-02-startup-recovery-publication.md) — + `parallel-safe` + +The tasks use disjoint source packages. Neither task changes a schema, generated contract, +dependency file, or shared package configuration. + +Wave 2: + +- [x] [Task 03: Document runtime ownership](task-03-runtime-ownership-docs.md) — depends on Task 01 + +Implementation remains sequential in the primary conversation unless the user explicitly +authorizes subagents. + +## Risks + +- File-lock behavior differs between Unix and Windows. Native Windows coverage is required. +- A process test can leak a child and retain a lock after an early test error. Each test must + register cleanup before its first assertion. +- A launcher restart must wait for the old backend to release ownership. Existing supervisors + already stop and join the old process before replacement startup. Regression coverage must keep + this behavior. +- Network filesystems can have different advisory-lock guarantees. This repair targets supported + local Kandev homes and local SQLite files. + +## Out of Scope + +- Automatic `KANDEV_HOME_DIR` selection for raw backend commands. +- Active-active backend support for Postgres or NATS. +- A sidebar or debug-panel redesign. +- A database migration or status-summary schema change. + +## Open Questions + +None. diff --git a/docs/plans/backend-runtime-state-ownership/task-01-runtime-state-ownership.md b/docs/plans/backend-runtime-state-ownership/task-01-runtime-state-ownership.md new file mode 100644 index 0000000000..f2acf72291 --- /dev/null +++ b/docs/plans/backend-runtime-state-ownership/task-01-runtime-state-ownership.md @@ -0,0 +1,83 @@ +--- +id: "01-runtime-state-ownership" +title: "Enforce runtime-state ownership" +status: completed +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/port-collision-safety/spec.md" +--- + +# Task 01: Enforce Runtime-State Ownership + +Add a cross-platform ownership lock before any backend shared-state initialization. + +## Acceptance + +- A second backend that uses the same canonical Kandev home exits with code 1 before persistent + backend initialization. +- Separate homes that reference one external SQLite database cannot open that database at the same + time. +- The operating system releases ownership after normal process exit or forced process exit. No + lock-file removal is necessary. + +## Verification + +Run these commands from the repository root: + +```bash +cd apps/backend && go test -tags fts5 ./internal/backendapp/ownershiplock ./internal/backendapp +make -C apps/backend test-windows +git diff --check +``` + +The first regression test must fail before the production change because no backend-home lock +exists. + +## Files Likely Touched + +- `apps/backend/internal/backendapp/main.go` +- `apps/backend/internal/backendapp/main_test.go` +- `apps/backend/internal/backendapp/ownershiplock/lock.go` +- `apps/backend/internal/backendapp/ownershiplock/lock_unix.go` +- `apps/backend/internal/backendapp/ownershiplock/lock_windows.go` +- `apps/backend/internal/backendapp/ownershiplock/lock_test.go` +- `apps/backend/internal/backendapp/ownershiplock/targets.go` +- `apps/backend/internal/backendapp/ownershiplock/targets_test.go` +- `apps/backend/Makefile` +- `.github/workflows/backend-tests.yml` + +## Dependencies + +None. + +## Parallelism + +`parallel-safe` with Task 02. This task owns `internal/backendapp`, the new lock package, and Windows +test-package configuration. Task 02 owns orchestrator and status-summary files. + +## Inputs + +- Spec section **Exclusive runtime-state ownership**. +- ADR `2026-08-09-exclusive-runtime-state-ownership`. +- Plan section **Runtime-state ownership locks**. +- Existing supervisor stop-before-start behavior in `internal/launcher/supervisor.go` and + `apps/cli/src/supervisor/child.ts`. + +## Risks + +- Do not erase a lock file during release. Another process can then lock a different file identity. +- Register subprocess cleanup before assertions so a failed test cannot retain ownership. +- Acquire every required target as one operation. Release earlier targets if a later target fails. + +## Output Contract + +Report the behavior, changed files, exact test results, blockers, and risks. Update this task and +`plan.md` in the same conversation. + +## Results + +- Added cross-platform ownership locks for the canonical Kandev home and external SQLite paths. +- Acquired ownership before backend logging and shared-state initialization; conflicts return exit code 1. +- Added subprocess, target-selection, startup-order, and Windows-sensitive package coverage. +- Verification: `go test -tags fts5 ./internal/backendapp/ownershiplock ./internal/backendapp` passed (304 tests). diff --git a/docs/plans/backend-runtime-state-ownership/task-02-startup-recovery-publication.md b/docs/plans/backend-runtime-state-ownership/task-02-startup-recovery-publication.md new file mode 100644 index 0000000000..171cba476f --- /dev/null +++ b/docs/plans/backend-runtime-state-ownership/task-02-startup-recovery-publication.md @@ -0,0 +1,75 @@ +--- +id: "02-startup-recovery-publication" +title: "Publish startup recovery state" +status: completed +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/platform/bounded-task-status-delivery.md" +--- + +# Task 02: Publish Startup Recovery State + +Publish authoritative session and activity state when startup recovery settles an active session. + +## Acceptance + +- A successful startup transition from `STARTING` or `RUNNING` to `WAITING_FOR_INPUT` emits the + semantic session-state event with current primary-session and activity fields. +- A stored `RUNNING` and `generating` summary advances to `WAITING_FOR_INPUT` with no generating + activity before backend readiness. +- Existing review-state, interrupted-marker, orphan-turn, executor-row, and resume behavior stays + unchanged. An already-waiting session remains a publication no-op. + +## Verification + +Run these commands from the repository root: + +```bash +cd apps/backend && go test -tags fts5 ./internal/orchestrator ./internal/task/service ./internal/task/statussummary +git diff --check +``` + +The orchestrator regression must fail before the production change because startup recovery writes +the session row directly and emits no session-state event. + +## Files Likely Touched + +- `apps/backend/internal/orchestrator/service.go` +- `apps/backend/internal/orchestrator/reconcile_restart_test.go` +- `apps/backend/internal/orchestrator/turn_lifecycle_test.go` +- `apps/backend/internal/task/statussummary/projector_test.go` + +## Dependencies + +None. + +## Parallelism + +`parallel-safe` with Task 01. This task owns orchestrator and status-summary files. Task 01 owns +backend startup, the new lock package, and Windows test-package configuration. + +## Inputs + +- Spec scenario for stale startup summaries in `bounded-task-status-delivery.md`. +- Spec scenario for the interrupted icon in `tasks/interrupted-task-indicator.md`. +- Plan section **Startup state publication**. +- The existing `updateTaskSessionStateWithHook` and `republishTaskActivityOnSettle` paths. + +## Risks + +- Do not publish a settled state when the conditional state update fails. +- Keep startup recovery free of agent launches and prompt dispatch. +- Preserve per-task event order so the summary cannot apply an older generating value last. + +## Output Contract + +Report the behavior, changed files, exact test results, blockers, and risks. Update this task and +`plan.md` in the same conversation. + +## Results + +- Routed startup session settlement through `updateTaskSessionStateWithHook`. +- Startup recovery now publishes authoritative session state with explicit empty activity and preserves the existing task-activity settle fallback. +- Added `STARTING`/`RUNNING` orchestrator regressions and stale-summary projector coverage. +- Verification: `go test -tags fts5 ./internal/orchestrator ./internal/task/service ./internal/task/statussummary` passed (2,658 tests). diff --git a/docs/plans/backend-runtime-state-ownership/task-03-runtime-ownership-docs.md b/docs/plans/backend-runtime-state-ownership/task-03-runtime-ownership-docs.md new file mode 100644 index 0000000000..bc77002145 --- /dev/null +++ b/docs/plans/backend-runtime-state-ownership/task-03-runtime-ownership-docs.md @@ -0,0 +1,66 @@ +--- +id: "03-runtime-ownership-docs" +title: "Document runtime ownership" +status: completed +wave: 2 +depends_on: ["01-runtime-state-ownership"] +plan: "plan.md" +spec: "../../specs/port-collision-safety/spec.md" +--- + +# Task 03: Document Runtime Ownership + +Document the one-home ownership rule and the safe command for an isolated backend-only instance. + +## Acceptance + +- Public development docs state that one backend owns a Kandev home at a time. +- Raw backend commands explain their default-home behavior and show the existing isolated + `KANDEV_HOME_DIR` command. +- The docs do not tell users to remove a stale lock file. + +## Verification + +Run these commands from the repository root: + +```bash +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +git diff --check +``` + +## Files Likely Touched + +- `docs/public/backend-development.md` +- `docs/public/contributing.md` +- `docs/public/configuration.md` + +## Dependencies + +Task 01. + +## Parallelism + +`sequential`. The final error behavior and ownership scope must match Task 01. + +## Inputs + +- Spec section **Exclusive runtime-state ownership**. +- Plan section **Public Documentation**. +- Docs-maintainer guidance for development and configuration pages. + +## Risks + +- Keep `make dev` distinct from raw `make dev-backend` and `make -C apps/backend dev` behavior. +- Do not expose the lock file as a user-managed control. + +## Output Contract + +Report the changed pages, their primary Diataxis type, exact test results, blockers, and risks. +Update this task and `plan.md` in the same conversation. + +## Results + +- Documented one-backend-per-home ownership in backend development, contribution, and configuration guidance. +- Documented separate home, database, and port requirements for intentional second instances. +- Verification: `node --test scripts/validate-public-docs.test.mjs` passed (58 tests); `node scripts/validate-public-docs.mjs` validated 41 published pages. diff --git a/docs/public/backend-development.md b/docs/public/backend-development.md index fa93724c9b..60157cef53 100644 --- a/docs/public/backend-development.md +++ b/docs/public/backend-development.md @@ -18,6 +18,8 @@ The Go module in `apps/backend/` builds both the unified `kandev` binary and the Use `make dev` for normal full-stack work: it isolates state, starts Vite, and configures the backend proxy. A raw `make dev-backend` uses the normal application home unless you set an isolated `KANDEV_HOME_DIR`; see [Contributing](contributing.md). +Only one backend can own a Kandev home at a time. A second raw backend using that home exits before it opens the database, writes logs, or performs startup recovery. For an intentional second backend, set `KANDEV_HOME_DIR` to a different directory and use a different port and database. The operating system releases ownership when the backend exits; no manual lock-file cleanup is required. + From the repository root: ```bash diff --git a/docs/public/configuration.md b/docs/public/configuration.md index 8caca1e880..7f83d33ec6 100644 --- a/docs/public/configuration.md +++ b/docs/public/configuration.md @@ -81,6 +81,8 @@ SQLite is the supported default and enables WAL mode. PostgreSQL deployments mus `database.path` is an advanced override. Persistence honors it, but the current **Settings → System → Database** and **Backups** services derive their displayed path, WAL files, backup source, and restore destination from `/data/kandev.db`. Those UI operations are therefore not reliable for a custom SQLite location. Use operator-managed backup/restore for a custom path and verify the actual database path printed at startup. +One backend owns a Kandev home at a time. When SQLite uses a custom path outside that home, the backend also owns that database path, so separate homes alone do not permit concurrent backends against one SQLite file. Use a separate home and database for an intentional second instance. Ownership is released when the backend exits. + Database-only snapshots also omit `/data/master.key`, the AES-256 key used to decrypt stored secrets. Preserve that owner-only key with an independently secured home/data backup; restoring the database without its matching key leaves encrypted credentials unreadable. See [Operations](./operations.md). ### Event bus and NATS diff --git a/docs/public/contributing.md b/docs/public/contributing.md index e0b17b8ffd..1239cc78cd 100644 --- a/docs/public/contributing.md +++ b/docs/public/contributing.md @@ -46,6 +46,8 @@ make dev PORT=38430 WEB_PORT=37430 KANDEV_HOME_DIR="$PWD/.kandev-dev" KANDEV_DEBUG_DEV_MODE=true make dev-backend ``` +One backend owns a Kandev home at a time. Raw backend commands use the normal home by default, so a second backend with that home stops before it changes shared state. For an intentional second backend, use a separate `KANDEV_HOME_DIR`, database, and port. + Use `make build` for a production build. Use `make start` for a production-shaped local start; it installs dependencies, builds and synchronizes the embedded web application, then launches Kandev and writes `/logs/backend-logs.log`. ## Find the owner diff --git a/docs/specs/platform/bounded-task-status-delivery.md b/docs/specs/platform/bounded-task-status-delivery.md index 667e86a0f3..31316339b6 100644 --- a/docs/specs/platform/bounded-task-status-delivery.md +++ b/docs/specs/platform/bounded-task-status-delivery.md @@ -1,7 +1,7 @@ --- status: approved created: 2026-08-01 -updated: 2026-08-02 +updated: 2026-08-09 owner: kandev --- @@ -41,6 +41,8 @@ detail surface. - `message.add` uses a stable client-generated message ID so an uncertain send can be reconciled or retried without creating or dispatching a duplicate. - Existing desktop and mobile status/icon precedence remains unchanged. +- Startup recovery publishes every repaired session state through the semantic task event path. + Persisted summaries converge before the backend reports readiness. ## Task status summary @@ -128,6 +130,8 @@ remain during migration, but switchers use the summary when present. events from publishing duplicate revisions or losing a newer value. - Missing rows are rebuilt from authoritative records. List and boot loaders batch summary reads and may batch repairs; they do not perform an N+1 query. +- Existing rows are repaired after startup recovery changes authoritative session state. Missing-row + hydration is not the only recovery path for a stale summary. - Live Git observations remain coalesced before persistence/publication. Running executions maintain the slow monitoring baseline independently of browser subscribers; active focus may request fast monitoring. Settled tasks @@ -256,6 +260,9 @@ intermediate replacement. - **GIVEN** a phone viewport, **WHEN** task summaries change, **THEN** the existing task-switcher sheet shows the same badges and precedence as the desktop sidebar without new navigation, scroll, or touch behavior. +- **GIVEN** a stored summary reports a `RUNNING` primary session with `generating` activity, + **WHEN** startup recovery changes that session to `WAITING_FOR_INPUT`, **THEN** a newer summary + reports the waiting state and no generating activity before the backend reports readiness. ## Out of scope @@ -280,3 +287,5 @@ intermediate replacement. [`../../plans/bounded-task-status-delivery/plan.md`](../../plans/bounded-task-status-delivery/plan.md) - Stream overload repair: [`../../plans/session-stream-overload-isolation/plan.md`](../../plans/session-stream-overload-isolation/plan.md) +- Startup-summary repair: + [`../../plans/backend-runtime-state-ownership/plan.md`](../../plans/backend-runtime-state-ownership/plan.md) diff --git a/docs/specs/port-collision-safety/spec.md b/docs/specs/port-collision-safety/spec.md index 8400758c2d..44edc74d97 100644 --- a/docs/specs/port-collision-safety/spec.md +++ b/docs/specs/port-collision-safety/spec.md @@ -1,9 +1,10 @@ --- status: building created: 2026-08-07 +updated: 2026-08-09 --- -# Port collision safety and launcher readiness +# Port collision and backend ownership safety ## Why @@ -17,6 +18,10 @@ On Windows, the agentctl instance allocator has a separate failure: the operatin WSAEADDRINUSE (10048), which does not match the synthetic Go syscall.EADDRINUSE value used by the current retry check. The allocator therefore stops instead of trying the next port. +A direct backend command can bypass launcher checks. A second backend can then open the same +Kandev home before its HTTP bind fails. This sequence can migrate the live database and reconcile +active sessions while the first backend still owns them. + This repair covers GitHub issues [#2370](https://github.com/kdlbs/kandev/issues/2370), [#2372](https://github.com/kdlbs/kandev/issues/2372), and @@ -70,6 +75,33 @@ Go syscall value and x/sys/windows.WSAEADDRINUSE on Windows. - Non-address-in-use bind errors still release the candidate and fail immediately. - String matching on the English error text is not part of the contract. +### Exclusive runtime-state ownership + +Every backend process must acquire exclusive ownership before it initializes the backend logger or +opens a persistent store. The ownership boundary covers these targets: + +- The canonical Kandev home, because it owns logs, secrets, worktrees, supervisor files, and the + default SQLite database. +- A custom SQLite database outside that home, because separate homes can still reference the same + database file. + +The backend holds each operating-system advisory lock until all backend cleanup is complete. A +crash releases the lock. The lock file can remain after exit because file existence does not prove +ownership. + +If another process holds a required lock, startup exits non-zero. It names the conflicting home or +database path and tells the operator to use a separate `KANDEV_HOME_DIR` for an intentional second +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. + +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. + +Intentional local instances need separate Kandev homes and separate SQLite databases. A backend +that uses Postgres still locks its local Kandev home. Active-active Postgres deployment behavior is +not part of this contract. + ## Scenarios ### Issue #2370: explicit port collisions @@ -102,6 +134,19 @@ Go syscall value and x/sys/windows.WSAEADDRINUSE on Windows. 3. Given a tunnel port is occupied on Windows, when a tunnel is requested, then the caller gets the existing clear “port is already in use” error. +### Concurrent backend startup + +1. Given one backend owns a Kandev home, when another backend uses the same home, then the second + process exits before any persistent backend initialization. +2. Given two homes reference the same external SQLite database, when both backends start, then + only one process opens or changes that database. +3. Given a backend process stops or crashes, when its successor starts with the same home, then the + successor acquires ownership without manual lock-file removal. +4. Given a live backend uses the default home, when a developer runs a direct backend target + 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. + ## Out of scope - Changing default ports, fallback ranges, or automatic-port selection policy. @@ -113,11 +158,16 @@ Go syscall value and x/sys/windows.WSAEADDRINUSE on Windows. - Renaming KANDEV_DESKTOP_HEALTH_TOKEN to a neutral variable; that would be a separate contract migration. - Changing service-install handling of KANDEV_SERVER_PORT, which is a separate installer issue. -- UI changes, database migrations, or new public authentication semantics. +- Database-schema changes or new public authentication semantics. +- Automatic home selection for direct backend commands. +- Active-active backend support for one Postgres database or one event namespace. +- New UI diagnostics for ownership or task-summary fields. ## Contract notes The existing desktop health-token contract in docs/specs/desktop-tauri-app/spec.md is the authority for the environment variable and response -header. This repair extends its use to CLI/native launcher ownership checks without changing the -backend route contract, so a new architecture decision record is not required. +header. Backend runtime-state ownership follows +[ADR-2026-08-09-exclusive-runtime-state-ownership](../../decisions/2026-08-09-exclusive-runtime-state-ownership.md). +The repair plan is +[Backend runtime-state ownership](../../plans/backend-runtime-state-ownership/plan.md). diff --git a/docs/specs/tasks/interrupted-task-indicator.md b/docs/specs/tasks/interrupted-task-indicator.md index 5810d4464d..1bfe1cd509 100644 --- a/docs/specs/tasks/interrupted-task-indicator.md +++ b/docs/specs/tasks/interrupted-task-indicator.md @@ -1,5 +1,6 @@ --- status: complete +updated: 2026-08-09 created: 2026-08-02 owner: kandev --- @@ -134,6 +135,9 @@ no effect beyond the icon. metadata contains `interrupted_at`, the task DTO reports `interrupted: true`, and the sidebar row, board card, graph node, rich list row, and mobile switcher row show the red alert icon. +- **GIVEN** that task also has a stored `generating` task summary, **WHEN** reconciliation changes + the primary session to `WAITING_FOR_INPUT`, **THEN** the summary clears `generating` and the red + alert icon is not hidden by a stale running spinner. - **GIVEN** a task whose session was `WAITING_FOR_INPUT` when the backend died, **WHEN** reconciliation runs, **THEN** the task is not marked and shows no red icon. @@ -167,3 +171,5 @@ no effect beyond the icon. the marker hooks into. - [Sidebar Task Completion Icons](../ui/sidebar-task-completion-icons.md) — the shared task-row icon precedence this feature extends. +- [Backend Runtime-State Ownership](../../plans/backend-runtime-state-ownership/plan.md) — + startup publication that clears stale generating activity.