fix: add TTL to job records to prevent memory leak#67
Conversation
Job records stored at {namespace}.jobs.{queue}.{pid} were accumulating
indefinitely because no TTL was set. This adds:
- TTL parameter to Connection interface set() and setArray() methods
- TTL support in Redis connection using setex()
- Configurable jobTtl property on Queue class (default: 24 hours)
- Cleanup of old job records after retry re-enqueue
WalkthroughAdds TTL support for stored jobs and adjusts retry behavior. The Queue constructor gains a public int property Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Queue/Broker/Redis.php`:
- Around line 152-158: The code currently calls $this->enqueue($queue,
$job->getPayload()) without checking its result and immediately deletes the job
record via $this->connection->remove(...), which can drop the job if enqueue
fails; update the logic to capture the result/exception from enqueue() and only
perform
$this->connection->remove("{$queue->namespace}.jobs.{$queue->name}.{$pid}") when
enqueue() succeeds, and if enqueue() fails, restore or re-add the PID to the
failed queue (or leave the job record intact) and log the failure so the job
isn't lost; reference the enqueue() call and the connection->remove(...)
invocation when making the change.
In `@src/Queue/Connection/Redis.php`:
- Around line 122-133: The RedisCluster class methods setArray and set must be
updated to match the Connection interface by adding the int $ttl = 0 parameter
to both signatures (setArray(string $key, array $value, int $ttl = 0) and
set(string $key, string $value, int $ttl = 0)), and implement TTL handling by
calling setex($key, $ttl, $value) when $ttl > 0 and getRedis()->set($key,
$value) otherwise; ensure setArray delegates to set($key, json_encode($value),
$ttl).
🧹 Nitpick comments (1)
src/Queue/Queue.php (1)
10-10: Consider adding validation forjobTtl.Negative TTL values would be passed to Redis
setex, which would cause an error. Consider validating thatjobTtlis positive, similar to thenamevalidation.🛡️ Proposed validation
public function __construct( public string $name, public string $namespace = 'utopia-queue', public int $jobTtl = 86400, ) { if (empty($this->name)) { throw new \InvalidArgumentException('Cannot create queue with empty name.'); } + if ($this->jobTtl <= 0) { + throw new \InvalidArgumentException('Job TTL must be a positive integer.'); + } }
Update setArray and set methods to match the Connection interface with TTL parameter support.
898b297 to
cd07b1b
Compare
Only delete job record after successful re-enqueue. If enqueue fails, re-add PID to failed queue so the job can be retried later.
cd07b1b to
659a51b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Queue/Broker/Redis.php`:
- Line 48: The current call to
$this->connection->setArray("{$queue->namespace}.jobs.{$queue->name}.{$message->getPid()}",
$nextMessage, $queue->jobTtl) starts the TTL at consume time and can let job
records expire during long processing; update the Broker\Redis logic so TTL is
not started at consume: either move TTL application to when a job is moved into
the failed list (apply TTL there), or refresh/set the TTL at that failure/retry
point, and update retry() to skip missing records instead of breaking if a job
key is absent; locate usages of setArray in the Redis class and modify the path
that sets jobTtl and the retry() implementation to handle missing keys
gracefully or defer TTL until failure.
In `@src/Queue/Queue.php`:
- Around line 7-11: The constructor for class Queue sets public int $jobTtl
without validating it, which can pass a negative TTL into Redis SETEX and cause
errors; update the __construct method to check the $jobTtl parameter (the public
property jobTtl) and either throw an InvalidArgumentException or normalize
negative values to 0 (choose one consistent behavior for the codebase) before
assigning it to the property so no negative TTL is stored or used.
Summary
Connectioninterfaceset()andsetArray()methodssetex()when TTL > 0jobTtlproperty toQueueclass (default: 0 / no expiration)Problem
Job records stored at
{namespace}.jobs.{queue}.{pid}were accumulating indefinitely because:Solution
jobTtlwhen creating a QueueUsage
Test plan
Summary by CodeRabbit