fix(coordinator): task dispatch fixes — priority order, proofs-ready guard, attempts refund#1813
Open
lispc wants to merge 3 commits into
Open
fix(coordinator): task dispatch fixes — priority order, proofs-ready guard, attempts refund#1813lispc wants to merge 3 commits into
lispc wants to merge 3 commits into
Conversation
… dispatch Previously GetTasks randomly shuffled the prover's requested task types and tried only the first one, rejecting the request with empty-proof-data when that type had no task available even if other types did. Now the requested types are tried in deterministic priority order (Bundle > Batch > Chunk), so the pipeline finalizes the earliest ready bundle first instead of proving unrelated chunks/batches further ahead, and a type with no available task no longer starves the others.
CheckIfBatchChunkProofsAreReady and CheckIfBundleBatchProofsAreReady previously counted only unverified children, so a batch with zero chunks (or a bundle with zero batches) vacuously counted as proofs-ready and could dispatch an aggregate proving task with no children. Both now return false when the child set is empty. Also fix the misleading 'Chunk.' prefix in the Batch method's error message.
When the coordinator failed to dispatch a freshly assigned task (formatProverTask / applyUniversal / InsertProverTask failure), only active_attempts was decremented while total_attempts stayed burned. After SessionAttempts (5) such failures the task became invisible to GetUnassigned* queries, yet had no prover_task row for the cron timeout check to mark it failed -- the task starved silently. Add RefundAttemptsByHash to the chunk/batch/bundle ORMs (decrement both counters, reset proving_status to unassigned, guarded against underflow and against touching verified rows) and use it from the prover tasks for fresh assignments; re-polls of already assigned tasks keep the old active-attempts-only behavior. This also adds the previously missing attempts recovery on the fixCompatibility failure path of batch/bundle tasks.
📝 WalkthroughWalkthroughThe coordinator now tries proof assignments in deterministic Bundle, Batch, Chunk priority order. Failed newly assigned tasks refund total and active attempts through new ORM methods, while re-polls reduce active attempts. Readiness checks reject empty sets, with ORM and API tests covering the updated states. ChangesTask assignment and recovery
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🧹 Nitpick comments (2)
coordinator/internal/logic/provertask/chunk_prover_task.go (1)
272-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
ctx.Copy()for consistent context propagation.For consistency with
batch_prover_task.go,bundle_prover_task.go, and the rest of this file, consider usingctx.Copy()when passing the Gin context to the ORM methods.♻️ Proposed refactor
func (cp *ChunkProverTask) recoverAttempts(ctx *gin.Context, taskCtx *proverTaskContext, chunkTask *orm.Chunk) { if taskCtx.hasAssignedTask == nil { - if err := cp.chunkOrm.RefundAttemptsByHash(ctx, chunkTask.Hash); err != nil { + if err := cp.chunkOrm.RefundAttemptsByHash(ctx.Copy(), chunkTask.Hash); err != nil { log.Error("failed to refund chunk attempts", "hash", chunkTask.Hash, "error", err) } return } - if err := cp.chunkOrm.DecreaseActiveAttemptsByHash(ctx, chunkTask.Hash); err != nil { + if err := cp.chunkOrm.DecreaseActiveAttemptsByHash(ctx.Copy(), chunkTask.Hash); err != nil { log.Error("failed to recover chunk active attempts", "hash", chunkTask.Hash, "error", err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coordinator/internal/logic/provertask/chunk_prover_task.go` around lines 272 - 285, Update recoverAttempts to pass a copied Gin context to both RefundAttemptsByHash and DecreaseActiveAttemptsByHash, matching the context propagation pattern used elsewhere. Use ctx.Copy() at each ORM call while preserving the existing refund and decrement behavior.coordinator/internal/controller/api/get_task.go (1)
139-151: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReturn pre-sorted slice directly for default task types.
When
TaskTypesis empty, you can return the default task types in priority order directly to skip the sorting overhead.♻️ Proposed refactor
- if len(proofTypes) == 0 { - proofTypes = []message.ProofType{ - message.ProofTypeChunk, - message.ProofTypeBatch, - message.ProofTypeBundle, - } - } - - // Bundle (3) > Batch (2) > Chunk (1) - sort.Slice(proofTypes, func(i, j int) bool { - return proofTypes[i] > proofTypes[j] - }) - return proofTypes + if len(proofTypes) == 0 { + return []message.ProofType{ + message.ProofTypeBundle, + message.ProofTypeBatch, + message.ProofTypeChunk, + } + } + + // Bundle (3) > Batch (2) > Chunk (1) + sort.Slice(proofTypes, func(i, j int) bool { + return proofTypes[i] > proofTypes[j] + }) + return proofTypes🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coordinator/internal/controller/api/get_task.go` around lines 139 - 151, Update the proofTypes defaulting logic in the surrounding task-type helper to return the default ProofType slice directly in priority order when no task types are provided, bypassing sort.Slice for that path. Preserve sorting for explicitly supplied task types and keep the existing Bundle > Batch > Chunk ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coordinator/internal/orm/orm_test.go`:
- Around line 143-163: Extend the refund tests around the existing chunk case
and the corresponding batch and bundle cases to cover two active/total attempts
before refunding one, asserting the counters become 1/1 and the status remains
ProvingTaskAssigned. Reuse the existing ORM setup and refund APIs for each
resource, while preserving the current 1/1 → 0/0 unassignment coverage.
---
Nitpick comments:
In `@coordinator/internal/controller/api/get_task.go`:
- Around line 139-151: Update the proofTypes defaulting logic in the surrounding
task-type helper to return the default ProofType slice directly in priority
order when no task types are provided, bypassing sort.Slice for that path.
Preserve sorting for explicitly supplied task types and keep the existing Bundle
> Batch > Chunk ordering.
In `@coordinator/internal/logic/provertask/chunk_prover_task.go`:
- Around line 272-285: Update recoverAttempts to pass a copied Gin context to
both RefundAttemptsByHash and DecreaseActiveAttemptsByHash, matching the context
propagation pattern used elsewhere. Use ctx.Copy() at each ORM call while
preserving the existing refund and decrement behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6e6f610-d3d3-4a98-a3bb-a1cefdbf7dd6
📒 Files selected for processing (9)
coordinator/internal/controller/api/get_task.gocoordinator/internal/logic/provertask/batch_prover_task.gocoordinator/internal/logic/provertask/bundle_prover_task.gocoordinator/internal/logic/provertask/chunk_prover_task.gocoordinator/internal/orm/batch.gocoordinator/internal/orm/bundle.gocoordinator/internal/orm/chunk.gocoordinator/internal/orm/orm_test.gocoordinator/test/api_test.go
Comment on lines
+143
to
+163
| // (a) charge via UpdateChunkAttempts then refund -> both counters back to 0, status back to unassigned | ||
| insertChunk(1, "0xchunk-a", int16(types.ProvingTaskUnassigned), 0, 0) | ||
| rowsAffected, err := chunkOrm.UpdateChunkAttempts(context.Background(), 1, 0, 0) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, int64(1), rowsAffected) | ||
| active, total, err := chunkOrm.GetAttemptsByHash(context.Background(), "0xchunk-a") | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, int16(1), active) | ||
| assert.Equal(t, int16(1), total) | ||
| status, err := chunkOrm.GetProvingStatusByHash(context.Background(), "0xchunk-a") | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, types.ProvingTaskAssigned, status) | ||
|
|
||
| assert.NoError(t, chunkOrm.RefundAttemptsByHash(context.Background(), "0xchunk-a")) | ||
| active, total, err = chunkOrm.GetAttemptsByHash(context.Background(), "0xchunk-a") | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, int16(0), active) | ||
| assert.Equal(t, int16(0), total) | ||
| status, err = chunkOrm.GetProvingStatusByHash(context.Background(), "0xchunk-a") | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, types.ProvingTaskUnassigned, status) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Cover refunds with multiple active assignments.
Add a 2/2 → 1/1 case for chunk, batch, and bundle. The expected status must remain ProvingTaskAssigned.
Also applies to: 204-224, 270-288
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 143-143: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int16(types.ProvingTaskUnassigned)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@coordinator/internal/orm/orm_test.go` around lines 143 - 163, Extend the
refund tests around the existing chunk case and the corresponding batch and
bundle cases to cover two active/total attempts before refunding one, asserting
the counters become 1/1 and the status remains ProvingTaskAssigned. Reuse the
existing ORM setup and refund APIs for each resource, while preserving the
current 1/1 → 0/0 unassignment coverage.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1813 +/- ##
===========================================
+ Coverage 35.44% 35.95% +0.50%
===========================================
Files 262 262
Lines 22596 22703 +107
===========================================
+ Hits 8010 8162 +152
+ Misses 13748 13689 -59
- Partials 838 852 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out from #1803 (first of two PRs). Three independent coordinator fixes, one commit each:
1.
get_task: deterministic dispatch priority Bundle > Batch > ChunkPreviously
GetTasksrandomly shuffled the prover's requested task types and tried only the first one, rejecting the request with empty-proof-data when that type had no task available even if other types did. Now requested types are tried in priority order (Bundle > Batch > Chunk), so the pipeline finalizes the earliest ready bundle first instead of proving unrelated chunks/batches further ahead, and a type with no available task no longer starves the others.2. ORM: treat empty chunk/batch sets as not proofs-ready
CheckIfBatchChunkProofsAreReady/CheckIfBundleBatchProofsAreReadypreviously counted only unverified children, so a batch with zero chunks (or a bundle with zero batches) vacuously counted as proofs-ready and could dispatch an aggregate proving task with no children. Both now returnfalsewhen the child set is empty. (Also fixes the misleadingChunk.prefix in theBatchmethod's error message.)3. Refund burned task attempts on dispatch failure
When the coordinator failed to dispatch a freshly assigned task (
formatProverTask/applyUniversal/InsertProverTaskfailure), onlyactive_attemptswas decremented whiletotal_attemptsstayed burned. AfterSessionAttempts(5) such failures the task became invisible toGetUnassigned*queries, yet had noprover_taskrow for the cron timeout check to mark it failed — the task starved silently.RefundAttemptsByHashto the chunk/batch/bundle ORMs: decrements both counters and resetsproving_statusto unassigned, guarded against underflow and against touching verified rows.fixCompatibilityfailure path of batch/bundle tasks.test/api_test.go'sTestProofGeneratedFailedis updated to assert the new semantics: failed proof submission keepstotal_attempts(chunk: assigned, 1/0, recoverable via the prover_task row), while failed dispatch fully refunds (batch: unassigned, 0/0).Test plan
go build ./...,go vet, gofmt/goimports,go mod tidycleanmake lint(golangci-lint v1.57.2) passesgo test ./internal/orm/passes (incl. new refund tests, testcontainers)go test -tags mock_verifier ./test/passes (TestApis / TestProxyClient / TestProxyClientCompatibleMode)Review notes
RefundAttemptsByHashis keyed by hash only (not task-context scoped): if two provers race the same task and one's dispatch fails, the refund also releases the other's charge, resetting status to unassigned. Impact is limited to extra duplicate dispatches (which the design already allows) on a rare failure path.Assignerror (not empty result) from a higher-priority type now short-circuits the request instead of falling through to lower-priority types.Summary by CodeRabbit
Bug Fixes
Tests