From 4e4b29b29e7680c0ed6c5de003a8dfe7e8b2fb94 Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 14 Sep 2026 00:45:53 +0800 Subject: [PATCH] fix(office): keep budget alerts honest when a policy changes mid-check (#3532) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../office/costs/budget_idempotency_test.go | 203 ++++++-- apps/backend/internal/office/costs/budgets.go | 58 ++- .../internal/office/costs/budgets_test.go | 87 +++- .../costs/handler_budget_revision_test.go | 121 +++++ apps/backend/internal/office/costs/service.go | 9 +- apps/backend/internal/office/models/models.go | 4 + .../internal/office/repository/sqlite/base.go | 59 ++- .../repository/sqlite/base_migrations.go | 12 + .../sqlite/budget_atomicity_internal_test.go | 8 +- .../sqlite/budget_claims_postgres_test.go | 197 +++++++- .../budget_claims_recreate_internal_test.go | 156 ++++++ ...budget_exceeded_atomicity_internal_test.go | 71 +++ .../sqlite/budget_revision_fencing_test.go | 424 ++++++++++++++++ .../office/repository/sqlite/budgets.go | 238 ++++++++- .../office/repository/sqlite/budgets_test.go | 36 +- .../sqlite/loop_liveness_activation.go | 3 +- .../sqlite/run_outcome_activation.go | 47 +- .../testdata/upgrades/v0.93.0/manifest.json | 4 +- .../testdata/upgrades/v0.93.0/postgres.sql | 14 +- .../testdata/upgrades/v0.93.0/sqlite.sql | 8 + .../budget-claim-revision-fencing/plan.md | 25 + .../task-01-fence-budget-claims.md | 41 ++ .../amendments.md | 51 ++ .../input-inventory.md | 60 +++ .../prior-art.md | 77 +++ .../budget-claim-revision-fencing/spec.md | 456 ++++++++++++++++++ .../verification.md | 91 ++++ 27 files changed, 2363 insertions(+), 197 deletions(-) create mode 100644 apps/backend/internal/office/costs/handler_budget_revision_test.go create mode 100644 apps/backend/internal/office/repository/sqlite/budget_claims_recreate_internal_test.go create mode 100644 apps/backend/internal/office/repository/sqlite/budget_exceeded_atomicity_internal_test.go create mode 100644 apps/backend/internal/office/repository/sqlite/budget_revision_fencing_test.go create mode 100644 docs/plans/budget-claim-revision-fencing/plan.md create mode 100644 docs/plans/budget-claim-revision-fencing/task-01-fence-budget-claims.md create mode 100644 docs/specs/budget-claim-revision-fencing/amendments.md create mode 100644 docs/specs/budget-claim-revision-fencing/input-inventory.md create mode 100644 docs/specs/budget-claim-revision-fencing/prior-art.md create mode 100644 docs/specs/budget-claim-revision-fencing/spec.md create mode 100644 docs/specs/budget-claim-revision-fencing/verification.md diff --git a/apps/backend/internal/office/costs/budget_idempotency_test.go b/apps/backend/internal/office/costs/budget_idempotency_test.go index 1b871983ac..e5b4cfc421 100644 --- a/apps/backend/internal/office/costs/budget_idempotency_test.go +++ b/apps/backend/internal/office/costs/budget_idempotency_test.go @@ -8,6 +8,10 @@ import ( "testing" "time" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/office/costs" "github.com/kandev/kandev/internal/office/models" @@ -54,13 +58,14 @@ func newIdempotencyTestPolicy(scopeID string, limit int64) *models.BudgetPolicy } } -// monthlyPeriodKey mirrors the RFC3339-UTC month-start rendering the +// monthlyPeriodKeyAt mirrors the RFC3339-UTC month-start rendering the // service computes internally, for tests that assert on stored claim rows. -func monthlyPeriodKey() string { - now := time.Now().UTC() +func monthlyPeriodKeyAt(now time.Time) string { return time.Date(now.Year(), now.Month(), 1, 0, 0, 0, 0, time.UTC).Format(time.RFC3339) } +func monthlyPeriodKey() string { return monthlyPeriodKeyAt(time.Now().UTC()) } + func TestCheckBudget_EvaluateTwice_AlertEmitsOnce(t *testing.T) { repo, _, execSQL := newBudgetTestRepo(t) ctx := context.Background() @@ -144,6 +149,10 @@ func TestCheckBudget_ExceededClaimsCompanionAlert(t *testing.T) { // own by a rollup query, only inferred via the companion claim. insertBudgetTestCostEvent(t, execSQL, "agent-companion", "task-1", 600) + // Captured before CheckBudget so this assertion can't compute a + // different month boundary than the evaluation did if the two calls + // straddle a UTC month rollover. + periodKey := monthlyPeriodKey() results, err := svc.CheckBudget(ctx, "ws-1", "agent-companion", "proj-1") if err != nil { t.Fatalf("CheckBudget: %v", err) @@ -158,7 +167,7 @@ func TestCheckBudget_ExceededClaimsCompanionAlert(t *testing.T) { var count int if err := queryRow( `SELECT COUNT(*) FROM office_budget_claims WHERE policy_id = ? AND period_key = ? AND level = 'alert'`, - policy.ID, monthlyPeriodKey(), + policy.ID, periodKey, ).Scan(&count); err != nil { t.Fatalf("count companion claim: %v", err) } @@ -167,13 +176,58 @@ func TestCheckBudget_ExceededClaimsCompanionAlert(t *testing.T) { } } +// TestCheckBudget_ExceededClaimHeld_AlertBandEvaluationSuppressed covers +// AC-OFFICE-COSTS-003.12 directly: while an exceeded-level claim is held +// for a policy, period and revision, an evaluation whose spend reaches only +// the alert level must not emit budget.alert for that period and revision. +// The exceeded claim's companion already holds the alert-level slot, so +// this follows from the ordinary primary-key conflict on a second +// alert-level insert, with no separate read of the exceeded claim. +func TestCheckBudget_ExceededClaimHeld_AlertBandEvaluationSuppressed(t *testing.T) { + repo, _, execSQL := newBudgetTestRepo(t) + ctx := context.Background() + createBudgetTestAgent(t, repo, "ws-1", "agent-exceeded-then-alert") + agents := &repoAgents{repo: repo} + spy := &budgetActivitySpy{} + svc := costs.NewCostService(repo, logger.Default(), spy, agents, agents) + + policy := newIdempotencyTestPolicy("agent-exceeded-then-alert", 1000) + if err := svc.CreateBudgetPolicy(ctx, policy); err != nil { + t.Fatalf("create policy: %v", err) + } + + periodKey := monthlyPeriodKey() + if claimed, err := repo.ClaimExceeded(ctx, policy.ID, periodKey, policy.Revision); err != nil || !claimed { + t.Fatalf("seed exceeded+companion claim: claimed=%v err=%v", claimed, err) + } + + // Spend in the alert band only: >= 80% of the 1000 limit, below it. + insertBudgetTestCostEvent(t, execSQL, "agent-exceeded-then-alert", "task-1", 900) + + results, err := svc.CheckBudget(ctx, "ws-1", "agent-exceeded-then-alert", "proj-1") + if err != nil { + t.Fatalf("CheckBudget: %v", err) + } + if !results[0].AlertFired { + t.Fatal("expected spend to reach the alert level") + } + if results[0].AlertSubmitted { + t.Fatal("budget.alert must not be submitted while an exceeded-level claim is held for this policy, period and revision") + } + if got := spy.count("budget.alert"); got != 0 { + t.Fatalf("budget.alert emissions = %d, want 0", got) + } +} + // TestCheckBudget_CompanionAlertClaimFault_StillSubmitsExceeded covers the -// AC-OFFICE-COSTS-002.5 fault-injection carve-out: when the companion -// alert-level claim errors, the evaluation still submits budget.exceeded -// (already earned by the exceeded-level claim) and the failure is logged -// and counted like any other claim-store error. The test deliberately does -// NOT assert the companion claim was recorded, per the spec's own -// instruction — that is the failure this test injects. +// AC-OFFICE-COSTS-003.10 fault-injection case: when the companion +// alert-level insert errors, the whole atomic pair rolls back — no exceeded +// claim row survives either — but the evaluation still submits +// budget.exceeded (already earned by the exceeded-level insert winning +// before the rollback) and the pair failure is logged and counted exactly +// once, not once per row. The test deliberately does NOT assert the +// companion claim was recorded, per the spec's own instruction — that is +// the failure this test injects. func TestCheckBudget_CompanionAlertClaimFault_StillSubmitsExceeded(t *testing.T) { repo, _, execSQL := newBudgetTestRepo(t) ctx := context.Background() @@ -191,6 +245,7 @@ func TestCheckBudget_CompanionAlertClaimFault_StillSubmitsExceeded(t *testing.T) } insertBudgetTestCostEvent(t, execSQL, "agent-companion-fault", "task-1", 600) + before := claimFailureCount(t) results, err := svc.CheckBudget(ctx, "ws-1", "agent-companion-fault", "proj-1") if err != nil { t.Fatalf("CheckBudget: %v", err) @@ -201,6 +256,9 @@ func TestCheckBudget_CompanionAlertClaimFault_StillSubmitsExceeded(t *testing.T) if got := spy.count("budget.exceeded"); got != 1 { t.Fatalf("budget.exceeded submissions = %d, want 1", got) } + if delta := claimFailureCount(t) - before; delta != 1 { + t.Fatalf("budget_claim_failures_total delta = %d, want 1 (the pair is one claim attempt)", delta) + } } // TestCheckBudget_ClaimStoreFault_EmitsAndReportsSubmittedTrue covers @@ -246,12 +304,14 @@ func TestCheckBudget_ClaimStoreFault_EmitsAndReportsSubmittedTrue(t *testing.T) } } -// TestCheckBudget_ClaimStoreFault_ExceededPath_EmitsAndCountsTwicePerEval +// TestCheckBudget_ClaimStoreFault_ExceededPath_EmitsAndCountsOncePerEval // extends TestCheckBudget_ClaimStoreFault_EmitsAndReportsSubmittedTrue to the -// exceeded level: claimAndEmit's afterClaim hook claims the alert-level -// companion right after the exceeded-level claim, so a broken store fails -// both claim attempts once per evaluation (AC-OFFICE-COSTS-002.14). -func TestCheckBudget_ClaimStoreFault_ExceededPath_EmitsAndCountsTwicePerEval(t *testing.T) { +// exceeded level: AC-OFFICE-COSTS-003.10 makes the exceeded claim and its +// alert-level companion one atomic attempt, so a broken store fails the +// whole pair exactly once per evaluation — one log line, one counter +// increment (AC-OFFICE-COSTS-003.14 amends AC-OFFICE-COSTS-002.14 to say +// so), not once per row as it would if the two claims were independent. +func TestCheckBudget_ClaimStoreFault_ExceededPath_EmitsAndCountsOncePerEval(t *testing.T) { repo, _, execSQL := newBudgetTestRepo(t) ctx := context.Background() createBudgetTestAgent(t, repo, "ws-1", "agent-fault-exc") @@ -283,9 +343,71 @@ func TestCheckBudget_ClaimStoreFault_ExceededPath_EmitsAndCountsTwicePerEval(t * if got := spy.count("budget.exceeded"); got != 2 { t.Fatalf("budget.exceeded = %d, want 2 (duplicates permitted under broken store)", got) } - // exceeded claim + companion alert claim both fail => 2 increments per evaluation. - if delta := claimFailureCount(t) - before; delta != 4 { - t.Fatalf("budget_claim_failures_total delta = %d, want 4 (2 per exceeded eval)", delta) + if delta := claimFailureCount(t) - before; delta != 2 { + t.Fatalf("budget_claim_failures_total delta = %d, want 2 (1 per exceeded eval, the pair is one claim attempt)", delta) + } +} + +// TestCheckBudget_ClaimStoreFault_LogsLevelFieldMatchingEmission covers +// AC-OFFICE-COSTS-003.14: the claim-store failure log's level field names +// the level whose emission the failed claim governs — "alert" for a +// standalone alert-band claim, "exceeded" for the atomic pair — never a +// sentinel shared by both. +func TestCheckBudget_ClaimStoreFault_LogsLevelFieldMatchingEmission(t *testing.T) { + core, logs := observer.New(zapcore.ErrorLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("create observer logger: %v", err) + } + injected := errors.New("injected claim store failure") + + repo, _, execSQL := newBudgetTestRepo(t) + ctx := context.Background() + agents := &repoAgents{repo: repo} + spy := &budgetActivitySpy{} + + createBudgetTestAgent(t, repo, "ws-1", "agent-log-alert") + alertFaulty := &claimFaultRepo{Repository: repo, failLevels: map[string]error{"alert": injected}} + alertSvc := costs.NewCostService(alertFaulty, log, spy, agents, agents) + alertPolicy := newIdempotencyTestPolicy("agent-log-alert", 1000) + if err := alertSvc.CreateBudgetPolicy(ctx, alertPolicy); err != nil { + t.Fatalf("create alert policy: %v", err) + } + insertBudgetTestCostEvent(t, execSQL, "agent-log-alert", "task-1", 850) + if _, err := alertSvc.CheckBudget(ctx, "ws-1", "agent-log-alert", "proj-1"); err != nil { + t.Fatalf("CheckBudget (alert): %v", err) + } + + createBudgetTestAgent(t, repo, "ws-1", "agent-log-exceeded") + exceededFaulty := &claimFaultRepo{Repository: repo, failLevels: map[string]error{"exceeded": injected}} + exceededSvc := costs.NewCostService(exceededFaulty, log, spy, agents, agents) + exceededPolicy := newIdempotencyTestPolicy("agent-log-exceeded", 500) + if err := exceededSvc.CreateBudgetPolicy(ctx, exceededPolicy); err != nil { + t.Fatalf("create exceeded policy: %v", err) + } + insertBudgetTestCostEvent(t, execSQL, "agent-log-exceeded", "task-1", 600) + if _, err := exceededSvc.CheckBudget(ctx, "ws-1", "agent-log-exceeded", "proj-1"); err != nil { + t.Fatalf("CheckBudget (exceeded): %v", err) + } + + entries := logs.FilterMessage("budget claim store failure").All() + if len(entries) != 2 { + t.Fatalf("claim-store-failure log entries = %d, want 2", len(entries)) + } + var sawAlert, sawExceeded bool + for _, e := range entries { + switch e.ContextMap()["level"] { + case "alert": + sawAlert = true + case "exceeded": + sawExceeded = true + } + } + if !sawAlert { + t.Fatal(`expected a log entry with level="alert" for the standalone alert-band claim failure`) + } + if !sawExceeded { + t.Fatal(`expected a log entry with level="exceeded" for the atomic pair's claim failure`) } } @@ -446,34 +568,6 @@ func TestCheckBudget_ConcurrentEvaluation_EmitsOnce(t *testing.T) { } } -// TestCheckBudgetAndEvaluateProjectBudget_ShareAlertClaim covers the two -// callers that can evaluate a project policy. They must use the same durable -// claim so a cost event followed by task reassignment emits one alert. -func TestCheckBudgetAndEvaluateProjectBudget_ShareAlertClaim(t *testing.T) { - spy := &budgetActivitySpy{} - svc, _, execSQL := newBudgetTestServiceWithActivity(t, spy) - ctx := context.Background() - - policy := newIdempotencyTestPolicy("proj-shared-claim", 1000) - policy.ScopeType = "project" - if err := svc.CreateBudgetPolicy(ctx, policy); err != nil { - t.Fatalf("create policy: %v", err) - } - insertBudgetTestTask(t, execSQL, "task-shared-claim", "ws-1", "proj-shared-claim") - insertBudgetTestCostEvent(t, execSQL, "agent-shared-claim", "task-shared-claim", 850) - - if _, err := svc.CheckBudget(ctx, "ws-1", "agent-shared-claim", "proj-shared-claim"); err != nil { - t.Fatalf("CheckBudget: %v", err) - } - if err := svc.EvaluateProjectBudget(ctx, "ws-1", "proj-shared-claim"); err != nil { - t.Fatalf("EvaluateProjectBudget: %v", err) - } - - if got := spy.count("budget.alert"); got != 1 { - t.Fatalf("budget.alert submissions across callers = %d, want 1", got) - } -} - // TestCheckBudget_PeriodKeyMatchesSpendBoundary covers AC-OFFICE-COSTS-002.6a: // the claim recorded for a monthly policy is keyed to the same window // boundary the spend rollup used, rendered as RFC3339 UTC. @@ -490,9 +584,14 @@ func TestCheckBudget_PeriodKeyMatchesSpendBoundary(t *testing.T) { } insertBudgetTestCostEvent(t, execSQL, "agent-period", "task-1", 850) + // The evaluation reads the clock once. Accept the month containing either + // endpoint because a test can straddle a UTC month rollover while the + // evaluation runs. + before := time.Now().UTC() if _, err := svc.CheckBudget(ctx, "ws-1", "agent-period", "proj-1"); err != nil { t.Fatalf("CheckBudget: %v", err) } + after := time.Now().UTC() var gotKey string if err := queryRow( @@ -500,8 +599,12 @@ func TestCheckBudget_PeriodKeyMatchesSpendBoundary(t *testing.T) { ).Scan(&gotKey); err != nil { t.Fatalf("query claim period_key: %v", err) } - if gotKey != monthlyPeriodKey() { - t.Fatalf("claim period_key = %q, want %q (must match the spend rollup's boundary)", gotKey, monthlyPeriodKey()) + wantKeys := map[string]struct{}{ + monthlyPeriodKeyAt(before): {}, + monthlyPeriodKeyAt(after): {}, + } + if _, ok := wantKeys[gotKey]; !ok { + t.Fatalf("claim period_key = %q, want one of %v (must match the spend rollup's boundary)", gotKey, wantKeys) } } @@ -554,7 +657,7 @@ func TestCheckBudget_SpendBelowLevelKeepsExistingClaim(t *testing.T) { } periodKey := monthlyPeriodKey() - if claimed, err := repo.Claim(ctx, policy.ID, periodKey, "alert"); err != nil || !claimed { + if claimed, err := repo.Claim(ctx, policy.ID, periodKey, "alert", policy.Revision); err != nil || !claimed { t.Fatalf("seed claim: claimed=%v err=%v", claimed, err) } @@ -564,7 +667,7 @@ func TestCheckBudget_SpendBelowLevelKeepsExistingClaim(t *testing.T) { t.Fatalf("CheckBudget: %v", err) } - claimed, err := repo.Claim(ctx, policy.ID, periodKey, "alert") + claimed, err := repo.Claim(ctx, policy.ID, periodKey, "alert", policy.Revision) if err != nil { t.Fatalf("re-claim: %v", err) } diff --git a/apps/backend/internal/office/costs/budgets.go b/apps/backend/internal/office/costs/budgets.go index 8d88524a80..797fb74385 100644 --- a/apps/backend/internal/office/costs/budgets.go +++ b/apps/backend/internal/office/costs/budgets.go @@ -138,43 +138,27 @@ func (s *CostService) evaluatePolicy( if policy.ActionOnExceed == budgetActionPauseAgent && policy.ScopeType == scopeAgent { result.AgentPaused = s.pauseAgentForBudget(ctx, policy.ScopeID) } - result.ExceededSubmitted = s.claimAndEmit(ctx, workspaceID, policy, spent, periodKey, claimLevelExceeded, - // A spend that jumps straight from below the alert level to - // above the limit must also hold the alert-level claim before - // budget.exceeded is emitted, so a concurrent evaluation - // landing in the alert band cannot win that claim in the gap - // and emit a budget.alert describing a reduction in spend. The - // outcome is discarded (claimed or already-claimed are equally - // fine here); a claim-store error is still logged and counted - // like any other. - func() { s.claimCompanionAlert(ctx, policy, periodKey) }) + result.ExceededSubmitted = s.claimExceededAndEmit(ctx, workspaceID, policy, spent, periodKey) case spent >= threshold: result.AlertFired = true - result.AlertSubmitted = s.claimAndEmit(ctx, workspaceID, policy, spent, periodKey, claimLevelAlert, nil) + result.AlertSubmitted = s.claimAndEmit(ctx, workspaceID, policy, spent, periodKey, claimLevelAlert) } return result, nil } // claimAndEmit resolves the three outcomes of claiming (policy, periodKey, -// level) and emits that level's activity row when the claim allows it. -// Reports whether this evaluation submitted the row. afterClaim, when -// non-nil, runs immediately after the claim and before the emission -// decision, regardless of the claim's outcome — the seam the exceeded -// level's alert-level companion claim uses so both claims land before -// either emission is decided. +// level) at the policy's current revision, and emits that level's activity +// row when the claim allows it. Reports whether this evaluation submitted +// the row. func (s *CostService) claimAndEmit( ctx context.Context, workspaceID string, policy *BudgetPolicy, spent int64, periodKey, level string, - afterClaim func(), ) bool { - claimed, err := s.repo.Claim(ctx, policy.ID, periodKey, level) - if afterClaim != nil { - afterClaim() - } + claimed, err := s.repo.Claim(ctx, policy.ID, periodKey, level, policy.Revision) if err != nil { s.recordClaimFailure(policy.ID, level, err) s.emitLevel(ctx, workspaceID, policy, spent, level) @@ -187,15 +171,29 @@ func (s *CostService) claimAndEmit( return true } -// claimCompanionAlert records the alert-level claim alongside an -// exceeded-level emission, so a spend that jumps straight past the limit -// still de-escalates correctly. Its outcome never changes the -// budget.exceeded emission; only a claim-store error is reported, exactly -// like any other claim attempt. -func (s *CostService) claimCompanionAlert(ctx context.Context, policy *BudgetPolicy, periodKey string) { - if _, err := s.repo.Claim(ctx, policy.ID, periodKey, claimLevelAlert); err != nil { - s.recordClaimFailure(policy.ID, claimLevelAlert, err) +// claimExceededAndEmit resolves the atomic exceeded-plus-companion claim +// pair at the policy's current revision and emits budget.exceeded when the +// exceeded-level row was won. A claim-store error anywhere in the pair — +// including on the companion insert — fails the whole pair open: the error +// is logged and counted once, and budget.exceeded still emits. +func (s *CostService) claimExceededAndEmit( + ctx context.Context, + workspaceID string, + policy *BudgetPolicy, + spent int64, + periodKey string, +) bool { + claimed, err := s.repo.ClaimExceeded(ctx, policy.ID, periodKey, policy.Revision) + if err != nil { + s.recordClaimFailure(policy.ID, claimLevelExceeded, err) + s.emitLevel(ctx, workspaceID, policy, spent, claimLevelExceeded) + return true } + if !claimed { + return false + } + s.emitLevel(ctx, workspaceID, policy, spent, claimLevelExceeded) + return true } func (s *CostService) emitLevel( diff --git a/apps/backend/internal/office/costs/budgets_test.go b/apps/backend/internal/office/costs/budgets_test.go index ff0c0c3ad9..b3d5d12790 100644 --- a/apps/backend/internal/office/costs/budgets_test.go +++ b/apps/backend/internal/office/costs/budgets_test.go @@ -74,14 +74,34 @@ type claimFaultRepo struct { missLevels map[string]bool } -func (r *claimFaultRepo) Claim(ctx context.Context, policyID, periodKey, level string) (bool, error) { +func (r *claimFaultRepo) Claim(ctx context.Context, policyID, periodKey, level string, revision int64) (bool, error) { if err, ok := r.failLevels[level]; ok { return false, err } if r.missLevels[level] { return false, nil } - return r.Repository.Claim(ctx, policyID, periodKey, level) + return r.Repository.Claim(ctx, policyID, periodKey, level, revision) +} + +// ClaimExceeded overrides the atomic pair the same way Claim does, keyed by +// the level whose outcome is being faulted: "exceeded" fails or misses the +// pair before either insert is attempted (mirroring a fenced exceeded +// insert refused or store-faulted); "alert" fails or misses only the +// companion half, after a real exceeded insert has already run for real, +// mirroring AC-OFFICE-COSTS-003.10's atomic rollback of the whole pair on a +// companion-side error. +func (r *claimFaultRepo) ClaimExceeded(ctx context.Context, policyID, periodKey string, revision int64) (bool, error) { + if err, ok := r.failLevels["exceeded"]; ok { + return false, err + } + if r.missLevels["exceeded"] { + return false, nil + } + if err, ok := r.failLevels["alert"]; ok { + return false, err + } + return r.Repository.ClaimExceeded(ctx, policyID, periodKey, revision) } // callOrderRecorder records events in the order they occur, for tests that @@ -113,12 +133,24 @@ type orderRecordingRepo struct { order *callOrderRecorder } -func (r *orderRecordingRepo) Claim(ctx context.Context, policyID, periodKey, level string) (bool, error) { - claimed, err := r.Repository.Claim(ctx, policyID, periodKey, level) +func (r *orderRecordingRepo) Claim(ctx context.Context, policyID, periodKey, level string, revision int64) (bool, error) { + claimed, err := r.Repository.Claim(ctx, policyID, periodKey, level, revision) r.order.record("claim:" + level) return claimed, err } +// ClaimExceeded records both of the atomic pair's levels immediately after +// the (single) real call returns, preserving the ordering property the test +// cares about: both claim events precede the emission decision. It cannot +// observe the two inserts individually from outside the transaction, but it +// doesn't need to — only their position relative to the emission matters. +func (r *orderRecordingRepo) ClaimExceeded(ctx context.Context, policyID, periodKey string, revision int64) (bool, error) { + claimed, err := r.Repository.ClaimExceeded(ctx, policyID, periodKey, revision) + r.order.record("claim:exceeded") + r.order.record("claim:alert") + return claimed, err +} + // orderRecordingActivity implements shared.ActivityLogger and records each // submitted action into the same callOrderRecorder as orderRecordingRepo, // so a test can assert the interleaving of claims and emissions. @@ -260,7 +292,7 @@ func insertBudgetTestCostEvent(t *testing.T, execSQL func(string, ...interface{} now := time.Now().UTC().Format(time.RFC3339) execSQL( `INSERT INTO office_cost_events (id, agent_profile_id, task_id, cost_subcents, occurred_at, created_at) - VALUES (?, ?, ?, ?, ?, ?)`, + VALUES (?, ?, ?, ?, ?, ?)`, uuid.NewString(), agentID, taskID, costSubcents, now, now, ) } @@ -283,13 +315,12 @@ func insertBudgetTestCostEventAt( func TestCheckBudget_PeriodWindowsExcludeOlderSpend(t *testing.T) { cases := []struct { - name string - period models.BudgetPeriod - olderBy time.Duration - wantLimit bool + name string + period models.BudgetPeriod + older time.Duration }{ - {name: "daily", period: models.BudgetPeriodDaily, olderBy: 48 * time.Hour}, - {name: "yearly", period: models.BudgetPeriodYearly, olderBy: 400 * 24 * time.Hour}, + {name: "daily", period: models.BudgetPeriodDaily, older: 48 * time.Hour}, + {name: "yearly", period: models.BudgetPeriodYearly, older: 400 * 24 * time.Hour}, } for _, tc := range cases { @@ -311,7 +342,7 @@ func TestCheckBudget_PeriodWindowsExcludeOlderSpend(t *testing.T) { t.Fatalf("create policy: %v", err) } - insertBudgetTestCostEventAt(t, execSQL, "agent-1", "task-1", 600, time.Now().UTC().Add(-tc.olderBy)) + insertBudgetTestCostEventAt(t, execSQL, "agent-1", "task-1", 600, time.Now().UTC().Add(-tc.older)) results, err := svc.CheckBudget(ctx, "ws-1", "agent-1", "project-1") if err != nil { t.Fatalf("CheckBudget: %v", err) @@ -319,8 +350,8 @@ func TestCheckBudget_PeriodWindowsExcludeOlderSpend(t *testing.T) { if len(results) != 1 { t.Fatalf("results = %d, want 1", len(results)) } - if results[0].LimitExceed != tc.wantLimit { - t.Errorf("LimitExceed = %t, want %t", results[0].LimitExceed, tc.wantLimit) + if results[0].LimitExceed { + t.Fatalf("%s spend outside the window must not exceed the limit", tc.name) } }) } @@ -593,6 +624,34 @@ func TestEvaluateProjectBudget_AlertAtThreshold(t *testing.T) { } } +// TestCheckBudgetAndEvaluateProjectBudget_ShareAlertClaim covers the two +// callers that can evaluate a project policy. They must use the same durable +// claim so a cost event followed by task reassignment emits one alert. +func TestCheckBudgetAndEvaluateProjectBudget_ShareAlertClaim(t *testing.T) { + spy := &budgetActivitySpy{} + svc, _, execSQL := newBudgetTestServiceWithActivity(t, spy) + ctx := context.Background() + + policy := newIdempotencyTestPolicy("proj-shared-claim", 1000) + policy.ScopeType = models.BudgetScopeProject + if err := svc.CreateBudgetPolicy(ctx, policy); err != nil { + t.Fatalf("create policy: %v", err) + } + insertBudgetTestTask(t, execSQL, "task-shared-claim", "ws-1", "proj-shared-claim") + insertBudgetTestCostEvent(t, execSQL, "agent-shared-claim", "task-shared-claim", 850) + + if _, err := svc.CheckBudget(ctx, "ws-1", "agent-shared-claim", "proj-shared-claim"); err != nil { + t.Fatalf("CheckBudget: %v", err) + } + if err := svc.EvaluateProjectBudget(ctx, "ws-1", "proj-shared-claim"); err != nil { + t.Fatalf("EvaluateProjectBudget: %v", err) + } + + if got := spy.count("budget.alert"); got != 1 { + t.Fatalf("budget.alert submissions across callers = %d, want 1", got) + } +} + // TestEvaluateProjectBudget_ExceededAtLimit covers the limit-exceeded branch. func TestEvaluateProjectBudget_ExceededAtLimit(t *testing.T) { spy := &budgetActivitySpy{} diff --git a/apps/backend/internal/office/costs/handler_budget_revision_test.go b/apps/backend/internal/office/costs/handler_budget_revision_test.go new file mode 100644 index 0000000000..10972648b5 --- /dev/null +++ b/apps/backend/internal/office/costs/handler_budget_revision_test.go @@ -0,0 +1,121 @@ +package costs_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/kandev/kandev/internal/office/costs" +) + +// newBudgetTestRouter mounts the budget routes over a fresh CostService, for +// tests that need to observe the HTTP-level wire contract rather than the +// service API directly. +func newBudgetTestRouter(t *testing.T) *gin.Engine { + t.Helper() + svc, _, _ := newBudgetTestService(t) + gin.SetMode(gin.TestMode) + router := gin.New() + costs.NewHandler(svc).RegisterRoutes(router.Group("/api/v1")) + return router +} + +func doBudgetRequest(t *testing.T, router *gin.Engine, method, path, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec +} + +// TestCreateBudget_ResponseCarriesServerAssignedRevision covers +// AC-OFFICE-COSTS-003.2 and .4 on the wire: the 201 response's budget +// carries "revision": 1, never the column default leaking through as 0, +// and a client-presented revision in the create body has no effect (the +// request DTO has no such field, so this is enforced structurally, but the +// response must still report what the row actually holds). +func TestCreateBudget_ResponseCarriesServerAssignedRevision(t *testing.T) { + router := newBudgetTestRouter(t) + + rec := doBudgetRequest(t, router, http.MethodPost, "/api/v1/workspaces/ws-1/budgets", `{ + "scope_type": "workspace", + "limit_subcents": 1000, + "period": "monthly", + "alert_threshold_pct": 80, + "action_on_exceed": "notify_only", + "revision": 999 + }`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 (body: %s)", rec.Code, rec.Body.String()) + } + + var body struct { + Budget struct { + ID string `json:"id"` + Revision int64 `json:"revision"` + } `json:"budget"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body %q: %v", rec.Body.String(), err) + } + if body.Budget.Revision != 1 { + t.Fatalf("create response revision = %d, want 1 (a presented 999 must not survive)", body.Budget.Revision) + } + if !strings.Contains(rec.Body.String(), `"revision"`) { + t.Fatalf("response body must serialize revision as a visible field, got %s", rec.Body.String()) + } +} + +// TestUpdateBudget_ClientRevisionFieldHasNoEffect covers +// AC-OFFICE-COSTS-003.7: the update request body has no revision field, so +// a client-supplied "revision" in the PATCH JSON is inert, and the response +// reports the server's own bump (2, after the create's 1) regardless of +// what the caller sent. +func TestUpdateBudget_ClientRevisionFieldHasNoEffect(t *testing.T) { + router := newBudgetTestRouter(t) + + createRec := doBudgetRequest(t, router, http.MethodPost, "/api/v1/workspaces/ws-1/budgets", `{ + "scope_type": "workspace", + "limit_subcents": 1000, + "period": "monthly", + "alert_threshold_pct": 80, + "action_on_exceed": "notify_only" + }`) + var created struct { + Budget struct { + ID string `json:"id"` + } `json:"budget"` + } + if err := json.Unmarshal(createRec.Body.Bytes(), &created); err != nil { + t.Fatalf("decode create body %q: %v", createRec.Body.String(), err) + } + + updateRec := doBudgetRequest(t, router, http.MethodPatch, "/api/v1/budgets/"+created.Budget.ID, `{ + "limit_subcents": 2000, + "revision": 999999 + }`) + if updateRec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", updateRec.Code, updateRec.Body.String()) + } + + var updated struct { + Budget struct { + Revision int64 `json:"revision"` + LimitSubcents int64 `json:"limit_subcents"` + } `json:"budget"` + } + if err := json.Unmarshal(updateRec.Body.Bytes(), &updated); err != nil { + t.Fatalf("decode update body %q: %v", updateRec.Body.String(), err) + } + if updated.Budget.Revision != 2 { + t.Fatalf("updated revision = %d, want 2 (a presented 999999 must not survive)", updated.Budget.Revision) + } + if updated.Budget.LimitSubcents != 2000 { + t.Fatalf("limit_subcents = %d, want 2000", updated.Budget.LimitSubcents) + } +} diff --git a/apps/backend/internal/office/costs/service.go b/apps/backend/internal/office/costs/service.go index bf74c8d17a..a620630eef 100644 --- a/apps/backend/internal/office/costs/service.go +++ b/apps/backend/internal/office/costs/service.go @@ -44,8 +44,13 @@ type Repository interface { GetWorkspaceBudgetDefault(ctx context.Context, workspaceID string) (limitSubcents int64, found bool, err error) SetWorkspaceBudgetDefault(ctx context.Context, workspaceID string, limitSubcents int64) error // Claim atomically records that (policyID, periodKey, level) may emit its - // budget notification. See docs/specs/office/requirements/costs.md. - Claim(ctx context.Context, policyID, periodKey, level string) (bool, error) + // budget notification, fenced to revision. See + // docs/specs/budget-claim-revision-fencing/spec.md. + Claim(ctx context.Context, policyID, periodKey, level string, revision int64) (bool, error) + // ClaimExceeded atomically records the exceeded-level claim and, only + // when this call wins it, the alert-level companion claim, both fenced + // to revision. See docs/specs/budget-claim-revision-fencing/spec.md. + ClaimExceeded(ctx context.Context, policyID, periodKey string, revision int64) (bool, error) } // CostService handles cost recording, summaries, and budget evaluation. diff --git a/apps/backend/internal/office/models/models.go b/apps/backend/internal/office/models/models.go index e5e5b95f93..8e3a81398d 100644 --- a/apps/backend/internal/office/models/models.go +++ b/apps/backend/internal/office/models/models.go @@ -375,6 +375,10 @@ type BudgetPolicy struct { ActionOnExceed BudgetActionOnExceed `json:"action_on_exceed" db:"action_on_exceed"` CreatedAt time.Time `json:"created_at" db:"created_at"` UpdatedAt time.Time `json:"updated_at" db:"updated_at"` + // Revision identifies which immutable set of the fields above a budget + // claim was evaluated against (REQ-OFFICE-COSTS-003). Server-assigned: + // starts at 1 and increases by exactly 1 on each successful update. + Revision int64 `json:"revision" db:"revision"` } // SpendWindow reports one scope's priced spend and degradation state for a diff --git a/apps/backend/internal/office/repository/sqlite/base.go b/apps/backend/internal/office/repository/sqlite/base.go index d0088a0a67..2e5b18498a 100644 --- a/apps/backend/internal/office/repository/sqlite/base.go +++ b/apps/backend/internal/office/repository/sqlite/base.go @@ -76,6 +76,24 @@ type Repository struct { // before the policy row update, so a test can prove the two are // transactional without a fault-injecting driver. failBudgetPolicyUpdateErr error + + // failBudgetClaimsRecreateErr is a test-only failpoint: when set, + // recreateBudgetClaimsForRevision returns it instead of touching the + // database, so a test can prove initSchema surfaces the error rather + // than swallowing it. + failBudgetClaimsRecreateErr error + + // failBudgetClaimsRecreateAfterDropErr is a test-only failpoint: when + // set, recreateBudgetClaimsForRevision returns it after DROP TABLE has + // run but before CREATE TABLE, inside the same transaction, so a test + // can prove the two are atomic rather than two independent statements. + failBudgetClaimsRecreateAfterDropErr error + + // failBudgetExceededCompanionErr is a test-only failpoint: when set, + // ClaimExceeded returns it after the exceeded-level insert has run but + // before the companion alert-level insert, so a test can prove the + // pair rolls back together. + failBudgetExceededCompanionErr error } // NewWithDB creates a new office repository with existing database connections. @@ -126,6 +144,12 @@ func (r *Repository) initSchema() error { if err := r.createCoreTables(); err != nil { return err } + // Runs after createCoreTables (which creates office_budget_claims) so + // it probes the real table rather than racing its creation. Its error + // is returned, not swallowed: see recreateBudgetClaimsForRevision. + if err := r.recreateBudgetClaimsForRevision(); err != nil { + return fmt.Errorf("recreate office_budget_claims: %w", err) + } if err := r.createExtensionTables(); err != nil { return err } @@ -338,7 +362,8 @@ func (r *Repository) createCostTables() error { alert_threshold_pct INTEGER DEFAULT 80, action_on_exceed TEXT DEFAULT 'notify_only', created_at TIMESTAMP NOT NULL, - updated_at TIMESTAMP NOT NULL + updated_at TIMESTAMP NOT NULL, + revision INTEGER NOT NULL DEFAULT 1 ); -- Built-in default spend ceiling (REQ-OFFICE-BUDGET-003): a stable @@ -349,26 +374,34 @@ func (r *Repository) createCostTables() error { limit_subcents INTEGER NOT NULL, updated_at TIMESTAMP NOT NULL ); + ` + budgetClaimsDDL) + return err +} - -- office_budget_claims must be created after office_budget_policies: - -- its foreign key references that table, and PostgreSQL rejects a - -- forward reference at CREATE TABLE time even though SQLite tolerates - -- it. One row per (policy, evaluation period, level) that has already - -- produced its budget.alert / budget.exceeded notification; the - -- primary key is the concurrency control for a claim attempt. - -- ON DELETE CASCADE removes a policy's claims on every deletion path - -- without a matching code change at any of them. +// budgetClaimsDDL is the final office_budget_claims shape: one row per +// (policy, evaluation period, level, policy revision) that has already +// produced its budget.alert / budget.exceeded notification. revision is +// part of the primary key, not just a fence input, so a claim written +// against a revision a concurrent update has already superseded can never +// match a later evaluation's fenced insert. Used both for fresh-database +// creation here and for the recreate in recreateBudgetClaimsForRevision, so +// a fresh database and a migrated one converge. office_budget_claims must +// be created after office_budget_policies: its foreign key references that +// table, and PostgreSQL rejects a forward reference at CREATE TABLE time +// even though SQLite tolerates it. ON DELETE CASCADE removes a policy's +// claims on every deletion path without a matching code change at any of +// them. +const budgetClaimsDDL = ` CREATE TABLE IF NOT EXISTS office_budget_claims ( policy_id TEXT NOT NULL, period_key TEXT NOT NULL, level TEXT NOT NULL, + revision INTEGER NOT NULL, claimed_at TIMESTAMP NOT NULL, - PRIMARY KEY (policy_id, period_key, level), + PRIMARY KEY (policy_id, period_key, level, revision), FOREIGN KEY (policy_id) REFERENCES office_budget_policies(id) ON DELETE CASCADE ); - `) - return err -} +` func (r *Repository) createRunTables() error { _, err := r.db.Exec(` diff --git a/apps/backend/internal/office/repository/sqlite/base_migrations.go b/apps/backend/internal/office/repository/sqlite/base_migrations.go index f559e12cb5..75f2b6c6e9 100644 --- a/apps/backend/internal/office/repository/sqlite/base_migrations.go +++ b/apps/backend/internal/office/repository/sqlite/base_migrations.go @@ -63,6 +63,7 @@ func (r *Repository) runMigrations() error { r.migrateWakeWaveColumns() r.migrate.Apply("task_workspace_groups.ownership_generation", `ALTER TABLE task_workspace_groups ADD COLUMN ownership_generation INTEGER NOT NULL DEFAULT 1`) + r.migrateBudgetPolicyRevision() r.migrateWorkspacePauseSkipAttribution() r.migrateLoopLivenessCausationID() if err := r.migrate.Err(); err != nil { @@ -86,6 +87,17 @@ func (r *Repository) backfillRoutineTriggerTimezones() error { return nil } +// migrateBudgetPolicyRevision adds office_budget_policies.revision for +// databases created before REQ-OFFICE-COSTS-003. The DEFAULT 1 backfills +// every existing row exactly like createCostTables' inline column, so a +// fresh database and a migrated one converge. A boot replay is a no-op: +// db.IsDuplicateColumnError is the only local classifier (ADR 0027), reused +// via MigrateLogger.Apply rather than adding a new one. +func (r *Repository) migrateBudgetPolicyRevision() { + _ = r.migrate.Apply("office_budget_policies.revision", + `ALTER TABLE office_budget_policies ADD COLUMN revision INTEGER NOT NULL DEFAULT 1`) +} + // migrateWorkspacePauseSkipAttribution adds the columns a blocked routine // fire uses to record why it was skipped and which pause blocked it. The // partial unique index runs after both ADD COLUMNs so it never executes diff --git a/apps/backend/internal/office/repository/sqlite/budget_atomicity_internal_test.go b/apps/backend/internal/office/repository/sqlite/budget_atomicity_internal_test.go index d28e6c38a6..4583f0326c 100644 --- a/apps/backend/internal/office/repository/sqlite/budget_atomicity_internal_test.go +++ b/apps/backend/internal/office/repository/sqlite/budget_atomicity_internal_test.go @@ -16,7 +16,8 @@ import ( // discard and the policy row update are one transaction // (AC-OFFICE-COSTS-002.8): when the row update fails after the discard has // already run, the whole transaction rolls back, so the claim survives -// alongside the un-updated policy. failBudgetPolicyUpdateErr is a +// alongside the un-updated policy, and the policy's revision does not +// advance either (AC-OFFICE-COSTS-003.6). failBudgetPolicyUpdateErr is a // test-only failpoint (see base.go) standing in for a fault-injecting // driver. func TestUpdateBudgetPolicy_FailedUpdateRollsBackDiscard(t *testing.T) { @@ -53,7 +54,7 @@ func TestUpdateBudgetPolicy_FailedUpdateRollsBackDiscard(t *testing.T) { if err := repo.CreateBudgetPolicy(ctx, policy); err != nil { t.Fatalf("create policy: %v", err) } - if _, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert"); err != nil { + if _, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert", policy.Revision); err != nil { t.Fatalf("claim: %v", err) } @@ -80,4 +81,7 @@ func TestUpdateBudgetPolicy_FailedUpdateRollsBackDiscard(t *testing.T) { if stored.LimitSubcents != 1000 { t.Fatalf("limit_subcents after rolled-back update = %d, want unchanged 1000", stored.LimitSubcents) } + if stored.Revision != 1 { + t.Fatalf("revision after rolled-back update = %d, want unchanged 1 (a rolled-back update must not consume a revision number)", stored.Revision) + } } diff --git a/apps/backend/internal/office/repository/sqlite/budget_claims_postgres_test.go b/apps/backend/internal/office/repository/sqlite/budget_claims_postgres_test.go index 755239a246..e4e19daa32 100644 --- a/apps/backend/internal/office/repository/sqlite/budget_claims_postgres_test.go +++ b/apps/backend/internal/office/repository/sqlite/budget_claims_postgres_test.go @@ -56,7 +56,7 @@ func TestPostgresBudgetClaims(t *testing.T) { t.Fatalf("create budget policy: %v", err) } - claimed, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert") + claimed, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert", policy.Revision) if err != nil { t.Fatalf("first claim: %v", err) } @@ -64,7 +64,7 @@ func TestPostgresBudgetClaims(t *testing.T) { t.Fatal("first claim should win") } - claimed, err = repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert") + claimed, err = repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert", policy.Revision) if err != nil { t.Fatalf("second claim: %v", err) } @@ -72,9 +72,9 @@ func TestPostgresBudgetClaims(t *testing.T) { t.Fatal("second claim on the same (policy, period, level) must not win") } - // A foreign-key violation (no such policy) must classify as an ordinary - // miss, not a store error — db.IsForeignKeyViolation's PostgreSQL branch. - claimed, err = repo.Claim(ctx, uuid.NewString(), "lifetime", "alert") + // A claim for a nonexistent policy must be an ordinary fenced miss, not a + // store error. The INSERT ... SELECT fence prevents an FK violation here. + claimed, err = repo.Claim(ctx, uuid.NewString(), "lifetime", "alert", 1) if err != nil { t.Fatalf("claim against a nonexistent policy must not be a store error, got: %v", err) } @@ -95,3 +95,190 @@ func TestPostgresBudgetClaims(t *testing.T) { t.Fatalf("claim count after policy delete = %d, want 0 (FK ON DELETE CASCADE)", claimCount) } } + +// TestPostgresBudgetClaims_RevisionFencingAndFourColumnKey is the PostgreSQL +// twin of the revision-fencing coverage in budget_revision_fencing_test.go: +// under READ COMMITTED a claim's WHERE EXISTS fence check and the row it +// inserts both need proving on a real PostgreSQL backend, not just SQLite's +// single-writer connection. It covers both halves called out by the fence +// design: the fence itself refuses an insert against a superseded revision +// (a stale evaluation cannot emit an obsolete claim), and revision being +// part of the primary key means a claim row already recorded for an old +// revision does not collide with — and so cannot suppress — a fresh +// evaluation's claim at the new revision. Skips unless +// KANDEV_TEST_POSTGRES_DSN is set. +func TestPostgresBudgetClaims_RevisionFencingAndFourColumnKey(t *testing.T) { + db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t)) + ctx := context.Background() + + if _, err := taskrepo.NewWithDB(db, db, nil); err != nil { + t.Fatalf("init task repo: %v", err) + } + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("boot: %v", err) + } + + policy := &models.BudgetPolicy{ + WorkspaceID: "pg-budget-fencing-ws", + ScopeType: "workspace", + ScopeID: "pg-budget-fencing-ws", + LimitSubcents: 1000, + Period: "monthly", + AlertThresholdPct: 80, + ActionOnExceed: "notify_only", + } + if err := repo.CreateBudgetPolicy(ctx, policy); err != nil { + t.Fatalf("create budget policy: %v", err) + } + staleRevision := policy.Revision + + // A claim recorded at the current revision, before any update. + claimed, err := repo.Claim(ctx, policy.ID, "lifetime", "alert", staleRevision) + if err != nil { + t.Fatalf("claim at revision %d: %v", staleRevision, err) + } + if !claimed { + t.Fatal("claim at the current revision should win") + } + + if err := repo.UpdateBudgetPolicy(ctx, policy); err != nil { + t.Fatalf("update policy: %v", err) + } + if policy.Revision != staleRevision+1 { + t.Fatalf("revision after update = %d, want %d", policy.Revision, staleRevision+1) + } + + // The fence: a claim presented with the now-superseded revision must be + // refused, even though it targets a different period so it collides + // with nothing already in the table. + claimed, err = repo.Claim(ctx, policy.ID, "stale-period", "alert", staleRevision) + if err != nil { + t.Fatalf("stale-revision claim must be an ordinary miss, got error: %v", err) + } + if claimed { + t.Fatal("a claim presented with a superseded revision must not win") + } + + // A surviving old-revision row must not suppress a fresh claim. The + // direct insert models a claim that was already committed by an older + // evaluation while the policy update moved the current revision. + if _, err := db.ExecContext(ctx, ` + INSERT INTO office_budget_claims (policy_id, period_key, level, revision, claimed_at) + VALUES ($1, 'lifetime', 'alert', $2, NOW()) + `, policy.ID, staleRevision); err != nil { + t.Fatalf("seed surviving old-revision claim: %v", err) + } + + // The four-column key lets the current revision claim the same period and + // level without colliding with that old-revision row. + claimed, err = repo.Claim(ctx, policy.ID, "lifetime", "alert", policy.Revision) + if err != nil { + t.Fatalf("claim at the new revision: %v", err) + } + if !claimed { + t.Fatal("claim at the current (post-update) revision should win") + } + + var claimCount int + if err := db.GetContext(ctx, &claimCount, + `SELECT COUNT(*) FROM office_budget_claims WHERE policy_id = $1`, policy.ID, + ); err != nil { + t.Fatalf("count claims: %v", err) + } + if claimCount != 2 { + t.Fatalf("claim rows for policy = %d, want 2 (old and current revisions must coexist)", claimCount) + } +} + +// TestPostgresBudgetClaims_LegacyTableRecreated verifies the destructive +// legacy-table cutover on a real PostgreSQL backend. The old three-column +// table is replaced in one transaction, and a later boot must replay without +// replacing the new table again. Skips unless KANDEV_TEST_POSTGRES_DSN is set. +func TestPostgresBudgetClaims_LegacyTableRecreated(t *testing.T) { + db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t)) + ctx := context.Background() + + if _, err := taskrepo.NewWithDB(db, db, nil); err != nil { + t.Fatalf("init task repo: %v", err) + } + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("initial boot: %v", err) + } + + policy := &models.BudgetPolicy{ + WorkspaceID: "pg-budget-legacy-ws", + ScopeType: "workspace", + ScopeID: "pg-budget-legacy-ws", + LimitSubcents: 1000, + Period: "monthly", + AlertThresholdPct: 80, + ActionOnExceed: "notify_only", + } + if err := repo.CreateBudgetPolicy(ctx, policy); err != nil { + t.Fatalf("create budget policy: %v", err) + } + + // Replace the current table with the pre-revision three-column shape and + // leave a row in it. The next office boot must take the recreate path. + if _, err := db.ExecContext(ctx, `DROP TABLE office_budget_claims`); err != nil { + t.Fatalf("drop current claims table: %v", err) + } + if _, err := db.ExecContext(ctx, ` + CREATE TABLE office_budget_claims ( + policy_id TEXT NOT NULL, + period_key TEXT NOT NULL, + level TEXT NOT NULL, + claimed_at TIMESTAMP NOT NULL, + PRIMARY KEY (policy_id, period_key, level), + FOREIGN KEY (policy_id) REFERENCES office_budget_policies(id) ON DELETE CASCADE + ) + `); err != nil { + t.Fatalf("create legacy claims table: %v", err) + } + if _, err := db.ExecContext(ctx, ` + INSERT INTO office_budget_claims (policy_id, period_key, level, claimed_at) + VALUES ($1, 'legacy', 'alert', NOW()) + `, policy.ID); err != nil { + t.Fatalf("seed legacy claim: %v", err) + } + + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("boot that recreates legacy claims table: %v", err) + } + + var revisionColumns int + if err := db.GetContext(ctx, &revisionColumns, ` + SELECT COUNT(*) FROM information_schema.columns + WHERE table_schema = current_schema() + AND table_name = 'office_budget_claims' + AND column_name = 'revision' + `); err != nil { + t.Fatalf("probe recreated claims table: %v", err) + } + if revisionColumns != 1 { + t.Fatalf("office_budget_claims.revision columns = %d, want 1", revisionColumns) + } + + claimed, err := repo.Claim(ctx, policy.ID, "replayed", "alert", policy.Revision) + if err != nil { + t.Fatalf("claim after legacy recreate: %v", err) + } + if !claimed { + t.Fatal("claim after legacy recreate should win") + } + + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("replay boot after recreate: %v", err) + } + var claimCount int + if err := db.GetContext(ctx, &claimCount, + `SELECT COUNT(*) FROM office_budget_claims WHERE policy_id = $1`, policy.ID, + ); err != nil { + t.Fatalf("count claims after replay: %v", err) + } + if claimCount != 1 { + t.Fatalf("claim rows after replay = %d, want 1", claimCount) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/budget_claims_recreate_internal_test.go b/apps/backend/internal/office/repository/sqlite/budget_claims_recreate_internal_test.go new file mode 100644 index 0000000000..26f16b4254 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/budget_claims_recreate_internal_test.go @@ -0,0 +1,156 @@ +package sqlite + +import ( + "errors" + "testing" + + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + + "github.com/kandev/kandev/internal/db" + runssqlite "github.com/kandev/kandev/internal/runs/repository/sqlite" +) + +// TestRecreateBudgetClaimsForRevision_FailureIsVisible covers the other +// half of AC-OFFICE-COSTS-003.3a: a failed recreate must reach the caller +// that initializes the schema, not be logged and swallowed the way the +// neighbouring runMigrations()/activateRunOutcome() calls are. +// failBudgetClaimsRecreateErr is a test-only failpoint (see base.go) +// standing in for a fault-injecting driver; the repo is built by hand +// (mirroring NewWithDB) so the failpoint can be set before initSchema runs, +// while the database still carries the pre-.3 three-column claims table the +// probe needs to find revision absent in. +func TestRecreateBudgetClaimsForRevision_FailureIsVisible(t *testing.T) { + sqlDB, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + sqlDB.SetMaxOpenConns(1) + t.Cleanup(func() { _ = sqlDB.Close() }) + + if _, err := sqlDB.Exec(` + CREATE TABLE office_budget_policies ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL, + scope_type TEXT NOT NULL, + scope_id TEXT NOT NULL, + limit_subcents INTEGER NOT NULL, + period TEXT NOT NULL, + alert_threshold_pct INTEGER DEFAULT 80, + action_on_exceed TEXT DEFAULT 'notify_only', + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL + ); + CREATE TABLE office_budget_claims ( + policy_id TEXT NOT NULL, + period_key TEXT NOT NULL, + level TEXT NOT NULL, + claimed_at TIMESTAMP NOT NULL, + PRIMARY KEY (policy_id, period_key, level), + FOREIGN KEY (policy_id) REFERENCES office_budget_policies(id) ON DELETE CASCADE + ); + `); err != nil { + t.Fatalf("seed legacy office_budget_claims: %v", err) + } + + repo := &Repository{ + Repository: runssqlite.NewWithDB(sqlDB, sqlDB), + db: sqlDB, + ro: sqlDB, + migrate: db.NewRequiredMigrateLogger(sqlDB, nil), + failBudgetClaimsRecreateErr: errors.New("injected recreate failure"), + } + + if err := repo.initSchema(); err == nil { + t.Fatal("expected initSchema to surface the injected recreate failure, got nil error") + } +} + +// budgetClaimsHasRevisionColumn reports whether office_budget_claims +// currently declares a revision column, distinguishing the old three-column +// shape from the final four-column one. +func budgetClaimsHasRevisionColumn(t *testing.T, sqlDB *sqlx.DB) bool { + t.Helper() + var count int + if err := sqlDB.Get(&count, + `SELECT COUNT(*) FROM pragma_table_info('office_budget_claims') WHERE name = 'revision'`, + ); err != nil { + t.Fatalf("probe office_budget_claims columns: %v", err) + } + return count == 1 +} + +// TestRecreateBudgetClaimsForRevision_AtomicAcrossDropAndCreate covers the +// crash-safety half of AC-OFFICE-COSTS-003.3a: DROP and CREATE must commit +// or fail together, not as two independent statements. A boot interrupted +// between them must not leave office_budget_claims absent, because a +// missing table and an old-shape table both probe as "revision absent" and +// an unconditional DROP TABLE (no IF EXISTS) on an already-missing table +// would error forever on every later boot. failBudgetClaimsRecreateAfterDropErr +// (see base.go) fires after DROP has run but before CREATE, inside the same +// transaction, standing in for a process crash at that point. +func TestRecreateBudgetClaimsForRevision_AtomicAcrossDropAndCreate(t *testing.T) { + sqlDB, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + sqlDB.SetMaxOpenConns(1) + t.Cleanup(func() { _ = sqlDB.Close() }) + + if _, err := sqlDB.Exec(` + CREATE TABLE office_budget_policies ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL, + scope_type TEXT NOT NULL, + scope_id TEXT NOT NULL, + limit_subcents INTEGER NOT NULL, + period TEXT NOT NULL, + alert_threshold_pct INTEGER DEFAULT 80, + action_on_exceed TEXT DEFAULT 'notify_only', + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL + ); + CREATE TABLE office_budget_claims ( + policy_id TEXT NOT NULL, + period_key TEXT NOT NULL, + level TEXT NOT NULL, + claimed_at TIMESTAMP NOT NULL, + PRIMARY KEY (policy_id, period_key, level), + FOREIGN KEY (policy_id) REFERENCES office_budget_policies(id) ON DELETE CASCADE + ); + `); err != nil { + t.Fatalf("seed legacy office_budget_claims: %v", err) + } + + repo := &Repository{ + Repository: runssqlite.NewWithDB(sqlDB, sqlDB), + db: sqlDB, + ro: sqlDB, + migrate: db.NewRequiredMigrateLogger(sqlDB, nil), + } + + repo.failBudgetClaimsRecreateAfterDropErr = errors.New("injected crash between drop and create") + if err := repo.initSchema(); err == nil { + t.Fatal("expected initSchema to surface the injected failure, got nil error") + } + if budgetClaimsHasRevisionColumn(t, sqlDB) { + t.Fatal("a failure between DROP and CREATE must not leave the new shape partially applied") + } + var count int + if err := sqlDB.Get(&count, + `SELECT COUNT(*) FROM pragma_table_info('office_budget_claims')`, + ); err != nil { + t.Fatalf("count office_budget_claims columns: %v", err) + } + if count == 0 { + t.Fatal("a failure between DROP and CREATE must not leave office_budget_claims absent; the DROP must roll back too") + } + + repo.failBudgetClaimsRecreateAfterDropErr = nil + if err := repo.initSchema(); err != nil { + t.Fatalf("retry boot after the injected failure must self-heal, got: %v", err) + } + if !budgetClaimsHasRevisionColumn(t, sqlDB) { + t.Fatal("retry boot must complete the recreate and leave the revision column present") + } +} diff --git a/apps/backend/internal/office/repository/sqlite/budget_exceeded_atomicity_internal_test.go b/apps/backend/internal/office/repository/sqlite/budget_exceeded_atomicity_internal_test.go new file mode 100644 index 0000000000..d9fe3f52f0 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/budget_exceeded_atomicity_internal_test.go @@ -0,0 +1,71 @@ +package sqlite + +import ( + "context" + "errors" + "testing" + + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + + settingsstore "github.com/kandev/kandev/internal/agent/settings/store" + "github.com/kandev/kandev/internal/office/models" +) + +// TestClaimExceeded_CompanionFailure_RollsBackWholePair proves the +// exceeded-level claim and its alert-level companion are one transaction +// (AC-OFFICE-COSTS-003.10): when the companion insert fails after the +// exceeded insert has already run, the whole transaction rolls back, so +// neither row survives. failBudgetExceededCompanionErr is a test-only +// failpoint (see base.go) standing in for a fault-injecting driver. +func TestClaimExceeded_CompanionFailure_RollsBackWholePair(t *testing.T) { + db, err := sqlx.Open("sqlite3", ":memory:?_foreign_keys=on") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + if _, err := db.Exec(`PRAGMA foreign_keys = ON`); err != nil { + t.Fatalf("pragma fk: %v", err) + } + if _, _, err := settingsstore.Provide(db, db, nil); err != nil { + t.Fatalf("settings store init: %v", err) + } + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("new repo: %v", err) + } + + ctx := context.Background() + policy := &models.BudgetPolicy{ + WorkspaceID: "ws-companion-rollback", + ScopeType: "workspace", + ScopeID: "ws-companion-rollback", + LimitSubcents: 1000, + Period: "monthly", + AlertThresholdPct: 80, + ActionOnExceed: "notify_only", + } + if err := repo.CreateBudgetPolicy(ctx, policy); err != nil { + t.Fatalf("create policy: %v", err) + } + + repo.failBudgetExceededCompanionErr = errors.New("injected companion failure") + claimed, err := repo.ClaimExceeded(ctx, policy.ID, "lifetime", policy.Revision) + if err == nil { + t.Fatal("expected the injected companion failure to surface") + } + if claimed { + t.Fatal("a failed pair must report claimed=false") + } + + var count int + if err := db.QueryRow( + `SELECT COUNT(*) FROM office_budget_claims WHERE policy_id = ?`, policy.ID, + ).Scan(&count); err != nil { + t.Fatalf("count claims: %v", err) + } + if count != 0 { + t.Fatalf("claim rows after a rolled-back pair = %d, want 0 (neither row must survive)", count) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/budget_revision_fencing_test.go b/apps/backend/internal/office/repository/sqlite/budget_revision_fencing_test.go new file mode 100644 index 0000000000..64a6cdc196 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/budget_revision_fencing_test.go @@ -0,0 +1,424 @@ +package sqlite_test + +import ( + "context" + "sync" + "testing" + + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + + "github.com/kandev/kandev/internal/office/repository/sqlite" +) + +// TestBudgetPolicyRevisionMigration_LegacyRowsBackfillToOne covers +// AC-OFFICE-COSTS-003.1: a database created before the revision column +// exists gets it added as NOT NULL DEFAULT 1, an existing row is backfilled +// to 1, and a boot replay is a no-op (no new local error classifier is +// exercised beyond db.IsDuplicateColumnError, ADR 0027). +func TestBudgetPolicyRevisionMigration_LegacyRowsBackfillToOne(t *testing.T) { + db, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + // Seed the pre-.3 office_budget_policies shape (no revision column) and + // a legacy row, before the repo (and its migration) ever runs. + if _, err := db.Exec(` + CREATE TABLE office_budget_policies ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL, + scope_type TEXT NOT NULL, + scope_id TEXT NOT NULL, + limit_subcents INTEGER NOT NULL, + period TEXT NOT NULL, + alert_threshold_pct INTEGER DEFAULT 80, + action_on_exceed TEXT DEFAULT 'notify_only', + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL + )`); err != nil { + t.Fatalf("seed legacy office_budget_policies: %v", err) + } + if _, err := db.Exec(` + INSERT INTO office_budget_policies ( + id, workspace_id, scope_type, scope_id, limit_subcents, period, created_at, updated_at + ) VALUES ('legacy-1', 'ws-legacy', 'workspace', 'ws-legacy', 1000, 'monthly', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) + `); err != nil { + t.Fatalf("seed legacy row: %v", err) + } + + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("first boot: %v", err) + } + + var revision int64 + if err := db.QueryRow(`SELECT revision FROM office_budget_policies WHERE id = 'legacy-1'`).Scan(&revision); err != nil { + t.Fatalf("select revision on legacy row: %v", err) + } + if revision != 1 { + t.Fatalf("legacy row revision = %d, want 1", revision) + } + + // Boot replay must be a no-op, not an error. + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("second boot (replay): %v", err) + } +} + +// legacyBudgetClaimsFixture builds an in-memory database still carrying +// PR #3287's three-column office_budget_claims shape (no revision, no FK +// dependency needed for these assertions), so the AC-OFFICE-COSTS-003.3a +// recreate actually has to run rather than being short-circuited by a fresh +// database's inline final-shape DDL. +func legacyBudgetClaimsFixture(t *testing.T) *sqlx.DB { + t.Helper() + db, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + if _, err := db.Exec(` + CREATE TABLE office_budget_policies ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL, + scope_type TEXT NOT NULL, + scope_id TEXT NOT NULL, + limit_subcents INTEGER NOT NULL, + period TEXT NOT NULL, + alert_threshold_pct INTEGER DEFAULT 80, + action_on_exceed TEXT DEFAULT 'notify_only', + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL + ); + CREATE TABLE office_budget_claims ( + policy_id TEXT NOT NULL, + period_key TEXT NOT NULL, + level TEXT NOT NULL, + claimed_at TIMESTAMP NOT NULL, + PRIMARY KEY (policy_id, period_key, level), + FOREIGN KEY (policy_id) REFERENCES office_budget_policies(id) ON DELETE CASCADE + ); + `); err != nil { + t.Fatalf("seed legacy office_budget_claims: %v", err) + } + return db +} + +// TestRecreateBudgetClaims_OldShapeDatabase_GuardedAcrossBoots covers +// AC-OFFICE-COSTS-003.3a: booting against a database still carrying the +// pre-.3 three-column claim table runs the recreate (destroying whatever +// claims existed before that boot, which AC-OFFICE-COSTS-003.3 expressly +// permits); a claim written *after* that boot survives a second boot, which +// must probe, find revision present, and do nothing. +func TestRecreateBudgetClaims_OldShapeDatabase_GuardedAcrossBoots(t *testing.T) { + db := legacyBudgetClaimsFixture(t) + if _, err := db.Exec(` + INSERT INTO office_budget_policies ( + id, workspace_id, scope_type, scope_id, limit_subcents, period, created_at, updated_at + ) VALUES ('p1', 'ws-1', 'workspace', 'ws-1', 1000, 'monthly', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) + `); err != nil { + t.Fatalf("seed policy: %v", err) + } + // A claim held before the cutover boot: the recreate is permitted to + // destroy it (AC-OFFICE-COSTS-003.3's accepted first-run behavior). + if _, err := db.Exec(` + INSERT INTO office_budget_claims (policy_id, period_key, level, claimed_at) + VALUES ('p1', 'lifetime', 'alert', CURRENT_TIMESTAMP) + `); err != nil { + t.Fatalf("seed pre-cutover claim: %v", err) + } + + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("first boot (cutover): %v", err) + } + + var count int + if err := db.QueryRow(`SELECT COUNT(*) FROM office_budget_claims`).Scan(&count); err != nil { + t.Fatalf("count claims after cutover: %v", err) + } + if count != 0 { + t.Fatalf("claims after cutover = %d, want 0 (pre-cutover claims are destroyed by the recreate)", count) + } + + var revisionColExists int + if err := db.QueryRow(`SELECT COUNT(*) FROM pragma_table_info('office_budget_claims') WHERE name = 'revision'`). + Scan(&revisionColExists); err != nil { + t.Fatalf("probe revision column: %v", err) + } + if revisionColExists != 1 { + t.Fatal("office_budget_claims.revision must exist after the cutover boot") + } + + // A claim written after the cutover boot must survive a second boot. + ctx := context.Background() + if claimed, err := repo.Claim(ctx, "p1", "lifetime", "alert", 1); err != nil || !claimed { + t.Fatalf("post-cutover claim: claimed=%v err=%v", claimed, err) + } + + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("second boot (replay): %v", err) + } + if err := db.QueryRow(`SELECT COUNT(*) FROM office_budget_claims`).Scan(&count); err != nil { + t.Fatalf("count claims after replay: %v", err) + } + if count != 1 { + t.Fatalf("claims after replay boot = %d, want 1 (the probe must no-op on a second boot)", count) + } +} + +// TestCreateBudgetPolicy_AssignsRevisionOne_IgnoringPresentedValue covers +// AC-OFFICE-COSTS-003.4: revision is always 1 on create, regardless of any +// value already set on the policy struct passed in, and the assigned value +// is written back so the caller sees what the row actually holds. +func TestCreateBudgetPolicy_AssignsRevisionOne_IgnoringPresentedValue(t *testing.T) { + repo, db := newBudgetClaimsRepoWithFK(t) + ctx := context.Background() + + policy := createTestBudgetPolicy(t, repo, "ws-revision-create") + policy.Revision = 777 // presented before a hypothetical re-create; must not survive. + policy.ID = "" // force a fresh insert + if err := repo.CreateBudgetPolicy(ctx, policy); err != nil { + t.Fatalf("create policy: %v", err) + } + if policy.Revision != 1 { + t.Fatalf("revision returned to caller = %d, want 1", policy.Revision) + } + + var stored int64 + if err := db.QueryRow(`SELECT revision FROM office_budget_policies WHERE id = ?`, policy.ID).Scan(&stored); err != nil { + t.Fatalf("select stored revision: %v", err) + } + if stored != 1 { + t.Fatalf("stored revision = %d, want 1", stored) + } +} + +// TestUpdateBudgetPolicy_NonexistentPolicy_ReturnsErrorNoCommit covers the +// AC-OFFICE-COSTS-003.5 tightening: updating a policy id that no longer +// exists rolls the transaction back and reports an error, a behavior change +// from #3287 where the update ignored its affected-row count and reported +// success. +func TestUpdateBudgetPolicy_NonexistentPolicy_ReturnsErrorNoCommit(t *testing.T) { + repo, _ := newBudgetClaimsRepoWithFK(t) + ctx := context.Background() + + policy := createTestBudgetPolicy(t, repo, "ws-revision-missing") + if err := repo.DeleteBudgetPolicy(ctx, policy.ID); err != nil { + t.Fatalf("delete policy: %v", err) + } + + policy.LimitSubcents = 9999 + if err := repo.UpdateBudgetPolicy(ctx, policy); err == nil { + t.Fatal("expected an error updating a policy id that no longer exists") + } +} + +// TestUpdateBudgetPolicy_ConcurrentUpdates_RevisionAdvancesByTwo covers +// concurrency case 6 of the forced-to-invent pass: two concurrent updates +// each bump by 1, and the final revision is r+2 with no increment lost — +// serialized by SQLite's single-writer connection, exactly as production +// runs (internal/db/pool.go). +func TestUpdateBudgetPolicy_ConcurrentUpdates_RevisionAdvancesByTwo(t *testing.T) { + repo, db := newBudgetClaimsRepoWithFK(t) + ctx := context.Background() + policy := createTestBudgetPolicy(t, repo, "ws-revision-concurrent") + + var wg sync.WaitGroup + wg.Add(2) + for i := 0; i < 2; i++ { + go func(limit int64) { + defer wg.Done() + p, err := repo.GetBudgetPolicy(ctx, policy.ID) + if err != nil { + t.Errorf("get policy: %v", err) + return + } + p.LimitSubcents = limit + if err := repo.UpdateBudgetPolicy(ctx, p); err != nil { + t.Errorf("update policy: %v", err) + } + }(int64(2000 + i)) + } + wg.Wait() + + var revision int64 + if err := db.QueryRow(`SELECT revision FROM office_budget_policies WHERE id = ?`, policy.ID).Scan(&revision); err != nil { + t.Fatalf("select final revision: %v", err) + } + if revision != 3 { + t.Fatalf("final revision = %d, want 3 (started at 1, two updates each +1)", revision) + } +} + +// TestClaim_RaceA_StaleRevisionRefused_FreshRevisionSucceeds covers +// AC-OFFICE-COSTS-003.8/.9/.15 (the verification.md "Race A regression" +// bullet): a claim presenting a superseded revision is refused as an +// ordinary miss (no error), and the slot for the new revision stays open so +// a claim at the current revision still wins. +func TestClaim_RaceA_StaleRevisionRefused_FreshRevisionSucceeds(t *testing.T) { + repo, _ := newBudgetClaimsRepoWithFK(t) + ctx := context.Background() + policy := createTestBudgetPolicy(t, repo, "ws-race-a") + staleRevision := policy.Revision // 1 + + // Concurrent UpdateBudgetPolicy commits, bumping the stored revision to 2. + policy.LimitSubcents = 2000 + if err := repo.UpdateBudgetPolicy(ctx, policy); err != nil { + t.Fatalf("update policy: %v", err) + } + if policy.Revision != staleRevision+1 { + t.Fatalf("revision after update = %d, want %d", policy.Revision, staleRevision+1) + } + + // The stale evaluation's claim, fenced to the superseded revision, must + // be refused with no error. + claimed, err := repo.Claim(ctx, policy.ID, "lifetime", "alert", staleRevision) + if err != nil { + t.Fatalf("stale claim must not be a store error, got: %v", err) + } + if claimed { + t.Fatal("a claim fenced to a superseded revision must not win") + } + + // The slot for the new revision must still be open. + claimed, err = repo.Claim(ctx, policy.ID, "lifetime", "alert", policy.Revision) + if err != nil { + t.Fatalf("fresh-revision claim: %v", err) + } + if !claimed { + t.Fatal("a claim fenced to the current revision must win even after a stale claim was refused") + } +} + +// TestClaim_InvalidInput_FailsOpenNotSilently covers AC-OFFICE-COSTS-003.13: +// each of the four primary-key components, when invalid, is a claim-store +// error (never the silent miss AC-OFFICE-COSTS-003.9 reserves for a refused +// fence), so the guard must run before the fenced statement rather than as +// part of it. +func TestClaim_InvalidInput_FailsOpenNotSilently(t *testing.T) { + repo, _ := newBudgetClaimsRepoWithFK(t) + ctx := context.Background() + policy := createTestBudgetPolicy(t, repo, "ws-invalid-input") + + cases := []struct { + name string + policyID, period string + level string + revision int64 + }{ + {"zero revision", policy.ID, "lifetime", "alert", 0}, + {"negative revision", policy.ID, "lifetime", "alert", -1}, + {"empty policy id", "", "lifetime", "alert", policy.Revision}, + {"empty period key", policy.ID, "", "alert", policy.Revision}, + {"empty level", policy.ID, "lifetime", "", policy.Revision}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + claimed, err := repo.Claim(ctx, tc.policyID, tc.period, tc.level, tc.revision) + if err == nil { + t.Fatalf("expected a claim-store error for %s, got claimed=%v err=nil", tc.name, claimed) + } + if claimed { + t.Fatalf("an errored claim attempt must report claimed=false, got true for %s", tc.name) + } + }) + } +} + +// TestClaimExceeded_HappyPath_BothRowsExistAtSameRevision covers +// AC-OFFICE-COSTS-003.10/.11 on a healthy store: a successful ClaimExceeded +// call commits both the exceeded-level and alert-level companion rows at +// the same policy, period and revision. +func TestClaimExceeded_HappyPath_BothRowsExistAtSameRevision(t *testing.T) { + repo, db := newBudgetClaimsRepoWithFK(t) + ctx := context.Background() + policy := createTestBudgetPolicy(t, repo, "ws-exceeded-happy") + + claimed, err := repo.ClaimExceeded(ctx, policy.ID, "lifetime", policy.Revision) + if err != nil { + t.Fatalf("claim exceeded: %v", err) + } + if !claimed { + t.Fatal("first exceeded claim should win") + } + + for _, level := range []string{"exceeded", "alert"} { + var count int + if err := db.QueryRow( + `SELECT COUNT(*) FROM office_budget_claims WHERE policy_id = ? AND period_key = 'lifetime' AND level = ? AND revision = ?`, + policy.ID, level, policy.Revision, + ).Scan(&count); err != nil { + t.Fatalf("count %s claim: %v", level, err) + } + if count != 1 { + t.Fatalf("%s claim rows = %d, want 1", level, count) + } + } +} + +// TestClaimExceeded_CompanionConflictIsPairSuccess covers the first +// verification.md "pair outcomes that are successes, not failures" case: +// with an alert-level claim already held, an over-limit evaluation's +// companion insert conflicts, but the pair still commits and the exceeded +// claim is won. +func TestClaimExceeded_CompanionConflictIsPairSuccess(t *testing.T) { + repo, _ := newBudgetClaimsRepoWithFK(t) + ctx := context.Background() + policy := createTestBudgetPolicy(t, repo, "ws-companion-conflict") + + if claimed, err := repo.Claim(ctx, policy.ID, "lifetime", "alert", policy.Revision); err != nil || !claimed { + t.Fatalf("seed alert claim: claimed=%v err=%v", claimed, err) + } + + claimed, err := repo.ClaimExceeded(ctx, policy.ID, "lifetime", policy.Revision) + if err != nil { + t.Fatalf("claim exceeded: %v", err) + } + if !claimed { + t.Fatal("exceeded claim must still win despite the companion insert conflicting") + } +} + +// TestClaimExceeded_AlreadyHeld_NoCompanionAttempted covers the second +// verification.md "pair outcomes" case: with an exceeded-level claim +// already held, a second over-limit evaluation's exceeded insert writes +// nothing, and no companion insert is attempted. +func TestClaimExceeded_AlreadyHeld_NoCompanionAttempted(t *testing.T) { + repo, db := newBudgetClaimsRepoWithFK(t) + ctx := context.Background() + policy := createTestBudgetPolicy(t, repo, "ws-exceeded-already-held") + + if claimed, err := repo.ClaimExceeded(ctx, policy.ID, "lifetime", policy.Revision); err != nil || !claimed { + t.Fatalf("first exceeded claim: claimed=%v err=%v", claimed, err) + } + // Remove the companion so a second, incorrectly-attempted companion + // insert would be observable. + if _, err := db.Exec( + `DELETE FROM office_budget_claims WHERE policy_id = ? AND level = 'alert'`, policy.ID, + ); err != nil { + t.Fatalf("remove companion: %v", err) + } + + claimed, err := repo.ClaimExceeded(ctx, policy.ID, "lifetime", policy.Revision) + if err != nil { + t.Fatalf("second exceeded claim: %v", err) + } + if claimed { + t.Fatal("a second exceeded claim at an already-held (policy, period, revision) must not win") + } + + var companionCount int + if err := db.QueryRow( + `SELECT COUNT(*) FROM office_budget_claims WHERE policy_id = ? AND level = 'alert'`, policy.ID, + ).Scan(&companionCount); err != nil { + t.Fatalf("count companion rows: %v", err) + } + if companionCount != 0 { + t.Fatal("no companion insert must be attempted when the exceeded insert itself writes no row") + } +} diff --git a/apps/backend/internal/office/repository/sqlite/budgets.go b/apps/backend/internal/office/repository/sqlite/budgets.go index ceacfe5574..8b2855824d 100644 --- a/apps/backend/internal/office/repository/sqlite/budgets.go +++ b/apps/backend/internal/office/repository/sqlite/budgets.go @@ -13,7 +13,11 @@ import ( "github.com/kandev/kandev/internal/office/models" ) -// CreateBudgetPolicy creates a new budget policy. +// CreateBudgetPolicy creates a new budget policy. Its revision is always +// assigned 1, regardless of any value already set on policy — including a +// nonzero one presented for creation. The assigned value is written back +// onto policy so the caller (the create response) reports what the row +// actually holds rather than leaning on the column's DEFAULT 1. func (r *Repository) CreateBudgetPolicy(ctx context.Context, policy *models.BudgetPolicy) error { if policy.ID == "" { policy.ID = uuid.New().String() @@ -21,15 +25,16 @@ func (r *Repository) CreateBudgetPolicy(ctx context.Context, policy *models.Budg now := time.Now().UTC() policy.CreatedAt = now policy.UpdatedAt = now + policy.Revision = 1 _, err := r.db.ExecContext(ctx, r.db.Rebind(` INSERT INTO office_budget_policies ( id, workspace_id, scope_type, scope_id, limit_subcents, period, - alert_threshold_pct, action_on_exceed, created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + alert_threshold_pct, action_on_exceed, created_at, updated_at, revision + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `), policy.ID, policy.WorkspaceID, policy.ScopeType, policy.ScopeID, policy.LimitSubcents, policy.Period, policy.AlertThresholdPct, - policy.ActionOnExceed, policy.CreatedAt, policy.UpdatedAt) + policy.ActionOnExceed, policy.CreatedAt, policy.UpdatedAt, policy.Revision) return err } @@ -78,6 +83,15 @@ func (r *Repository) UpdateBudgetPolicy(ctx context.Context, policy *models.Budg return tx.Commit() } +// updateBudgetPolicyTx discards the policy's claims, bumps its revision by +// exactly 1 in-SQL, and reads the new revision back onto policy, all in the +// caller's transaction. The bump is expressed as revision = revision + 1 +// rather than a value computed from a prior read, so two concurrent updates +// can never write the same revision. If no row matches id — the policy was +// deleted between the caller's read and this write — the RETURNING +// read-back yields sql.ErrNoRows, which propagates as an error so the +// caller rolls the whole transaction back rather than committing an empty +// update. func (r *Repository) updateBudgetPolicyTx(ctx context.Context, tx *sqlx.Tx, policy *models.BudgetPolicy) error { if _, err := tx.ExecContext(ctx, tx.Rebind( `DELETE FROM office_budget_claims WHERE policy_id = ?`), policy.ID); err != nil { @@ -86,14 +100,20 @@ func (r *Repository) updateBudgetPolicyTx(ctx context.Context, tx *sqlx.Tx, poli if r.failBudgetPolicyUpdateErr != nil { return r.failBudgetPolicyUpdateErr } - _, err := tx.ExecContext(ctx, tx.Rebind(` + err := tx.QueryRowContext(ctx, tx.Rebind(` UPDATE office_budget_policies SET scope_type = ?, scope_id = ?, limit_subcents = ?, period = ?, - alert_threshold_pct = ?, action_on_exceed = ?, updated_at = ? + alert_threshold_pct = ?, action_on_exceed = ?, updated_at = ?, + revision = revision + 1 WHERE id = ? + RETURNING revision `), policy.ScopeType, policy.ScopeID, policy.LimitSubcents, policy.Period, - policy.AlertThresholdPct, policy.ActionOnExceed, policy.UpdatedAt, policy.ID) + policy.AlertThresholdPct, policy.ActionOnExceed, policy.UpdatedAt, policy.ID, + ).Scan(&policy.Revision) if err != nil { + if err == sql.ErrNoRows { + return fmt.Errorf("update policy: not found: %s", policy.ID) + } return fmt.Errorf("update policy: %w", err) } return nil @@ -107,27 +127,207 @@ func (r *Repository) DeleteBudgetPolicy(ctx context.Context, id string) error { return err } -// Claim atomically records that (policyID, periodKey, level) may emit its -// budget notification. claimed=true means this call won and the caller -// should emit; claimed=false with a nil error means an earlier evaluation -// already holds the claim, or the referenced policy no longer exists — a -// foreign-key violation is deliberately reported as an ordinary miss, not a -// store failure. -func (r *Repository) Claim(ctx context.Context, policyID, periodKey, level string) (bool, error) { - res, err := r.db.ExecContext(ctx, r.db.Rebind(` - INSERT INTO office_budget_claims (policy_id, period_key, level, claimed_at) - VALUES (?, ?, ?, ?) - ON CONFLICT(policy_id, period_key, level) DO NOTHING - `), policyID, periodKey, level, time.Now().UTC()) +// budgetClaimLevelExceeded and budgetClaimLevelAlert are the claim-level +// literals ClaimExceeded writes for its atomic pair. Level is otherwise +// caller-supplied data, not a package concept, but ClaimExceeded's exported +// signature does not take a level parameter, so its two writes need names +// of their own. +const ( + budgetClaimLevelExceeded = "exceeded" + budgetClaimLevelAlert = "alert" +) + +// execRebinder is the subset of *sqlx.DB / *sqlx.Tx a fenced claim insert +// needs, so the same statement can run standalone or inside ClaimExceeded's +// transaction. +type execRebinder interface { + ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error) + Rebind(string) string +} + +// validateClaimInput reports whether a claim attempt's primary-key +// components are all well-formed. Every failure here is a programming +// error, not a superseded snapshot, so the caller treats it as a +// claim-store error and fails open rather than as the silent miss a +// refused fence produces. +func validateClaimInput(policyID, periodKey, level string, revision int64) error { + if revision <= 0 { + return fmt.Errorf("claim: revision must be positive, got %d", revision) + } + if policyID == "" { + return fmt.Errorf("claim: policy id must not be empty") + } + if periodKey == "" { + return fmt.Errorf("claim: period key must not be empty") + } + if level == "" { + return fmt.Errorf("claim: level must not be empty") + } + return nil +} + +// claimInsertFenced inserts a claim row only if policyID still holds +// exactly revision at the moment the statement runs, checked as part of the +// same statement so no evaluation can observe a matching revision and then +// insert against a superseded one. A policy that no longer exists at all +// fails the same EXISTS check, which is how a deleted policy's claim +// attempt is refused without ever reaching the foreign-key constraint. The +// FK violation classifier is kept as a defensive fallback only: dropping it +// would let a genuine foreign-key error read as an ordinary miss. +func claimInsertFenced(ctx context.Context, ex execRebinder, policyID, periodKey, level string, revision int64) (bool, error) { + res, err := ex.ExecContext(ctx, ex.Rebind(` + INSERT INTO office_budget_claims (policy_id, period_key, level, revision, claimed_at) + SELECT ?, ?, ?, ?, ? + WHERE EXISTS (SELECT 1 FROM office_budget_policies WHERE id = ? AND revision = ?) + ON CONFLICT(policy_id, period_key, level, revision) DO NOTHING + `), policyID, periodKey, level, revision, time.Now().UTC(), policyID, revision) if err != nil { if db.IsForeignKeyViolation(err) { return false, nil } return false, err } + return rowsAffectedOne(res) +} + +// claimInsertCompanion inserts the alert-level companion row for an +// already-won exceeded claim, keyed to the same revision with no fence of +// its own — its key already names the revision, so a row written against a +// revision that has since moved is inert rather than wrong. Any error, +// including a foreign-key violation from a policy deleted mid-transaction, +// is returned unmodified so ClaimExceeded rolls the whole pair back rather +// than committing a lone exceeded claim. +func claimInsertCompanion(ctx context.Context, tx *sqlx.Tx, policyID, periodKey string, revision int64) (bool, error) { + res, err := tx.ExecContext(ctx, tx.Rebind(` + INSERT INTO office_budget_claims (policy_id, period_key, level, revision, claimed_at) + VALUES (?, ?, ?, ?, ?) + ON CONFLICT(policy_id, period_key, level, revision) DO NOTHING + `), policyID, periodKey, budgetClaimLevelAlert, revision, time.Now().UTC()) + if err != nil { + return false, err + } + return rowsAffectedOne(res) +} + +func rowsAffectedOne(res sql.Result) (bool, error) { affected, err := res.RowsAffected() if err != nil { return false, err } return affected == 1, nil } + +// Claim atomically records that (policyID, periodKey, level) may emit its +// budget notification, fenced to revision: the claim is recorded only while +// revision is still the policy's stored revision. claimed=true means this +// call won and the caller should emit; claimed=false with a nil error means +// an earlier evaluation already holds the claim, the referenced policy no +// longer exists, or the presented revision has been superseded — all three +// are the same ordinary miss. +func (r *Repository) Claim(ctx context.Context, policyID, periodKey, level string, revision int64) (bool, error) { + if err := validateClaimInput(policyID, periodKey, level, revision); err != nil { + return false, err + } + return claimInsertFenced(ctx, r.db, policyID, periodKey, level, revision) +} + +// ClaimExceeded atomically records the exceeded-level claim and, only when +// that claim is won by this call, its alert-level companion claim, in one +// transaction: once it commits, either both rows exist for that policy, +// period and revision, or this call wrote neither. The fence is evaluated +// once, on the exceeded insert; the companion insert carries no fence of +// its own. Returns whether the exceeded-level row was inserted by this +// call — the companion's own outcome (won, or an ordinary conflict with an +// alert-level claim an earlier evaluation already holds) never changes it. +func (r *Repository) ClaimExceeded(ctx context.Context, policyID, periodKey string, revision int64) (bool, error) { + if err := validateClaimInput(policyID, periodKey, budgetClaimLevelExceeded, revision); err != nil { + return false, err + } + tx, err := r.db.BeginTxx(ctx, nil) + if err != nil { + return false, err + } + claimed, err := r.claimExceededTx(ctx, tx, policyID, periodKey, revision) + if err != nil { + if rollbackErr := tx.Rollback(); rollbackErr != nil { + return false, fmt.Errorf("%w; rollback: %v", err, rollbackErr) + } + return false, err + } + if !claimed { + return false, tx.Commit() + } + if err := tx.Commit(); err != nil { + return false, err + } + return true, nil +} + +func (r *Repository) claimExceededTx( + ctx context.Context, tx *sqlx.Tx, policyID, periodKey string, revision int64, +) (bool, error) { + claimed, err := claimInsertFenced(ctx, tx, policyID, periodKey, budgetClaimLevelExceeded, revision) + if err != nil { + return false, err + } + if !claimed { + // Refused by the fence, or conflicting with an exceeded claim + // already held: no emission to protect, so the companion insert + // must not be attempted. + return false, nil + } + if r.failBudgetExceededCompanionErr != nil { + return false, r.failBudgetExceededCompanionErr + } + if _, err := claimInsertCompanion(ctx, tx, policyID, periodKey, revision); err != nil { + return false, err + } + return true, nil +} + +// recreateBudgetClaimsForRevision takes effect at most once per database: +// it probes whether office_budget_claims already declares revision and does +// nothing when it does, which is every boot after the first on a fresh +// database (createCostTables already declares the final shape there) and +// every boot after the first successful recreate on an upgraded one. On a +// database still carrying the pre-revision three-column table, it drops and +// recreates office_budget_claims with the four-column primary key the +// current schema requires, as one transaction: DROP and CREATE either both +// commit or neither does, so a boot interrupted between them leaves the +// table exactly as it was rather than absent, and a retried boot always +// finds the table in a state the probe can classify. It is not expressed as +// an r.migrate.Apply statement: that runner re-executes its SQL on every +// boot and is idempotent only because an already-exists error is +// swallowed, which would destroy claim state on every boot here. It +// returns its error to its caller rather than swallowing it, so a failed +// recreate is visible rather than silently leaving the old three-column +// key in place. +func (r *Repository) recreateBudgetClaimsForRevision() error { + exists, err := db.ColumnExists(r.db, "office_budget_claims", "revision") + if err != nil { + return fmt.Errorf("probe office_budget_claims.revision: %w", err) + } + if exists { + return nil + } + if r.failBudgetClaimsRecreateErr != nil { + return r.failBudgetClaimsRecreateErr + } + tx, err := r.db.Beginx() + if err != nil { + return fmt.Errorf("begin office_budget_claims recreate: %w", err) + } + if _, err := tx.Exec(`DROP TABLE IF EXISTS office_budget_claims`); err != nil { + _ = tx.Rollback() + return fmt.Errorf("drop office_budget_claims: %w", err) + } + if r.failBudgetClaimsRecreateAfterDropErr != nil { + _ = tx.Rollback() + return r.failBudgetClaimsRecreateAfterDropErr + } + if _, err := tx.Exec(budgetClaimsDDL); err != nil { + _ = tx.Rollback() + return fmt.Errorf("recreate office_budget_claims: %w", err) + } + return tx.Commit() +} diff --git a/apps/backend/internal/office/repository/sqlite/budgets_test.go b/apps/backend/internal/office/repository/sqlite/budgets_test.go index ff5080f4e9..4c63626261 100644 --- a/apps/backend/internal/office/repository/sqlite/budgets_test.go +++ b/apps/backend/internal/office/repository/sqlite/budgets_test.go @@ -37,10 +37,16 @@ func newBudgetClaimsRepoWithFK(t *testing.T) (*sqlite.Repository, *sqlx.DB) { } // office_task_tree_holds and friends carry an FK to the task-package // owned "tasks" table; with foreign_keys=ON that table must exist (even - // empty) for a workspace-wide DELETE to validate the constraint. + // empty) for a workspace-wide DELETE to validate the constraint. title/ + // description/identifier are included so migrateTaskFTS's backfill (which + // selects them unconditionally once a "tasks" table exists) doesn't fail + // schema init on this minimal fixture. if _, err := db.Exec(`CREATE TABLE IF NOT EXISTS tasks ( id TEXT PRIMARY KEY, - workspace_id TEXT NOT NULL DEFAULT '' + workspace_id TEXT NOT NULL DEFAULT '', + title TEXT DEFAULT '', + description TEXT DEFAULT '', + identifier TEXT DEFAULT '' )`); err != nil { t.Fatalf("create tasks: %v", err) } @@ -84,7 +90,7 @@ func TestClaim_FirstCallWinsSecondCallDoesNotReclaim(t *testing.T) { ctx := context.Background() policy := createTestBudgetPolicy(t, repo, "ws-claim") - claimed, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert") + claimed, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert", policy.Revision) if err != nil { t.Fatalf("first claim: %v", err) } @@ -92,7 +98,7 @@ func TestClaim_FirstCallWinsSecondCallDoesNotReclaim(t *testing.T) { t.Fatal("first claim should win") } - claimed, err = repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert") + claimed, err = repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert", policy.Revision) if err != nil { t.Fatalf("second claim: %v", err) } @@ -106,15 +112,15 @@ func TestClaim_DifferentPeriodOrLevelIsIndependent(t *testing.T) { ctx := context.Background() policy := createTestBudgetPolicy(t, repo, "ws-claim-2") - if claimed, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert"); err != nil || !claimed { + if claimed, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert", policy.Revision); err != nil || !claimed { t.Fatalf("claim september alert: claimed=%v err=%v", claimed, err) } // A different period (new window) is unclaimed (AC-OFFICE-COSTS-002.7). - if claimed, err := repo.Claim(ctx, policy.ID, "2026-10-01T00:00:00Z", "alert"); err != nil || !claimed { + if claimed, err := repo.Claim(ctx, policy.ID, "2026-10-01T00:00:00Z", "alert", policy.Revision); err != nil || !claimed { t.Fatalf("claim october alert: claimed=%v err=%v", claimed, err) } // A different level in the same period is unclaimed. - if claimed, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "exceeded"); err != nil || !claimed { + if claimed, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "exceeded", policy.Revision); err != nil || !claimed { t.Fatalf("claim september exceeded: claimed=%v err=%v", claimed, err) } } @@ -123,7 +129,7 @@ func TestClaim_ForeignKeyViolationReturnsFalseWithNoError(t *testing.T) { repo, _ := newBudgetClaimsRepoWithFK(t) ctx := context.Background() - claimed, err := repo.Claim(ctx, uuid.NewString(), "lifetime", "alert") + claimed, err := repo.Claim(ctx, uuid.NewString(), "lifetime", "alert", 1) if err != nil { t.Fatalf("claim against a nonexistent policy must not be a store error, got: %v", err) } @@ -137,7 +143,7 @@ func TestDeleteBudgetPolicy_CascadesClaims(t *testing.T) { ctx := context.Background() policy := createTestBudgetPolicy(t, repo, "ws-cascade-1") - if _, err := repo.Claim(ctx, policy.ID, "lifetime", "alert"); err != nil { + if _, err := repo.Claim(ctx, policy.ID, "lifetime", "alert", policy.Revision); err != nil { t.Fatalf("claim: %v", err) } if got := countBudgetClaims(t, db, policy.ID); got != 1 { @@ -157,7 +163,7 @@ func TestDeleteWorkspaceData_CascadesBudgetClaims(t *testing.T) { ctx := context.Background() policy := createTestBudgetPolicy(t, repo, "ws-cascade-2") - if _, err := repo.Claim(ctx, policy.ID, "lifetime", "alert"); err != nil { + if _, err := repo.Claim(ctx, policy.ID, "lifetime", "alert", policy.Revision); err != nil { t.Fatalf("claim: %v", err) } @@ -174,7 +180,7 @@ func TestDeleteBudgetPoliciesForRemovedScopes_CascadesClaims(t *testing.T) { ctx := context.Background() policy := createTestBudgetPolicy(t, repo, "ws-cascade-3") - if _, err := repo.Claim(ctx, policy.ID, "lifetime", "alert"); err != nil { + if _, err := repo.Claim(ctx, policy.ID, "lifetime", "alert", policy.Revision); err != nil { t.Fatalf("claim: %v", err) } @@ -193,7 +199,7 @@ func TestUpdateBudgetPolicy_DiscardsClaims(t *testing.T) { ctx := context.Background() policy := createTestBudgetPolicy(t, repo, "ws-update-1") - if _, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert"); err != nil { + if _, err := repo.Claim(ctx, policy.ID, "2026-09-01T00:00:00Z", "alert", policy.Revision); err != nil { t.Fatalf("claim: %v", err) } if got := countBudgetClaims(t, db, policy.ID); got != 1 { @@ -204,6 +210,9 @@ func TestUpdateBudgetPolicy_DiscardsClaims(t *testing.T) { if err := repo.UpdateBudgetPolicy(ctx, policy); err != nil { t.Fatalf("update budget policy: %v", err) } + if policy.Revision != 2 { + t.Fatalf("revision returned to caller = %d, want 2", policy.Revision) + } if got := countBudgetClaims(t, db, policy.ID); got != 0 { t.Fatalf("claim count after update = %d, want 0 (AC-OFFICE-COSTS-002.8)", got) } @@ -215,6 +224,9 @@ func TestUpdateBudgetPolicy_DiscardsClaims(t *testing.T) { if updated.LimitSubcents != 2000 { t.Fatalf("limit_subcents = %d, want 2000", updated.LimitSubcents) } + if updated.Revision != 2 { + t.Fatalf("stored revision = %d, want 2", updated.Revision) + } } func TestListBudgetPolicies_OrdersByCreatedAtThenID(t *testing.T) { diff --git a/apps/backend/internal/office/repository/sqlite/loop_liveness_activation.go b/apps/backend/internal/office/repository/sqlite/loop_liveness_activation.go index 85d8822f3f..41863056fa 100644 --- a/apps/backend/internal/office/repository/sqlite/loop_liveness_activation.go +++ b/apps/backend/internal/office/repository/sqlite/loop_liveness_activation.go @@ -5,6 +5,7 @@ import ( "go.uber.org/zap" + "github.com/kandev/kandev/internal/db" "github.com/kandev/kandev/internal/persistence" ) @@ -37,7 +38,7 @@ func (r *Repository) activateLoopLiveness() { {"agent_wakeup_requests", causationIDColumn}, {"runs", causationIDColumn}, } { - exists, err := columnExists(r.db, probe.table, probe.column) + exists, err := db.ColumnExists(r.db, probe.table, probe.column) if err != nil { r.logActivationWarn("loop liveness schema probe failed", err) return diff --git a/apps/backend/internal/office/repository/sqlite/run_outcome_activation.go b/apps/backend/internal/office/repository/sqlite/run_outcome_activation.go index 52972c1969..d2cc057c4d 100644 --- a/apps/backend/internal/office/repository/sqlite/run_outcome_activation.go +++ b/apps/backend/internal/office/repository/sqlite/run_outcome_activation.go @@ -1,13 +1,11 @@ package sqlite import ( - "database/sql" - "fmt" "time" "go.uber.org/zap" - "github.com/kandev/kandev/internal/db/dialect" + "github.com/kandev/kandev/internal/db" "github.com/kandev/kandev/internal/persistence" ) @@ -35,7 +33,7 @@ func (r *Repository) activateRunOutcome() { r.logActivationWarn("ensure kandev_meta table failed", err) return } - exists, err := columnExists(r.db, "runs", "outcome") + exists, err := db.ColumnExists(r.db, "runs", "outcome") if err != nil { r.logActivationWarn("runs.outcome schema probe failed", err) return @@ -55,44 +53,3 @@ func (r *Repository) logActivationWarn(msg string, err error) { r.log.Warn(msg, zap.Error(err)) } } - -// columnExists reports whether table declares column, on either dialect. -// SQLite has no information_schema, so it falls back to PRAGMA table_info; -// Postgres is queried through the standard catalog view scoped to the -// current schema so callers never see another schema's same-named table. -func columnExists(db interface { - DriverName() string - Query(string, ...interface{}) (*sql.Rows, error) - QueryRow(string, ...interface{}) *sql.Row - Rebind(string) string -}, table, column string) (bool, error) { - if dialect.IsPostgres(db.DriverName()) { - var exists bool - err := db.QueryRow(db.Rebind(` - SELECT EXISTS ( - SELECT 1 FROM information_schema.columns - WHERE table_schema = current_schema() - AND table_name = ? - AND column_name = ? - ) - `), table, column).Scan(&exists) - return exists, err - } - rows, err := db.Query(fmt.Sprintf("PRAGMA table_info(%s)", table)) - if err != nil { - return false, err - } - defer func() { _ = rows.Close() }() - for rows.Next() { - var cid, notnull, pk int - var name, ctype string - var dflt sql.NullString - if err := rows.Scan(&cid, &name, &ctype, ¬null, &dflt, &pk); err != nil { - return false, err - } - if name == column { - return true, nil - } - } - return false, rows.Err() -} diff --git a/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/manifest.json b/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/manifest.json index 3a86cffd5a..05be637a04 100644 --- a/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/manifest.json +++ b/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/manifest.json @@ -5,7 +5,7 @@ { "engine": "sqlite3", "file": "sqlite.sql", - "sha256": "e1b490607af347314fce11bb3352bf91599eab4c651a1521a60142e73a3b2d5d", + "sha256": "b7c763f6c3f1f6000e8a8f90a123ee3b114582bdd3a0aabf162eedae18cf57a3", "sentinels": { "fixture.sentinel": "v0.93.0-sqlite-sentinel" }, @@ -37,7 +37,7 @@ { "engine": "pgx", "file": "postgres.sql", - "sha256": "f4f26d8d67610ef94a2d5cc308392be7a353efdd3e36e8469fb5ed11d774d165", + "sha256": "8dd108b73d1daaf9cd35ee2255f06f2800f90a7101ba3337c9ee4b0951a51de6", "sentinels": { "fixture.sentinel": "v0.93.0-postgres-sentinel" }, diff --git a/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/postgres.sql b/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/postgres.sql index 5a5e37e676..b7d21c0cb2 100644 --- a/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/postgres.sql +++ b/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/postgres.sql @@ -455,8 +455,6 @@ CREATE TABLE office_budget_policies ( updated_at timestamp without time zone NOT NULL ); - - CREATE TABLE office_channels ( id text NOT NULL, workspace_id text NOT NULL, @@ -2010,6 +2008,18 @@ ALTER TABLE ONLY office_budget_policies +CREATE TABLE office_budget_claims ( + policy_id text NOT NULL, + period_key text NOT NULL, + level text NOT NULL, + claimed_at timestamp without time zone NOT NULL, + CONSTRAINT office_budget_claims_pkey PRIMARY KEY (policy_id, period_key, level), + CONSTRAINT office_budget_claims_policy_id_fkey FOREIGN KEY (policy_id) + REFERENCES office_budget_policies(id) ON DELETE CASCADE +); + + + ALTER TABLE ONLY office_channels ADD CONSTRAINT office_channels_pkey PRIMARY KEY (id); diff --git a/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/sqlite.sql b/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/sqlite.sql index bba5d37111..304575b80f 100644 --- a/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/sqlite.sql +++ b/apps/backend/internal/persistence/storeconformance/testdata/upgrades/v0.93.0/sqlite.sql @@ -367,6 +367,14 @@ CREATE TABLE office_budget_policies ( created_at TIMESTAMP NOT NULL, updated_at TIMESTAMP NOT NULL ); +CREATE TABLE office_budget_claims ( + policy_id TEXT NOT NULL, + period_key TEXT NOT NULL, + level TEXT NOT NULL, + claimed_at TIMESTAMP NOT NULL, + PRIMARY KEY (policy_id, period_key, level), + FOREIGN KEY (policy_id) REFERENCES office_budget_policies(id) ON DELETE CASCADE +); CREATE TABLE office_channels ( id TEXT PRIMARY KEY, workspace_id TEXT NOT NULL, diff --git a/docs/plans/budget-claim-revision-fencing/plan.md b/docs/plans/budget-claim-revision-fencing/plan.md new file mode 100644 index 0000000000..fd64c32930 --- /dev/null +++ b/docs/plans/budget-claim-revision-fencing/plan.md @@ -0,0 +1,25 @@ +--- +created: 2026-09-13 +status: done +requirements: + - REQ-OFFICE-COSTS-002 +system_design: + - ../../specs/office/system-design/costs-03.md +--- + +# Implementation Plan: Budget Claim Revision Fencing + +This plan records the revision fence and atomic companion claim changes for +budget notifications. The work keeps the existing Office budget notification +contract and its idempotent claim store. + +## Work order + +- [Task 01: Fence budget claims to policy revisions](task-01-fence-budget-claims.md) + +## Outcome + +Budget claims carry the policy revision that earned them. Policy updates bump +the revision and discard old claims, stale evaluations cannot claim a newer +revision, and exceeded evaluations claim their alert companion in one +transaction. diff --git a/docs/plans/budget-claim-revision-fencing/task-01-fence-budget-claims.md b/docs/plans/budget-claim-revision-fencing/task-01-fence-budget-claims.md new file mode 100644 index 0000000000..fcb97af6d6 --- /dev/null +++ b/docs/plans/budget-claim-revision-fencing/task-01-fence-budget-claims.md @@ -0,0 +1,41 @@ +--- +id: "01-fence-budget-claims" +title: "Fence budget claims to policy revisions" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-OFFICE-COSTS-002 +acceptance_criteria: + - AC-OFFICE-COSTS-002.1 + - AC-OFFICE-COSTS-002.2 + - AC-OFFICE-COSTS-002.3 + - AC-OFFICE-COSTS-002.5 + - AC-OFFICE-COSTS-002.8 + - AC-OFFICE-COSTS-002.10 + - AC-OFFICE-COSTS-002.11 + - AC-OFFICE-COSTS-002.13 + - AC-OFFICE-COSTS-002.14 +system_design: + - ../../specs/office/system-design/costs-03.md +--- + +# Task 01: Fence Budget Claims to Policy Revisions + +## Scope + +- Add a server-owned revision counter to each budget policy. +- Include the evaluated revision in the durable claim key and fence inserts to + the current policy row. +- Recreate the legacy three-column claim table once when an upgraded database + needs the four-column key. +- Claim the exceeded level and its alert companion atomically. +- Return the stored revision from create and update operations and cover the + SQLite and PostgreSQL persistence paths. + +## Verification + +- Office cost, repository, and store conformance tests. +- PostgreSQL-gated revision and migration tests when a PostgreSQL DSN is + available. diff --git a/docs/specs/budget-claim-revision-fencing/amendments.md b/docs/specs/budget-claim-revision-fencing/amendments.md new file mode 100644 index 0000000000..07f6ea33f8 --- /dev/null +++ b/docs/specs/budget-claim-revision-fencing/amendments.md @@ -0,0 +1,51 @@ +# Budget claim fencing: amendments to REQ-OFFICE-COSTS-002 + +Companion to [spec.md](spec.md) (`REQ-OFFICE-COSTS-003`), alongside +[verification.md](verification.md), [prior-art.md](prior-art.md) and +[input-inventory.md](input-inventory.md). Split out for the specification linter's +per-file size ceiling; the five files are one spec. + +Every entry below amends a criterion of `REQ-OFFICE-COSTS-002`. Nothing here is a new +acceptance criterion of `REQ-OFFICE-COSTS-003`; those are all in +[spec.md](spec.md#acceptance-criteria). + +This spec is a delta on a contract that is itself unmerged. Each amendment is named so +Spec Review can check it against #3287 rather than re-deriving it. + +- **`AC-OFFICE-COSTS-002.8`** — its stale-evaluation carve-out ("An evaluation already in + flight when the discard commits ... may therefore insert a claim keyed to the pre-update + period and level, suppressing the first post-update notification; that outcome is + permitted rather than prevented") is **superseded**. `AC-OFFICE-COSTS-003.8`/`.9`/`.15` + now prevent it. The criterion's atomicity requirement is unchanged and now also covers + the revision bump. The rejection of a lock spanning evaluation and update stands. +- **`AC-OFFICE-COSTS-002.5`** — the companion alert claim is no longer a separate, + independently-failing write. Its "conditional on the claim store accepting that companion + write" clause and the consequence it describes ("the de-escalation protection lapses for + that one period") are **removed**: under `AC-OFFICE-COSTS-003.10` the pair is atomic, so + a companion failure is a pair failure and lands on the ordinary + `AC-OFFICE-COSTS-002.14` fail-open path. Its precedence rule is unchanged. A + fault-injection test still shall not assert the companion claim was recorded. +- **`AC-OFFICE-COSTS-002.10`** — unchanged in force. Its scope widens: "exactly one + submission" now also holds for the previously unaddressed alert-band-versus-over-limit + pair, in the direction stated by `AC-OFFICE-COSTS-003.12`. +- **`AC-OFFICE-COSTS-002.13`** — unchanged in meaning; one case joins its enumeration: an + evaluation whose claim was refused on a superseded revision reports `submitted` false, + because no row went out. Its existing rule already settles this. +- **`AC-OFFICE-COSTS-002.14a`** — unchanged in outcome; the mechanism moves from the + foreign-key classifier to the fence's existence check, which subsumes it. +- **`AC-OFFICE-COSTS-002.14`** — unchanged in force; its scope widens twice. The invalid + claim inputs of `AC-OFFICE-COSTS-003.13` become claim-store errors it governs, and the + atomic pair of `AC-OFFICE-COSTS-003.10` is a single claim attempt under it, so a pair + failure is one fail-open emission, one log and one counter increment rather than two; + `AC-OFFICE-COSTS-003.14` fixes which level that log names. Its exclusion of a failed + claim *discard* is unchanged and now also covers a failed revision bump, which rides the + same transaction and is reported to the caller by `AC-OFFICE-COSTS-002.8`. +- **`AC-OFFICE-COSTS-002.15`** — not amended. A claim still survives a spend that falls + back below the level it was taken at; revision changes nothing there, because only an + update or a period rollover releases a claim and both already did. +- **Not amended:** `.1`, `.2`, `.2a`, `.3`, `.4`, `.4a`, `.6`, `.6a`, `.7`, `.9`, `.11`, + `.12`, `.16`, `.17`, and every entry in `REQ-OFFICE-COSTS-002`'s `## Out of scope`. +- **Documentation follow-up, not this card:** once #3287 merges, fold these criteria into + `docs/specs/office/requirements/costs.md` as `REQ-OFFICE-COSTS-003` with a matching + system-design part and retire this slug directory. Editing that file now would conflict + with the unmerged PR this spec depends on. diff --git a/docs/specs/budget-claim-revision-fencing/input-inventory.md b/docs/specs/budget-claim-revision-fencing/input-inventory.md new file mode 100644 index 0000000000..d7d635abc7 --- /dev/null +++ b/docs/specs/budget-claim-revision-fencing/input-inventory.md @@ -0,0 +1,60 @@ +# Budget claim fencing: input inventory + +Companion to [spec.md](spec.md) (`REQ-OFFICE-COSTS-003`), alongside +[verification.md](verification.md), [prior-art.md](prior-art.md) and +[amendments.md](amendments.md). Split out for the specification linter's per-file size +ceiling; the five files are one spec. + +Every shape below was read out of the code on #3287's head, not inferred. It is the +sampled state Build starts from — reference, not acceptance criteria. + + +Sampled by reading the code, not inferred. + +**`office_budget_policies`** (`repository/sqlite/base.go`) — columns `id, workspace_id, +scope_type, scope_id, limit_subcents, period, alert_threshold_pct, action_on_exceed, +created_at, updated_at`. **No revision or version column.** `GetBudgetPolicy` and +`ListBudgetPolicies` both use `SELECT *` with `sqlx` `StructScan`/`SelectContext` into +`models.BudgetPolicy`, so adding a column **requires** a matching struct field — `sqlx` +fails with `missing destination name ` otherwise. This is a hard coupling, not a +style choice. + +**`office_budget_claims`** (added by #3287) — `policy_id, period_key, level, claimed_at`, +`PRIMARY KEY (policy_id, period_key, level)`, `FOREIGN KEY (policy_id) REFERENCES +office_budget_policies(id) ON DELETE CASCADE`. Created by `createCostTables()` with +`CREATE TABLE IF NOT EXISTS`, **after** `office_budget_policies` (Postgres rejects the +forward reference). `Repository.Claim` is `INSERT ... ON CONFLICT(policy_id, period_key, +level) DO NOTHING`; `claimed` is `RowsAffected() == 1`; `db.IsForeignKeyViolation(err)` +maps to `(false, nil)`. + +**`Repository.UpdateBudgetPolicy`** (#3287) — `BeginTxx`; `DELETE FROM +office_budget_claims WHERE policy_id = ?`; `UPDATE office_budget_policies SET ... WHERE id += ?`; `Commit`. A test-only failpoint `failBudgetPolicyUpdateErr` sits between the two. + +**`evaluatePolicy`** (#3287) — reads the clock once, derives `boundary` once, derives +`periodKey` once, then `switch { case spent >= limit: ...; case spent >= threshold: ... }`. +The exceeded branch calls `claimAndEmit(..., claimLevelExceeded, func() { +s.claimCompanionAlert(...) })`; the `afterClaim` callback runs **between** the exceeded +claim and the emission decision. This callback seam is the mechanism Race B exploits. + +**`office_activity_log`** — `id TEXT PRIMARY KEY` (a random UUID), `created_at TIMESTAMP`, +index `(workspace_id, created_at DESC)`. Every reader orders `ORDER BY created_at DESC` +with **no tiebreak column**. `CreateActivityEntry` sets `created_at = time.Now().UTC()` at +write time, only when the caller left it zero. `ActivityLogger.LogActivity` returns +nothing. + +**Budget surfaces on current `main`** — `dashboard/service_inbox.go:170` +`listBudgetInboxEntries` lists `budget.alert` **and** `budget.exceeded` as two separate +`ListActivityEntriesByAction` calls concatenated (`append(alerts, exceeded...)`), each +already sorted internally — **not** interleaved by timestamp. That arrived with PR #3276 +(`677e01204`), which merged after #3287 branched, so #3287's statement that "only +`budget.alert` feeds the Office inbox list" is **stale against `main`**. +`apps/web/.../activity/activity-feed.tsx` renders `res.activity` in server order with no +client-side sort, so the **activity feed** is the one surface where the two rows interleave +and `created_at` alone decides. + +**`updateBudget` handler** (`costs/handler.go:168`) — `GetBudgetPolicy` → +`applyBudgetPatch` → `UpdateBudgetPolicy`, an unfenced read-modify-write that returns the +in-memory `policy` as the response body. `models.BudgetPolicy` is mapped by hand to +`apps/web/lib/state/slices/office/types.ts:138` in camelCase, which ignores unrecognised +JSON fields. diff --git a/docs/specs/budget-claim-revision-fencing/prior-art.md b/docs/specs/budget-claim-revision-fencing/prior-art.md new file mode 100644 index 0000000000..6961fd3ff3 --- /dev/null +++ b/docs/specs/budget-claim-revision-fencing/prior-art.md @@ -0,0 +1,77 @@ +# Budget claim fencing: prior art + +Companion to [spec.md](spec.md) (`REQ-OFFICE-COSTS-003`), alongside +[verification.md](verification.md), [input-inventory.md](input-inventory.md) and +[amendments.md](amendments.md). Split out for the specification linter's per-file size +ceiling; the five files are one spec. This file +is the research record behind [Design decisions](spec.md#design-decisions) — it is +provenance, not contract, and nothing in it is an acceptance criterion. + + +Three legs, each opening with a receipt. + +### Our own prior reasoning (wiki) — searched, unavailable on this runner + +Receipt: `wiki-query` could not be run. `ls -d ~/.claude/skills/*wiki*` → no match; +`~/.obsidian-wiki/` does not exist; `OBSIDIAN_VAULT_PATH` is unset; `qmd` is not on +`PATH`. This card runs on the **neo** SSH executor, whose home is `/Users/neo`; neither the +vault nor the `qmd` MCP server is provisioned here. Intended query: idempotent alerting, +fencing tokens, crossing-versus-level notification semantics. Nothing retrieved and nothing +claimed — the forks below are **not** established as unopposed by prior positions; re-run +with vault access before treating them as settled. #3287's own `costs-03.md` recorded the +same negative result from a different machine for a different cause, so two independent +runs have now failed this leg. + +### What other products shipped (saas-kb) — unavailable + +Receipt: the `saas-kb` MCP server and its `search_fsm_docs` tool are absent from this +session's tool list; no tool discovery is exposed. Intended query: `category: "ai_sdlc"`, +budget/spend-limit alerting and its concurrency handling in agent platforms (Devin, +OpenHands, Warp, Factory.ai, Augment). Nothing retrieved; not substituted. + +### In-repo prior art — found, and it decides the mechanism + +Receipt: `grep -rn "revision\|generation" apps/backend/internal --include='*.go'`, plus +reading each hit's write path. Three implementations of this fencing pattern already +exist, and they agree on all four details: + +| Site | Column | Bump | Fence | +| --- | --- | --- | --- | +| `office/repository/sqlite/workspace_groups.go:233,267` | `ownership_generation INTEGER NOT NULL DEFAULT 1` | `task/repository/sqlite/task.go:1555` `= ownership_generation + 1` | `WHERE id = ? AND ownership_generation = ?`, `(bool, error)` from `RowsAffected() == 1` | +| `gitlab/store_config.go:57` | `revision` | `= gitlab_configs.revision + 1` | `RETURNING revision` reads the post-bump value back | +| `user/store/sqlite.go:342` | `settings_revision` | `= settings_revision + 1` | `WHERE id = ? AND settings_revision = ?` + `RETURNING` — a full CAS | + +`ClaimWorkspaceGroupCleanup`'s comment states the governing principle: *"The active-member +predicate is part of the same write so a caller cannot race a membership admission between +its read and this claim."* That is Race A, already solved, one package away. It is on +`main` but not on #3287's head, which branched earlier — read it there, not on the build +base. + +A fourth precedent decides the *migration* shape rather than the fence. +`office/repository/sqlite/run_outcome_activation.go` guards a schema-dependent action +behind a `columnExists` probe and states why in its own comment: `MigrateLogger.Apply` +"swallows failures at WARN, so this probe is the only thing standing between a failed +migration and a consumer wrongly believing the mechanism is live." That helper sits in the +same package as `office_budget_claims`; it and its reasoning carry +`AC-OFFICE-COSTS-003.3a`. + +**Researched, then cut from the contract — provenance for deferred work.** Serializing the +recreate so two initializers cannot both perform it was researched here and then excluded; +it lives in [spec.md's Out of scope](spec.md#out-of-scope), and this paragraph is the record +that seeds it. `task/repository/sqlite/worktree_ownership_migration.go` is the repository's +one-time destructive cutover. It probes for a legacy artifact with `tableExists` and returns +early when it is gone, takes a fixed advisory lock whose comment states the rule — +*"All instances must derive the same bigint so a second boot waits for the first to finish +the cutover instead of racing it"* — and, in its own words, "never uses the best-effort +MigrateLogger path, whose contract swallows unexpected failures." It is the only advisory-lock +precedent in play: `run_outcome_activation.go` above takes no lock. A follow-up should read it +there and choose a new lock id rather than reusing `taskWorktreeCutoverLockID`. + +**What this spec takes:** the column shape, the in-SQL `+ 1` bump (never +read-modify-write in Go), the predicate-inside-the-write fence, `(claimed bool, err error)` +read from `RowsAffected`, and `RETURNING revision` for the read-back. **What it departs +from:** `users.settings_revision` fences a *user-facing* write against a client-supplied +expected revision. This spec's revision is **server-assigned and never accepted from a +client** — it fences a background evaluation against a user-facing write, the opposite +direction. Client-supplied would make it optimistic concurrency for the budget PATCH +endpoint, excluded under [spec.md's Out of scope](spec.md#out-of-scope). diff --git a/docs/specs/budget-claim-revision-fencing/spec.md b/docs/specs/budget-claim-revision-fencing/spec.md new file mode 100644 index 0000000000..7ba851e13d --- /dev/null +++ b/docs/specs/budget-claim-revision-fencing/spec.md @@ -0,0 +1,456 @@ +# Budget claim fencing: a claim must belong to the policy revision that earned it + +**Status:** spec +**Area:** backend — `internal/office/costs` (evaluation), `internal/office/repository/sqlite` +(claim store, policy store), `internal/office/models` (policy struct) +**Slug:** `budget-claim-revision-fencing` +**Requirement id:** `REQ-OFFICE-COSTS-003` +**Depends on:** PR #3287 (`REQ-OFFICE-COSTS-002`) — **open, unmerged**; see +[Dependency and sequencing](#dependency-and-sequencing) +**Amends:** `AC-OFFICE-COSTS-002.5`, `.8`, `.10`, `.13`, `.14a` as introduced by PR #3287 — +see [Amendments](#amendments-to-req-office-costs-002) + +--- + +## Why + +`REQ-OFFICE-COSTS-002` (PR #3287) makes `budget.alert` / `budget.exceeded` idempotent with +a durable claim table keyed `(policy_id, period_key, level)`. Two reviewers found the same +structural gap from opposite ends: **the claim key carries no evidence of which policy +revision earned it, nor of what else has already been claimed for that period.** #3287 +deliberately accepted both (`AC-OFFICE-COSTS-002.8`'s stale-evaluation carve-out). This +spec closes them. + +**Race A — stale-snapshot claim poisoning** +([coderabbitai](https://github.com/kdlbs/kandev/pull/3287#discussion_r3914827075)). +`CheckBudget` calls `ListBudgetPolicies` once, then hands each already-read +`*BudgetPolicy` to `evaluatePolicy`, so an evaluation works from a snapshot. If +`UpdateBudgetPolicy` discards that policy's claims and commits new values mid-evaluation, +the evaluation's `Claim` insert lands **after** the discard and writes the old threshold's +outcome into the new revision's claim slot. Two user-visible harms: + +- **(A1) obsolete emission** — a `budget.alert` row whose `limit_subcents` describes the + policy the user just replaced; +- **(A2) suppressed legitimate crossing** — the next evaluation that genuinely reaches a + level under the *new* policy finds the slot taken and stays silent until the next update + or period rollover. + +**Race B — `budget.alert` submitted after `budget.exceeded`** +([cubic-dev-ai](https://github.com/kdlbs/kandev/pull/3287#discussion_r3914856011)). +`claimAndEmit`'s `afterClaim` seam runs `claimCompanionAlert` *after* the exceeded claim, +not atomically with it. An alert-band evaluation racing an over-limit one can win the alert +claim in that gap and emit `budget.alert` describing a below-limit spend after +`budget.exceeded` has gone out. Both rows land once each; the defect is that a period +already over its limit still produces an alert-band notification at all. + +## Dependency and sequencing + +**This work cannot be built on `origin/main`.** Verified 2026-09-07 at `origin/main` = +`cd7823631`: `office_budget_claims`, `Repository.Claim`, `periodKeyFor` and +`BudgetCheckResult.AlertSubmitted` do not exist there, and `gh pr view 3287` reports +`"state":"OPEN"`, `"mergedAt":null`. Every symbol this spec modifies is introduced by +#3287 (head `feature/budget-alerts-should-c2t`). + +Build shall branch from #3287's head, not `main`, and shall record which commit in the task +plan. If #3287 has merged by then, branch from `main` as normal. This is a sequencing fact, +not an open question: no contract below turns on it except `AC-OFFICE-COSTS-003.3`, +where it selects only which file the claim-table DDL is edited in — never what schema the +running database ends up with, which `AC-OFFICE-COSTS-003.3a` settles on every boot. + +## Prior art + +Three legs, each opening with a receipt, in [prior-art.md](prior-art.md): the wiki leg and +the saas-kb leg both came back **unavailable on this runner** and are recorded there with +their receipts and intended queries, so neither fork below is established as unopposed by +prior positions. The in-repo leg **found four precedents and decided both mechanisms** — +three agreeing fencing implementations (`ownership_generation`, `gitlab_configs.revision`, +`users.settings_revision`) that fix the column shape, the in-SQL `+ 1` bump, the +predicate-inside-the-write fence and the `RETURNING` read-back, and +`run_outcome_activation.go`'s `columnExists` probe, which fixes the migration shape behind +`AC-OFFICE-COSTS-003.3a`. Where this spec departs from a precedent is stated there too. + +## Input inventory + +Sampled by reading the code on #3287's head, not inferred, and recorded in +[input-inventory.md](input-inventory.md): the two tables and their exact columns, the +`SELECT *` / `StructScan` coupling that makes `AC-OFFICE-COSTS-003.2` mandatory rather than +cosmetic, `UpdateBudgetPolicy`'s transaction shape and its test-only failpoint, +`evaluatePolicy`'s snapshot-then-claim ordering and the `afterClaim` seam Race B exploits, +`office_activity_log`'s missing tiebreak column, the two budget surfaces on `main` +(including the PR #3276 change that makes #3287's inbox claim stale), and the `updateBudget` +handler's unfenced read-modify-write. + +## Design decisions + +**Fork 1 — fence, or serialize?** coderabbitai offered both. **Fence.** #3287's +`AC-OFFICE-COSTS-002.8` already rejected serialization with a reason this spec accepts +verbatim: a lock spanning the evaluation and the update *"would put a user-facing write +behind a background evaluation."* A revision fence costs one column and one predicate and +holds no lock across the evaluation. + +**Fork 2 — reuse `updated_at`, or add a column?** **Add `revision`.** `updated_at` is +already bumped on every update, so it looks free, but two updates inside one clock tick +produce the same value, timestamp round-tripping through the driver is not guaranteed to +survive exact-equality comparison, and a backwards clock step breaks monotonicity. An +`INTEGER` counter has none of those failure modes, and all three in-repo precedents chose +one. + +**Fork 3 — is `revision` a fence only, or also part of the claim's key? Both, and both are +load-bearing.** The fence alone is sufficient on SQLite, whose single writer connection +(`internal/db/pool.go`) serializes the claim insert against the update transaction. It is +**not** sufficient on PostgreSQL under `READ COMMITTED`: a claim insert whose `WHERE +EXISTS` reads the pre-update revision before the update commits will insert a row that the +update's already-executed `DELETE` cannot remove, leaving a stale claim behind. Putting +`revision` in the claim's primary key neutralises that survivor — it is keyed to a +superseded revision and can never match a later evaluation. So: **the fence prevents the +obsolete emission (A1); the revision in the key prevents the suppression (A2).** Removing +either reopens one half on one dialect. + +This does not contradict #3287's exclusion of `workspace_id` from the claim row. That +exclusion was about *duplicating a fact the policy already owns*, which could drift. +`revision` on a claim is not a copy of the policy's current revision — it is the identity +of *which* revision earned this claim, which is a different fact and cannot drift. + +**Fork 4 — how to close Race B.** cubic proposed making the exceeded and companion claims +atomic, *or* making alert claims reject an already-claimed exceeded level. **Atomicity is +necessary; the rejection rule is not, once atomicity holds.** Atomicity is also +*insufficient* as a fix for the ordering symptom — worth stating, because that is the +obvious reading of the comment: if the alert-band evaluation wins the alert claim *first*, +atomicity of the exceeded pair changes nothing and both rows still go out unordered. What +atomicity buys is the invariant **"an exceeded claim implies an alert claim at the same +revision"**; given it, an alert-band evaluation arriving after an exceeded claim finds the +alert row present and suppresses via the ordinary primary-key conflict. The rejection rule +would be a second, divergent answer to a question the key already answers — the pattern +#3287 explicitly refuses. + +**Fork 5 — the residual write-order inversion is excluded, deliberately.** Rationale and +bound in [Out of scope](#out-of-scope). + +## Contract + +### REQ-OFFICE-COSTS-003: Budget claims are fenced to the policy revision that earned them + +**Intent:** A budget claim shall be valid only for the policy revision the evaluation +actually read, and an evaluation whose snapshot has been superseded shall neither emit nor +suppress. + +**User story:** As a workspace owner who has just raised a budget limit, I want the next +real crossing of the *new* limit to notify me, and I do not want a notification quoting +the limit I replaced. + +#### Acceptance criteria + +Terminology (`evaluation period`, `alert level`, `limit level`, `reaches a level`, `claim`, +`emit`, `submit`) is inherited unchanged from `REQ-OFFICE-COSTS-002`'s `## Terminology`. +**Policy revision** is added: a per-policy integer that identifies one immutable set of +policy field values, assigned by the server, starting at 1 and increasing by exactly 1 on +each successful update. + +- **AC-OFFICE-COSTS-003.1:** `office_budget_policies` shall carry a `revision` column, + `INTEGER NOT NULL DEFAULT 1`, added by a replayable `ALTER TABLE ... ADD COLUMN` + migration registered through the existing `r.migrate.Apply(name, sql)` mechanism, so + that existing rows are backfilled to 1 and a boot replay is a no-op. No new local + error-string classifier shall be introduced; `db.IsDuplicateColumnError` is the only + one (ADR 0027). +- **AC-OFFICE-COSTS-003.2:** `models.BudgetPolicy` shall carry the matching field, so + that the existing `SELECT *` reads in `GetBudgetPolicy` and `ListBudgetPolicies` keep + scanning. The field shall be serialized in the policy's JSON representation as a + read-only diagnostic under the JSON key `revision`; the frontend `BudgetPolicy` type + need not change, and the field shall not be suppressed from JSON. +- **AC-OFFICE-COSTS-003.3:** `office_budget_claims` shall carry a `revision INTEGER NOT + NULL` column, and its primary key shall be `(policy_id, period_key, level, revision)`. + `createCostTables` shall declare that shape, **and** the recreate of + `AC-OFFICE-COSTS-003.3a` — dropping `office_budget_claims` and recreating it with that + same four-column primary key — shall run on every boot, whatever #3287's state. Declaring the + final shape is not sufficient on its own, and an implementation that relies on it alone + does not satisfy this criterion: `createCostTables` issues `CREATE TABLE IF NOT EXISTS`, + which is a no-op against a database that has already executed #3287's three-column + `CREATE TABLE`. Such a database would keep the three-column table and its three-column + primary key, so every fenced insert would fail on the missing column and every + evaluation would take the `AC-OFFICE-COSTS-002.14` fail-open path from then on — + emitting a duplicate notification, an error log and a counter increment on each pass, + violating `AC-OFFICE-COSTS-002.11`, and leaving this requirement's fence permanently + inert — while a suite that only ever builds a fresh database stays green. + `AC-OFFICE-COSTS-003.3a`'s probe is what closes that, and it costs nothing where the old + shape never existed: it finds `revision` present and does nothing, which is exactly its + second-boot behavior. Whether #3287 has merged — `gh pr view 3287 --json state`, the + only check Build shall consult, deployment state being neither observable from the + repository nor to be consulted — therefore selects only **where the final-shape DDL is + edited**: #3287's own `CREATE TABLE` statement while it is open, the same statement on + `main` once it has merged. It does not decide whether the recreate runs. Deciding that + from a probe of the live schema rather than from the fork is the point: the fork is + answered when Build authors the change, whereas the shape of the database this code + meets is settled later, when it deploys. No `r.migrate.Apply` migration shall be added + for this table; `AC-OFFICE-COSTS-003.3a` is expressly not one. Claims are pure + suppression state, and losing them costs at most one additional notification per policy + per period, which + `AC-OFFICE-COSTS-002.17` already describes as the accepted first-run behavior. +- **AC-OFFICE-COSTS-003.3a:** The recreate path shall take effect at most once per + database. It shall first probe whether `office_budget_claims` already declares + `revision` — the office repository's existing `columnExists` helper answers this on both + dialects — and shall do nothing when it does. It shall **not** be expressed as an + `r.migrate.Apply` statement: that runner re-executes its SQL on every boot and is + idempotent only because an already-exists error is swallowed, so an unguarded + `DROP TABLE` / `CREATE TABLE` pair succeeds on every boot and destroys claim state each + time, violating `AC-OFFICE-COSTS-002.11`. It shall return its error to its caller rather + than swallowing it, so a failed recreate is visible rather than silently leaving the old + three-column key in place. It shall run after `office_budget_claims` exists, so it probes + the real table rather than racing its creation. A claim attempt arriving from an + already-running backend while the recreate is in flight is **not** required to succeed: + it is an ordinary claim-store error and takes the `AC-OFFICE-COSTS-002.14` fail-open path, + costing one further notification per evaluation that attempts a claim inside the recreate + window, beyond the one `AC-OFFICE-COSTS-003.3` already accepts for discarding the claims + themselves: that path emits without recording a claim, so it does not suppress the next + healthy evaluation. This criterion therefore does not require the recreate to lock + concurrent claim traffic out, and an implementation that does lock it out satisfies it + equally. +- **AC-OFFICE-COSTS-003.4:** When a budget policy is created, the system shall assign it + revision 1, regardless of any revision value presented for creation — zero, or any other. + The system shall make that assigned value available to the caller on the created policy, + for the same reason `AC-OFFICE-COSTS-003.5` requires the post-update value to be read back: + the create response carries the policy, and `AC-OFFICE-COSTS-003.2` makes `revision` a + field of it, so a create that stores 1 while handing its caller a zero would report a + revision the database does not hold. Satisfying this criterion by the column's `DEFAULT 1` + alone therefore does not satisfy it. +- **AC-OFFICE-COSTS-003.5:** When a budget policy is updated, the system shall increase + that policy's revision by exactly 1, in the same transaction as the row update and the + claim discard that `AC-OFFICE-COSTS-002.8` requires. The increase shall be expressed as + an in-SQL `revision = revision + 1`, never as a value computed in application code from + a prior read, so that two concurrent updates cannot both write the same revision. The + system shall read the post-update revision back within that transaction and shall make + it available to the caller, so that the value returned to whoever performed the update + is the value now stored. If the update matches no policy row — the policy was deleted + between the caller's read and this write — the read-back returns nothing, and the system + shall roll the transaction back and return an error to the caller rather than committing + an empty update. This tightens the pre-`REQ-OFFICE-COSTS-003` behavior, where the update + ignored its affected-row count and reported success. The obligation is at the repository + boundary only: the `updateBudget` handler already maps every update error to HTTP 500, + and giving this particular case a 404 instead is a separate user-facing change, excluded + under [Out of scope](#out-of-scope). +- **AC-OFFICE-COSTS-003.6:** If an update transaction is rolled back for any reason, then + the policy's revision shall be unchanged. A rolled-back update shall not consume a + revision number. +- **AC-OFFICE-COSTS-003.7:** The system shall not accept a revision from an API client. + The budget-policy update request body shall have no revision field, and supplying one + shall have no effect on the stored revision. +- **AC-OFFICE-COSTS-003.8:** When an evaluation attempts a claim **whose outcome decides + an emission**, it shall present the revision from the same policy snapshot it computed + spend and levels from, and the claim shall be recorded only while that revision is still + the policy's stored revision. The revision check and the insert shall be a single + statement, so no evaluation can observe a matching revision and then insert against a + superseded one. That single-statement rule governs the fence check only. It does not + forbid the input guard `AC-OFFICE-COSTS-003.13` requires, which shall run **before** the + statement is issued: once the statement has run, a zero-row result carries no distinction + between a refused fence, a policy that no longer exists and an already-held claim, all + three being the silent miss of `AC-OFFICE-COSTS-003.9`, so any outcome that must be told + apart from them has to be settled before the statement rather than read out of it. A + claim that decides no emission — the companion of `AC-OFFICE-COSTS-003.10` — is exempt; + see there. +- **AC-OFFICE-COSTS-003.9:** When a claim is refused because the presented revision is no + longer the policy's stored revision, the system shall treat it exactly as + `AC-OFFICE-COSTS-002.3` treats an already-held claim: no activity row, no error log, no + counter increment, and the evaluation shall report `submitted` false for that level and + shall still return its result to its caller without error. Enforcement + (`AC-OFFICE-COSTS-002.12`) is unaffected: `pause_agent` and the pre-execution denial + still run off the level check. +- **AC-OFFICE-COSTS-003.10:** When an evaluation reaches the limit level, the system shall + record the exceeded-level claim and the companion alert-level claim of + `AC-OFFICE-COSTS-002.5` **atomically**: once the transaction commits either both rows + exist for that policy, period and revision, or this evaluation wrote neither. The + `budget.exceeded` emission shall be decided by + whether the exceeded-level row was inserted by this evaluation; the companion row's + outcome shall not change it. The fence shall be evaluated **once for the pair**, on the + exceeded-level insert; the companion insert shall carry no fence of its own, because its + key already names the revision, so a companion row written against a revision that has + since moved is inert rather than wrong. Re-checking the fence on the companion insert is + prohibited: it could refuse there after succeeding on the exceeded insert, breaking + `AC-OFFICE-COSTS-003.11`. Two outcomes of the pair are named here so that neither is left + to inference. **A companion insert that conflicts with an alert-level claim already held + for that policy, period and revision is a success for the pair,** not a failure: that row + is the ordinary trace of an earlier alert-band evaluation, `AC-OFFICE-COSTS-003.11`'s + invariant is satisfied by its presence however it arrived, and the transaction shall + commit and the `budget.exceeded` row shall go out. Reading the companion's zero affected + rows as a pair failure is prohibited — it would withhold a `budget.exceeded` that the + exceeded insert had already earned, which `AC-OFFICE-COSTS-002.5` forbids. **When the + exceeded insert itself writes no row** — refused by the fence, or conflicting with an + exceeded claim already held — the companion insert shall not be attempted and the pair + shall commit having written nothing: there is no emission to protect, and + `AC-OFFICE-COSTS-003.11` is not engaged. +- **AC-OFFICE-COSTS-003.11:** The system shall maintain the invariant: **if an + exceeded-level claim exists for a given policy, period and revision, an alert-level + claim exists for that same policy, period and revision.** No operation shall create an + exceeded-level claim without it. +- **AC-OFFICE-COSTS-003.12:** While an exceeded-level claim is held for a policy, period + and revision, an evaluation of that policy whose spend reaches the alert level shall not + emit `budget.alert` for that period and revision. This shall follow from + `AC-OFFICE-COSTS-003.11` and the claim's primary key, and shall not be implemented as a + separate read of the exceeded claim before emitting. +- **AC-OFFICE-COSTS-003.13:** If a claim attempt presents a revision less than or equal to + zero, an empty policy id, an empty period key, or an empty level, then the system shall + report a claim-store error, which `AC-OFFICE-COSTS-002.14` then handles by emitting, + logging and incrementing the counter. Those four are exactly the components of the claim's + primary key, and the guard covers all four rather than a subset: such input is a + programming error rather than a superseded snapshot, and shall degrade toward an extra + notification, never toward silence. An empty level leaves + `AC-OFFICE-COSTS-003.14`'s log unambiguous — that field names the level whose emission the + failed claim governs, which is fixed by the calling branch rather than by the rejected + value. +- **AC-OFFICE-COSTS-003.14:** The claim-store failure counter shall keep its single label + value `op=claim`. The accompanying structured log's `level` field shall carry the level + whose *emission* the failed claim governs — `exceeded` for the atomic pair of + `AC-OFFICE-COSTS-003.10`, `alert` for an alert-band claim — so the field never carries a + sentinel or two meanings. +- **AC-OFFICE-COSTS-003.15:** A claim row whose revision is no longer the policy's stored + revision shall never suppress an emission and shall not be read for any decision. The + claim discard of `AC-OFFICE-COSTS-002.8` shall remove **all** of a policy's claims + regardless of revision, and shall not be narrowed to the current revision, so such rows + are reclaimed by the next update or by the policy's deletion cascade. No retention job + shall be added. + +## Forced-to-invent pass + +**Ordering.** Policy evaluation order is unchanged: `created_at ASC, id ASC` +(`AC-OFFICE-COSTS-002.16`). Within the update transaction: discard claims, update the row +(including the revision bump), read the revision back, commit — discard first, as #3287 +specifies. Within `AC-OFFICE-COSTS-003.10`'s atomic pair the exceeded row is inserted +first, because its insertion decides the emission; the companion alert row follows in the +same transaction. The relative order of the two resulting `office_activity_log` rows is +**not** specified — see [Out of scope](#out-of-scope). + +**Idempotency and retry.** Nothing here is retried. A refused claim (already held, +superseded revision, deleted policy) is terminal for that evaluation; the next trigger +re-evaluates from a fresh snapshot. Re-running an identical evaluation after a successful +claim produces no second row. An update with unchanged field values still bumps the +revision and still discards claims — the revision counts *update operations*, not *value +changes*, matching `AC-OFFICE-COSTS-002.8`'s refusal to compare old and new values. + +**Concurrency — two callers, same row.** Enumerated exhaustively: + +1. *Two evaluations, same policy, period, level, same revision, healthy store.* The + primary key admits one. Exactly one submission (`AC-OFFICE-COSTS-002.10` preserved). +2. *Evaluation vs. `UpdateBudgetPolicy`, evaluation claims after the update commits.* The + fence refuses. No obsolete row; the claim slot for the new revision stays open, so the + next evaluation that reaches a level emits. **Race A closed.** +3. *Evaluation vs. `UpdateBudgetPolicy`, evaluation claims before the update commits.* The + claim is accepted and the row emitted — correctly, since the policy still held those + values at that instant. The update's discard then removes the claim (SQLite), or it + survives keyed to the superseded revision and is inert (PostgreSQL `READ COMMITTED`). + Either way no later evaluation is suppressed. +4. *Alert-band evaluation vs. over-limit evaluation, exceeded claims first.* The + alert-band evaluation finds the companion alert row present and suppresses. Only + `budget.exceeded` is emitted. **Race B's semantic half closed.** +5. *Alert-band evaluation vs. over-limit evaluation, alert claims first.* Both emit, once + each. Their activity-log order is unconstrained — the named residual. +6. *Two concurrent `UpdateBudgetPolicy` calls.* Serialized by the single writer (SQLite) + or the row lock (PostgreSQL). Each bumps by 1; the final revision is `r+2`; no + increment is lost. Which caller's *field values* win is the pre-existing unfenced + read-modify-write in `updateBudget`, excluded below. +7. *Evaluation vs. `DeleteBudgetPolicy`.* The fence's existence check fails, so the claim + is refused with no row, no log and no counter — `AC-OFFICE-COSTS-002.14a` preserved, + now reached through the fence rather than through the foreign key. The foreign-key + classifier is retained because the constraint still carries `AC-OFFICE-COSTS-002.9`'s + cascade, and dropping the classifier would let a genuine foreign-key error read as a + store fault. + +**Nil / empty / error.** Revision `<= 0`, empty policy id, empty period key, empty level: +claim-store error, fail open (`AC-OFFICE-COSTS-003.13`). Claim-store error during the +atomic pair: the transaction rolls back, no claim is held, the evaluation emits +`budget.exceeded` and records the failure — `AC-OFFICE-COSTS-002.14` continues to take precedence over +`AC-OFFICE-COSTS-002.5` and `.10`. Activity write failure after a successful claim: one +notification lost, not retried, not reported (`AC-OFFICE-COSTS-002.2a`, unchanged). +Discard failure during an update: whole update rolls back, revision unchanged, error +returned to the caller (`AC-OFFICE-COSTS-002.8`, extended by +`AC-OFFICE-COSTS-003.6`). + +**Defaults and boundaries.** Revision starts at 1, is `int64`, never resets and is never +reused for a policy id; a period rollover, a backend restart and a claim discard all leave +it alone. Existing rows migrate to 1. A `period` change still side-steps this whole class +of races by moving `period_key`. Row growth stays bounded by `policies × periods × 2` in +steady state, plus case 3's inert survivors, which the next update or deletion reclaims — +no retention job (`AC-OFFICE-COSTS-003.15`). + +## Amendments to REQ-OFFICE-COSTS-002 + +Which `REQ-OFFICE-COSTS-002` criteria this spec supersedes, widens, or leaves untouched — +all 21 of them, named individually — is in [amendments.md](amendments.md). No acceptance +criterion of `REQ-OFFICE-COSTS-003` lives there. + +## Out of scope + +- **Serializing `AC-OFFICE-COSTS-003.3a`'s recreate across two initializers.** `.3a` requires + the recreate to take effect at most once per database, guarded by its `columnExists` probe; + it does **not** require two backends initializing the same database to be prevented from + both performing it. **Window:** two backends cold-starting simultaneously against one + PostgreSQL database still carrying the pre-`.3` three-column `office_budget_claims` — not + reachable on SQLite, whose writer pool is `MaxOpenConns(1)` (`internal/db/pool.go`), and not + reachable at all once any boot has completed the recreate. **Residual:** both probe, both + find `revision` absent, both drop and recreate; the table still ends in the four-column + shape `AC-OFFICE-COSTS-003.3` mandates. Worst case one initializer's `CREATE` races the + other's `DROP` and errors, which `.3a` already requires be returned to its caller, so that + backend fails to boot, restarts, probes, finds `revision` present and does nothing. Claim + loss across the window is already permitted by `AC-OFFICE-COSTS-003.3` via + `AC-OFFICE-COSTS-002.17`. **Mechanism for a follow-up:** a shared advisory lock, as in + `task/repository/sqlite/worktree_ownership_migration.go:216`, which takes + `pg_advisory_xact_lock` on a fixed bigint so every instance derives the same value and a + second boot waits for the first; a follow-up must choose a **new** lock id, not reuse that + file's `taskWorktreeCutoverLockID`, which would cross-block an unrelated migration. + **Why not here:** no in-repo precedent for this mechanism carries a concurrency test, so a + follow-up has to decide whether to set that bar rather than assume it exists. +- **The residual `budget.alert`-after-`budget.exceeded` write-order inversion.** When an + alert-band evaluation wins the alert claim strictly before a concurrent over-limit + evaluation claims `exceeded` (concurrency case 5), both rows are submitted and the order + in which they reach `office_activity_log` is unconstrained, so the activity feed can + render them exceeded-then-alert. **Bound:** at most one such pair per policy, period and + revision, under genuine concurrency only. **Harm:** display only — both rows are + individually truthful about the spend they observed, neither is duplicated, neither is + lost, and the Office inbox is unaffected because `listBudgetInboxEntries` concatenates + the two actions as separate already-sorted lists rather than interleaving them. **Why not + fixed:** closing it needs either a lock spanning claim and emit — the mechanism + `AC-OFFICE-COSTS-002.8` rejected and Fork 1 upholds — or a monotonic ordering key on + `office_activity_log`, a table shared by every Office surface whose only ordering today + is `created_at DESC` with no tiebreak, so two rows written in one clock tick already have + undefined relative order. Promising an order for this one pair would be a new table-wide + guarantee, not a fix; a follow-up wanting it should specify the guarantee for the whole + activity log first. +- **Optimistic concurrency for the budget-policy HTTP API.** `updateBudget` does an + unfenced `GetBudgetPolicy` → patch → `UpdateBudgetPolicy`, so two concurrent PATCHes can + lose one caller's field values. The revision column added here would make that fixable — + `AC-OFFICE-COSTS-003.7` deliberately declines — but an expected-revision request field, a + conflict status code, and the frontend threading are a separate user-facing change. The + same exclusion covers the status code for `AC-OFFICE-COSTS-003.5`'s deleted-policy case: + the handler's existing "any update error is a 500" mapping stands, and turning that one + case into a 404 belongs with the rest of this endpoint's error contract, not here. +- **Everything `REQ-OFFICE-COSTS-002` already excluded**, inherited unchanged rather than + restated: unifying the three disagreeing spend-window implementations + (`costs.periodCutoff`, `backendapp.budgetEvaluator.spendForPolicy`, + `cron.periodStartFor`, and `BudgetPeriod.Valid()` accepting a wider `period` set than + `periodCutoff` honours), changing when a policy reaches a level, the cron budget-alert + trigger, resolving or expiring inbox items, `budget.exceeded` visibility, and a + user-facing notification-frequency setting. Nothing here depends on any of them. +- **Correcting #3287's stale claim that only `budget.alert` feeds the Office inbox.** PR + #3276 changed that on `main` after #3287 branched; it is recorded in + [Input inventory](#input-inventory) so Build does not rely on it. Editing #3287's own + design documents is not this card's work. + +## Verification + +Test strategy, per criterion, is in [verification.md](verification.md); research +provenance is in [prior-art.md](prior-art.md); sampled code shapes are in +[input-inventory.md](input-inventory.md); the `REQ-OFFICE-COSTS-002` deltas are in +[amendments.md](amendments.md). They are separate files because this document sits against +the specification linter's 32,768-byte ceiling for its kind, the same reason +`costs-03.md` / `costs-04.md` are split. The five files are one spec. + +## User-visible surfaces (E2E decision input) + +Backend-only: no new HTTP route, no request-body change, no required frontend change, no +new user-facing string. The two surfaces that read these rows — the Office inbox +(`/office/inbox`) and the workspace activity feed (`/office/workspace/.../activity`) — are +unchanged in code and see strictly fewer and more accurate rows. `revision` appears as an +additional read-only field in the budget-policy JSON, which the hand-written frontend type +ignores. + +**Recommendation: no new E2E coverage.** The behavior is a concurrency property between two +backend goroutines and a database transaction, observable at the repository and evaluation +boundaries and not reproducible through the UI. Go integration tests plus the PostgreSQL +twin are the right level. diff --git a/docs/specs/budget-claim-revision-fencing/verification.md b/docs/specs/budget-claim-revision-fencing/verification.md new file mode 100644 index 0000000000..5862e32f02 --- /dev/null +++ b/docs/specs/budget-claim-revision-fencing/verification.md @@ -0,0 +1,91 @@ +# Budget claim fencing: verification + +Companion to [spec.md](spec.md) (`REQ-OFFICE-COSTS-003`), alongside +[prior-art.md](prior-art.md), [input-inventory.md](input-inventory.md) and +[amendments.md](amendments.md). Criterion ids below are spec.md's. Split out for the +specification linter's per-file size ceiling; the five files are one spec. + +## Test strategy + +Every criterion is observable at the repository or evaluation boundary. Assert submissions +on the activity **logger**, never with a `SELECT` against `office_activity_log` +(`AC-OFFICE-COSTS-002.2a`). + +- **Race A regression (`.8`, `.9`, `.15`).** Read a policy snapshot at revision `r`; run + `UpdateBudgetPolicy` to completion; attempt the snapshot's claim. Assert no activity row + submitted, no error log, no counter increment. Then evaluate at revision `r+1` with spend + reaching a level under the new values and assert exactly one row **is** submitted. That + second half is the point — a test asserting only the first half also passes against an + implementation that has merely broken emission. +- **Race B regression (`.10`, `.11`, `.12`).** With an exceeded-level claim already held + for `(policy, period, revision)`, evaluate the same policy with spend in the alert band. + Assert zero `budget.alert` submissions. Separately assert the invariant directly: after + any successful exceeded claim, both rows exist at that revision. +- **Atomicity of the pair (`.10`).** Inject a failure between the two inserts using the + same style of failpoint #3287 added for the discard (`failBudgetPolicyUpdateErr`, + `budget_atomicity_internal_test.go`), and assert neither row exists and the evaluation + fails open per `AC-OFFICE-COSTS-002.14`. +- **Revision lifecycle (`.4`, `.5`, `.6`).** Create → 1, asserted on what the create path + hands back to its caller and not only on the stored row — the two disagree if the + implementation leans on the column's `DEFAULT 1` — and asserted again after presenting a + nonzero revision for creation, which must not survive. Update → 2, and the value returned + to the updater is 2. Rolled-back update → unchanged. Two concurrent updates → `r+2`. + Update naming a policy id that does not exist → the transaction rolls back and the caller + gets an error, asserted as a behavior change from #3287, where that update committed and + reported success. +- **Pair outcomes that are successes, not failures (`.10`, `.11`).** Two cases, both + asserting a `budget.exceeded` row IS submitted. With an alert-level claim already held at + `(policy, period, revision)`, run an over-limit evaluation: the companion insert conflicts, + the pair still commits, `budget.exceeded` goes out, and both rows exist. Then, with an + exceeded-level claim already held, run another over-limit evaluation: the exceeded insert + writes nothing, no companion insert is attempted, nothing is emitted, and no claim-store + failure is logged or counted. A test that only proves the happy pair misses both. +- **Invalid claim input is not a silent miss (`.13`, and `.8`'s guard ordering).** Call the + claim path with revision `0`, then with an empty policy id, then with an empty period key, + then with an empty level. Each shall report a claim-store error and fail open — the + notification IS emitted, the + counter increments, the error is logged — which is the opposite of `.9`'s silent refusal. + This is the criterion that fails if the guard is implemented as a `WHERE` predicate + instead of running before the fenced statement. +- **Which level the failure log names (`.14`).** With the claim store faulted, assert the + structured log's `level` field is `exceeded` for the atomic pair of `.10` and `alert` for + a standalone alert-band claim, and that a pair failure produces exactly one log line and + one counter increment rather than two. The counter's label stays `op=claim` throughout. +- **Revision is server-owned on the wire (`.2`, `.7`).** Assert `revision` appears in the + budget-policy JSON under that key and is not suppressed. Then PATCH a budget policy with + a `revision` field in the request body and assert the stored revision is the server's + own bump, unaffected by the value supplied. +- **Migration replay (`.1`, ADR 0027).** Fresh boot and boot replay on one connection, + asserting the column exists once and replay is a no-op, with no new classifier. +- **The recreate is guarded (`.3a`).** Boot once against a database still carrying the + pre-`.3` three-column claim table — so the cutover actually runs rather than being + short-circuited by the probe — **then** write claims, **then** boot a second time and + assert those claims are still there. The regression this guards is + `AC-OFFICE-COSTS-002.11`, and an unguarded `DROP`/`CREATE` pair passes a single-boot test + and fails this one. That ordering is load-bearing rather than incidental: claims written + *before* the first boot are destroyed by the cutover itself, which + `AC-OFFICE-COSTS-003.3` expressly permits, so a test that holds claims across the first + boot asserts something a correct implementation does not promise. Assert the probe finds + `revision` present on the second boot and does nothing. This bullet applies on every + build: `AC-OFFICE-COSTS-003.3` runs the recreate whatever #3287's state, so there is no + longer a merge precondition to record. +- **A failed recreate is visible, not swallowed (`.3a`).** Inject a failure into the + recreate — the `failBudgetPolicyUpdateErr` failpoint style #3287 already uses in + `budget_atomicity_internal_test.go`, or the shape of `TestCutover_RollbackAtEveryFailpoint` + in `worktree_ownership_migration_test.go` — and assert the error reaches the caller that + initializes the schema rather than being logged and dropped. This is the bullet that + separates a correct implementation from the most likely wrong one, which is why it is + worth its own case: the two calls sitting immediately beside the recreate's slot in + `initSchema`, `r.runMigrations()` and `r.activateRunOutcome()`, both return nothing and + are invoked bare, and the migration runner they use "swallows failures at WARN". A + recreate wired to match its neighbours passes every other bullet here while violating + `.3a`'s requirement that a failed recreate be visible rather than silently leaving the + old three-column key in place. +- **PostgreSQL twin (`.3`, `.8`, Fork 3).** Extend #3287's `TestPostgresBudgetClaims` + (`KANDEV_TEST_POSTGRES_DSN`) to cover the fenced insert and the four-column key. Fork 3's + argument is dialect-specific, so a SQLite-only proof does not establish it. If the DSN is + unavailable on the build runner, record that in the task plan with the skip output as the + receipt — do not silently omit it. +- **Suppression boundary (`AC-OFFICE-COSTS-002.12`).** A refused claim on a superseded + revision must still pause an unpaused in-scope agent and still deny a pre-execution + check.