From 0187390245c168181ced0acb3bf4034b33ff063b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Wed, 8 Jul 2026 18:30:15 +0100 Subject: [PATCH] fix: stop kanban spinner for not-started sessions and fix truncated manual backup size (#1622) --- .../internal/system/backups/retention.go | 4 + apps/backend/internal/system/backups/store.go | 56 +++++++- .../internal/system/backups/store_test.go | 133 ++++++++++++++++++ apps/web/lib/ui/state-icons.test.tsx | 18 +++ apps/web/lib/ui/state-icons.tsx | 18 ++- 5 files changed, 223 insertions(+), 6 deletions(-) diff --git a/apps/backend/internal/system/backups/retention.go b/apps/backend/internal/system/backups/retention.go index d3a876a63f..5a41437af7 100644 --- a/apps/backend/internal/system/backups/retention.go +++ b/apps/backend/internal/system/backups/retention.go @@ -32,6 +32,10 @@ const ( autoPrefix = "kandev-" manualPrefix = "manual-" dbSuffix = ".db" + // tmpSuffix marks an in-progress VACUUM INTO sidecar. classify() skips + // it (no .db suffix) so it is never listed, and runCreate sweeps any + // leftovers from a crashed run before writing a new snapshot. + tmpSuffix = ".tmp" ) // preResetPrefix marks recovery snapshots written by the Factory Reset diff --git a/apps/backend/internal/system/backups/store.go b/apps/backend/internal/system/backups/store.go index cd632e79c6..ab32756136 100644 --- a/apps/backend/internal/system/backups/store.go +++ b/apps/backend/internal/system/backups/store.go @@ -10,6 +10,8 @@ import ( "strings" "time" + "go.uber.org/zap" + "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/db" "github.com/kandev/kandev/internal/persistence" @@ -130,15 +132,34 @@ func (s *Service) runCreate(_ context.Context) (map[string]interface{}, error) { if err := s.ensureBackupsDir(); err != nil { return nil, err } + // A .tmp sidecar is normally renamed away on success and removed on the + // error paths below, but a crash between SnapshotSQLite and os.Rename + // leaves one behind. classify() hides it from List()/Delete(), so it can + // never be cleaned up through the UI and would leak disk (up to the size + // of the live DB) indefinitely. Sweep any leftovers before writing a new + // one so crash debris is reclaimed on the next manual backup. + s.sweepStaleTmpFiles() // Nanosecond precision so double-clicks or concurrent /backups POSTs do // not collide on the same filename and silently overwrite one job's // snapshot with another. name := fmt.Sprintf("%s%d%s", manualPrefix, time.Now().UTC().UnixNano(), dbSuffix) path := filepath.Join(s.backupsDir(), name) - size, err := persistence.SnapshotSQLite(s.pool.Writer(), path) + // VACUUM INTO writes the multi-hundred-MB snapshot incrementally. If we + // wrote directly to the final "manual-*.db" name, a concurrent List() + // (the UI refetches immediately after the 202) would os.Stat a + // half-written file and report a truncated size. Write to a ".tmp" + // sidecar first — classify() ignores non-.db suffixes so it is never + // listed — then atomically rename it into place at its full size. + tmpPath := path + tmpSuffix + size, err := persistence.SnapshotSQLite(s.pool.Writer(), tmpPath) if err != nil { + _ = os.Remove(tmpPath) return nil, err } + if err := os.Rename(tmpPath, path); err != nil { + _ = os.Remove(tmpPath) + return nil, fmt.Errorf("rename snapshot into place: %w", err) + } return map[string]interface{}{ "name": name, "path": path, @@ -146,6 +167,39 @@ func (s *Service) runCreate(_ context.Context) (map[string]interface{}, error) { }, nil } +// staleTmpAge is how old a ".tmp" sidecar must be before the sweep reclaims +// it. Concurrent backup-create jobs are not serialized (jobs.Tracker runs each +// in its own goroutine), so a just-created sidecar may belong to another +// in-flight VACUUM INTO. Only files older than this are treated as crash debris, +// which keeps concurrent creates safe while still reclaiming leaked files. The +// threshold is far above any realistic VACUUM INTO duration. +const staleTmpAge = 10 * time.Minute + +// sweepStaleTmpFiles removes leftover ".tmp" VACUUM INTO sidecars from a +// previously crashed runCreate, skipping any modified within staleTmpAge so a +// concurrent create's in-progress sidecar is never deleted out from under it. +// Best-effort: read/stat/remove failures are logged and ignored so a stale +// file never blocks a fresh backup. +func (s *Service) sweepStaleTmpFiles() { + entries, err := os.ReadDir(s.backupsDir()) + if err != nil { + return + } + for _, e := range entries { + if e.IsDir() || !strings.HasSuffix(e.Name(), tmpSuffix) { + continue + } + info, err := e.Info() + if err != nil || time.Since(info.ModTime()) < staleTmpAge { + continue + } + p := filepath.Join(s.backupsDir(), e.Name()) + if err := os.Remove(p); err != nil && s.log != nil { + s.log.Warn("backups: failed to remove stale tmp snapshot", zap.String("path", p), zap.Error(err)) + } + } +} + // Restore validates the confirm token, then runs the restore as a job. // Returns the job ID on success, or an error if the token is wrong. func (s *Service) Restore(ctx context.Context, name, confirm string) (string, error) { diff --git a/apps/backend/internal/system/backups/store_test.go b/apps/backend/internal/system/backups/store_test.go index e927cc9c9e..eafd1abc12 100644 --- a/apps/backend/internal/system/backups/store_test.go +++ b/apps/backend/internal/system/backups/store_test.go @@ -160,6 +160,139 @@ func TestCreate_ProducesManualVacuumIntoFile(t *testing.T) { } } +// Regression for the truncated-size bug: the manual snapshot must be written +// to a ".tmp" sidecar and atomically renamed into place, so a concurrent +// List() never stats a half-written file. After a successful Create there +// must be exactly one "manual-*.db" and no leftover ".tmp" file, and the +// reported size must equal the final file's on-disk size. +func TestCreate_AtomicRenameLeavesNoTmpAndReportsFullSize(t *testing.T) { + svc, dataDir := newTestService(t) + id := svc.Create(context.Background()) + waitForJob(t, svc.jobs, id, jobs.StateSucceeded) + + backupsDir := filepath.Join(dataDir, "backups") + entries, err := os.ReadDir(backupsDir) + if err != nil { + t.Fatalf("readdir: %v", err) + } + var manualFile string + for _, e := range entries { + if strings.HasSuffix(e.Name(), ".tmp") { + t.Errorf("leftover tmp file after Create: %s", e.Name()) + } + if strings.HasPrefix(e.Name(), "manual-") && strings.HasSuffix(e.Name(), ".db") { + manualFile = filepath.Join(backupsDir, e.Name()) + } + } + if manualFile == "" { + t.Fatalf("manual-*.db not produced; dir contents: %+v", entries) + } + + // The size reported in the job result must match the final file on disk, + // not a partial VACUUM INTO write. + info, err := os.Stat(manualFile) + if err != nil { + t.Fatalf("stat manual file: %v", err) + } + job := svc.jobs.Get(id) + if job == nil { + t.Fatal("job not found") + } + reported, ok := job.Result["size_bytes"].(int64) + if !ok { + t.Fatalf("size_bytes missing or wrong type in job result: %+v", job.Result) + } + if reported != info.Size() { + t.Errorf("reported size %d != on-disk size %d", reported, info.Size()) + } +} + +// A ".tmp" sidecar (an in-progress VACUUM INTO) must never appear in List(), +// so the UI cannot show or restore a half-written snapshot. +func TestList_IgnoresTmpSidecar(t *testing.T) { + svc, dataDir := newTestService(t) + backupsDir := filepath.Join(dataDir, "backups") + if err := os.MkdirAll(backupsDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + seed := []string{ + "manual-1700000000.db", + "manual-1700000001.db.tmp", + } + for _, n := range seed { + if err := os.WriteFile(filepath.Join(backupsDir, n), []byte("x"), 0o644); err != nil { + t.Fatalf("write %s: %v", n, err) + } + } + + got, err := svc.List() + if err != nil { + t.Fatalf("List: %v", err) + } + if len(got) != 1 || got[0].Name != "manual-1700000000.db" { + t.Errorf("expected only the completed .db snapshot, got %+v", got) + } +} + +// A crash between SnapshotSQLite and os.Rename leaves a ".tmp" sidecar that +// classify() hides from List()/Delete(), so it could never be cleaned up via +// the UI. Create must sweep such stale (old) sidecars before writing a new +// snapshot. +func TestCreate_SweepsStaleTmpSidecar(t *testing.T) { + svc, dataDir := newTestService(t) + backupsDir := filepath.Join(dataDir, "backups") + if err := os.MkdirAll(backupsDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + stale := filepath.Join(backupsDir, "manual-1700000000.db.tmp") + if err := os.WriteFile(stale, []byte("crashed vacuum debris"), 0o644); err != nil { + t.Fatalf("seed stale tmp: %v", err) + } + // Backdate past the age guard so the sweep treats it as crash debris. + old := time.Now().Add(-2 * staleTmpAge) + if err := os.Chtimes(stale, old, old); err != nil { + t.Fatalf("chtimes stale tmp: %v", err) + } + + id := svc.Create(context.Background()) + waitForJob(t, svc.jobs, id, jobs.StateSucceeded) + + if _, err := os.Stat(stale); !os.IsNotExist(err) { + t.Errorf("stale tmp sidecar not swept: stat err = %v", err) + } + entries, err := os.ReadDir(backupsDir) + if err != nil { + t.Fatalf("readdir: %v", err) + } + for _, e := range entries { + if strings.HasSuffix(e.Name(), ".tmp") { + t.Errorf("leftover tmp file after Create: %s", e.Name()) + } + } +} + +// A recently-modified ".tmp" sidecar may be the in-progress VACUUM INTO of a +// concurrent Create job (jobs are not serialized), so the sweep must leave it +// alone — deleting it would make the other job's os.Rename fail with ENOENT. +func TestCreate_PreservesRecentTmpSidecar(t *testing.T) { + svc, dataDir := newTestService(t) + backupsDir := filepath.Join(dataDir, "backups") + if err := os.MkdirAll(backupsDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + recent := filepath.Join(backupsDir, "manual-1700000001.db.tmp") + if err := os.WriteFile(recent, []byte("another job's in-progress vacuum"), 0o644); err != nil { + t.Fatalf("seed recent tmp: %v", err) + } + + id := svc.Create(context.Background()) + waitForJob(t, svc.jobs, id, jobs.StateSucceeded) + + if _, err := os.Stat(recent); err != nil { + t.Errorf("recent tmp sidecar was swept but should have been preserved: %v", err) + } +} + // Core retention property: persistence.PruneBackups(dir, 0) must remove all // auto snapshots but must NOT touch manual snapshots. This is the contract // that lets manual snapshots survive the existing pre-migration pruning. diff --git a/apps/web/lib/ui/state-icons.test.tsx b/apps/web/lib/ui/state-icons.test.tsx index 2e7a8693ed..043c87726d 100644 --- a/apps/web/lib/ui/state-icons.test.tsx +++ b/apps/web/lib/ui/state-icons.test.tsx @@ -72,6 +72,24 @@ describe("shouldShowTaskRunningSpinner", () => { expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "IDLE")).toBe(false); }); + it("suppresses the spinner for a CREATED primary session on an inactive task", () => { + // Repro from the stuck kanban cards (PR #11571 / #11502): task CREATED, + // sitting in a Waiting column, with a primary session that was persisted in + // CREATED and never advanced (no executor, no turns). CREATED means "agent + // not started", so it must defer to the task state instead of spinning. + expect(shouldShowTaskRunningSpinner("CREATED", "CREATED")).toBe(false); + expect(shouldShowTaskRunningSpinner("REVIEW", "CREATED")).toBe(false); + expect(shouldShowTaskRunningSpinner("COMPLETED", "CREATED")).toBe(false); + }); + + it("still spins for a CREATED primary session during a genuine launch", () => { + // During an actual launch the task state is SCHEDULING/IN_PROGRESS while the + // session momentarily sits in CREATED. Deferring to the task state keeps the + // spinner on for that startup window. + expect(shouldShowTaskRunningSpinner("SCHEDULING", "CREATED")).toBe(true); + expect(shouldShowTaskRunningSpinner("IN_PROGRESS", "CREATED")).toBe(true); + }); + it("suppresses the spinner for TODO regardless of primary session state", () => { // TODO is the queued/not-started column. A stale primary session state // (e.g. task moved back from IN_PROGRESS with the session still alive) diff --git a/apps/web/lib/ui/state-icons.tsx b/apps/web/lib/ui/state-icons.tsx index 0bc1346fe0..d789a164f1 100644 --- a/apps/web/lib/ui/state-icons.tsx +++ b/apps/web/lib/ui/state-icons.tsx @@ -73,11 +73,10 @@ export function shouldUseQuestionTaskIcon( } // Session states where the agent is actively running work. Anything outside -// this set (WAITING_FOR_INPUT, IDLE, COMPLETED, FAILED, CANCELLED) is paused -// or terminal and must not drive the spinner — even when the task is still -// in the IN_PROGRESS workflow column. +// this set (CREATED, WAITING_FOR_INPUT, IDLE, COMPLETED, FAILED, CANCELLED) is +// not-yet-started, paused, or terminal and must not drive the spinner on its +// own — even when the task is still in the IN_PROGRESS workflow column. const ACTIVE_SESSION_STATES: ReadonlySet = new Set([ - "CREATED", "STARTING", "RUNNING", ]); @@ -93,6 +92,11 @@ const ACTIVE_SESSION_STATES: ReadonlySet = new Set([ * we still show the spinner so users see the imminent work; otherwise we * require an active session state. * + * `CREATED` means the session row exists but the agent has not started. During + * a genuine launch the task state is SCHEDULING/IN_PROGRESS, so we defer to the + * task state; but an orphaned/resting CREATED session on an otherwise inactive + * task (e.g. task CREATED, sitting in a Waiting column) must not spin. + * * Exception: `TODO` is the queued/not-started column. Any active session * state reported there is stale (task moved back from IN_PROGRESS, session * still alive) or transient, and the spinner would mislead — suppress it. @@ -102,7 +106,11 @@ export function shouldShowTaskRunningSpinner( primarySessionState?: string | null, ): boolean { if (taskState === "TODO") return false; - if (primarySessionState) return ACTIVE_SESSION_STATES.has(primarySessionState); + const sessionIsKnownAndNotCreated = + primarySessionState != null && primarySessionState !== "CREATED"; + if (sessionIsKnownAndNotCreated) { + return ACTIVE_SESSION_STATES.has(primarySessionState); + } return taskState === "IN_PROGRESS" || taskState === "SCHEDULING"; }