Prevent orphaned cohort version sets when a refresh fails mid-flight

[adrianliu0815]

Problem

A cohort version's member set (...:cohort:{id}:User:{lastModified}) is written with no TTL, and only receives one when a successor refresh completes successfully (the EXPIRE at the end of complete()). Any refresh that dies mid-flight — Redis command timeout, pod restart, CohortTooLargeException — strands the full member set in Redis forever.

For multi-million-member cohorts this leaks ~0.5 GB per failed refresh, and the cohort hash tag pins every leaked copy to the same cluster slot. In one production cluster we found 58 orphaned version sets (~25 GB) for a single 9.8M-member cohort, accumulated over two months of intermittent refresh failures (CLUSTER GETKEYSINSLOT census). The effect is self-compounding: large-cohort refreshes are the slowest and most timeout-prone, so the leak concentrates on exactly the shards already carrying the largest sets.

There is a second, related defect: the EXPIRE of the previous version ran in a finally block, so a refresh that failed mid-membership-update expired the old — still published — version set. Reads of the live cohort then break (Cohort description found, but members missing) until the next successful refresh.

Fix

1. Pending version sets self-clean. Apply a 24h TTL to the new version's member set as soon as ingestion starts writing it (addMembers), and PERSIST it at promotion (just before the description is published in complete()). An aborted refresh now leaves nothing behind once the TTL fires. On the retry path Amplitude returns the same lastModified, so the retry re-uses (and re-arms) the same key — no interference.
2. Temp diff keys get the same backstop TTL immediately after the two SDIFFSTOREs, in case the process dies before the finally DELs run.
3. Retire the previous version only after successful promotion, instead of in the finally block. A failed refresh now leaves the published version untouched; the success path expires it exactly as before.

Redis.persist() is added to the interface and both connection implementations (plain + cluster), mirroring expire().

Behavior notes

• Success-path behavior is unchanged: previous version set + blob still get the same ttl (cohort sync interval) after a successful update; temp keys still deleted in finally.
• PERSIST/EXPIRE on the pending set are O(1) and once-per-refresh — no measurable overhead.
• 24h for the pending TTL is deliberately far above any plausible download+diff duration (the loading lock itself is 900s).

Testing

• ./gradlew check green.
• New tests: pending version set carries a TTL during ingestion and loses it on promotion; an unpromoted (failed) refresh leaves the published version un-expired and readable while the stranded pending set keeps its self-clean TTL.

Relationship to #38

Independent and complementary: #38 removes the blocking SDIFFSTORE (the main cause of failed refreshes on giant cohorts); this PR makes any remaining failure mode unable to leak storage or expire the live version.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes cohort refresh promotion and Redis key lifecycle on a critical read path; behavior on success is intended to match prior semantics, but mis-timed TTL/PERSIST could affect live cohort availability.

Overview
Fixes Redis leaks and broken reads when a cohort refresh aborts mid-flight (timeouts, restarts, etc.).

In-flight versions self-clean: a 24h TTL is set on the new version’s member set as soon as ingestion writes (addMembers), and on added/removed diff temp keys after SDIFFSTORE. On successful promotion, PERSIST clears the TTL on the new member set before the description is published.

Live cohort stays readable on failure: expiring the previous version’s member set and blob is moved out of the finally block and runs only after successful promotion—so a failed refresh no longer expires the still-published version.

Adds Redis.persist() (interface, cluster, and standalone) plus tests for pending TTL, promotion, and aborted refresh behavior.

^{Reviewed by Cursor Bugbot for commit 26f1232. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.}

^{Reviewed by Cursor Bugbot for commit 26f1232. Configure here.}

[cursor]

Persist clears TTL before publish

Medium Severity

complete() calls redis.persist(newCohortKey) before the cohort description is written with hset. If that refresh aborts after persist but before hset, the pending member set loses the 24h PENDING_VERSION_TTL from ingestion while the live description still points at the previous version, so the partial set can remain in Redis indefinitely instead of self-cleaning.

 

^{Reviewed by Cursor Bugbot for commit 26f1232. Configure here.}

[vaibhav-jain-exp]

Thanks — this is a real problem well diagnosed (we've seen the orphaned version sets too), and the lifecycle design is sound: arming the TTL at first write, PERSIST at promotion, and retiring the previous version only after publish is exactly the right shape. The prev.lastModified != description.lastModified guard on the retire block also quietly fixes a latent bug where an unchanged-version re-complete could expire the live members set. Happy to merge this with one change:

Requested change: arm each temp diff key's backstop TTL immediately after its own SDIFFSTORE rather than after both. As written, addedKey has no TTL for the entire duration of the second SDIFFSTORE — itself a multi-second blocking command on large cohorts and the likeliest point for the process to die — which strands a potentially full-cohort-sized key, i.e. exactly the leak this PR fixes. The reorder is safe: SDIFFSTORE clears its destination's TTL, and addedKey isn't a source of the second SDIFFSTORE. Ready-made commit you can cherry-pick: f5e7acf on pr39-backstop-per-sdiffstore.

On the Cursor bugbot comment (persist-before-hset): the window is real, but please don't fix it by swapping the order alone — a crash after hset but before persist would then leave the live, published version with a 24h fuse and no code path that ever clears it (the next sync short-circuits on lastModified), which trades a rare self-healing leak for a rare unrecoverable read breakage. The full fix needs both halves: publish-then-persist plus a repair that re-PERSISTs the published version's members key on the not-modified sync path. We've implemented that (with a fault-injection test for the crash-at-publish window) in #40, which incorporates this PR — so feel free to leave bugbot's finding to #40 rather than grow this PR's scope.

[vaibhav-jain-exp]

We've applied the backstop-ordering tweak on our side: #41 carries this PR's commit unchanged plus that one fix, targeting main. Once #41 merges, GitHub will mark this PR as merged automatically — nothing needed from you. Thanks again!