-
Notifications
You must be signed in to change notification settings - Fork 3
fix: add TTL to job records to prevent memory leak #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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