fix(control-plane): make the execution cleanup initial delay testable - #897
Conversation
cleanupLoop waited on a hardcoded 30 second timer before its first cleanup pass. That branch could not be reached by any test without sleeping for 30 seconds, so the first-pass path was never exercised. Move the value to a named constant and hold it in an unexported field that the constructor always populates. Production timing is unchanged: NewExecutionCleanupService is the only construction site, so every instance still gets 30 seconds, and no config or exported API surface is added. Tests in this package can shorten the field directly. A guard test asserts the default is still 30 seconds, so the value is now pinned by an assertion rather than left as an implicit assumption. Adds loop coverage for the initial-cleanup branch, the ticker branch, and the RetryStaleWorkflowExecutions error path. Each loop test neutralises the other timer so it can only pass for the right reason. execution_cleanup.go now reports 100% across all seven functions (cleanupLoop 84.6% -> 100%, performCleanup 97.9% -> 100%).
|
|
santoshkumarradha
left a comment
santoshkumarradha
left a comment
There was a problem hiding this comment.
Checked this in a clean worktree. The new initial-delay field keeps production behavior pinned at 30s, and the added loop tests hit the startup, ticker, and retry-error paths without broadening config surface. I also reran go test ./internal/handlers from control-plane/ on this branch locally and it passed.
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
3 participants
Found while looking at #118. That issue turns out to be already satisfied by #195, which I have noted there. Separately, one real gap did surface, which this PR addresses.
Problem
cleanupLoopwaits on a hardcoded 30 second timer before its first cleanup pass:That branch cannot be reached by a test without actually sleeping for 30 seconds, so the first cleanup pass after startup has never been exercised by anything. It is the path that runs on every server boot.
Change
Move the value to a named constant and hold it in an unexported field the constructor always populates.
Production timing is unchanged.
NewExecutionCleanupServiceis the only construction site in the repo (there are no struct literals elsewhere), so every instance still gets 30 seconds. Nothing is added toExecutionCleanupConfig, so no new YAML key, no new env var, and no exported API change. Operators cannot alter it. Tests in this package shorten the field directly.A guard test asserts the default is still 30 seconds, so the value is now pinned by an assertion instead of sitting as an implicit assumption inside a loop.
Tests
Three previously unreachable branches now have coverage:
cleanupLoopcleanupLoopRetryStaleWorkflowExecutionserror path inperformCleanupEach loop test neutralises the other timer, setting
CleanupIntervalto an hour when testing the initial pass andinitialDelayto an hour when testing the ticker, so neither can pass for the wrong reason.Result
execution_cleanup.gois now at 100% across all seven functions.cleanupLoopperformCleanupAll 18 tests touching the cleanup service pass. No existing tests were modified.