Fix an issue with updating data in redis #1708
Conversation
…s keys with no expiration (TTL = 0), but the Update method was trying to use redis.KeepTTL, which fails because there's no TTL to preserve.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| // Execute transaction | ||
| err = s.redisClient.Set(ctx, key, newData, redis.KeepTTL).Err() | ||
| err = s.redisClient.Set(ctx, key, newData, 0).Err() |
There was a problem hiding this comment.
Setting TTL to 0 means no expiration. Both Add() (line 29) and now Update() store keys without expiration, but the comment on line 28 says "with max expiration little bit longer than max instance length to prevent leaking". This means sandbox data will persist indefinitely in Redis unless explicitly removed via Remove(). If the cleanup logic fails, this could cause memory leaks. Consider either: (1) setting an actual TTL value (e.g., based on max instance lifetime), or (2) updating the comment to reflect that Redis is not being used as a TTL-based cache for sandboxes.
![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: 0f6a85f0bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // Execute transaction | ||
| err = s.redisClient.Set(ctx, key, newData, redis.KeepTTL).Err() | ||
| err = s.redisClient.Set(ctx, key, newData, 0).Err() |
There was a problem hiding this comment.
Using Set(..., 0) explicitly removes any existing TTL on the key. If sandbox entries ever have expirations set (e.g., via EXPIRE elsewhere or from prior versions), this update makes them persistent after any write, so they will never expire and can leak. The previous redis.KeepTTL preserved those expirations. Consider keeping TTLs (or reapplying the current TTL) when updating so updates don’t disable expiry for keys that have one.
Useful? React with 👍 / 👎.
|
Double checked and it's just intermittent, so this isn't probably the case. Will check it more later |
There's no ttl, so redis returns an error if you try to keep it. You have to explicitly set 0 (disabled)
Note
Fixes Redis update behavior to avoid TTL-related errors.
packages/api/internal/sandbox/storage/redis/operations.go, changeSet(..., redis.KeepTTL)toSet(..., 0)inUpdateso updates succeed when the key has no TTL, aligning withAdd's behavior.Written by Cursor Bugbot for commit 0f6a85f. This will update automatically on new commits. Configure here.