From 058ac29b8f129d3ca610f3f3cd8f8ecdd2b6779e Mon Sep 17 00:00:00 2001 From: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Date: Wed, 4 Mar 2026 20:25:01 -0800 Subject: [PATCH] Address review comments + more tests --- backend/internal/job/scim_job.go | 2 +- backend/internal/service/app_lock_service.go | 5 +- .../internal/service/app_lock_service_test.go | 112 ++++++++++++++++++ .../sqlite/20260304090200_indexes.up.sql | 8 +- 4 files changed, 124 insertions(+), 3 deletions(-) diff --git a/backend/internal/job/scim_job.go b/backend/internal/job/scim_job.go index 29762f48..cfddd2ba 100644 --- a/backend/internal/job/scim_job.go +++ b/backend/internal/job/scim_job.go @@ -17,7 +17,7 @@ func (s *Scheduler) RegisterScimJobs(ctx context.Context, scimService *service.S jobs := &ScimJobs{scimService: scimService} // Register the job to run every hour (with some jitter) - return s.RegisterJob(ctx, "SyncScim", gocron.DurationJob(24*time.Hour), jobs.SyncScim, true) + return s.RegisterJob(ctx, "SyncScim", gocron.DurationJob(time.Hour), jobs.SyncScim, true) } func (j *ScimJobs) SyncScim(ctx context.Context) error { diff --git a/backend/internal/service/app_lock_service.go b/backend/internal/service/app_lock_service.go index f61ee9c5..0a309b15 100644 --- a/backend/internal/service/app_lock_service.go +++ b/backend/internal/service/app_lock_service.go @@ -73,7 +73,10 @@ func (lv *lockValue) Unmarshal(raw string) error { // Acquire obtains the lock. When force is true, the lock is stolen from any existing owner. // If the lock is forcefully acquired, it blocks until the previous lock has expired. func (s *AppLockService) Acquire(ctx context.Context, force bool) (waitUntil time.Time, err error) { - tx := s.db.Begin() + tx := s.db.WithContext(ctx).Begin() + if tx.Error != nil { + return time.Time{}, fmt.Errorf("begin lock transaction: %w", tx.Error) + } defer func() { tx.Rollback() }() diff --git a/backend/internal/service/app_lock_service_test.go b/backend/internal/service/app_lock_service_test.go index 95b22f51..8f829dff 100644 --- a/backend/internal/service/app_lock_service_test.go +++ b/backend/internal/service/app_lock_service_test.go @@ -49,6 +49,23 @@ func readLockValue(t *testing.T, db *gorm.DB) lockValue { return value } +func lockDatabaseForWrite(t *testing.T, db *gorm.DB) *gorm.DB { + t.Helper() + + tx := db.Begin() + require.NoError(t, tx.Error) + + // Keep a write transaction open to block other queries. + err := tx.Exec( + `INSERT INTO kv (key, value) VALUES (?, ?) ON CONFLICT(key) DO NOTHING`, + lockKey, + `{"expires_at":0}`, + ).Error + require.NoError(t, err) + + return tx +} + func TestAppLockServiceAcquire(t *testing.T) { t.Run("creates new lock when none exists", func(t *testing.T) { db := testutils.NewDatabaseForTest(t) @@ -99,6 +116,66 @@ func TestAppLockServiceAcquire(t *testing.T) { require.Equal(t, service.hostID, stored.HostID) require.Greater(t, stored.ExpiresAt, time.Now().Unix()) }) + + t.Run("force acquisition returns wait duration when stealing active lock", func(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + service := newTestAppLockService(t, db) + + existing := lockValue{ + ProcessID: 99, + HostID: "other-host", + LockID: "other-lock-id", + ExpiresAt: time.Now().Add(ttl).Unix(), + } + insertLock(t, db, existing) + + waitUntil, err := service.Acquire(context.Background(), true) + require.NoError(t, err) + require.WithinDuration(t, time.Unix(existing.ExpiresAt, 0), waitUntil, time.Second) + }) + + t.Run("force acquisition does not wait when lock id is unchanged", func(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + service := newTestAppLockService(t, db) + + insertLock(t, db, lockValue{ + ProcessID: 99, + HostID: "other-host", + LockID: service.lockID, + ExpiresAt: time.Now().Add(ttl).Unix(), + }) + + waitUntil, err := service.Acquire(context.Background(), true) + require.NoError(t, err) + require.True(t, waitUntil.IsZero()) + }) + + t.Run("returns error when existing lock value is invalid JSON", func(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + service := newTestAppLockService(t, db) + + raw := "this-is-not-json" + err := db.Create(&model.KV{Key: lockKey, Value: &raw}).Error + require.NoError(t, err) + + _, err = service.Acquire(context.Background(), false) + require.ErrorContains(t, err, "decode existing lock value") + }) + + t.Run("returns context deadline exceeded when database is locked", func(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + service := newTestAppLockService(t, db) + + tx := lockDatabaseForWrite(t, db) + defer tx.Rollback() + + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + + _, err := service.Acquire(ctx, false) + require.ErrorIs(t, err, context.DeadlineExceeded) + require.ErrorContains(t, err, "begin lock transaction") + }) } func TestAppLockServiceRelease(t *testing.T) { @@ -134,6 +211,24 @@ func TestAppLockServiceRelease(t *testing.T) { stored := readLockValue(t, db) require.Equal(t, existing, stored) }) + + t.Run("returns context deadline exceeded when database is locked", func(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + service := newTestAppLockService(t, db) + + _, err := service.Acquire(context.Background(), false) + require.NoError(t, err) + + tx := lockDatabaseForWrite(t, db) + defer tx.Rollback() + + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + + err = service.Release(ctx) + require.ErrorIs(t, err, context.DeadlineExceeded) + require.ErrorContains(t, err, "release lock failed") + }) } func TestAppLockServiceRenew(t *testing.T) { @@ -186,4 +281,21 @@ func TestAppLockServiceRenew(t *testing.T) { err = service.renew(context.Background()) require.ErrorIs(t, err, ErrLockLost) }) + + t.Run("returns context deadline exceeded when database is locked", func(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + service := newTestAppLockService(t, db) + + _, err := service.Acquire(context.Background(), false) + require.NoError(t, err) + + tx := lockDatabaseForWrite(t, db) + defer tx.Rollback() + + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + + err = service.renew(ctx) + require.ErrorIs(t, err, context.DeadlineExceeded) + }) } diff --git a/backend/resources/migrations/sqlite/20260304090200_indexes.up.sql b/backend/resources/migrations/sqlite/20260304090200_indexes.up.sql index b288f044..f8a32142 100644 --- a/backend/resources/migrations/sqlite/20260304090200_indexes.up.sql +++ b/backend/resources/migrations/sqlite/20260304090200_indexes.up.sql @@ -1,6 +1,12 @@ +PRAGMA foreign_keys= OFF; +BEGIN; + CREATE INDEX IF NOT EXISTS idx_webauthn_sessions_expires_at ON webauthn_sessions (expires_at); CREATE INDEX IF NOT EXISTS idx_one_time_access_tokens_expires_at ON one_time_access_tokens (expires_at); CREATE INDEX IF NOT EXISTS idx_oidc_authorization_codes_expires_at ON oidc_authorization_codes (expires_at); CREATE INDEX IF NOT EXISTS idx_oidc_refresh_tokens_expires_at ON oidc_refresh_tokens (expires_at); CREATE INDEX IF NOT EXISTS idx_reauthentication_tokens_expires_at ON reauthentication_tokens (expires_at); -CREATE INDEX IF NOT EXISTS idx_email_verification_tokens_expires_at ON email_verification_tokens (expires_at); \ No newline at end of file +CREATE INDEX IF NOT EXISTS idx_email_verification_tokens_expires_at ON email_verification_tokens (expires_at); + +COMMIT; +PRAGMA foreign_keys=ON;