Skip to content

fix(coordinator): log update errors in proof readiness checks - #1819

Open
myetcd wants to merge 1 commit into
scroll-tech:developfrom
myetcd:fix/coordinator-log-update-errors
Open

myetcd wants to merge 1 commit into
scroll-tech:developfrom
myetcd:fix/coordinator-log-update-errors

Conversation

@myetcd

@myetcd myetcd commented Sep 30, 2026 •

Copy link
Copy Markdown

Purpose or design rationale of this PR

Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?

What does this PR do? Fixes two log.Warn calls in
coordinator/internal/controller/cron/collect_proof.go that print the wrong error variable.

Why does it do it? In both places the branch is entered because the database update failed,
but the value logged is checkErr — the error of the readiness check that ran earlier:

allReady, checkErr := c.chunkOrm.CheckIfBatchChunkProofsAreReady(c.ctx, batch.Hash)
if checkErr != nil {
    log.Warn("checkBatchAllChunkReady CheckIfBatchChunkProofsAreReady failure", "error", checkErr, "hash", batch.Hash)
    continue // checkErr != nil never reaches the code below
}
if !allReady {
    continue
}

if updateErr := c.batchOrm.UpdateChunkProofsStatusByBatchHash(c.ctx, batch.Hash, types.ChunkProofsStatusReady); updateErr != nil {
    log.Warn("checkBatchAllChunkReady UpdateChunkProofsStatusByBatchHash failure", "error", checkErr, "hash", batch.Hash)
}

The continue above guarantees checkErr == nil by the time the update runs, so the warning that
is supposed to describe a failed DB write logs error=nil and the real updateErr is dropped.
That is exactly the line an operator needs when chunks/batches never get marked proofs-ready — the
warning fires, but it carries no information about why the write failed. The same copy-paste exists
in checkBundleAllBatchReady (UpdateBatchProofsStatusByBundleHash).

How does it do it? Log updateErr instead of checkErr in both call sites:

-						log.Warn("checkBatchAllChunkReady UpdateChunkProofsStatusByBatchHash failure", "error", checkErr, "hash", batch.Hash)
+						log.Warn("checkBatchAllChunkReady UpdateChunkProofsStatusByBatchHash failure", "error", updateErr, "hash", batch.Hash)
...
-						log.Warn("checkBundleAllBatchReady UpdateBatchProofsStatusByBundleHash failure", "error", checkErr, "hash", bundle.Hash)
+						log.Warn("checkBundleAllBatchReady UpdateBatchProofsStatusByBundleHash failure", "error", updateErr, "hash", bundle.Hash)

No control flow, signature or success-path behaviour changes; checkErr is still used by the
preceding readiness-check warnings, so nothing becomes unused. These are the only two sites of this
kind in the repository — every other updateErr != nil log in coordinator/, rollup/ and
bridge-history-api/ already logs updateErr.

Both lines date back to the introduction of these functions (checkBatchAllChunkReady in #862,
checkBundleAllBatchReady in #1394), where the new update branch was written by copying the
preceding checkErr branch.

Validation

  • gofmt -l coordinator/internal/controller/cron/collect_proof.go → no output.
  • go build ./internal/controller/cron/ (in coordinator/) → OK (only the pre-existing
    github.com/rjeczalik/notify cgo deprecation warnings from the macOS SDK).
  • go vet ./internal/controller/cron/ → no findings.
  • Reproduced the log output against the real logger (github.com/scroll-tech/go-ethereum/log,
    version as pinned in coordinator/go.mod), driving the exact control flow with a failing update:
    • before: WARN ... UpdateChunkProofsStatusByBatchHash failure error=nil hash=0xabc
    • after: WARN ... UpdateChunkProofsStatusByBatchHash failure error="db: connection refused" hash=0xabc
  • No unit test is added: the change only alters a log argument, and both call sites live in a
    Collector that needs a live PostgreSQL/ORM to drive. If a test is wanted here, the cleanest
    option is an ORM-level one that asserts the returned error — happy to add it if you prefer.

PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

Deployment tag versioning

Has tag in common/version.go been updated or have you added bump-version label to this PR?

  • No, this PR doesn't involve a new deployment, git tag, docker image tag
  • Yes

Breaking change label

Does this PR have the breaking-change label?

  • No, this PR is not a breaking change
  • Yes

Summary by CodeRabbit

  • Bug Fixes
    • Error logs now report the failure encountered while updating proof status for a batch or bundle.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 2e50e59f-54da-4e49-9018-51abce85aab0

📥 Commits

Reviewing files that changed from the base of the PR and between bced8c2 and 7767f4f.

📒 Files selected for processing (1)
  • coordinator/internal/controller/cron/collect_proof.go

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Warnings for failed chunk-proof and batch-proof status updates now report the error returned by the update operation.

Changes

Proof status logging

Layer / File(s) Summary
Report status-update errors
coordinator/internal/controller/cron/collect_proof.go
The warnings for failed chunk-proof and batch-proof status updates now report updateErr instead of checkErr.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 7767f

Both warnings now identify the failed status update. No material merge risk is evident in this narrowly scoped logging change.

Architecture Summary

Architecture risk: 🔵 Low · up to 7767f

The change affects 1 system.

Changed systems: coordinator

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — coordinator (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in coordinator/internal/controller/cron/collect_proof.go: When updating a ready batch’s chunk-proof status fails, the warning now reports updateErr instead of checkErr.
  • observed — Modified behavior in coordinator/internal/controller/cron/collect_proof.go: When updating a ready bundle’s batch-proof status fails, the warning now reports updateErr instead of checkErr.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main bug fix and follows the required Conventional Commits format with the fix type and coordinator scope.
Description check ✅ Passed The description explains what changed, why the change was needed, and how it was implemented. It also covers title conventions, deployment tag versioning, breaking changes, and validation results.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant