fix: Prevent cache TTL expiry from racing read-write persistence tests - #444
Draft
kinyoklion wants to merge 1 commit into
Draft
fix: Prevent cache TTL expiry from racing read-write persistence tests#444kinyoklion wants to merge 1 commit into
kinyoklion wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a flaky failure in
persistent data store/<store>/read-write/cache mode ttl/ignores dropped flags(observed on go-server-sdk CI asflag was never updatedfrom theRequireNeverat the 500ms window; the same hazard exists for the consul and dynamodb variants and for the siblingignores direct database modificationstest).Root cause: the SDK's cache entry for
flag-keyis created when the data source delivers the flag at client startup, so the 1-second TTL clock starts then. Before the test opens its 500msRequireNeverwindow it still has to runeventuallyRequireDataStoreInit(polls up to 1s),pollUntilFlagValueUpdated, and the storeReset. On a slow runner those setup steps consume enough of the TTL that the cache expires inside theRequireNeverwindow; the SDK then re-reads the emptied store, the flag evaluates to the default value, and the assertion fails. CI timestamps match this exactly: the failure landed ~1.19s after the subtest started, right at TTL expiry.Reproduced deterministically against the real Go SDK test service and a local redis by inserting a 700ms delay before the
Resetcall (simulating a slow runner); the test then fails every time with the CI failure signature, and passes every time with this change.The fix gives the timing assumption real margin instead of ~500ms:
ignores direct database modifications: use a fixed 500msRequireNeverwindow instead of TTL/2, which both shortens the test and keeps the window far from the expiry.ignores dropped flags: scale the post-expiryRequireEventuallywait to the TTL instead of a hardcoded 1 second, matching how the sibling test already scales it.The daemon-mode cache-duration tests keep their 1-second TTL: they anchor their timing to a read performed immediately before their check windows, so they do not have this race.
Validated by running the full
persistent data storesuite (redis, consul, dynamodb; 76 tests) against the go-server-sdk test service locally: all pass, total suite runtime ~58s.