From 5e8003fa185220ba2df5c6140be192acc01d9c3b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 1 Dec 2025 15:10:42 +0000 Subject: [PATCH 1/5] feat(coderd/database/dbpurge): make API keys retention configurable Replace hardcoded 7-day retention for expired API keys with configurable retention from deployment settings. Falls back to global retention when not set, and skips deletion entirely when effective retention is 0. Depends on #21021 Updates #20743 --- coderd/database/dbpurge/dbpurge.go | 33 ++-- coderd/database/dbpurge/dbpurge_test.go | 196 ++++++++++++++++++++++++ 2 files changed, 217 insertions(+), 12 deletions(-) diff --git a/coderd/database/dbpurge/dbpurge.go b/coderd/database/dbpurge/dbpurge.go index 4b1f9f7628e16..daa1915295651 100644 --- a/coderd/database/dbpurge/dbpurge.go +++ b/coderd/database/dbpurge/dbpurge.go @@ -82,18 +82,27 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, vals *coder if err := tx.ExpirePrebuildsAPIKeys(ctx, dbtime.Time(start)); err != nil { return xerrors.Errorf("failed to expire prebuilds user api keys: %w", err) } - expiredAPIKeys, err := tx.DeleteExpiredAPIKeys(ctx, database.DeleteExpiredAPIKeysParams{ - // Leave expired keys for a week to allow the backend to know the difference - // between a 404 and an expired key. This purge code is just to bound the size of - // the table to something more reasonable. - Before: dbtime.Time(start.Add(time.Hour * 24 * 7 * -1)), - // There could be a lot of expired keys here, so set a limit to prevent this - // taking too long. - // This runs every 10 minutes, so it deletes ~1.5m keys per day at most. - LimitCount: 10000, - }) - if err != nil { - return xerrors.Errorf("failed to delete expired api keys: %w", err) + + var expiredAPIKeys int64 + apiKeysRetention := vals.Retention.APIKeys.Value() + if apiKeysRetention == 0 { + apiKeysRetention = vals.Retention.Global.Value() + } + if apiKeysRetention > 0 { + // Delete keys that have been expired for at least the retention period. + // This allows the backend to return a more helpful error when a user + // tries to use an expired key. + deleteExpiredKeysBefore := start.Add(-apiKeysRetention) + expiredAPIKeys, err = tx.DeleteExpiredAPIKeys(ctx, database.DeleteExpiredAPIKeysParams{ + Before: dbtime.Time(deleteExpiredKeysBefore), + // There could be a lot of expired keys here, so set a limit to prevent + // this taking too long. This runs every 10 minutes, so it deletes + // ~1.5m keys per day at most. + LimitCount: 10000, + }) + if err != nil { + return xerrors.Errorf("failed to delete expired api keys: %w", err) + } } deleteOldTelemetryLocksBefore := start.Add(-maxTelemetryHeartbeatAge) if err := tx.DeleteOldTelemetryLocks(ctx, deleteOldTelemetryLocksBefore); err != nil { diff --git a/coderd/database/dbpurge/dbpurge_test.go b/coderd/database/dbpurge/dbpurge_test.go index 26e778b00a559..569412e0a370a 100644 --- a/coderd/database/dbpurge/dbpurge_test.go +++ b/coderd/database/dbpurge/dbpurge_test.go @@ -1245,3 +1245,199 @@ func TestDeleteOldAuditLogs(t *testing.T) { require.NotContains(t, logIDs, oldCreateLog.ID, "old create log should be deleted by audit logs retention") }) } + +func TestDeleteExpiredAPIKeys(t *testing.T) { + t.Parallel() + + t.Run("RetentionEnabled", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + + clk := quartz.NewMock(t) + now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) + retentionPeriod := 7 * 24 * time.Hour // 7 days + expiredLongAgo := now.Add(-retentionPeriod).Add(-24 * time.Hour) // Expired 8 days ago (should be deleted) + expiredRecently := now.Add(-retentionPeriod).Add(24 * time.Hour) // Expired 6 days ago (should be kept) + notExpired := now.Add(24 * time.Hour) // Expires tomorrow (should be kept) + clk.Set(now).MustWait(ctx) + + db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + user := dbgen.User(t, db, database.User{}) + + // Create API key that expired long ago (should be deleted) + oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: expiredLongAgo, + TokenName: "old-expired-key", + }) + + // Create API key that expired recently (should be kept) + recentExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: expiredRecently, + TokenName: "recent-expired-key", + }) + + // Create API key that hasn't expired yet (should be kept) + activeKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: notExpired, + TokenName: "active-key", + }) + + // Run the purge with configured retention period + done := awaitDoTick(ctx, t, clk) + closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ + Retention: codersdk.RetentionConfig{ + APIKeys: serpent.Duration(retentionPeriod), + }, + }, clk) + defer closer.Close() + testutil.TryReceive(ctx, t, done) + + // Verify results + _, err := db.GetAPIKeyByID(ctx, oldExpiredKey.ID) + require.Error(t, err, "old expired key should be deleted") + + _, err = db.GetAPIKeyByID(ctx, recentExpiredKey.ID) + require.NoError(t, err, "recently expired key should be kept") + + _, err = db.GetAPIKeyByID(ctx, activeKey.ID) + require.NoError(t, err, "active key should be kept") + }) + + t.Run("RetentionDisabled", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + + clk := quartz.NewMock(t) + now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) + expiredLongAgo := now.Add(-365 * 24 * time.Hour) // Expired 1 year ago + clk.Set(now).MustWait(ctx) + + db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + user := dbgen.User(t, db, database.User{}) + + // Create API key that expired long ago (should NOT be deleted when retention is 0) + oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: expiredLongAgo, + TokenName: "old-expired-key", + }) + + // Run the purge with retention disabled (0) + done := awaitDoTick(ctx, t, clk) + closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ + Retention: codersdk.RetentionConfig{ + APIKeys: serpent.Duration(0), // disabled + }, + }, clk) + defer closer.Close() + testutil.TryReceive(ctx, t, done) + + // Verify old expired key is still present + _, err := db.GetAPIKeyByID(ctx, oldExpiredKey.ID) + require.NoError(t, err, "old expired key should NOT be deleted when retention is disabled") + }) + + t.Run("GlobalRetentionFallback", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + + clk := quartz.NewMock(t) + now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) + retentionPeriod := 14 * 24 * time.Hour // 14 days global + expiredLongAgo := now.Add(-retentionPeriod).Add(-24 * time.Hour) // Expired 15 days ago (should be deleted) + expiredRecently := now.Add(-retentionPeriod).Add(24 * time.Hour) // Expired 13 days ago (should be kept) + clk.Set(now).MustWait(ctx) + + db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + user := dbgen.User(t, db, database.User{}) + + // Create API key that expired long ago (should be deleted) + oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: expiredLongAgo, + TokenName: "old-expired-key", + }) + + // Create API key that expired recently (should be kept) + recentExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: expiredRecently, + TokenName: "recent-expired-key", + }) + + // Run the purge with global retention (API keys retention is 0, so it falls back) + done := awaitDoTick(ctx, t, clk) + closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ + Retention: codersdk.RetentionConfig{ + Global: serpent.Duration(retentionPeriod), // Use global + APIKeys: serpent.Duration(0), // Not set, should fall back to global + }, + }, clk) + defer closer.Close() + testutil.TryReceive(ctx, t, done) + + // Verify results + _, err := db.GetAPIKeyByID(ctx, oldExpiredKey.ID) + require.Error(t, err, "old expired key should be deleted via global retention") + + _, err = db.GetAPIKeyByID(ctx, recentExpiredKey.ID) + require.NoError(t, err, "recently expired key should be kept") + }) + + t.Run("CustomRetention30Days", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + + clk := quartz.NewMock(t) + now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) + retentionPeriod := 30 * 24 * time.Hour // 30 days + expiredLongAgo := now.Add(-retentionPeriod).Add(-24 * time.Hour) // Expired 31 days ago (should be deleted) + expiredRecently := now.Add(-retentionPeriod).Add(24 * time.Hour) // Expired 29 days ago (should be kept) + clk.Set(now).MustWait(ctx) + + db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + user := dbgen.User(t, db, database.User{}) + + // Create API key that expired long ago (should be deleted) + oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: expiredLongAgo, + TokenName: "old-expired-key", + }) + + // Create API key that expired recently (should be kept) + recentExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: expiredRecently, + TokenName: "recent-expired-key", + }) + + // Run the purge with 30-day retention + done := awaitDoTick(ctx, t, clk) + closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ + Retention: codersdk.RetentionConfig{ + APIKeys: serpent.Duration(retentionPeriod), + }, + }, clk) + defer closer.Close() + testutil.TryReceive(ctx, t, done) + + // Verify results + _, err := db.GetAPIKeyByID(ctx, oldExpiredKey.ID) + require.Error(t, err, "old expired key should be deleted with 30-day retention") + + _, err = db.GetAPIKeyByID(ctx, recentExpiredKey.ID) + require.NoError(t, err, "recently expired key should be kept with 30-day retention") + }) +} From a8c815db3f719d30f9332adf95e99fd1f2a8407b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 1 Dec 2025 16:19:05 +0000 Subject: [PATCH 2/5] fix: improve comment wording for API keys retention --- coderd/database/dbpurge/dbpurge.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbpurge/dbpurge.go b/coderd/database/dbpurge/dbpurge.go index daa1915295651..dd14e84f9ead7 100644 --- a/coderd/database/dbpurge/dbpurge.go +++ b/coderd/database/dbpurge/dbpurge.go @@ -90,8 +90,8 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, vals *coder } if apiKeysRetention > 0 { // Delete keys that have been expired for at least the retention period. - // This allows the backend to return a more helpful error when a user - // tries to use an expired key. + // A higher retention period allows the backend to return a more helpful + // error message when a user tries to use an expired key. deleteExpiredKeysBefore := start.Add(-apiKeysRetention) expiredAPIKeys, err = tx.DeleteExpiredAPIKeys(ctx, database.DeleteExpiredAPIKeysParams{ Before: dbtime.Time(deleteExpiredKeysBefore), From 85235cc32046d22242ddbbb1c63a5540169c0249 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 1 Dec 2025 16:38:39 +0000 Subject: [PATCH 3/5] test(coderd/database/dbpurge): convert API keys tests to table-driven --- coderd/database/dbpurge/dbpurge_test.go | 293 ++++++++++-------------- 1 file changed, 118 insertions(+), 175 deletions(-) diff --git a/coderd/database/dbpurge/dbpurge_test.go b/coderd/database/dbpurge/dbpurge_test.go index 569412e0a370a..8d02c33168797 100644 --- a/coderd/database/dbpurge/dbpurge_test.go +++ b/coderd/database/dbpurge/dbpurge_test.go @@ -1249,195 +1249,138 @@ func TestDeleteOldAuditLogs(t *testing.T) { func TestDeleteExpiredAPIKeys(t *testing.T) { t.Parallel() - t.Run("RetentionEnabled", func(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - - clk := quartz.NewMock(t) - now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) - retentionPeriod := 7 * 24 * time.Hour // 7 days - expiredLongAgo := now.Add(-retentionPeriod).Add(-24 * time.Hour) // Expired 8 days ago (should be deleted) - expiredRecently := now.Add(-retentionPeriod).Add(24 * time.Hour) // Expired 6 days ago (should be kept) - notExpired := now.Add(24 * time.Hour) // Expires tomorrow (should be kept) - clk.Set(now).MustWait(ctx) - - db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - user := dbgen.User(t, db, database.User{}) - - // Create API key that expired long ago (should be deleted) - oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ - UserID: user.ID, - ExpiresAt: expiredLongAgo, - TokenName: "old-expired-key", - }) - - // Create API key that expired recently (should be kept) - recentExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ - UserID: user.ID, - ExpiresAt: expiredRecently, - TokenName: "recent-expired-key", - }) - - // Create API key that hasn't expired yet (should be kept) - activeKey, _ := dbgen.APIKey(t, db, database.APIKey{ - UserID: user.ID, - ExpiresAt: notExpired, - TokenName: "active-key", - }) + now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) - // Run the purge with configured retention period - done := awaitDoTick(ctx, t, clk) - closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ - Retention: codersdk.RetentionConfig{ - APIKeys: serpent.Duration(retentionPeriod), + testCases := []struct { + name string + retentionConfig codersdk.RetentionConfig + oldExpiredTime time.Time + recentExpiredTime *time.Time // nil means no recent expired key created + activeTime *time.Time // nil means no active key created + expectOldExpiredDeleted bool + expectedKeysRemaining int + }{ + { + name: "RetentionEnabled", + retentionConfig: codersdk.RetentionConfig{ + APIKeys: serpent.Duration(7 * 24 * time.Hour), // 7 days }, - }, clk) - defer closer.Close() - testutil.TryReceive(ctx, t, done) - - // Verify results - _, err := db.GetAPIKeyByID(ctx, oldExpiredKey.ID) - require.Error(t, err, "old expired key should be deleted") - - _, err = db.GetAPIKeyByID(ctx, recentExpiredKey.ID) - require.NoError(t, err, "recently expired key should be kept") - - _, err = db.GetAPIKeyByID(ctx, activeKey.ID) - require.NoError(t, err, "active key should be kept") - }) - - t.Run("RetentionDisabled", func(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - - clk := quartz.NewMock(t) - now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) - expiredLongAgo := now.Add(-365 * 24 * time.Hour) // Expired 1 year ago - clk.Set(now).MustWait(ctx) - - db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - user := dbgen.User(t, db, database.User{}) - - // Create API key that expired long ago (should NOT be deleted when retention is 0) - oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ - UserID: user.ID, - ExpiresAt: expiredLongAgo, - TokenName: "old-expired-key", - }) - - // Run the purge with retention disabled (0) - done := awaitDoTick(ctx, t, clk) - closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ - Retention: codersdk.RetentionConfig{ - APIKeys: serpent.Duration(0), // disabled + oldExpiredTime: now.Add(-8 * 24 * time.Hour), // Expired 8 days ago + recentExpiredTime: ptr(now.Add(-6 * 24 * time.Hour)), // Expired 6 days ago + activeTime: ptr(now.Add(24 * time.Hour)), // Expires tomorrow + expectOldExpiredDeleted: true, + expectedKeysRemaining: 2, // recent expired + active + }, + { + name: "RetentionDisabled", + retentionConfig: codersdk.RetentionConfig{ + APIKeys: serpent.Duration(0), }, - }, clk) - defer closer.Close() - testutil.TryReceive(ctx, t, done) - - // Verify old expired key is still present - _, err := db.GetAPIKeyByID(ctx, oldExpiredKey.ID) - require.NoError(t, err, "old expired key should NOT be deleted when retention is disabled") - }) - - t.Run("GlobalRetentionFallback", func(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - - clk := quartz.NewMock(t) - now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) - retentionPeriod := 14 * 24 * time.Hour // 14 days global - expiredLongAgo := now.Add(-retentionPeriod).Add(-24 * time.Hour) // Expired 15 days ago (should be deleted) - expiredRecently := now.Add(-retentionPeriod).Add(24 * time.Hour) // Expired 13 days ago (should be kept) - clk.Set(now).MustWait(ctx) - - db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - user := dbgen.User(t, db, database.User{}) + oldExpiredTime: now.Add(-365 * 24 * time.Hour), // Expired 1 year ago + recentExpiredTime: nil, + activeTime: nil, + expectOldExpiredDeleted: false, + expectedKeysRemaining: 1, // old expired is kept + }, + { + name: "GlobalRetentionFallback", + retentionConfig: codersdk.RetentionConfig{ + Global: serpent.Duration(14 * 24 * time.Hour), // 14 days global + APIKeys: serpent.Duration(0), // Not set, should fall back to global + }, + oldExpiredTime: now.Add(-15 * 24 * time.Hour), // Expired 15 days ago + recentExpiredTime: ptr(now.Add(-13 * 24 * time.Hour)), // Expired 13 days ago + activeTime: nil, + expectOldExpiredDeleted: true, + expectedKeysRemaining: 1, // only recent expired remains + }, + { + name: "CustomRetention30Days", + retentionConfig: codersdk.RetentionConfig{ + APIKeys: serpent.Duration(30 * 24 * time.Hour), // 30 days + }, + oldExpiredTime: now.Add(-31 * 24 * time.Hour), // Expired 31 days ago + recentExpiredTime: ptr(now.Add(-29 * 24 * time.Hour)), // Expired 29 days ago + activeTime: nil, + expectOldExpiredDeleted: true, + expectedKeysRemaining: 1, // only recent expired remains + }, + } - // Create API key that expired long ago (should be deleted) - oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ - UserID: user.ID, - ExpiresAt: expiredLongAgo, - TokenName: "old-expired-key", - }) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() - // Create API key that expired recently (should be kept) - recentExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ - UserID: user.ID, - ExpiresAt: expiredRecently, - TokenName: "recent-expired-key", - }) + ctx := testutil.Context(t, testutil.WaitShort) + clk := quartz.NewMock(t) + clk.Set(now).MustWait(ctx) - // Run the purge with global retention (API keys retention is 0, so it falls back) - done := awaitDoTick(ctx, t, clk) - closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ - Retention: codersdk.RetentionConfig{ - Global: serpent.Duration(retentionPeriod), // Use global - APIKeys: serpent.Duration(0), // Not set, should fall back to global - }, - }, clk) - defer closer.Close() - testutil.TryReceive(ctx, t, done) + db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + user := dbgen.User(t, db, database.User{}) - // Verify results - _, err := db.GetAPIKeyByID(ctx, oldExpiredKey.ID) - require.Error(t, err, "old expired key should be deleted via global retention") + // Create API key that expired long ago. + oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: tc.oldExpiredTime, + TokenName: "old-expired-key", + }) - _, err = db.GetAPIKeyByID(ctx, recentExpiredKey.ID) - require.NoError(t, err, "recently expired key should be kept") - }) + // Create API key that expired recently if specified. + var recentExpiredKey database.APIKey + if tc.recentExpiredTime != nil { + recentExpiredKey, _ = dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: *tc.recentExpiredTime, + TokenName: "recent-expired-key", + }) + } - t.Run("CustomRetention30Days", func(t *testing.T) { - t.Parallel() + // Create API key that hasn't expired yet if specified. + var activeKey database.APIKey + if tc.activeTime != nil { + activeKey, _ = dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: *tc.activeTime, + TokenName: "active-key", + }) + } - ctx := testutil.Context(t, testutil.WaitShort) + // Run the purge. + done := awaitDoTick(ctx, t, clk) + closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ + Retention: tc.retentionConfig, + }, clk) + defer closer.Close() + testutil.TryReceive(ctx, t, done) - clk := quartz.NewMock(t) - now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) - retentionPeriod := 30 * 24 * time.Hour // 30 days - expiredLongAgo := now.Add(-retentionPeriod).Add(-24 * time.Hour) // Expired 31 days ago (should be deleted) - expiredRecently := now.Add(-retentionPeriod).Add(24 * time.Hour) // Expired 29 days ago (should be kept) - clk.Set(now).MustWait(ctx) + // Verify total keys remaining. + keys, err := db.GetAPIKeysLastUsedAfter(ctx, time.Time{}) + require.NoError(t, err) + require.Len(t, keys, tc.expectedKeysRemaining, "unexpected number of keys remaining") - db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - user := dbgen.User(t, db, database.User{}) + // Verify results. + _, err = db.GetAPIKeyByID(ctx, oldExpiredKey.ID) + if tc.expectOldExpiredDeleted { + require.Error(t, err, "old expired key should be deleted") + } else { + require.NoError(t, err, "old expired key should NOT be deleted") + } - // Create API key that expired long ago (should be deleted) - oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ - UserID: user.ID, - ExpiresAt: expiredLongAgo, - TokenName: "old-expired-key", - }) + if tc.recentExpiredTime != nil { + _, err = db.GetAPIKeyByID(ctx, recentExpiredKey.ID) + require.NoError(t, err, "recently expired key should be kept") + } - // Create API key that expired recently (should be kept) - recentExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ - UserID: user.ID, - ExpiresAt: expiredRecently, - TokenName: "recent-expired-key", + if tc.activeTime != nil { + _, err = db.GetAPIKeyByID(ctx, activeKey.ID) + require.NoError(t, err, "active key should be kept") + } }) + } +} - // Run the purge with 30-day retention - done := awaitDoTick(ctx, t, clk) - closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ - Retention: codersdk.RetentionConfig{ - APIKeys: serpent.Duration(retentionPeriod), - }, - }, clk) - defer closer.Close() - testutil.TryReceive(ctx, t, done) - - // Verify results - _, err := db.GetAPIKeyByID(ctx, oldExpiredKey.ID) - require.Error(t, err, "old expired key should be deleted with 30-day retention") - - _, err = db.GetAPIKeyByID(ctx, recentExpiredKey.ID) - require.NoError(t, err, "recently expired key should be kept with 30-day retention") - }) +// ptr is a helper to create a pointer to a value. +func ptr[T any](v T) *T { + return &v } From 1a09ab43ff978229eb13ebff94aa318645c53413 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 2 Dec 2025 10:05:39 +0000 Subject: [PATCH 4/5] chore: remove global retention fallback for API keys API keys retention is now explicit - it's enabled when --api-keys-retention is set to a non-zero duration (default 7d), and disabled when set to 0. No fallback to global retention. --- coderd/database/dbpurge/dbpurge.go | 3 --- coderd/database/dbpurge/dbpurge_test.go | 13 +------------ 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/coderd/database/dbpurge/dbpurge.go b/coderd/database/dbpurge/dbpurge.go index dd14e84f9ead7..7f953633bf64d 100644 --- a/coderd/database/dbpurge/dbpurge.go +++ b/coderd/database/dbpurge/dbpurge.go @@ -85,9 +85,6 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, vals *coder var expiredAPIKeys int64 apiKeysRetention := vals.Retention.APIKeys.Value() - if apiKeysRetention == 0 { - apiKeysRetention = vals.Retention.Global.Value() - } if apiKeysRetention > 0 { // Delete keys that have been expired for at least the retention period. // A higher retention period allows the backend to return a more helpful diff --git a/coderd/database/dbpurge/dbpurge_test.go b/coderd/database/dbpurge/dbpurge_test.go index 8d02c33168797..5b8bb35d8969f 100644 --- a/coderd/database/dbpurge/dbpurge_test.go +++ b/coderd/database/dbpurge/dbpurge_test.go @@ -1282,18 +1282,7 @@ func TestDeleteExpiredAPIKeys(t *testing.T) { expectOldExpiredDeleted: false, expectedKeysRemaining: 1, // old expired is kept }, - { - name: "GlobalRetentionFallback", - retentionConfig: codersdk.RetentionConfig{ - Global: serpent.Duration(14 * 24 * time.Hour), // 14 days global - APIKeys: serpent.Duration(0), // Not set, should fall back to global - }, - oldExpiredTime: now.Add(-15 * 24 * time.Hour), // Expired 15 days ago - recentExpiredTime: ptr(now.Add(-13 * 24 * time.Hour)), // Expired 13 days ago - activeTime: nil, - expectOldExpiredDeleted: true, - expectedKeysRemaining: 1, // only recent expired remains - }, + { name: "CustomRetention30Days", retentionConfig: codersdk.RetentionConfig{ From a6744f0217cd55c082e7a2674c7fbf0fa31fb1c3 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 2 Dec 2025 11:34:24 +0000 Subject: [PATCH 5/5] refactor: optimize DeleteExpiredAPIKeys to use :execrows Use :execrows instead of :one to simplify the query by removing the extra CTE wrapper. This lets PostgreSQL return the row count directly via RowsAffected() instead of requiring an explicit COUNT(*) scan. --- coderd/database/queries.sql.go | 29 +++++++++++++---------------- coderd/database/queries/apikeys.sql | 21 ++++++++------------- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 82d8639abfeb1..d102c3283db07 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1107,24 +1107,20 @@ func (q *sqlQuerier) DeleteApplicationConnectAPIKeysByUserID(ctx context.Context return err } -const deleteExpiredAPIKeys = `-- name: DeleteExpiredAPIKeys :one +const deleteExpiredAPIKeys = `-- name: DeleteExpiredAPIKeys :execrows WITH expired_keys AS ( SELECT id FROM api_keys -- expired keys only WHERE expires_at < $1::timestamptz LIMIT $2 -), -deleted_rows AS ( - DELETE FROM - api_keys - USING - expired_keys - WHERE - api_keys.id = expired_keys.id - RETURNING api_keys.id - ) -SELECT COUNT(deleted_rows.id) AS deleted_count FROM deleted_rows +) +DELETE FROM + api_keys +USING + expired_keys +WHERE + api_keys.id = expired_keys.id ` type DeleteExpiredAPIKeysParams struct { @@ -1133,10 +1129,11 @@ type DeleteExpiredAPIKeysParams struct { } func (q *sqlQuerier) DeleteExpiredAPIKeys(ctx context.Context, arg DeleteExpiredAPIKeysParams) (int64, error) { - row := q.db.QueryRowContext(ctx, deleteExpiredAPIKeys, arg.Before, arg.LimitCount) - var deleted_count int64 - err := row.Scan(&deleted_count) - return deleted_count, err + result, err := q.db.ExecContext(ctx, deleteExpiredAPIKeys, arg.Before, arg.LimitCount) + if err != nil { + return 0, err + } + return result.RowsAffected() } const expirePrebuildsAPIKeys = `-- name: ExpirePrebuildsAPIKeys :exec diff --git a/coderd/database/queries/apikeys.sql b/coderd/database/queries/apikeys.sql index 2e2be542c9601..226eda7ebe323 100644 --- a/coderd/database/queries/apikeys.sql +++ b/coderd/database/queries/apikeys.sql @@ -85,25 +85,20 @@ DELETE FROM WHERE user_id = $1; --- name: DeleteExpiredAPIKeys :one +-- name: DeleteExpiredAPIKeys :execrows WITH expired_keys AS ( SELECT id FROM api_keys -- expired keys only WHERE expires_at < @before::timestamptz LIMIT @limit_count -), -deleted_rows AS ( - DELETE FROM - api_keys - USING - expired_keys - WHERE - api_keys.id = expired_keys.id - RETURNING api_keys.id - ) -SELECT COUNT(deleted_rows.id) AS deleted_count FROM deleted_rows; -; +) +DELETE FROM + api_keys +USING + expired_keys +WHERE + api_keys.id = expired_keys.id; -- name: ExpirePrebuildsAPIKeys :exec -- Firstly, collect api_keys owned by the prebuilds user that correlate