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

[cristim]

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.

[cristim]

Verified resolved at 3c0f8ac: the sleep-synchronised .Maybe() UpdateAPIKeyLastUsed expectations are gone from both test files, and last-used tracking now runs through Service.RecordUsageAsync (booked once per request at internal/server/app.go:1197; the store write sets last_used_at). The async write is asserted deterministically: TestRecordUsageAsync_PanicIsRecovered registers a non-optional expectation with a done channel and AssertExpectations, and the concurrency test waits on a done channel with a 5s fatal timeout, so deleting the goroutine fails both. Evidence: internal/auth/service_apikeys_test.go:852 and internal/auth/service_apikeys.go:338 (#1523). Residual axes checked: no time.Sleep remains in internal/auth tests; the remaining .Maybe() registrations for RecordAPIKeyUsage are either negative assertions (validation must book nothing) or paired with a done channel plus fatal timeout (internal/server/apikey_usage_booking_test.go:161-173), so none is vacuous. Closing as completed; reopen if the behaviour recurs.