Skip to content

fix: Prevent cache TTL expiry from racing read-write persistence tests - #444

Draft
kinyoklion wants to merge 1 commit into
v2from
rlamb/persistence-ttl-flake
Draft

fix: Prevent cache TTL expiry from racing read-write persistence tests#444
kinyoklion wants to merge 1 commit into
v2from
rlamb/persistence-ttl-flake

Conversation

@kinyoklion

Copy link
Copy Markdown
Member

Summary

Fixes a flaky failure in persistent data store/<store>/read-write/cache mode ttl/ignores dropped flags (observed on go-server-sdk CI as flag was never updated from the RequireNever at the 500ms window; the same hazard exists for the consul and dynamodb variants and for the sibling ignores direct database modifications test).

Root cause: the SDK's cache entry for flag-key is created when the data source delivers the flag at client startup, so the 1-second TTL clock starts then. Before the test opens its 500ms RequireNever window it still has to run eventuallyRequireDataStoreInit (polls up to 1s), pollUntilFlagValueUpdated, and the store Reset. On a slow runner those setup steps consume enough of the TTL that the cache expires inside the RequireNever window; 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 Reset call (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:

  • Raise the TTL used by the read-write cache-mode tests from 1 second to 5 seconds, so setup latency cannot plausibly reach the check windows.
  • ignores direct database modifications: use a fixed 500ms RequireNever window instead of TTL/2, which both shortens the test and keeps the window far from the expiry.
  • ignores dropped flags: scale the post-expiry RequireEventually wait 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 store suite (redis, consul, dynamodb; 76 tests) against the go-server-sdk test service locally: all pass, total suite runtime ~58s.

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.

1 participant