Skip to content

Fix flaky TestDisposeStaleCryptoKeys caused by mock clock ticker handling#7128

Open
amartinezfayo wants to merge 2 commits into
spiffe:mainfrom
amartinezfayo:fix-flaky-dispose-stale-crypto-keys
Open

Fix flaky TestDisposeStaleCryptoKeys caused by mock clock ticker handling#7128
amartinezfayo wants to merge 2 commits into
spiffe:mainfrom
amartinezfayo:fix-flaky-dispose-stale-crypto-keys

Conversation

@amartinezfayo

Copy link
Copy Markdown
Member

TestDisposeStaleCryptoKeys drives disposeCryptoKeysTask through the mock clock, but its correctness depended on goroutine scheduling in two ways, so it would occasionally hang until the test timed out.

disposeCryptoKeysTask creates its ticker and only then emits an initialization notification, yet the test advanced the clock before draining that notification. When the task goroutine won the race and created its ticker at the starting time, the ticker's baseline was earlier than the test assumed. The first dispose run then executed while the CryptoKeys were not yet stale, and because the task blocks on the unbuffered notification channel after each run, that non-stale run's notification stalled the single task goroutine before it could reach the run that schedules destruction. The test then waited forever for a destroy notification that never arrived.

The test also advanced the clock by many ticker periods at once. github.com/andres-erbsen/clock advances its notion of "now" incrementally as it fires each intermediate tick and yields between them, so a multi-period advance fires the ticker while "now" is still at an intermediate value. The dispose run could observe a not-yet-stale clock even though the advance targeted a stale time.

This PR makes the handshake deterministic. The test drains the initialization notification before advancing the clock, so the ticker's baseline is fixed. The CryptoKeys start with a last-update timestamp older than maxStaleDuration so they are already stale, and the clock is advanced exactly one ticker period at a time, which fires the ticker once with the clock already at the advanced time. Stale detection no longer depends on which intermediate clock value the running task happens to observe.

This is a follow-up to #7081, which addressed the Sleep-based retry loops in other gcp_kms tests but not the ticker handshake in this one.

An example of a failure is here: https://github.com/spiffe/spire/actions/runs/28537752222/job/84603474025

panic: test timed out after 1m30s
	running tests:
		TestDisposeStaleCryptoKeys (1m30s)

goroutine 437 [select]:
...gcpkms.waitForSignal(...)
	.../gcpkms/gcpkms_test.go:1796
...gcpkms.TestDisposeStaleCryptoKeys(...)
	.../gcpkms/gcpkms_test.go:519

…ling

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR stabilizes TestDisposeStaleCryptoKeys in the gcp_kms key manager plugin tests by making mock-clock/ticker synchronization deterministic, avoiding races and intermediate-time observations that could previously lead to hangs/timeouts.

Changes:

  • Make test CryptoKeys already stale by setting spire-last-update older than maxStaleDuration.
  • Drain the dispose task “initialized” notification before advancing the mock clock, fixing the ticker baseline race.
  • Advance the mock clock exactly one ticker period at a time and remove the now-unneeded maxDuration helper.

Comment thread pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go Outdated
Comment thread pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go Outdated
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
@amartinezfayo amartinezfayo marked this pull request as ready for review July 8, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants