Fix batch plugin interaction with REQUEUE - #40
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a requeue context key and 🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@batch/middleware_test.go`:
- Around line 566-568: Replace the direct time.Sleep(100 * time.Millisecond) in
the async callback checks with a polling helper that repeatedly calls the queue
fetcher until either a job/callback appears or a timeout elapses; implement a
helper like waitForCallback(t, cl, queue, timeout) that loops calling
cl.Fetch(queue) (or the existing fetch function used in these tests), sleeps
briefly (e.g., 10ms) between attempts, asserts no fetch error, and returns the
job when non-nil (then use require.NotNil/require.NoError on its result), and
use this helper in place of the Sleep in the tests that currently use time.Sleep
(including the instance around the async callback and the one at the other noted
location).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ad53132-79c6-43bd-822c-8a12f07ed28f
📒 Files selected for processing (5)
batch/middleware.gobatch/middleware_test.gobatch/test_helper_test.gorequeue/context.gorequeue/requeue.go
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
batch/middleware_test.go (1)
643-643:⚠️ Potential issue | 🟡 MinorReplace fixed sleep with bounded polling for stability.
At Line 643,
time.Sleep(100 * time.Millisecond)can be flaky under load and slower than needed on fast runs.Proposed fix
- time.Sleep(100 * time.Millisecond) - - statusResult, err = cl.Generic("BATCH STATUS " + bid) - require.NoError(t, err) - err = json.Unmarshal([]byte(statusResult), &status) - require.NoError(t, err) + deadline := time.Now().Add(500 * time.Millisecond) + for time.Now().Before(deadline) { + statusResult, err = cl.Generic("BATCH STATUS " + bid) + require.NoError(t, err) + err = json.Unmarshal([]byte(statusResult), &status) + require.NoError(t, err) + if status.Pending == 0 { + break + } + time.Sleep(10 * time.Millisecond) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/middleware_test.go` at line 643, Replace the brittle fixed sleep (time.Sleep(100 * time.Millisecond)) with a bounded polling pattern: use a short ticker (e.g., 10–20ms) and a total timeout (e.g., 1s) to repeatedly check the expected condition until it becomes true, failing the test if the timeout elapses. Locate the call to time.Sleep(100 * time.Millisecond) in the test (in batch/middleware_test.go) and implement either a small helper like waitForCondition(func() bool, timeout, interval time.Duration) or use require.Eventually/assert.Eventually with the condition to poll; ensure the check inspects the same state the test expects and that the test returns/fails on timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@batch/middleware_test.go`:
- Around line 533-535: The test's single immediate call to cl.Fetch("callbacks")
can miss a delayed enqueue and give a false negative; update the assertion to
wait/poll for a short timeout (e.g., 100-500ms) and repeatedly call
cl.Fetch("callbacks") until the timeout to ensure no callback is enqueued
asynchronously. You can implement this by using testify's require.Eventually
with a closure that does cl.Fetch("callbacks") and asserts callback==nil and
err==nil, or by adding a simple loop with time.Sleep and a deadline that calls
cl.Fetch("callbacks") each iteration and fails if any non-nil callback appears
before the deadline; reference the existing cl.Fetch usage and the callback
variable in the test when making the change.
---
Duplicate comments:
In `@batch/middleware_test.go`:
- Line 643: Replace the brittle fixed sleep (time.Sleep(100 * time.Millisecond))
with a bounded polling pattern: use a short ticker (e.g., 10–20ms) and a total
timeout (e.g., 1s) to repeatedly check the expected condition until it becomes
true, failing the test if the timeout elapses. Locate the call to time.Sleep(100
* time.Millisecond) in the test (in batch/middleware_test.go) and implement
either a small helper like waitForCondition(func() bool, timeout, interval
time.Duration) or use require.Eventually/assert.Eventually with the condition to
poll; ensure the check inspects the same state the test expects and that the
test returns/fails on timeout.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 924ff5e6-f8e1-447f-ac65-b6ca7e731a80
📒 Files selected for processing (1)
batch/middleware_test.go
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
batch/middleware_test.go (2)
533-537:⚠️ Potential issue | 🟠 MajorUse a “never happens” assertion for callback absence (current check is unstable).
At Line 533,
require.Eventually(... callback == nil ...)is not the right semantic for “callback must not fire,” and combined with repeatedFetchcan produce flaky behavior (matches the current CI failure around Lines 533/535). Use a bounded negative assertion (assert.Never) or reusewaitForCallbackand assert nil.Proposed fix
- // Callback should NOT have fired - poll for a short period to ensure no delayed enqueue - require.Eventually(t, func() bool { - callback, err := cl.Fetch("callbacks") - require.NoError(t, err) - return callback == nil - }, 200*time.Millisecond, 10*time.Millisecond, "complete callback should not fire after REQUEUE") + // Callback should NOT have fired during the observation window + callback := waitForCallback(t, cl, "callbacks", 300*time.Millisecond) + assert.Nil(t, callback, "complete callback should not fire after REQUEUE")#!/bin/bash # Verify all async callback-negative assertions and fetch usage patterns. rg -n -C2 'callback should NOT have fired|require\.Eventually\(|assert\.Never\(|Fetch\("callbacks"\)' batch/middleware_test.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/middleware_test.go` around lines 533 - 537, The test uses require.Eventually with callback == nil and repeated cl.Fetch("callbacks"), which is flaky for asserting "callback must not fire"; replace this pattern by using assert.Never (or reuse the existing waitForCallback helper) to assert the callback never appears: call a closure that does cl.Fetch("callbacks") and returns true if callback != nil and pass that to assert.Never (or call waitForCallback and assert the result is nil) instead of require.Eventually, updating the test around require.Eventually / cl.Fetch("callbacks") to use assert.Never or waitForCallback for a bounded negative assertion.
645-645:⚠️ Potential issue | 🟡 MinorReplace fixed sleep with deterministic polling after final ACK.
Line 645 reintroduces timing-based flakiness. Poll for the expected async signal (callback or status transition) instead of sleeping for a fixed 100ms.
Proposed fix
- time.Sleep(100 * time.Millisecond) + callback := waitForCallback(t, cl, "callbacks", 500*time.Millisecond) + require.NotNil(t, callback, "complete callback should fire after final ACK") + assert.Equal(t, "CompleteCallback", callback.Type)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/middleware_test.go` at line 645, The test currently uses a fixed time.Sleep(100 * time.Millisecond) which causes timing-based flakiness; remove that sleep and replace it with deterministic polling for the final ACK/expected async signal (e.g., check the callback-invocation flag, ack channel, or status transition used in this test) by looping with a short tick interval and an overall timeout, returning immediately when the condition is met and failing the test on timeout—replace the single time.Sleep call in middleware_test.go (the spot referring to the final ACK) with this polling loop that checks the exact condition the test expects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@batch/middleware_test.go`:
- Around line 533-537: The test uses require.Eventually with callback == nil and
repeated cl.Fetch("callbacks"), which is flaky for asserting "callback must not
fire"; replace this pattern by using assert.Never (or reuse the existing
waitForCallback helper) to assert the callback never appears: call a closure
that does cl.Fetch("callbacks") and returns true if callback != nil and pass
that to assert.Never (or call waitForCallback and assert the result is nil)
instead of require.Eventually, updating the test around require.Eventually /
cl.Fetch("callbacks") to use assert.Never or waitForCallback for a bounded
negative assertion.
- Line 645: The test currently uses a fixed time.Sleep(100 * time.Millisecond)
which causes timing-based flakiness; remove that sleep and replace it with
deterministic polling for the final ACK/expected async signal (e.g., check the
callback-invocation flag, ack channel, or status transition used in this test)
by looping with a short tick interval and an overall timeout, returning
immediately when the condition is met and failing the test on timeout—replace
the single time.Sleep call in middleware_test.go (the spot referring to the
final ACK) with this polling loop that checks the exact condition the test
expects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2de80b4f-0e33-4b6f-a918-a7ac0e9db8cf
📒 Files selected for processing (1)
batch/middleware_test.go
…t to pre-emptively over-engineer the tests
da235f1 to
3388ea3
Compare
1 participant
Summary
The REQUEUE command (
REQUEUE {"jid":"..."}) has two bugs when used on a job that belongs to a batch:Bug 1: Race condition causing premature callback firing and job loss
REQUEUE works by calling
Acknowledge()(which fires the ACK middleware) then re-pushing the job (which fires the PUSH middleware). Between these two steps, the batch plugin'sackMiddlewaredecrementspending— potentially to 0 — and spawns a goroutine to check callbacks. If that goroutine runs before the PUSH middleware re-incrementspending, it seespending=0and fires the complete callback. The subsequent PUSH then fails with "cannot add jobs to batch after callbacks have started" because the Lua script checks callback state. The job is now lost: it was ACK'd out of the working set but never re-enqueued.Bug 2: Inflated
totalcounterEven without the race, each REQUEUE increments
totalvia the push middleware while the ACK middleware decrementspending. The PUSH middleware then increments bothtotalandpendingback. Net effect:totalgrows by 1 per REQUEUE. A 2-job batch where one job is requeued 3 times showstotal=5.Root cause
Both bugs stem from the same issue: the batch plugin's ACK and PUSH middleware treat REQUEUE as a completion followed by a new job addition, when semantically REQUEUE means "put the job back, it's still pending."
Fix
The requeue plugin now sets a context flag (
RequeueContextKey) before callingAcknowledge()and the push middleware chain. The batch plugin'sackMiddlewareandpushMiddlewarecheck for this flag and skip all batch accounting when it's set. During REQUEUE, batch counters (total,pending,failed) are completely untouched — the job remains "pending" from the batch's perspective.Test plan
TestRequeueDoesNotAffectBatchCounters— REQUEUE a batch job, verify total/pending/failed unchangedTestRequeueLastPendingJobDoesNotFireCallbacks— REQUEUE the last pending job, verify callbacks don't fireTestRequeueThenAckCompletesBatch— REQUEUE then ACK, verify batch completes correctlyTestRequeueMultipleTimesCountersCorrect— REQUEUE 3 times then ACK, verify counters are correct