mirror of
https://github.com/coder/coder.git
synced 2026-10-31 02:43:21 +08:00
test(coderd): fix flaky TestExpireAPIKey timing assertions (#28455)
> 🤖 This PR was written by Coder Agents on behalf of Jake Howell. Closes [DEVEX-730](https://linear.app/codercom/issue/DEVEX-730/flake-testexpireapikey-multiple-variants). ## Problem `TestExpireAPIKey` flakes on Windows CI (`test-go-pg (windows-2022)`), failing `AdminCanExpireOtherUsersToken`, `DeletingExpiredTokenSucceeds`, and the parent test. The subtests asserted: ```go require.True(t, key.ExpiresAt.Before(dbtime.Now())) ``` The `expireAPIKey` handler sets `ExpiresAt: dbtime.Now()`, and `dbtime.Now()` rounds to microsecond precision. On platforms with coarse clock resolution (Windows, ~1–15ms), the handler's timestamp and the test's later `dbtime.Now()` can round to the *same* value, so `Before()` is `false` (equal is not before) and the assertion fails. On Linux the nanosecond-resolution clock makes the two reads differ, which is why it never reproduces there. ## Fix Use the timing-safe assertion already used by the sibling `ExpiringAlreadyExpiredTokenSucceeds` subtest: ```go require.LessOrEqual(t, key.ExpiresAt, dbtime.Now(), "key should be expired") ``` Applied to all three occurrences of the flaky pattern (`OwnerCanExpireOwnToken`, `AdminCanExpireOtherUsersToken`, `DeletingExpiredTokenSucceeds`). ## Prior art This is the established fix for this class of flake in this repo; `require.LessOrEqual(x, now)` is equivalent to `require.False(x.After(now))` (both allow equality, which is the point): - **[#22685](https://github.com/coder/coder/pull/22685)** — *use dbtime.Now() instead of time.Now() in test assertions against DB timestamps*. States the principle: `time.Now()` is nanosecond precision while Postgres timestamps are microsecond, so `Before`/`After` comparisons carry non-zero flake risk. That sweep **edited this same file** (`coderd/apikey_test.go`, 11 `ExpiresAt` comparisons) plus 10 others; these three assertions pre-dated it. - **[#22684](https://github.com/coder/coder/pull/22684)** — *fix TestTokens harder*. The direct predecessor ("Follows from #22684"), fixing the identical token-expiry flake with the equality-tolerant `require.False(t, token.ExpiresAt.After(dbtime.Now()))`. - **[#21783](https://github.com/coder/coder/pull/21783)** — introduced the sibling `ExpiringAlreadyExpiredTokenSucceeds` subtest in this file, which already uses `require.LessOrEqual(t, key.ExpiresAt, dbtime.Now(), "key should be expired")`. This PR just makes the other three subtests consistent with it. ## Verification - Ran the original `TestExpireAPIKey` 100× against Postgres on Linux: 100/100 passed (as expected, the flake is Windows-specific). - Confirmed the root cause by reading the handler and `dbtime.Now()` (microsecond rounding). - Re-ran the test after the fix; passes. <details> <summary>Investigation notes</summary> - CI reference: https://github.com/coder/coder/actions/runs/29892773275 (job `test-go-pg (windows-2022)`). - Error snippet showed `key.ExpiresAt.Before(dbtime.Now())` expected true but observed false (token appeared unexpired). - Handler: `coderd/apikey.go` `expireAPIKey` sets `ExpiresAt: dbtime.Now()`. - `dbtime.Now()` returns `time.Now().UTC()` rounded to `time.Microsecond`. - The equal-timestamp collision only manifests where the OS clock resolution is coarse (Windows), which matches the CI-only reproduction. </details>
This commit is contained in:
@@ -578,7 +578,7 @@ func TestExpireAPIKey(t *testing.T) {
|
||||
// Verify the token is expired.
|
||||
key, err = adminClient.APIKeyByID(ctx, codersdk.Me, keyID)
|
||||
require.NoError(t, err)
|
||||
require.True(t, key.ExpiresAt.Before(dbtime.Now()))
|
||||
require.LessOrEqual(t, key.ExpiresAt, dbtime.Now(), "key should be expired")
|
||||
|
||||
// Verify audit log.
|
||||
als := auditor.AuditLogs()
|
||||
@@ -605,7 +605,7 @@ func TestExpireAPIKey(t *testing.T) {
|
||||
// Verify the token is expired.
|
||||
key, err := memberClient.APIKeyByID(ctx, codersdk.Me, keyID)
|
||||
require.NoError(t, err)
|
||||
require.True(t, key.ExpiresAt.Before(dbtime.Now()))
|
||||
require.LessOrEqual(t, key.ExpiresAt, dbtime.Now(), "key should be expired")
|
||||
})
|
||||
|
||||
t.Run("MemberCannotExpireOtherUsersToken", func(t *testing.T) {
|
||||
@@ -685,7 +685,7 @@ func TestExpireAPIKey(t *testing.T) {
|
||||
// Verify it's expired.
|
||||
key, err := adminClient.APIKeyByID(ctx, codersdk.Me, keyID)
|
||||
require.NoError(t, err)
|
||||
require.True(t, key.ExpiresAt.Before(dbtime.Now()))
|
||||
require.LessOrEqual(t, key.ExpiresAt, dbtime.Now(), "key should be expired")
|
||||
|
||||
// Delete the expired token - should succeed.
|
||||
err = adminClient.DeleteAPIKey(ctx, codersdk.Me, keyID)
|
||||
|
||||
Reference in New Issue
Block a user