fix(test): async API-key last-used update is sleep-synchronised and .Maybe(), so it can never be asserted #1599

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Where

• internal/auth/service_apikeys_test.go:513-521
• internal/auth/service_apikeys_api_test.go:415-423
• The correct pattern already exists in the repo at internal/api/handler_history_test.go:185

What

Both tests register:

mockStore.On("UpdateAPIKeyLastUsed", mock.Anything, "key-1").Return(nil).Maybe()

then do time.Sleep(10 * time.Millisecond) // Allow goroutine to complete before mockStore.AssertExpectations(t).

.Maybe() makes the expectation optional, so AssertExpectations never requires that the goroutine ran. The sleep therefore synchronises nothing that is subsequently checked. The two constructs cancel each other out: the sleep exists to make the assertion reliable, and the .Maybe() removes the assertion.

The likely history is visible in the shape: the 10 ms sleep is too short to be reliable under CI load, the test flaked, and the flake was resolved by making the expectation optional rather than by synchronising properly.

Failure scenario

Delete the go func(){ store.UpdateAPIKeyLastUsed(...) }() from ValidateUserAPIKey entirely. API-key last-used tracking silently dies, which breaks stale-key auditing (there is then no signal for which API keys are dormant and should be revoked), and both tests still pass. Nothing in the suite covers the behaviour they are named for.

Fix direction

Replace the sleep-plus-.Maybe() pair in both files with the deterministic pattern already used correctly at internal/api/handler_history_test.go:185:

done := make(chan struct{})
mockStore.On("UpdateAPIKeyLastUsed", mock.Anything, "key-1").
    Return(nil).
    Run(func(mock.Arguments) { close(done) })
// ... exercise ...
select {
case <-done:
case <-time.After(2 * time.Second):
    t.Fatal("UpdateAPIKeyLastUsed was never called")
}

Drop .Maybe(). Confirm the fix the right way round: delete the goroutine from ValidateUserAPIKey, check the tests now fail, then restore it. If they still pass, the fix is not done.

Related

• TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite cloud-commitments-platform#35 (TEST-03: .On() without AssertExpectations) - overlapping theme, but these two files do call AssertExpectations; the defect is that .Maybe() makes the call vacuous, which TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite cloud-commitments-platform#35's fix (adding AssertExpectations) would not address.
• TEST-04: Wall-clock sleeps for async synchronization (~90 frontend setTimeout waits, several Go sleeps) cloud-commitments-platform#36 (TEST-04: Wall-clock sleeps for async synchronization) - the sleep half of this. TEST-04: Wall-clock sleeps for async synchronization (~90 frontend setTimeout waits, several Go sleeps) cloud-commitments-platform#36 does not list these two files, and its recommendation ("inject a hook/channel for the negative assertion") is right but scoped to different sites.
• Repo convention feedback_no_sleep_in_tests: wire a done channel into the mock's Run callback instead of sleeping.