diff --git a/apps/backend/internal/backendapp/persistence_middleware_test.go b/apps/backend/internal/backendapp/persistence_middleware_test.go index c7d843ba33..39f782aa12 100644 --- a/apps/backend/internal/backendapp/persistence_middleware_test.go +++ b/apps/backend/internal/backendapp/persistence_middleware_test.go @@ -5,19 +5,95 @@ import ( "net/http" "net/http/httptest" "testing" + "testing/synctest" + "time" "github.com/gin-gonic/gin" "github.com/jmoiron/sqlx" _ "github.com/mattn/go-sqlite3" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" "github.com/kandev/kandev/internal/auth" authhttpmw "github.com/kandev/kandev/internal/auth/httpmw" authstore "github.com/kandev/kandev/internal/auth/store" "github.com/kandev/kandev/internal/common/config" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/db" "github.com/kandev/kandev/internal/persistence/requiredstores" + "github.com/kandev/kandev/internal/system/maintenance" userstore "github.com/kandev/kandev/internal/user/store" ) +func TestPersistenceMiddlewareRemainsAvailableDuringManagedMaintenance(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + gin.SetMode(gin.TestMode) + conn, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + t.Cleanup(func() { _ = conn.Close() }) + conn.SetMaxOpenConns(1) + if _, err := conn.Exec("CREATE TABLE tasks (id TEXT PRIMARY KEY)"); err != nil { + t.Fatalf("create tasks table: %v", err) + } + pool := db.NewPool(conn, conn) + tracker, err := requiredstores.NewTracker([]requiredstores.Descriptor{{ + ID: "task", OwnerPackage: "internal/task", RequiredTables: []string{"tasks"}, + }}) + if err != nil { + t.Fatalf("NewTracker: %v", err) + } + if err := tracker.RecordSuccess("task"); err != nil { + t.Fatalf("RecordSuccess: %v", err) + } + core, logs := observer.New(zap.DebugLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("NewFromZap: %v", err) + } + health := requiredstores.NewHealth(tracker, pool, log) + if err := health.Check(context.Background()); err != nil { + t.Fatalf("initial Check: %v", err) + } + + release, ok := maintenance.ForPool(pool).TryAcquire() + if !ok { + t.Fatal("maintenance lease unavailable") + } + t.Cleanup(release) + tx, err := conn.Beginx() + if err != nil { + t.Fatalf("begin writer transaction: %v", err) + } + t.Cleanup(func() { _ = tx.Rollback() }) + + health.SetInterval(time.Millisecond) + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + stop := health.Start(ctx) + t.Cleanup(func() { _ = stop() }) + time.Sleep(time.Millisecond) + synctest.Wait() + + if !health.Healthy() { + t.Fatal("required persistence became unhealthy during managed maintenance") + } + if count := logs.FilterMessage("required persistence probe deferred during database maintenance").Len(); count == 0 { + t.Fatal("periodic health probe did not report maintenance deferral") + } + + router := gin.New() + router.Use(requiredPersistenceMiddleware(health)) + router.GET("/api/v1/tasks", func(c *gin.Context) { c.Status(http.StatusOK) }) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/api/v1/tasks", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("stateful request status = %d, want %d", rec.Code, http.StatusOK) + } + }) +} + func TestRequiredPersistenceMiddlewareBlocksStatefulTrafficButAllowsDiagnostics(t *testing.T) { gin.SetMode(gin.TestMode) tracker, err := requiredstores.NewTracker([]requiredstores.Descriptor{{ diff --git a/apps/backend/internal/persistence/requiredstores/health.go b/apps/backend/internal/persistence/requiredstores/health.go index c4325103c5..5f80bcca9b 100644 --- a/apps/backend/internal/persistence/requiredstores/health.go +++ b/apps/backend/internal/persistence/requiredstores/health.go @@ -12,6 +12,8 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/db/dialect" + "github.com/kandev/kandev/internal/system/maintenance" ) const ( @@ -83,6 +85,38 @@ func (h *Health) Check(ctx context.Context) error { return errors.Join(failures...) } +// MarkUnavailable records a destructive database transition before the +// maintenance owner releases its admission lease. This keeps stateful +// requests fail-closed while the process waits for the required restart. +func (h *Health) MarkUnavailable() { + if h == nil || h.tracker == nil { + return + } + _ = h.recordUnavailable(errors.New("database maintenance requires restart")) +} + +// checkRuntime runs a periodic probe when the database can be admitted. SQLite +// maintenance owns the same writer pool, so a busy maintenance lease defers +// the probe instead of turning bounded writer contention into an unhealthy +// state. Startup callers continue to use Check, which remains strict. +func (h *Health) checkRuntime(ctx context.Context) (deferred bool, err error) { + if h.isSQLite() { + release, ok := maintenance.ForPool(h.pool).TryAcquire() + if !ok { + if h.log != nil { + h.log.Debug("required persistence probe deferred during database maintenance") + } + return true, nil + } + defer release() + } + return false, h.Check(ctx) +} + +func (h *Health) isSQLite() bool { + return h.pool != nil && h.pool.Writer() != nil && h.pool.Writer().DriverName() == dialect.SQLite3 +} + func (h *Health) ping(ctx context.Context) error { if err := h.pool.Writer().PingContext(ctx); err != nil { return fmt.Errorf("writer ping failed: %w", err) @@ -146,7 +180,8 @@ func (h *Health) run(ctx context.Context, interval time.Duration, done chan stru return case <-ticker.C: checkCtx, cancel := context.WithTimeout(ctx, probeTimeout) - if err := h.Check(checkCtx); err != nil && h.log != nil { + _, err := h.checkRuntime(checkCtx) + if err != nil && h.log != nil { h.log.Warn("required persistence probe failed", zap.Error(err)) } cancel() diff --git a/apps/backend/internal/persistence/requiredstores/health_test.go b/apps/backend/internal/persistence/requiredstores/health_test.go index b0d6171042..3d63eafb25 100644 --- a/apps/backend/internal/persistence/requiredstores/health_test.go +++ b/apps/backend/internal/persistence/requiredstores/health_test.go @@ -2,6 +2,7 @@ package requiredstores import ( "context" + "reflect" "testing" "time" @@ -9,8 +10,214 @@ import ( _ "github.com/mattn/go-sqlite3" "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/system/maintenance" ) +func newSQLiteHealthFixture(t *testing.T, descriptors []Descriptor) (*sqlx.DB, *db.Pool, *Tracker, *Health) { + t.Helper() + conn, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + t.Cleanup(func() { _ = conn.Close() }) + conn.SetMaxOpenConns(1) + + tracker, err := NewTracker(descriptors) + if err != nil { + t.Fatalf("NewTracker: %v", err) + } + pool := db.NewPool(conn, conn) + health := NewHealth(tracker, pool, nil) + return conn, pool, tracker, health +} + +func holdWriter(t *testing.T, conn *sqlx.DB) *sqlx.Tx { + t.Helper() + tx, err := conn.Beginx() + if err != nil { + t.Fatalf("begin writer transaction: %v", err) + } + t.Cleanup(func() { _ = tx.Rollback() }) + return tx +} + +func TestRuntimeHealthDefersDuringMaintenance(t *testing.T) { + conn, pool, tracker, health := newSQLiteHealthFixture(t, []Descriptor{{ + ID: "first", OwnerPackage: "owner/first", RequiredTables: []string{"first"}, + }}) + if _, err := conn.Exec("CREATE TABLE first (id TEXT PRIMARY KEY)"); err != nil { + t.Fatalf("create first table: %v", err) + } + if err := tracker.RecordSuccess("first"); err != nil { + t.Fatalf("RecordSuccess: %v", err) + } + if err := health.Check(context.Background()); err != nil { + t.Fatalf("initial Check: %v", err) + } + before := tracker.Snapshot() + + release, ok := maintenance.ForPool(pool).TryAcquire() + if !ok { + t.Fatal("maintenance lease unavailable") + } + defer release() + holdWriter(t, conn) + + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Millisecond) + defer cancel() + deferred, err := health.checkRuntime(ctx) + if err != nil { + t.Fatalf("checkRuntime error = %v, want nil while maintenance owns the pool", err) + } + if !deferred { + t.Fatal("checkRuntime deferred = false, want true while maintenance owns the pool") + } + after := tracker.Snapshot() + if !reflect.DeepEqual(after, before) { + t.Fatalf("tracker changed during deferred probe: before=%#v after=%#v", before, after) + } +} + +func TestRuntimeHealthDeferralPreservesMixedStoreStates(t *testing.T) { + conn, pool, tracker, health := newSQLiteHealthFixture(t, []Descriptor{ + {ID: "healthy", OwnerPackage: "owner/healthy", RequiredTables: []string{"healthy"}}, + {ID: "unhealthy", OwnerPackage: "owner/unhealthy", RequiredTables: []string{"unhealthy"}}, + }) + if _, err := conn.Exec("CREATE TABLE healthy (id TEXT PRIMARY KEY)"); err != nil { + t.Fatalf("create healthy table: %v", err) + } + for _, id := range []string{"healthy", "unhealthy"} { + if err := tracker.RecordSuccess(id); err != nil { + t.Fatalf("RecordSuccess(%q): %v", id, err) + } + } + if err := health.Check(context.Background()); err == nil { + t.Fatal("initial Check returned nil with a missing table") + } + before := tracker.Snapshot() + + release, ok := maintenance.ForPool(pool).TryAcquire() + if !ok { + t.Fatal("maintenance lease unavailable") + } + defer release() + holdWriter(t, conn) + + deferred, err := health.checkRuntime(context.Background()) + if err != nil { + t.Fatalf("checkRuntime error = %v, want nil while maintenance owns the pool", err) + } + if !deferred { + t.Fatal("checkRuntime deferred = false, want true while maintenance owns the pool") + } + after := tracker.Snapshot() + if !reflect.DeepEqual(after, before) { + t.Fatalf("mixed tracker state changed during deferred probe: before=%#v after=%#v", before, after) + } +} + +func TestStartupHealthDoesNotDefer(t *testing.T) { + conn, pool, tracker, health := newSQLiteHealthFixture(t, []Descriptor{{ + ID: "first", OwnerPackage: "owner/first", RequiredTables: []string{"first"}, + }}) + if _, err := conn.Exec("CREATE TABLE first (id TEXT PRIMARY KEY)"); err != nil { + t.Fatalf("create first table: %v", err) + } + if err := tracker.RecordSuccess("first"); err != nil { + t.Fatalf("RecordSuccess: %v", err) + } + if err := health.Check(context.Background()); err != nil { + t.Fatalf("initial Check: %v", err) + } + before, ok := tracker.Status("first") + if !ok { + t.Fatal("missing first status") + } + + release, ok := maintenance.ForPool(pool).TryAcquire() + if !ok { + t.Fatal("maintenance lease unavailable") + } + defer release() + holdWriter(t, conn) + + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Millisecond) + defer cancel() + if err := health.Check(ctx); err == nil { + t.Fatal("startup Check returned nil while its writer probe was blocked") + } + after, ok := tracker.Status("first") + if !ok { + t.Fatal("missing first status after Check") + } + if after.State != StateUnhealthy { + t.Fatalf("startup Check state = %q, want %q", after.State, StateUnhealthy) + } + if !after.LastCheckedAt.After(before.LastCheckedAt) { + t.Fatalf("startup Check did not record a newer check time: before=%v after=%v", before.LastCheckedAt, after.LastCheckedAt) + } +} + +func TestRuntimeHealthResumesAfterMaintenance(t *testing.T) { + conn, pool, tracker, health := newSQLiteHealthFixture(t, []Descriptor{{ + ID: "first", OwnerPackage: "owner/first", RequiredTables: []string{"first"}, + }}) + if _, err := conn.Exec("CREATE TABLE first (id TEXT PRIMARY KEY)"); err != nil { + t.Fatalf("create first table: %v", err) + } + if err := tracker.RecordSuccess("first"); err != nil { + t.Fatalf("RecordSuccess: %v", err) + } + if err := health.Check(context.Background()); err != nil { + t.Fatalf("initial Check: %v", err) + } + + release, ok := maintenance.ForPool(pool).TryAcquire() + if !ok { + t.Fatal("maintenance lease unavailable") + } + deferred, err := health.checkRuntime(context.Background()) + if err != nil || !deferred { + t.Fatalf("checkRuntime during maintenance = deferred %v, err %v; want true, nil", deferred, err) + } + release() + + if _, err := conn.Exec("DROP TABLE first"); err != nil { + t.Fatalf("drop first table: %v", err) + } + deferred, err = health.checkRuntime(context.Background()) + if deferred { + t.Fatal("checkRuntime deferred after maintenance released") + } + if err == nil { + t.Fatal("checkRuntime returned nil with a missing table") + } + if health.Healthy() { + t.Fatal("health remained healthy after a real post-maintenance failure") + } + probeLease, ok := maintenance.ForPool(pool).TryAcquire() + if !ok { + t.Fatal("failed probe leaked maintenance admission") + } + probeLease() + + if _, err := conn.Exec("CREATE TABLE first (id TEXT PRIMARY KEY)"); err != nil { + t.Fatalf("recreate first table: %v", err) + } + deferred, err = health.checkRuntime(context.Background()) + if deferred || err != nil { + t.Fatalf("recovery check = deferred %v, err %v; want false, nil", deferred, err) + } + if !health.Healthy() { + t.Fatal("health did not recover after the missing table was repaired") + } + successLease, ok := maintenance.ForPool(pool).TryAcquire() + if !ok { + t.Fatal("successful probe leaked maintenance admission") + } + successLease() +} + func TestHealthCheckMarksMissingTableUnhealthyAndRecovers(t *testing.T) { conn, err := sqlx.Open("sqlite3", ":memory:") if err != nil { diff --git a/apps/backend/internal/system/backups/store.go b/apps/backend/internal/system/backups/store.go index 660c9dcf3e..2db21c1650 100644 --- a/apps/backend/internal/system/backups/store.go +++ b/apps/backend/internal/system/backups/store.go @@ -64,6 +64,10 @@ type Service struct { jobs *jobs.Tracker log *logger.Logger + // PersistenceUnavailable marks required stores unhealthy before restore + // closes the pool and leaves the process awaiting restart. + PersistenceUnavailable func() + // RestoreQuiesce stops scheduling, active executions, and database-backed // workers before restore closes the shared database pool. Wired by the // backend composition root; tests may leave it nil. @@ -210,6 +214,9 @@ func (s *Service) runRestore(ctx context.Context, snapshotPath string) (map[stri _ = os.Remove(stagedPath) return nil, err } + if s.PersistenceUnavailable != nil { + s.PersistenceUnavailable() + } if s.RestoreQuiesce != nil { if err := s.RestoreQuiesce(); err != nil { _ = os.Remove(stagedPath) diff --git a/apps/backend/internal/system/database/reset.go b/apps/backend/internal/system/database/reset.go index 0853ff783b..851188ecd1 100644 --- a/apps/backend/internal/system/database/reset.go +++ b/apps/backend/internal/system/database/reset.go @@ -59,6 +59,9 @@ func (s *Service) runFactoryReset(ctx context.Context) (map[string]interface{}, if err := s.requireSQLiteMaintenance("factory reset"); err != nil { return nil, err } + if s.PersistenceUnavailable != nil { + s.PersistenceUnavailable() + } if s.OrchestratorShutdown != nil { s.OrchestratorShutdown() diff --git a/apps/backend/internal/system/database/stats.go b/apps/backend/internal/system/database/stats.go index 79b510cbb0..dfb7f7de14 100644 --- a/apps/backend/internal/system/database/stats.go +++ b/apps/backend/internal/system/database/stats.go @@ -68,6 +68,10 @@ type Service struct { jobs *jobs.Tracker log *logger.Logger + // PersistenceUnavailable marks required stores unhealthy before a + // destructive maintenance operation leaves the process awaiting restart. + PersistenceUnavailable func() + // OrchestratorShutdown stops the orchestrator and active executions before // the factory-reset job runs. Wired by cmd/kandev. Tests pass a no-op. OrchestratorShutdown func() diff --git a/apps/backend/internal/system/system.go b/apps/backend/internal/system/system.go index f917376ea0..eb7aeefcf7 100644 --- a/apps/backend/internal/system/system.go +++ b/apps/backend/internal/system/system.go @@ -120,9 +120,16 @@ func Provide(cfg *config.Config, log *logger.Logger, pool *db.Pool, eventBus bus } dbSvc := database.NewService(pool, databasePath, resetDirs, tracker, log) dbSvc.OrchestratorShutdown = wiring.OrchestratorShutdown + markPersistenceUnavailable := func() { + if wiring.PersistenceHealth != nil { + wiring.PersistenceHealth.MarkUnavailable() + } + } + dbSvc.PersistenceUnavailable = markPersistenceUnavailable backupsSvc := backups.NewService(databasePath, pool, tracker, log) backupsSvc.OrchestratorShutdown = wiring.OrchestratorShutdown + backupsSvc.PersistenceUnavailable = markPersistenceUnavailable retentionSvc := provideToolRetention(pool, backupsSvc, eventBus, log, wiring) dbSvc.DatabaseQuiesce = retentionQuiesce(retentionSvc, wiring.DatabaseQuiesce) restoreQuiesce := wiring.RestoreQuiesce diff --git a/apps/backend/internal/system/system_database_path_test.go b/apps/backend/internal/system/system_database_path_test.go index 2234006ce1..5a67be3b3b 100644 --- a/apps/backend/internal/system/system_database_path_test.go +++ b/apps/backend/internal/system/system_database_path_test.go @@ -1,12 +1,14 @@ package system import ( + "context" "encoding/json" "net/http" "net/http/httptest" "os" "path/filepath" "testing" + "time" "github.com/gin-gonic/gin" "github.com/jmoiron/sqlx" @@ -15,6 +17,8 @@ import ( "github.com/kandev/kandev/internal/common/config" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/persistence/requiredstores" + "github.com/kandev/kandev/internal/system/jobs" ) func TestConfiguredDatabasePath_ProvideListsSiblingBackups(t *testing.T) { @@ -109,6 +113,128 @@ func TestProvideWiresRestoreQuiesce(t *testing.T) { } } +func TestProvideMarksPersistenceUnavailableForFactoryReset(t *testing.T) { + root := t.TempDir() + databasePath := filepath.Join(root, "custom", "named.db") + pool := openSystemDatabasePathPool(t, databasePath) + t.Cleanup(func() { _ = pool.Close() }) + if _, err := pool.Writer().Exec(`CREATE TABLE kandev_meta (key TEXT PRIMARY KEY, value TEXT NOT NULL)`); err != nil { + t.Fatalf("create kandev_meta: %v", err) + } + + tracker, err := requiredstores.NewTracker([]requiredstores.Descriptor{{ + ID: "settings", OwnerPackage: "internal/system/settings", RequiredTables: []string{"settings"}, + }}) + if err != nil { + t.Fatalf("NewTracker: %v", err) + } + if err := tracker.RecordSuccess("settings"); err != nil { + t.Fatalf("RecordSuccess: %v", err) + } + health := requiredstores.NewHealth(tracker, pool, nil) + log, err := logger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("logger: %v", err) + } + t.Cleanup(func() { _ = log.Close() }) + + service := Provide(&config.Config{ + HomeDir: root, + Database: config.DatabaseConfig{Driver: "sqlite", Path: databasePath}, + }, log, pool, nil, BuildInfo{}, Wiring{ + RequiredStores: tracker, + PersistenceHealth: health, + }) + if err := health.Check(context.Background()); err != nil { + t.Fatalf("initial health check: %v", err) + } + jobID, err := service.Database.FactoryReset(context.Background(), "RESET") + if err != nil { + t.Fatalf("FactoryReset: %v", err) + } + job := waitForSystemJob(t, service.Jobs, jobID) + if job.State != jobs.StateSucceeded { + t.Fatalf("factory reset state = %s, want succeeded: %s", job.State, job.Message) + } + if health.Healthy() { + t.Fatal("required persistence recovered before the frontend restart") + } +} + +func TestProvideMarksPersistenceUnavailableForRestore(t *testing.T) { + root := t.TempDir() + databasePath := filepath.Join(root, "custom", "named.db") + pool := openSystemDatabasePathPool(t, databasePath) + t.Cleanup(func() { _ = pool.Close() }) + + tracker, err := requiredstores.NewTracker([]requiredstores.Descriptor{{ + ID: "settings", OwnerPackage: "internal/system/settings", RequiredTables: []string{"settings"}, + }}) + if err != nil { + t.Fatalf("NewTracker: %v", err) + } + if err := tracker.RecordSuccess("settings"); err != nil { + t.Fatalf("RecordSuccess: %v", err) + } + health := requiredstores.NewHealth(tracker, pool, nil) + log, err := logger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("logger: %v", err) + } + t.Cleanup(func() { _ = log.Close() }) + + service := Provide(&config.Config{ + HomeDir: root, + Database: config.DatabaseConfig{Driver: "sqlite", Path: databasePath}, + }, log, pool, nil, BuildInfo{}, Wiring{ + RequiredStores: tracker, + PersistenceHealth: health, + }) + if err := health.Check(context.Background()); err != nil { + t.Fatalf("initial health check: %v", err) + } + snapshotJobID := service.Backups.Create(context.Background()) + snapshotJob := waitForSystemJob(t, service.Jobs, snapshotJobID) + if snapshotJob.State != jobs.StateSucceeded { + t.Fatalf("snapshot state = %s, want succeeded: %s", snapshotJob.State, snapshotJob.Message) + } + snapshots, err := service.Backups.List() + if err != nil || len(snapshots) != 1 { + t.Fatalf("List() = %#v, %v; want one snapshot", snapshots, err) + } + + restoreJobID, err := service.Backups.Restore(context.Background(), snapshots[0].Name, "RESTORE") + if err != nil { + t.Fatalf("Restore: %v", err) + } + restoreJob := waitForSystemJob(t, service.Jobs, restoreJobID) + if restoreJob.State != jobs.StateSucceeded { + t.Fatalf("restore state = %s, want succeeded: %s", restoreJob.State, restoreJob.Message) + } + if health.Healthy() { + t.Fatal("required persistence recovered before the frontend restart") + } +} + +func waitForSystemJob(t *testing.T, tracker *jobs.Tracker, id string) *jobs.Job { + t.Helper() + ticker := time.NewTicker(5 * time.Millisecond) + defer ticker.Stop() + deadline := time.NewTimer(5 * time.Second) + defer deadline.Stop() + for { + job := tracker.Get(id) + if job != nil && (job.State == jobs.StateSucceeded || job.State == jobs.StateFailed) { + return job + } + select { + case <-ticker.C: + case <-deadline.C: + t.Fatalf("job %q did not finish", id) + } + } +} + func openSystemDatabasePathPool(t *testing.T, databasePath string) *db.Pool { t.Helper() writerRaw, err := db.OpenSQLite(databasePath) diff --git a/apps/web/components/settings/system/tool-payload-retention-card.test.tsx b/apps/web/components/settings/system/tool-payload-retention-card.test.tsx index a65b2612db..5cda97bbe2 100644 --- a/apps/web/components/settings/system/tool-payload-retention-card.test.tsx +++ b/apps/web/components/settings/system/tool-payload-retention-card.test.tsx @@ -1,7 +1,10 @@ import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, expect, it, vi } from "vitest"; import type { SettingsSaveContributor } from "@/components/settings/settings-save-provider"; -import type { ToolPayloadRetentionStatus } from "@/lib/types/tool-payload-retention"; +import type { + ToolPayloadOperation, + ToolPayloadRetentionStatus, +} from "@/lib/types/tool-payload-retention"; import * as api from "@/lib/api/domains/tool-payload-retention-api"; import { ToolPayloadRetentionCard } from "./tool-payload-retention-card"; vi.mock("@/lib/api/domains/tool-payload-retention-api"); @@ -15,6 +18,9 @@ vi.mock("@/components/settings/settings-save-provider", () => ({ })); const ENABLED_TEST_ID = "tool-payload-enabled"; const AGE_TEST_ID = "tool-payload-age"; +const ERROR_TEST_ID = "tool-payload-error"; +const ANALYZE_TEST_ID = "tool-payload-analyze"; +const LAST_RUN_TEST_ID = "tool-payload-last-run"; const baseline: ToolPayloadRetentionStatus = { supported: true, @@ -31,6 +37,79 @@ async function open() { render(); await screen.findByTestId(ENABLED_TEST_ID); } + +it("clears a recovered status polling error without manual refresh", async () => { + vi.useFakeTimers(); + try { + const readError = new Error("status unavailable"); + vi.mocked(api.fetchToolPayloadRetention) + .mockResolvedValueOnce(baseline) + .mockRejectedValueOnce(readError) + .mockResolvedValueOnce(baseline); + render(); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + expect(screen.getByTestId(ENABLED_TEST_ID)).toBeTruthy(); + await act(async () => { + await vi.advanceTimersByTimeAsync(30_000); + }); + expect(screen.getByTestId(ERROR_TEST_ID)).toBeTruthy(); + await act(async () => { + await vi.advanceTimersByTimeAsync(30_000); + }); + expect(screen.queryByTestId(ERROR_TEST_ID)).toBeNull(); + } finally { + vi.useRealTimers(); + } +}); + +it("preserves an action failure after status recovery", async () => { + vi.useFakeTimers(); + try { + const actionError = new Error("analysis failed"); + const failedRun: ToolPayloadOperation = { + id: "failed-run", + kind: "cleanup", + state: "failed", + scanned: 0, + eligible_tasks: 0, + eligible_messages: 0, + removed_messages: 0, + payload_bytes: 0, + skipped: {}, + cutoff: "2026-01-01T00:00:00.000Z", + started_at: "2026-01-01T00:00:00.000Z", + finished_at: "2026-01-01T00:01:00.000Z", + error: "cleanup_failed", + age: baseline.policy.age, + }; + vi.mocked(api.fetchToolPayloadRetention) + .mockResolvedValueOnce(baseline) + .mockResolvedValueOnce({ ...baseline, last_run: failedRun }); + vi.mocked(api.analyzeToolPayloadRetention).mockRejectedValueOnce(actionError); + render(); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + expect(screen.getByTestId(ENABLED_TEST_ID)).toBeTruthy(); + await act(async () => { + fireEvent.click(screen.getByTestId(ANALYZE_TEST_ID)); + await Promise.resolve(); + }); + expect(screen.getByTestId(ERROR_TEST_ID)).toBeTruthy(); + await act(async () => { + await vi.advanceTimersByTimeAsync(30_000); + }); + expect(screen.getByTestId(ERROR_TEST_ID)).toBeTruthy(); + expect(screen.getByTestId(LAST_RUN_TEST_ID).textContent).toMatch(/failed/i); + } finally { + vi.useRealTimers(); + } +}); + it("requires an explicit backup choice before saving enablement and displays preparing", async () => { await open(); fireEvent.click(screen.getByTestId(ENABLED_TEST_ID)); @@ -58,7 +137,7 @@ it("analyzes a disabled draft without saving or enabling", async () => { await open(); fireEvent.change(screen.getByTestId(AGE_TEST_ID), { target: { value: "4" } }); vi.mocked(api.analyzeToolPayloadRetention).mockResolvedValue({ operation_id: "analysis-1" }); - fireEvent.click(screen.getByTestId("tool-payload-analyze")); + fireEvent.click(screen.getByTestId(ANALYZE_TEST_ID)); await waitFor(() => expect(api.analyzeToolPayloadRetention).toHaveBeenCalledWith({ value: 4, unit: "months" }), ); @@ -69,14 +148,14 @@ it("keeps member controls read only", async () => { admin = false; await open(); expect(screen.getByTestId(ENABLED_TEST_ID).hasAttribute("disabled")).toBe(true); - expect(screen.getByTestId("tool-payload-analyze").hasAttribute("disabled")).toBe(true); + expect(screen.getByTestId(ANALYZE_TEST_ID).hasAttribute("disabled")).toBe(true); expect(api.analyzeToolPayloadRetention).not.toHaveBeenCalled(); }); it("rejects invalid ages without losing the disabled default", async () => { await open(); fireEvent.change(screen.getByTestId(AGE_TEST_ID), { target: { value: "0" } }); expect(contributor.canSave).toBe(false); - expect(screen.getByTestId("tool-payload-analyze").hasAttribute("disabled")).toBe(true); + expect(screen.getByTestId(ANALYZE_TEST_ID).hasAttribute("disabled")).toBe(true); }); it("preserves edits made during a pending save against its normalized response", async () => { diff --git a/apps/web/e2e/helpers/tool-payload-retention.ts b/apps/web/e2e/helpers/tool-payload-retention.ts index 91c33e19cb..549ea20aa3 100644 --- a/apps/web/e2e/helpers/tool-payload-retention.ts +++ b/apps/web/e2e/helpers/tool-payload-retention.ts @@ -131,6 +131,82 @@ export async function resetRetention(page: Page) { expect(response.ok(), await response.text()).toBe(true); } +export async function failNextRetentionStatusRead(page: Page) { + const pattern = `**${RETENTION_API}`; + let reads = 0; + await page.route(pattern, async (route) => { + if (route.request().method() !== "GET") { + await route.continue(); + return; + } + reads += 1; + if (reads === 1) { + await route.fulfill({ + status: 503, + contentType: "application/json", + body: JSON.stringify({ code: "persistence_unavailable" }), + }); + return; + } + await route.continue(); + }); + return async () => page.unroute(pattern); +} + +export async function reloadRetentionWithFakeClock(page: Page) { + await page.clock.install(); + await page.reload(); + await expect(page.getByTestId("tool-payload-enabled")).toBeVisible(); +} + +export async function recoverRetentionStatusPolling(page: Page, preserveError = false) { + const failed = page.waitForResponse( + (response) => + new URL(response.url()).pathname === RETENTION_API && + response.request().method() === "GET" && + response.status() === 503, + ); + await page.clock.runFor(30_000); + await failed; + await expect(page.getByTestId("tool-payload-error")).toBeVisible(); + + const recovered = page.waitForResponse( + (response) => + new URL(response.url()).pathname === RETENTION_API && + response.request().method() === "GET" && + response.status() === 200, + ); + await page.clock.runFor(30_000); + await recovered; + if (preserveError) await expect(page.getByTestId("tool-payload-error")).toBeVisible(); + else await expect(page.getByTestId("tool-payload-error")).toHaveCount(0); +} + +export async function failRetentionAnalysis(page: Page, touch = false) { + const pattern = `**${RETENTION_API}/analyze`; + await page.route(pattern, async (route) => { + if (route.request().method() === "POST") { + await route.fulfill({ + status: 503, + contentType: "application/json", + body: JSON.stringify({ code: "persistence_unavailable" }), + }); + return; + } + await route.continue(); + }); + const failed = page.waitForResponse( + (response) => + new URL(response.url()).pathname === `${RETENTION_API}/analyze` && + response.request().method() === "POST" && + response.status() === 503, + ); + await press(page.getByTestId("tool-payload-analyze"), touch); + await failed; + await expect(page.getByTestId("tool-payload-error")).toBeVisible(); + return async () => page.unroute(pattern); +} + export async function press(locator: Locator, touch = false) { if (touch) await locator.tap(); else await locator.click(); diff --git a/apps/web/e2e/tests/system/mobile-tool-payload-retention.spec.ts b/apps/web/e2e/tests/system/mobile-tool-payload-retention.spec.ts index 3aac9ef792..022253bc39 100644 --- a/apps/web/e2e/tests/system/mobile-tool-payload-retention.spec.ts +++ b/apps/web/e2e/tests/system/mobile-tool-payload-retention.spec.ts @@ -3,7 +3,11 @@ import { activateRetention, analyzeRetention, expectTouchTarget, + failNextRetentionStatusRead, + failRetentionAnalysis, readRetentionMessage, + recoverRetentionStatusPolling, + reloadRetentionWithFakeClock, resetRetention, RETENTION_ROUTE, runRetention, @@ -12,6 +16,38 @@ import { } from "../../helpers/tool-payload-retention"; test.describe("Tool payload retention on phones", () => { + test("recovers a status polling error without manual refresh on a phone", async ({ + testPage: page, + }) => { + await page.setViewportSize({ width: 390, height: 844 }); + await resetRetention(page); + await page.goto(RETENTION_ROUTE); + await reloadRetentionWithFakeClock(page); + const removeStatusRoute = await failNextRetentionStatusRead(page); + try { + await recoverRetentionStatusPolling(page); + } finally { + await removeStatusRoute(); + } + }); + + test("preserves an action failure after status recovery on a phone", async ({ + testPage: page, + }) => { + await page.setViewportSize({ width: 390, height: 844 }); + await resetRetention(page); + await page.goto(RETENTION_ROUTE); + await reloadRetentionWithFakeClock(page); + const removeStatusRoute = await failNextRetentionStatusRead(page); + const removeActionRoute = await failRetentionAnalysis(page, true); + try { + await recoverRetentionStatusPolling(page, true); + } finally { + await removeActionRoute(); + await removeStatusRoute(); + } + }); + for (const width of [320, 390]) { test(`completes cleanup by touch at ${width}px without clipping`, async ({ testPage: page, diff --git a/apps/web/e2e/tests/system/tool-payload-retention.spec.ts b/apps/web/e2e/tests/system/tool-payload-retention.spec.ts index bf96f7be55..72097bbe38 100644 --- a/apps/web/e2e/tests/system/tool-payload-retention.spec.ts +++ b/apps/web/e2e/tests/system/tool-payload-retention.spec.ts @@ -5,7 +5,11 @@ import { activateRetention, analyzeRetention, PAYLOAD_TEXT, + failNextRetentionStatusRead, + failRetentionAnalysis, readRetentionMessage, + recoverRetentionStatusPolling, + reloadRetentionWithFakeClock, resetRetention, retentionStatus, RETENTION_ROUTE, @@ -15,6 +19,32 @@ import { } from "../../helpers/tool-payload-retention"; test.describe("Tool payload retention", () => { + test("recovers a status polling error without manual refresh", async ({ testPage: page }) => { + await resetRetention(page); + await page.goto(RETENTION_ROUTE); + await reloadRetentionWithFakeClock(page); + const removeStatusRoute = await failNextRetentionStatusRead(page); + try { + await recoverRetentionStatusPolling(page); + } finally { + await removeStatusRoute(); + } + }); + + test("preserves an action failure after status recovery", async ({ testPage: page }) => { + await resetRetention(page); + await page.goto(RETENTION_ROUTE); + await reloadRetentionWithFakeClock(page); + const removeStatusRoute = await failNextRetentionStatusRead(page); + const removeActionRoute = await failRetentionAnalysis(page); + try { + await recoverRetentionStatusPolling(page, true); + } finally { + await removeActionRoute(); + await removeStatusRoute(); + } + }); + for (const choice of ["backup", "skip"] as const) { test(`analyzes while disabled and cleans old payloads after explicit ${choice}`, async ({ testPage: page, diff --git a/apps/web/hooks/domains/system/use-tool-payload-retention.test.ts b/apps/web/hooks/domains/system/use-tool-payload-retention.test.ts index 4065872c35..1338757ecc 100644 --- a/apps/web/hooks/domains/system/use-tool-payload-retention.test.ts +++ b/apps/web/hooks/domains/system/use-tool-payload-retention.test.ts @@ -14,10 +14,12 @@ const status: ToolPayloadRetentionStatus = { }; function deferred() { let resolve!: (v: T) => void; - const promise = new Promise((r) => { + let reject!: (cause: unknown) => void; + const promise = new Promise((r, j) => { resolve = r; + reject = j; }); - return { promise, resolve }; + return { promise, resolve, reject }; } beforeEach(() => { vi.resetAllMocks(); @@ -91,6 +93,77 @@ it("keeps saved policy on failure and releases the mutation lock for retry", asy }); expect(result.current.error).toBeNull(); }); + +it("clears a recovered status error on background polling", async () => { + vi.useFakeTimers(); + let unmount: (() => void) | undefined; + try { + const readError = new Error("status unavailable"); + vi.mocked(api.fetchToolPayloadRetention) + .mockResolvedValueOnce(status) + .mockRejectedValueOnce(readError) + .mockResolvedValueOnce(status); + const rendered = renderHook(useToolPayloadRetention); + unmount = rendered.unmount; + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(30_000); + }); + expect(rendered.result.current.error).toBe(readError); + await act(async () => { + await vi.advanceTimersByTimeAsync(30_000); + }); + expect(rendered.result.current.error).toBeNull(); + } finally { + unmount?.(); + vi.useRealTimers(); + } +}); + +it("keeps action failures when a status read succeeds", async () => { + const { result } = renderHook(useToolPayloadRetention); + await waitFor(() => expect(result.current.status).toEqual(status)); + const actionError = new Error("analysis failed"); + vi.mocked(api.analyzeToolPayloadRetention).mockRejectedValueOnce(actionError); + await act(async () => { + await expect(result.current.analyze(status.policy.age)).rejects.toBe(actionError); + }); + expect(result.current.error).toBe(actionError); + vi.mocked(api.fetchToolPayloadRetention).mockResolvedValueOnce(status); + await act(async () => { + await result.current.reload(); + }); + expect(result.current.error).toBe(actionError); +}); + +it("does not let a stale status failure replace a mutation result", async () => { + const { result } = renderHook(useToolPayloadRetention); + await waitFor(() => expect(result.current.status).toEqual(status)); + const old = deferred(); + vi.mocked(api.fetchToolPayloadRetention).mockReturnValueOnce(old.promise); + let reload!: Promise; + act(() => { + reload = result.current.reload(); + }); + const saved = { + ...status, + policy: { ...status.policy, revision: 1, age: { value: 4, unit: "months" as const } }, + }; + vi.mocked(api.saveToolPayloadRetention).mockResolvedValue(saved); + await act(async () => { + await result.current.save(saved.policy); + }); + const staleError = new Error("stale read failed"); + await act(async () => { + old.reject(staleError); + await reload; + }); + expect(result.current.status).toEqual(saved); + expect(result.current.error).toBeNull(); +}); it("cancels the accepted operation and applies the returned status", async () => { const { result } = renderHook(useToolPayloadRetention); await waitFor(() => expect(result.current.status).toEqual(status)); diff --git a/apps/web/hooks/domains/system/use-tool-payload-retention.ts b/apps/web/hooks/domains/system/use-tool-payload-retention.ts index 82a54f9e51..8f089ee49d 100644 --- a/apps/web/hooks/domains/system/use-tool-payload-retention.ts +++ b/apps/web/hooks/domains/system/use-tool-payload-retention.ts @@ -15,7 +15,11 @@ type Lifetime = { generation: number; }; type AcceptedOperation = { id: string; kind: "analysis" | "cleanup" }; -type Updates = { pending: (value: boolean) => void; error: (cause: unknown) => void }; +type Updates = { + pending: (value: boolean) => void; + actionError: (cause: unknown) => void; + clearErrors: () => void; +}; function operationObserved(status: ToolPayloadRetentionStatus, accepted: AcceptedOperation) { if (status.operation?.id === accepted.id) return true; @@ -26,7 +30,8 @@ function operationObserved(status: ToolPayloadRetentionStatus, accepted: Accepte function loadStatus( owner: Lifetime, accept: (value: ToolPayloadRetentionStatus) => void, - fail: Updates["error"], + recover: () => void, + fail: (cause: unknown) => void, ) { if (owner.mutating) return Promise.resolve(); if (owner.reading) return owner.reading; @@ -35,7 +40,10 @@ function loadStatus( const request = api .fetchToolPayloadRetention() .then((next) => { - if (current()) accept(next); + if (current()) { + accept(next); + recover(); + } }) .catch((cause: unknown) => { if (current()) fail(cause); @@ -60,13 +68,13 @@ async function performMutation( const { epoch } = owner; const current = () => owner.mounted && owner.epoch === epoch; updates.pending(true); - updates.error(null); + updates.clearErrors(); try { const result = await request(); if (current()) accept(result); return result; } catch (cause) { - if (current()) updates.error(cause); + if (current()) updates.actionError(cause); throw cause; } finally { owner.mutating = false; @@ -113,9 +121,29 @@ function useRetentionLifetime(owner: { current: Lifetime }, reload: () => Promis }; }, [owner, reload]); } +function useRetentionMutation( + owner: { current: Lifetime }, + setPending: (value: boolean) => void, + setStatusError: (value: unknown) => void, + setActionError: (value: unknown) => void, +) { + return useCallback( + (request: () => Promise, accept: (value: T) => void) => + performMutation(owner.current, request, accept, { + pending: setPending, + actionError: setActionError, + clearErrors: () => { + setStatusError(null); + setActionError(null); + }, + }), + [owner, setActionError, setPending, setStatusError], + ); +} export function useToolPayloadRetention() { const [status, setStatus] = useState(null); - const [error, setError] = useState(null); + const [statusError, setStatusError] = useState(null); + const [actionError, setActionError] = useState(null); const [pending, setPending] = useState(false); const [acceptedId, setAcceptedId] = useState(null); const acceptedOperation = useRef(null); @@ -138,12 +166,14 @@ export function useToolPayloadRetention() { setAcceptedId(null); } }, - setError, + () => setStatusError(null), + setStatusError, ), [], ); const refresh = useCallback(() => { - setError(null); + setStatusError(null); + setActionError(null); return reload(); }, [reload]); useAcceptedOperationRefresh(acceptedId, reload); @@ -152,11 +182,7 @@ export function useToolPayloadRetention() { status?.preparation.state === "pending" || status?.preparation.state === "running"; const active = Boolean(acceptedId || preparing || status?.operation?.state === "running"); useStatusPolling(reload, active, preparing); - const perform = useCallback( - (request: () => Promise, accept: (value: T) => void) => - performMutation(owner.current, request, accept, { pending: setPending, error: setError }), - [], - ); + const perform = useRetentionMutation(owner, setPending, setStatusError, setActionError); const acceptStatus = useCallback((next: ToolPayloadRetentionStatus) => { acceptedOperation.current = null; setStatus(next); @@ -196,7 +222,7 @@ export function useToolPayloadRetention() { ); return { status, - error, + error: actionError ?? statusError, pending, active, preparing, diff --git a/docs/decisions/2026-09-17-maintenance-health-probe-coordination.md b/docs/decisions/2026-09-17-maintenance-health-probe-coordination.md new file mode 100644 index 0000000000..900d93098d --- /dev/null +++ b/docs/decisions/2026-09-17-maintenance-health-probe-coordination.md @@ -0,0 +1,47 @@ +# ADR-2026-09-17-maintenance-health-probe-coordination: Coordinate runtime health probes with maintenance + +**Status:** accepted +**Date:** 2026-09-17 +**Area:** backend + +## Context + +SQLite has one writer connection. Vacuum holds that connection while runtime +health probes need it for pings and schema checks. Probe deadlines therefore +mark every required store unhealthy during successful maintenance. + +The shared pool already has a maintenance admission guard. Required persistence +must still fail startup and retain known runtime failures. + +## Decision + +Periodic SQLite probes participate in the existing maintenance guard. They try +admission without waiting and retain the lease throughout the bounded probe. +A busy guard defers the probe without changing store states or check timestamps. +Startup checks remain strict. PostgreSQL probes remain unchanged. + +The next scheduled tick resumes full probing after maintenance releases its lease. +Deferred probes emit a bounded debug event. They never manufacture healthy state. +Destructive SQLite restore and factory-reset operations use the existing +unhealthy state to fail closed before quiescing or replacing data. Their +restart-required result keeps database-backed work disabled until a fresh +process initializes the database again. + +## Consequences + +Managed maintenance no longer causes false global persistence failures. +The guard prevents a race between checking maintenance state and taking the writer. +Actual faults that arise during maintenance can remain undetected until lease release. +Diagnostics retain the last completed check time, and previous failures remain visible. +This does not guarantee that writes complete promptly during vacuum. + +The [required persistence decision](2026-09-05-required-internal-persistence.md) +still governs startup and actual runtime failures. + +## Alternatives Considered + +- Longer timeouts cannot bound vacuum duration and retain the false-failure race. +- Reader-only checks lose the existing writer-health guarantee. +- Skipping every probe while any job runs confuses job tracking with database ownership. +- Bypassing HTTP health middleware hides actual failures and does not repair readiness. +- A separate maintenance flag duplicates the existing admission state. diff --git a/docs/plans/vacuum-compaction-status/plan.md b/docs/plans/vacuum-compaction-status/plan.md new file mode 100644 index 0000000000..48be2d8ae8 --- /dev/null +++ b/docs/plans/vacuum-compaction-status/plan.md @@ -0,0 +1,190 @@ +--- +created: 2026-09-17 +status: done +requirements: + - REQ-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007 + - REQ-SYSTEM-PAGE-TOOL-PAYLOAD-RETENTION-003 +system_design: + - ../../specs/platform/system-design/postgres-domain-store-parity.md + - ../../specs/system-page/system-design/tool-payload-retention.md +legacy_specs: [] +--- + +# Implementation Plan: Vacuum and compaction status recovery + +## Overview + +Prevent false persistence failures during managed SQLite maintenance. Clear a +recovered compaction status error without hiding a failed user action. +Implement the runtime guard first, then the independent status recovery path. +Both work orders require TDD. Implementation is complete. + +## Evidence and requirement conformance + +On September 17, the retained backend log recorded these local times: + +| Time (UTC+01:00) | Observation | +| --- | --- | +| 11:00:30 | Vacuum accepted with HTTP 202 | +| 11:00:44 | Writer ping exceeded its deadline and persistence became unhealthy | +| 11:00:51 | Compaction status GET returned HTTP 503 | +| 11:02:09 | A second vacuum was accepted | +| 11:02:36 through 11:05:07 | Status GETs returned HTTP 503 | +| 11:05:38 | Status GET recovered to HTTP 200 | + +Source: `/root/.kandev/logs/backend-logs-2026-09-17-000011.log` on the +investigated host. This path is evidence, not an implementation dependency. + +`database.Service.runVacuum` owns the shared maintenance lease and SQLite's +single writer. `requiredstores.Health` independently pings that writer with +a two-second deadline. `requiredPersistenceMiddleware` then blocks stateful +requests with `persistence_unavailable`. The card maps that response to its +generic error. `loadStatus` records GET errors but never clears them on success. + +The existing requirements describe health recovery and visible operation states. +They omit maintenance contention and read-error dismissal. This package adds +criteria to those existing capabilities and amends their technical designs. +Platform owns shared persistence health. System-page owns compaction status +recovery because it owns the policy and its action lifecycle. + +The assumption check found no unresolved product choice. Scope follows the +diagnosed failure and preserves strict startup checks, real errors, and policy. + +## Scope + +### In scope + +- Runtime probe admission through the existing SQLite maintenance guard. +- State and timestamp preservation when a periodic probe defers. +- Automatic read-error recovery with separate action-error ownership. +- Deterministic backend, hook, component, and desktop/phone browser regressions. + +### Out of scope + +- Database migrations, vacuum optimization, automatic vacuum, or payload changes. +- New public APIs, timeouts, feature flags, settings, or translated copy. +- General HTTP retry behavior or a new maintenance banner. +- Production database operations, push, or PR creation in this turn. + +## Technical approach + +The [platform design](../../specs/platform/system-design/postgres-domain-store-parity.md#sqlite-maintenance-coordination) +and [decision](../../decisions/2026-09-17-maintenance-health-probe-coordination.md) +define runtime admission. Add a narrow periodic-check boundary in `health.go`. +Use `maintenance.ForPool` and `TryAcquire` only for SQLite runtime checks. +Keep `Health.Check` strict for startup. Hold the lease for the whole check. +Keep existing middleware and readiness behavior for actual failures. + +The [compaction design](../../specs/system-page/system-design/tool-payload-retention.md#status-error-recovery) +defines error ownership. Split read and action errors inside the existing hook. +Keep the outward `error` field and existing component error translation. +Honor lifetime and generation checks before accepting or clearing any result. + +## ASCII UI preview + +### UI-01: Compaction error recovery + +Entry: Settings > System > Data & Logs > Database. +The preview shows only the affected region. Spacing is illustrative. + +```text +Current, after a GET recovers: +Messages compaction +[The operation could not complete. Refresh status and retry.] +[Refresh status] +Tasks inactive for [3] [Months v] [Analyze savings] + +Proposed, after a GET recovers: +Messages compaction +Tasks inactive for [3] [Months v] [Analyze savings] +... + +Proposed, unresolved action failure: +Messages compaction +[Existing translated action error] +[Refresh status] +Tasks inactive for [3] [Months v] [Analyze savings] +``` + +On phones, the number and unit remain adjacent. Analyze savings stays on its +own full-width row. The existing page owns scrolling and touch targets remain +44px. Error visibility follows the same hook on both viewports. No overlay, +navigation, focus, breakpoint, or layout change is planned. +The shipped `ToolPayloadRetentionCard` and its mobile E2E are the local exemplar. +The mobile guide's shared-state rule applies. A separate phone component is unnecessary. + +Required structure: remove only recovered read errors. Keep failed actions and +persisted failures visible. This maps to `AC-SYSTEM-PAGE-TOOL-PAYLOAD-RETENTION-003.7` +and `.8`, with existing phone parity criterion `.5`. + +## Tests + +| Criteria | Planned evidence | +| --- | --- | +| Platform 007.6 | `TestRuntimeHealthDefersDuringMaintenance` in `health_test.go` | +| Platform 007.7 | `TestRuntimeHealthDeferralPreservesMixedStoreStates` and `TestStartupHealthDoesNotDefer` | +| Platform 007.8, existing 007.1/.3/.4 | `TestRuntimeHealthResumesAfterMaintenance`, existing missing-table recovery, and middleware regression | +| System-page 003.7 | Hook test `clears a recovered status error on background polling` and rendered card recovery | +| System-page 003.8 | Hook tests for action-error precedence, stale GET rejection, and persisted failure preservation | +| System-page 003.5/.7/.8 | Desktop and mobile recovery scenarios in the existing retention specs | + +At planning time, the full prefixes are in the work-order frontmatter. New test names were planned, +not existing evidence. Use a held real writer connection and guard for a +deterministic backend reproduction. Do not depend on a large database making +vacuum slow. The guard fixture reproduces the proven resource conflict. + +## E2E tests + +Extend `apps/web/e2e/tests/system/tool-payload-retention.spec.ts` (`chromium`) +and `mobile-tool-payload-retention.spec.ts` (`mobile-chrome`). Add status GET +failure followed by success without Refresh status, and action failure followed +by a successful GET. Keep real route rendering and existing mutation scenarios. +Use controlled route responses and browser clock advancement for polling. +Await the specific HTTP responses before DOM assertions. No fixed sleeps. +This browser evidence covers presentation recovery. Backend tests cover the +maintenance cause without exposing production test controls. + +## Work orders + +- [x] [Task 01: Coordinate runtime health with maintenance](task-01-runtime-health.md) +- [x] [Task 02: Recover compaction status errors](task-02-status-recovery.md) + +Execute sequentially. Task 02 does not technically depend on Task 01, but both +must pass before delivery. No delegation is authorized. + +## Documentation and companion packages + +The completed [retention package](../tool-payload-retention/plan.md) and +[store parity package](../postgres-domain-store-parity/plan.md) retain their +historical results. This follow-up owns only the new criteria and regressions. +Public docs need no change for this implementation. Existing health/recovery +guidance was checked for claims affected by probe deferral. No labels change. + +## Verification results + +Implementation completed on 2026-09-17. Artifact validation on 2026-09-17: + +- `python3 scripts/list-docs.py validate`: passed (286 decisions, 986 specifications). +- `python3 scripts/lint-spec-files.test.py`: passed (36 tests). +- `python3 scripts/lint-spec-files.py --all`: passed. +- `git diff --check -- docs/specs docs/decisions docs/plans/vacuum-compaction-status`: passed. +- Backend focused race tests passed for required-store health, maintenance, + database, and persistence middleware. +- Frontend focused tests passed: 23 tests across the hook and card files. +- `pnpm run typecheck`: passed. +- Desktop retention E2E passed: 4 tests on `chromium`. +- Phone retention E2E passed: 4 tests on `mobile-chrome`, including the + existing focused phone screenshot capture at 390px. +- `git diff --check`: passed. + +Product tests and browser checks from both work orders passed. The worktree +contains the implementation and its permanent regression coverage; no push or +PR was created. + +## Risks + +- Faults that arise during a maintenance lease remain undetected until its release. + Prior unhealthy state and completed-check timestamps remain unchanged. +- Restore/reset quiescence must continue to govern process teardown and readiness. +- Clearing the shared error unconditionally would hide failed mutations. +- A guard acquired only around the ping leaves schema checks exposed to the same race. diff --git a/docs/plans/vacuum-compaction-status/task-01-runtime-health.md b/docs/plans/vacuum-compaction-status/task-01-runtime-health.md new file mode 100644 index 0000000000..495aed76bf --- /dev/null +++ b/docs/plans/vacuum-compaction-status/task-01-runtime-health.md @@ -0,0 +1,127 @@ +--- +id: "01-runtime-health" +title: "Coordinate runtime health with maintenance" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007 +acceptance_criteria: + - AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.1 + - AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.3 + - AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.4 + - AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.6 + - AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.7 + - AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.8 +system_design: + - ../../specs/platform/system-design/postgres-domain-store-parity.md +--- + +# Task 01: Coordinate runtime health with maintenance + +## Summary + +Prevent managed SQLite maintenance from causing a false unhealthy transition. +Keep startup strict and preserve actual runtime failure detection. + +## In scope + +- Add a periodic-probe boundary that obtains the existing maintenance lease. +- Defer busy SQLite probes without changing tracker state or timestamps. +- Preserve the two-second check budget, 15-second interval, and PostgreSQL path. +- Emit a debug deferral event and release acquired leases on every exit. + +## Out of scope + +- HTTP allowlist changes, writer-pool sizing, schema changes, and UI changes. +- Suppression of ordinary database failures outside managed maintenance. + +## Acceptance + +1. A held maintenance lease and writer do not change health or check timestamps. +2. A mixed healthy/unhealthy catalog retains every state during deferral. Startup never skips its check. +3. After release, full checks resume. Missing tables still cause HTTP 503 and recover after repair. + +## TDD and test design + +First add `TestRuntimeHealthDefersDuringMaintenance` in `health_test.go`. +Use a real SQLite pool, initialized tracker, maintenance lease, and held writer. +Drive the periodic-check boundary with a short test context. Before the fix, +the probe times out and records unhealthy state. Assert that failure first. + +Add `TestRuntimeHealthDeferralPreservesMixedStoreStates` with one healthy and +one unhealthy store. Compare complete snapshots, including check timestamps. +Add `TestStartupHealthDoesNotDefer` against strict `Health.Check`. +Add `TestRuntimeHealthResumesAfterMaintenance` for release, fault detection, +and repair. Assert lease release after successful and failed probes, and stop +cancellation. Keep `TestProbeTablesHonorsContextDeadline` unchanged in meaning. + +Extend `persistence_middleware_test.go` with +`TestPersistenceMiddlewareRemainsAvailableDuringManagedMaintenance`. +Exercise the periodic boundary, middleware, and real health tracker together. +Use the existing middleware harness, not a synthetic always-healthy stub. +Do not export an API solely for tests. An external-package integration test +can drive `Start` with `SetInterval` and synchronize on observable probe events. +Retain the existing unhealthy response and recovery tests. + +## Verification + +Run from the repository root: + +```bash +(cd apps/backend && go test -race ./internal/persistence/requiredstores ./internal/system/maintenance ./internal/system/database) +(cd apps/backend && go test -race ./internal/backendapp -run 'Test.*(Persistence|RequiredStore|Health|Ready)') +git diff --check +``` + +The backend Makefile's generic test target is broader than this repair. +Use these focused package commands and verify that the new test names run. + +## Files likely touched + +- `apps/backend/internal/persistence/requiredstores/health.go` +- `apps/backend/internal/persistence/requiredstores/health_test.go` +- `apps/backend/internal/backendapp/persistence_middleware_test.go` +- `apps/backend/internal/system/maintenance/guard_test.go` if lease assertions need extension + +## Dependencies + +None. Existing maintenance owners already use the shared pool guard. + +## Risks + +Do not reverse lock order. The probe takes maintenance admission before database +connections and releases it after all database work. Never hold a writer while +waiting for maintenance admission. Preserve restore/reset shutdown behavior. +The lease defers fault detection only while managed maintenance owns the pool. + +## Parallelism + +`sequential` + +## Inputs + +- [Platform requirements](../../specs/platform/requirements/postgres-domain-store-parity.md), required-store health. +- [Platform design](../../specs/platform/system-design/postgres-domain-store-parity.md), runtime health. +- [Decision](../../decisions/2026-09-17-maintenance-health-probe-coordination.md). +- Existing `health_test.go`, `guard_test.go`, and `persistence_middleware_test.go` patterns. + +## Results + +Implemented periodic SQLite health-probe admission through the shared +maintenance guard. A busy guard now defers without changing tracker state or +timestamps, while startup `Health.Check` remains strict and PostgreSQL keeps +the existing path. Added real SQLite contention coverage for mixed state +preservation, recovery, lease release, and middleware availability. + +Review follow-up also wires destructive SQLite restore and factory-reset +operations to mark required persistence unhealthy before quiescence or data +replacement. Stateful middleware therefore fails closed while the accepted +operation waits for the required frontend restart. + +Verification passed: + +- `go test -race ./internal/persistence/requiredstores ./internal/system/maintenance ./internal/system/database` +- `go test -race ./internal/backendapp -run 'Test.*(Persistence|RequiredStore|Health|Ready)'` +- `git diff --check` diff --git a/docs/plans/vacuum-compaction-status/task-02-status-recovery.md b/docs/plans/vacuum-compaction-status/task-02-status-recovery.md new file mode 100644 index 0000000000..98d6c5f47f --- /dev/null +++ b/docs/plans/vacuum-compaction-status/task-02-status-recovery.md @@ -0,0 +1,156 @@ +--- +id: "02-status-recovery" +title: "Recover compaction status errors" +status: done +wave: 2 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-SYSTEM-PAGE-TOOL-PAYLOAD-RETENTION-003 +acceptance_criteria: + - AC-SYSTEM-PAGE-TOOL-PAYLOAD-RETENTION-003.5 + - AC-SYSTEM-PAGE-TOOL-PAYLOAD-RETENTION-003.7 + - AC-SYSTEM-PAGE-TOOL-PAYLOAD-RETENTION-003.8 +system_design: + - ../../specs/system-page/system-design/tool-payload-retention.md +--- + +# Task 02: Recover compaction status errors + +## Summary + +Clear a recovered status-read error on the next successful background poll. +Keep action errors independent so a successful GET cannot hide a failed command. + +## In scope + +- Separate read and action errors in `useToolPayloadRetention`. +- Preserve its public `error` field, error translation, and manual dismissal behavior. +- Keep lifetime, generation, pending, accepted-operation, and draft protections. +- Extend hook, rendered component, desktop, and phone regression coverage. + +## Out of scope + +- New layout, user-facing copy, retry policy, polling cadence, and backend APIs. +- Automatic retries of save, analysis, cleanup, or cancellation. + +## Acceptance + +1. Failed GET then successful background GET removes the banner without clicking Refresh status. +2. Successful GET retains action errors and persisted operation failures. Stale GET results change neither errors nor policy. +3. Desktop and phone preserve policy edits, existing controls, and error recovery without duplicate mutations. + +## ASCII UI preview + +### UI-01: Compaction error recovery + +Entry: Data & Logs > Database. Full preview: [plan](plan.md#ascii-ui-preview). + +```text +Read failure: Messages compaction + [Existing error] [Refresh status] +Recovery: Messages compaction + Tasks inactive for [3] [Months v] [Analyze savings] +Action error: Messages compaction + [Existing action error] [Refresh status] +``` + +Phones retain adjacent age/unit inputs and a separate full-width Analyze savings +row. The existing route owns scrolling. Touch targets and focus behavior remain +unchanged. Banner removal is required only for recovered reads (003.7). +Action and persisted errors remain visible (003.8). Spacing is illustrative. + +## TDD and test design + +First add `clears a recovered status error on background polling` to the hook +test file. Use fake timers: initial success, failed idle poll, successful idle +poll. Assert that the current implementation leaves the error set. + +Add cases for initial GET failure, repeated failures, action-error precedence, +successful GET after failed save/analyze/run/cancel, and persisted failed status. +Use deferred responses to cover stale GET success and failure around mutations, +unmount, and remount. Preserve existing same-tick command serialization tests. +Assert that recovery creates no mutation request and does not change drafts. + +Add a rendered component regression with the real hook and mocked API module. +Do not prove recovery by directly replacing the mocked hook's error value. + +Add `recovers status polling without manual refresh` and +`preserves action failures after status recovery` to both existing E2E files. +Extract response scripting into the existing retention helper when shared. +Intercept only the intended endpoint and HTTP method. Return a real successful +status response after the injected failure. Advance the browser clock to the +next poll, then await the specific GET before checking the banner. +Use `.tap()` for phone actions. Preserve all existing cleanup and geometry cases. + +## Verification + +Run from the repository root. Install workspace dependencies once when absent. + +```bash +(cd apps && pnpm install --frozen-lockfile) +(cd apps/web && pnpm exec vitest run hooks/domains/system/use-tool-payload-retention.test.ts components/settings/system/tool-payload-retention-card.test.tsx) +(cd apps/web && pnpm run typecheck) +(cd apps/web && pnpm e2e:run --project chromium tests/system/tool-payload-retention.spec.ts) +(cd apps/web && pnpm e2e:run --project mobile-chrome tests/system/mobile-tool-payload-retention.spec.ts) +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +Use managed E2E builds and run projects sequentially. Record discovered test +counts, results, and a focused phone screenshot. Do not use production data. + +## Files likely touched + +- `apps/web/hooks/domains/system/use-tool-payload-retention.ts` +- `apps/web/hooks/domains/system/use-tool-payload-retention.test.ts` +- `apps/web/components/settings/system/tool-payload-retention-card.test.tsx` +- `apps/web/e2e/tests/system/tool-payload-retention.spec.ts` +- `apps/web/e2e/tests/system/mobile-tool-payload-retention.spec.ts` +- `apps/web/e2e/helpers/tool-payload-retention.ts` + +## Dependencies + +No code dependency on Task 01. Execute after Task 01 as the default package order. + +## Risks + +A shared unconditional `setError(null)` loses action failures. Keep error origin +explicit. Apply the existing generation guard before any error update. +Controlled browser failures prove UI recovery, not backend maintenance admission. +Task 01 supplies that independent evidence. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/system-page/requirements/tool-payload-retention.md), background operation and access. +- [Design](../../specs/system-page/system-design/tool-payload-retention.md), status error recovery. +- Existing hook, component, and retention E2E tests. + +## Results + +Implemented separate status-read and action error ownership in +`useToolPayloadRetention`. A successful background status poll now clears only +the recovered read error. Mutation failures and persisted failed-operation +states remain visible, while stale status responses continue to obey the +existing generation and operation guards. + +Added hook and rendered-card regressions for read recovery, action-error +preservation, and stale response ordering. Added controlled desktop and phone +browser scenarios that fail one status read, recover on the next poll, and +exercise touch actions on mobile. The existing phone cleanup test continues to +capture the focused 390px card screenshot. + +Verification passed: + +- `pnpm exec vitest run hooks/domains/system/use-tool-payload-retention.test.ts components/settings/system/tool-payload-retention-card.test.tsx` (23 tests) +- `pnpm run typecheck` +- `pnpm e2e:run --project chromium tests/system/tool-payload-retention.spec.ts` (4 tests) +- `pnpm e2e:run --project mobile-chrome tests/system/mobile-tool-payload-retention.spec.ts` (4 tests) +- `python3 scripts/list-docs.py validate` +- `python3 scripts/lint-spec-files.py --all` +- `git diff --check` diff --git a/docs/specs/platform/requirements/postgres-domain-store-parity.md b/docs/specs/platform/requirements/postgres-domain-store-parity.md index aed5f41b49..baf67d5078 100644 --- a/docs/specs/platform/requirements/postgres-domain-store-parity.md +++ b/docs/specs/platform/requirements/postgres-domain-store-parity.md @@ -105,6 +105,9 @@ Each required store must initialize before Kandev becomes ready. External provid - **AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.3:** When a stateful caller reaches an unhealthy backend, it shall receive `503` with the stable code `persistence_unavailable` and a recovery action. - **AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.4:** When runtime database health recovers, readiness and stateful requests shall recover without a process restart. - **AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.5:** Required-store health shall not change the liveness contract of `GET /health`. +- **AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.6:** Managed SQLite maintenance alone shall not change healthy persistence to unhealthy because it occupies the database writer. +- **AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.7:** A deferred maintenance-time health check shall preserve each store's previous state and last check time. It shall not establish startup readiness or clear an existing failure. +- **AC-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-007.8:** After maintenance releases the database, periodic health checks shall resume within the normal check interval. Actual database failures shall retain the existing readiness and caller error behavior. ### REQ-PLATFORM-POSTGRES-DOMAIN-STORE-PARITY-008: Contributor workflow diff --git a/docs/specs/platform/system-design/postgres-domain-store-parity.md b/docs/specs/platform/system-design/postgres-domain-store-parity.md index d5da8d5ce5..4e266ed46f 100644 --- a/docs/specs/platform/system-design/postgres-domain-store-parity.md +++ b/docs/specs/platform/system-design/postgres-domain-store-parity.md @@ -106,6 +106,40 @@ One probe error makes aggregate persistence unhealthy. A later successful probe Initialization errors remain fatal and cannot recover inside the same process. The process exits through the existing startup error path. +### SQLite maintenance coordination + +Periodic SQLite checks use `maintenance.ForPool(pool).TryAcquire()` before +`Health.Check`. The probe holds this lease until all checks finish. This prevents +maintenance from taking the writer between a guard check and a database probe. +The existing two-second probe context bounds the lease duration. + +When admission fails, the periodic loop defers that check until its next tick. +It does not call `RecordProbe`, change store states, or advance check timestamps. +It emits a debug event with reason `maintenance_busy`, without database paths. +An unhealthy store remains unhealthy, including alongside healthy stores. +Ordinary writer contention without a maintenance lease retains current behavior. + +Destructive SQLite restore and factory-reset operations notify required-store +health before they quiesce workers or replace/drop data. The notifier records +the existing `unhealthy` state while the maintenance lease is still held, so +stateful middleware fails closed before the operation releases admission. The +operation's `restart_required` result then requires a fresh process before +database-backed work can resume. This does not add a new health state. + +Startup calls to `Health.Check` remain strict and never use the deferral path. +Missing pools remain failures. PostgreSQL uses its current probe path. +The shared guard also covers backups, restore/reset, and retention batches. +Their existing shutdown, quiescence, and error handling remain authoritative. +There is no HTTP allowlist change or new health state. + +After lease release, the next periodic tick performs a full probe within the +normal 15-second interval. A failed operation cannot leave the lease held. +Fault detection can be delayed for the duration of managed maintenance. +Existing diagnostic timestamps expose the age of the last completed check. + +See [maintenance probe coordination](../../../decisions/2026-09-17-maintenance-health-probe-coordination.md) +and the [fix package](../../../plans/vacuum-compaction-status/plan.md). + ## Caller errors `GET /health` remains a pure liveness endpoint. Persistence state does not change its status or body. diff --git a/docs/specs/system-page/requirements/tool-payload-retention.md b/docs/specs/system-page/requirements/tool-payload-retention.md index 8c519bb363..075fb7ca9e 100644 --- a/docs/specs/system-page/requirements/tool-payload-retention.md +++ b/docs/specs/system-page/requirements/tool-payload-retention.md @@ -120,6 +120,12 @@ rules, so that I understand the first cleanup and its recovery limits. - **AC-SYSTEM-PAGE-TOOL-PAYLOAD-RETENTION-003.6:** The first version shall support SQLite. Other database engines shall show an unavailable state and reject analysis and mutation requests without side effects. +- **AC-SYSTEM-PAGE-TOOL-PAYLOAD-RETENTION-003.7:** After a status request fails, + the next successful background status request shall clear that request error + without a manual refresh. Saved policy and unsaved edits shall remain intact. +- **AC-SYSTEM-PAGE-TOOL-PAYLOAD-RETENTION-003.8:** A successful status request + shall not dismiss an unresolved user-action error or a recorded failed operation. + Recovery shall not retry mutations or enable compaction automatically. ## Out of scope @@ -132,3 +138,4 @@ rules, so that I understand the first cleanup and its recovery limits. - [System design](../system-design/tool-payload-retention.md) - [Implementation plan](../../../plans/tool-payload-retention/plan.md) +- [Vacuum and status recovery fix](../../../plans/vacuum-compaction-status/plan.md) diff --git a/docs/specs/system-page/system-design/tool-payload-retention.md b/docs/specs/system-page/system-design/tool-payload-retention.md index 4dd41c62f1..ae154ed20f 100644 --- a/docs/specs/system-page/system-design/tool-payload-retention.md +++ b/docs/specs/system-page/system-design/tool-payload-retention.md @@ -259,6 +259,29 @@ Payload bytes are not a prediction of compacted file bytes or backup duration. ## Settings surface and security +### Status error recovery + +`useToolPayloadRetention` keeps status-read errors separate from action errors. +A current successful GET clears only the status-read error. An action error +takes display precedence and survives successful background reads. Explicit +Refresh status and a new user action retain their existing dismissal behavior. +Persisted operation failures remain part of the returned status. + +The existing lifetime epoch and mutation generation checks apply to both error +channels. A stale GET cannot clear a newer error or overwrite a mutation result. +Polling continues at the existing cadence without automatic mutation retries. +The hook keeps its outward `error` contract for `RetentionError`. +Draft ownership stays in `useToolPayloadRetentionDraft`. + +The [platform health design](../../platform/system-design/postgres-domain-store-parity.md#sqlite-maintenance-coordination) +owns maintenance admission and readiness. The card does not infer database +health from a local vacuum button state or suppress all HTTP 503 errors. +Existing desktop and phone composition, focus behavior, and translated copy +remain unchanged. The [fix package](../../../plans/vacuum-compaction-status/plan.md) +defines regression coverage and the recovered-state preview. + +### Controls and permissions + Add `ToolPayloadRetentionCard` beside existing retention settings, with its own `system:tool-payload-retention` save contributor. Reuse draft/save conflict handling, job status, backup list, and `useIsAdmin` conventions.