@bhuvan-somisetty Thanks for your PR. Look at the current code and the propose changes, I wonder if it really fixing any obvious issue which can be replicated and testable by human ?

• About issue #1: Fix Parameter Binding Type Mismatch

Actually, the current code works well and does not require any change. The $id in this case, will be an empty array or contain only one value. Although it looks a bit strange, $lockQuery->bind(':taskId', $id, ParameterType::INTEGER) works fine with that.

• About issue #2: Query by Timestamp is fine, too. The code run before that (only call the method if $affectedRows == 1) already warrant that there is only one task looked with that timestamp

• About issue #3: As explained in #2, there will always a task with that timestamp, so the null check as you added is redundant

• About issue #4: I'm not really sure, but adding the condition will prevent the failed task (it could be because timed out....) never executed again and that's not what we want.

I'm not saying your changes are not right (for #1, #2 and #3), but the current code is working fine, too, so I would be hesitate to accept this change to avoid any possible regressions.

@bhuvan-somisetty Thanks for your PR. Look at the current code and the propose changes, I wonder if it really fixing any obvious issue which can be replicated and testable by human ?

• About issue 1: Fix Parameter Binding Type Mismatch

Actually, the current code works well and does not require any change. The $id in this case, will be an empty array or contain only one value. Although it looks a bit strange, $lockQuery->bind(':taskId', $id, ParameterType::INTEGER) works fine with that.

• About issue 2: Query by Timestamp is fine, too. The code run before that (only call the method if $affectedRows == 1) already warrant that there is only one task looked with that timestamp

• About issue 3: As explained in 2 above, there will always a task with that timestamp, so the null check as you added is redundant

• About issue 4: I'm not really sure, but adding the condition will prevent the failed task (it could be because timed out....) never executed again and that's not what we want.

I'm not saying your changes are not right (for #1, #2 and #3), but the current code is working fine, too, so I would be hesitate to accept this change to avoid any possible regressions.

@joomdonation Thanks for the feedback!

1. Issue 4 (Timed-out tasks): You're completely right here. Having \WHERE locked IS NULL\ prevented stalled/timed-out tasks from being picked up again for retry. I've removed that condition so timed-out tasks are handled properly as before.

2. Issue 2 & 3 (Query by ID vs Timestamp): The main issue with \WHERE locked = :now\ is when concurrency is involved (e.g. \�llowConcurrent = true\ or multiple triggers firing within the same second). Since the timestamp has second precision, two different tasks locked in the same second share the same \locked\ value. \WHERE locked = :now\ with \loadObject()\ can then return the wrong task to the runner. Fetching by primary key (\WHERE id = :taskId) ensures deterministic, race-free task retrieval.

3. Issue 1 (Parameter binding): Using (int) loadResult()\ instead of \loadColumn()\ returns a scalar integer rather than an array, aligning cleanly with \�ind(':taskId', , ParameterType::INTEGER)\ and avoiding binding an array to a single placeholder.

Hope this clarifies the rationale, and thank you again for pointing out the timeout behavior!

@joomdonation Thanks for the feedback!

1. Issue 4 (Timed-out tasks): You're completely right here. Having WHERE locked IS NULL prevented stalled/timed-out tasks from being picked up again for retry. I've removed that condition so timed-out tasks are handled properly as before.

2. Issue 2 & 3 (Query by ID vs Timestamp): The main issue with WHERE locked = :now is when concurrency is involved (e.g. allowConcurrent = true or multiple triggers firing within the same second). Since the timestamp has second precision, two different tasks locked in the same second share the same locked value. WHERE locked = :now with loadObject() can then return the wrong task to the runner. Fetching by primary key (WHERE id = :taskId) ensures deterministic, race-free task retrieval.

3. Issue 1 (Parameter binding): Using (int) loadResult() instead of loadColumn() returns a scalar integer rather than an array, aligning cleanly with bind(':taskId', $taskId, ParameterType::INTEGER) and avoiding binding an array to a single placeholder.

Hope this clarifies the rationale, and thank you again for pointing out the timeout behavior!

For issue 1 : As mentioned, although it looks strange, but $lockQuery->bind(':taskId', $id, ParameterType::INTEGER) works perfect fine when $id is array with one element. You can check bind method of DatabaseQuery to confirm (not saying that your code is wrong, but current code works well)

For 2 and 3: I will have to look at the code again. But from what I see, the fetchTask method will only be executed when there is one row with locked set to that timestamp. There is a check before that:

if ($affectedRows != 1) {
       return null;
}

So it should match and return only one right task and current code should work well (unless you can have a way for us - human - to see the issue somehow).

looks like AI generated stuff to me

Fair concern re: AI-slop. Fwiw the real bug here is the race window between the UPDATE and the fetch, not the parameter binding (that part was harmless, just a cleanup).

Look at the flow: getTask() calls unlockTables() in the finally block, and only after that does it call fetchTask(). So by the time fetchTask() runs its WHERE locked = :now query, the table lock is already released. If a second scheduler run grabs a different task in that same second (timestamp only has second precision), both processes end up with rows where locked = $now, and loadObject() with no ordering can hand either process the wrong row.

Querying by the id we just locked instead of by timestamp closes that window. It's a narrow edge case (needs two runs landing in the same second), which is probably why it's hard to notice by inspection alone, but it's real under concurrent/cron-overlap scenarios.

Happy to drop the array-vs-scalar binding change if you'd rather keep that as-is since you're right it works fine either way - it was just tidying up alongside the actual fix.

Hello @bhuvan-somisetty

thank you for your contribution. We do not want to talk to AI generated text nor want to review AI code. So I'm closing here.

If you're willing to contribute following the AI policy you checked, I'm looking forward to your human contribution in the future 🙂️