diff --git a/pkg/appview/server.go b/pkg/appview/server.go index c73e753..782db6f 100644 --- a/pkg/appview/server.go +++ b/pkg/appview/server.go @@ -201,11 +201,8 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, slog.Info("Lease manager initialized", "component", "leases", "enabled", cfg.Leases.Enabled, "holder", s.Leases.HolderID()) - // Session, OAuth and device-flow cleanup. Idempotent DELETEs, so running it - // on every instance would be harmless but wasteful; more importantly it - // belongs with the other leased workers rather than buried in the database - // constructor, where it had no way to reach the lease manager. - s.startCleanupWorker(workerCtx) + // The cleanup worker starts later, once the hold authorizer exists: it + // clears the crew denial backoffs as its first act and needs a handle on it. // Initialize OAuth components slog.Info("Initializing OAuth components") @@ -271,14 +268,10 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, middleware.SetGlobalAuthorizer(s.HoldAuthorizer) slog.Info("Hold authorizer initialized with database caching") - // Clear all denial caches on startup for a clean slate - if remote, ok := s.HoldAuthorizer.(*auth.RemoteHoldAuthorizer); ok { - go func() { - if err := remote.ClearAllDenials(); err != nil { - slog.Warn("Failed to clear denial caches on startup", "error", err) - } - }() - } + // Session, OAuth and device-flow cleanup, plus the one-shot denial-cache + // clear. Started here rather than next to the other workers because it needs + // the hold authorizer above. + s.startCleanupWorker(workerCtx) // Initialize billing manager appviewDID := DIDFromBaseURL(baseURL) @@ -982,10 +975,33 @@ func (s *AppViewServer) waitForLeasedWorkers() { s.Leases.Wait(leaseDrainTimeout) } -// startCleanupWorker runs the periodic expiry sweep under the cleanup lease. +// startCleanupWorker runs the periodic expiry sweep under the cleanup lease, +// after clearing the crew denial backoffs once on acquiring it. +// +// That clear used to run on every boot, unconditionally, as +// "DELETE FROM hold_crew_denials" with no scoping. The intent is sound: a +// restart usually means a fix has shipped, and users sitting on a backoff of up +// to an hour should get to retry immediately rather than wait it out. The +// implementation stopped being sound the moment there was more than one +// instance, because then a rolling deploy wipes the shared table once per +// instance and every scale-out event wipes it again. +// +// Moving it under the lease keeps the behavior (a deploy still clears the +// backoffs) while making it happen once. Note the in-memory half of +// ClearAllDenials was always a no-op at startup, since a fresh process has an +// empty map; the database wipe was the only thing it ever really did. func (s *AppViewServer) startCleanupWorker(ctx context.Context) { const cleanupInterval = time.Hour + authorizer := s.HoldAuthorizer + s.Leases.Go(ctx, db.LeaseCleanup, func(leaseCtx context.Context) error { + if remote, ok := authorizer.(*auth.RemoteHoldAuthorizer); ok { + if err := remote.ClearAllDenials(); err != nil { + // Not fatal: the denials carry their own next_retry_at and will + // lapse on their own. + slog.Warn("Failed to clear denial caches", "error", err) + } + } return db.RunPeriodicCleanup(leaseCtx, s.Database, s.SessionStore, cleanupInterval) }) } diff --git a/pkg/auth/hold_remote.go b/pkg/auth/hold_remote.go index dcc7b9b..06d2afa 100644 --- a/pkg/auth/hold_remote.go +++ b/pkg/auth/hold_remote.go @@ -674,8 +674,15 @@ func (a *RemoteHoldAuthorizer) ClearCrewDenial(ctx context.Context, holdDID, use return nil } -// ClearAllDenials removes all crew denials from both in-memory and database caches -// Called on startup to ensure a clean slate +// ClearAllDenials removes all crew denials from both in-memory and database +// caches, giving users sitting on a long backoff an immediate retry after a +// deploy. +// +// Called by the AppView's cleanup worker when it acquires the cleanup lease, NOT +// on every boot. The database half is a table-wide DELETE shared by every +// instance, so calling it unconditionally at startup meant a rolling deploy +// wiped the backoffs once per instance and every scale-out event wiped them +// again. Keep it behind the lease. func (a *RemoteHoldAuthorizer) ClearAllDenials() error { // Clear all in-memory denials a.recentDenials.Range(func(key, value any) bool {