Inline Redis collection TTL metadata#997
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements inline TTL for Redis collections (list, hash, set, zset, stream) and HLL payloads by embedding expiration timestamps directly in their metadata anchors, supported by a background migrator and an option to disable the legacy secondary index fallback. The code review identified several critical issues: first, applyPositiveExpire does not update collection metadata with the new TTL, leading to desynchronization; second, the background migrator batches too many keys in a single transaction, risking frequent write conflicts; and third, both hasExpired and decodeHLLSetValueAt bypass the disableLegacyTTLReadFallback gate by unconditionally calling legacyIndexTTLAt. Additionally, the migrator should be made more robust to handle legacy TTL updates on already-migrated keys.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if err := c.dispatchCompaction(ctx, readTS, elems); err != nil { | ||
| c.logger.WarnContext(ctx, "ttl inline migrator: batch dispatch failed", | ||
| "type", h.typeName, "error", err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
migrateTTLInlineHandler accumulates migration elements for up to 512 different user keys (ttlInlineMigrationTickScanLimit) and dispatches them all in a single Raft transaction via c.dispatchCompaction. In a production environment, a single transaction containing up to 1024 elements (migration writes + TTL index syncs) across hundreds of distinct keys is highly susceptible to write conflicts. If a concurrent client write touches even one of these keys during the transaction, the entire batch of 512 keys will fail with ErrWriteConflict and abort. This leads to wasted CPU/network resources, high transaction retry overhead, and can severely slow down or stall the background migration. Recommendation: Batch the migration dispatches into much smaller groups (e.g., 16 or 32 keys per transaction), or dispatch them individually/in very small batches so that a write conflict on a single hot key does not roll back the migration progress of the entire scanned window.
| if err == nil && !found { | ||
| ttl, err = r.legacyIndexTTLAt(ctx, userKey, readTS) | ||
| } |
There was a problem hiding this comment.
In hasExpired, when nonStringOnly is true and collectionTTLAt misses (found == false), legacyIndexTTLAt is called unconditionally. This completely bypasses the r.disableLegacyTTLReadFallback gate. Even after the operator disables the legacy fallback to optimize hot-path reads, any non-string check (such as EXISTS on a non-existent key or a key without a TTL) will still perform an expensive, redundant point lookup on the !redis|ttl|<key> index. Recommendation: Wrap the legacyIndexTTLAt call in a check for !r.disableLegacyTTLReadFallback.
| if err == nil && !found { | |
| ttl, err = r.legacyIndexTTLAt(ctx, userKey, readTS) | |
| } | |
| if err == nil && !found && !r.disableLegacyTTLReadFallback { | |
| ttl, err = r.legacyIndexTTLAt(ctx, userKey, readTS) | |
| } |
| if ttlMs == 0 { | ||
| ttlMs, err = legacyTTLMillisAt(ctx, c.st, userKey, readTS) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
In simpleTTLInlineMigrationHandler, the migrator only queries the legacy TTL index via legacyTTLMillisAt if ttlMs == 0 (i.e., the metadata key is in the legacy format or has no inline TTL). If a collection key has already been migrated to the inline format (so ttlMs > 0), but a subsequent EXPIRE command executed inside a MULTI/EXEC transaction or a Lua script updates the legacy TTL index without updating the metadata key, the migrator will completely skip reading the legacy TTL index. As a result, the metadata key will remain stuck with the stale TTL, and ttlAt (which prioritizes inline metadata) will return the stale TTL. While the primary fix should be ensuring that transactions and Lua scripts update the metadata key directly, the migrator could also be made more robust by always syncing the metadata TTL if the legacy TTL index has a different, newer value during the migration window.
| if !embedded { | ||
| ttl, err = r.legacyIndexTTLAt(ctx, key, readTS) | ||
| if err != nil { | ||
| return redisSetValue{}, err | ||
| } | ||
| } |
There was a problem hiding this comment.
In decodeHLLSetValueAt, if the HLL payload is in the legacy format (embedded == false), legacyIndexTTLAt is called unconditionally. This ignores the r.disableLegacyTTLReadFallback gate, meaning we will still perform a secondary index lookup for legacy HLL keys even after the fallback has been disabled. Recommendation: Respect r.disableLegacyTTLReadFallback before calling legacyIndexTTLAt.
| if !embedded { | |
| ttl, err = r.legacyIndexTTLAt(ctx, key, readTS) | |
| if err != nil { | |
| return redisSetValue{}, err | |
| } | |
| } | |
| if !embedded && !r.disableLegacyTTLReadFallback { | |
| ttl, err = r.legacyIndexTTLAt(ctx, key, readTS) | |
| if err != nil { | |
| return redisSetValue{}, err | |
| } | |
| } |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50d34a5e09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Handled the latest TTL-inline review blockers.
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 607ee9c82d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Handled the latest TTL-inline review blockers.
Validation:
Note: go test ./adapter -count=1 -timeout=300s reached the package-level timeout after the focused coverage and lint above had passed. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b26fd2a95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Fixed the latest HLL TTL precedence finding. Changes:
Caller audit:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd45d8ccfb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Handled the latest TTL-inline review blockers.
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1b3251649
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if legacyTTL != 0 && legacyTTL != ttlMs { | ||
| ttlMs = legacyTTL |
There was a problem hiding this comment.
When this handler scans an already-current hash/set/zset meta value, ttlMs came from the inline metadata and is authoritative, but this fallback unconditionally re-imports any stale !redis|ttl| row. For a current 16-byte meta with ExpireAt=0 plus an old scan-index TTL, the migrator rewrites the collection with that stale TTL and the key can later expire even though the inline anchor says it is persistent. Only consult the legacy TTL when pair.Value is the legacy 8-byte meta format.
Useful? React with 👍 / 👎.
| if legacyTTL != 0 && legacyTTL != meta.ExpireAt { | ||
| meta.ExpireAt = legacyTTL |
There was a problem hiding this comment.
For a current-format list meta row, meta.ExpireAt is already the authoritative TTL, including 0 for no TTL; this unconditional legacy fallback will copy a stale !redis|ttl| value back into the inline meta. A list that was made persistent but still has an old scan-index row will be rewritten as expiring by the migrator. Gate this fallback on the legacy 24-byte list meta format; the stream migrator below repeats the same pattern and needs the same guard for 24-byte stream meta.
Useful? React with 👍 / 👎.
| prefix: []byte(redisStrPrefix), | ||
| extractUserKey: trimKnownPrefix([]byte(redisStrPrefix)), |
There was a problem hiding this comment.
This string migration pass only scans !redis|str| anchors, but the read path still supports pre-prefix bare string keys whose TTL lives in !redis|ttl|<key>. Those bare strings are never rewritten into an inline anchor, so after WithRedisLegacyTTLReadFallback(false) is enabled, ttlAt/keyTypeAt stop consulting their only TTL source and expired bare strings can look persistent to TTL/EXISTS/type checks. Either include a bare-string migration path or keep the fallback gate from being advertised as safe while bare legacy strings may exist.
Useful? React with 👍 / 👎.
| } | ||
| userKey := []byte(key) | ||
| typ := t.collectionExpireTypes[key] | ||
| built, ok, err := t.server.collectionExpireElems(ctx, userKey, t.startTS, typ, redisExpireAtMillis(*st.value)) |
There was a problem hiding this comment.
When the same EXEC recreates a list and then expires it, such as DEL k; RPUSH k v; EXPIRE k 60 against an existing list, this call rebuilds TTL metadata from t.startTS instead of the staged list state. prepareDispatch appends these collection TTL elems after buildListElems, so the stale meta Put from the pre-transaction list can overwrite the newly created inline meta and leave the list with the old head/length rather than the RPUSH result. Skip keys whose collection meta is already rewritten in the transaction, or update the staged state before emitting elems.
Useful? React with 👍 / 👎.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Design doc
Validation
Author: bootjp