fix: persist snapshot once for avoiding badger vlog#3153
Conversation
📝 WalkthroughWalkthroughMutations to the cache no longer persist snapshots on the hot path; snapshot persistence is now explicit via SaveToStore. Tests and a submitter test were updated to call SaveToStore where they previously relied on implicit persistence. CHANGELOG and a few workflow and module files were also adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Cache as Cache (in-memory)
participant Saver as SaveToStore
participant Store as Persistent Store
Cache->>Cache: mutate state (set/remove/delete)
Note right of Cache: No implicit persist
Cache->>Saver: SaveToStore() called explicitly (e.g., shutdown/test)
Saver->>Store: serialize snapshot & write
Store-->>Saver: ack write
Saver-->>Cache: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 1m 31s —— View job Code Review
SummaryThe change is well-targeted and correct. Removing the implicit Issues Found1. Stale docstring on
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3153 +/- ##
==========================================
- Coverage 60.23% 60.22% -0.02%
==========================================
Files 115 115
Lines 11899 11896 -3
==========================================
- Hits 7167 7164 -3
+ Misses 3922 3921 -1
- Partials 810 811 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 `@block/internal/cache/generic_cache.go`:
- Around line 202-203: SaveToStore (which calls persistSnapshot) is now the only
durable checkpoint but the shutdown path that invokes SaveToStore treats
failures as informational; change that shutdown caller to return (or propagate)
the error instead of just logging it, or implement a bounded retry/backoff loop
before giving up, so a failed SaveToStore is not silently ignored. Locate the
shutdown/stop routine that calls SaveToStore and replace the log-only handling
with proper error propagation (return the error) or a retry mechanism, ensuring
callers handle the returned error up the stack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d534481-293e-478a-8805-35e71d7c34a4
📒 Files selected for processing (4)
CHANGELOG.mdblock/internal/cache/generic_cache.goblock/internal/cache/generic_cache_test.goblock/internal/submitting/submitter_test.go
| // persistSnapshot writes all current in-flight [blockHeight, daHeight] pairs to the store under a single key. | ||
| // Only called explicitly via SaveToStore. NEVER CALL IT ON HOT-PATH TO AVOID BAGER WRITE AMPLIFICATION. |
There was a problem hiding this comment.
Don't leave the new shutdown flush as best-effort.
These lines make SaveToStore the only durable snapshot write, but the production shutdown path in block/components.go:120-135 still just logs that error and returns success. A failed flush now silently loses the only restart checkpoint. Please propagate the shutdown error (or retry it) instead of treating it as informational.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/cache/generic_cache.go` around lines 202 - 203, SaveToStore
(which calls persistSnapshot) is now the only durable checkpoint but the
shutdown path that invokes SaveToStore treats failures as informational; change
that shutdown caller to return (or propagate) the error instead of just logging
it, or implement a bounded retry/backoff loop before giving up, so a failed
SaveToStore is not silently ignored. Locate the shutdown/stop routine that calls
SaveToStore and replace the log-only handling with proper error propagation
(return the error) or a retry mechanism, ensuring callers handle the returned
error up the stack.
This reverts commit f74b456.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| - name: Log in to GHCR | ||
| uses: docker/login-action@v4 | ||
| uses: docker/login-action@v3 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/docker-build-push.yml (1)
36-41:⚠️ Potential issue | 🟠 MajorPin this GHCR login action to an immutable commit SHA and document the downgrade reason.
Line 37 uses a mutable tag
docker/login-action@v3in an authenticated publish job. Pin it to a commit SHA for supply chain security. Additionally, if the v4 → v3 downgrade is intentional, document the reason in a comment (e.g., self-hosted runners or GHES with Actions Runner < v2.327.1 cannot run v4's Node 24 runtime). Use commit SHAc94ce9fb468520275223c153574b00df6fe4bcc9for v3 orb45d80f862d83dbcd57f89517bcf500b2ab88fb2for v4.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-build-push.yml around lines 36 - 41, Replace the mutable action reference "docker/login-action@v3" in the "Log in to GHCR" step with the provided immutable commit SHA (e.g., use c94ce9fb468520275223c153574b00df6fe4bcc9 for v3 or b45d80f862d83dbcd57f89517bcf500b2ab88fb2 for v4) to pin the action for supply-chain security, and add a brief comment above that step explaining why v3 is used (if you intentionally downgraded from v4 — e.g., compatibility with self-hosted runners or GHES Actions Runner < v2.327.1) so the rationale is recorded.
🧹 Nitpick comments (1)
pkg/da/types/namespace_test.go (1)
39-42: Unreachablereturnaftert.Fatal.
t.Fatalalready terminates the test by callingruntime.Goexit(), making the subsequentreturnstatements unreachable. While harmless, this adds dead code.That said, some static analyzers don't recognize
t.Fatalas a terminating call, so addingreturncan silence nil-dereference warnings for the code that follows. If that's the intent here, consider adding a brief comment to clarify (e.g.,// appease static analysis).Also applies to: 121-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/da/types/namespace_test.go` around lines 39 - 42, The test contains unreachable return statements immediately after t.Fatal (e.g., in the nil check for ns where code calls t.Fatal("expected non-nil namespace but got nil") then `return`); remove the redundant `return` or, if the `return` was added to appease static analyzers, keep it but add a short clarifying comment like `// appease static analysis` next to the `return`; apply the same change to the other occurrence around the ns-related check later in the file (the block at lines similar to 121-124).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/evm/go.mod`:
- Around line 5-8: The apps/evm module contains local replace directives
("replace github.com/evstack/ev-node => ../../" and "replace
github.com/evstack/ev-node/execution/evm => ../../execution/evm") that break
standalone releases; remove these replace lines from the apps/evm go.mod and
instead add equivalent replace entries into a repo-root go.work workspace file
(using Go workspaces supported in Go 1.25.6) so local monorepo development works
but published modules remain usable via go get.
In `@apps/testapp/go.mod`:
- Line 5: The go.mod in apps/testapp contains a relative replace directive
("replace github.com/evstack/ev-node => ../../.") which must be removed because
apps/testapp is a standalone module; delete that replace line from the module's
go.mod and instead wire local module replacements via a workspace file (go.work)
at the monorepo root when developing locally so published/checked-out versions
of github.com/evstack/ev-node/apps/testapp do not depend on a relative path.
---
Outside diff comments:
In @.github/workflows/docker-build-push.yml:
- Around line 36-41: Replace the mutable action reference
"docker/login-action@v3" in the "Log in to GHCR" step with the provided
immutable commit SHA (e.g., use c94ce9fb468520275223c153574b00df6fe4bcc9 for v3
or b45d80f862d83dbcd57f89517bcf500b2ab88fb2 for v4) to pin the action for
supply-chain security, and add a brief comment above that step explaining why v3
is used (if you intentionally downgraded from v4 — e.g., compatibility with
self-hosted runners or GHES Actions Runner < v2.327.1) so the rationale is
recorded.
---
Nitpick comments:
In `@pkg/da/types/namespace_test.go`:
- Around line 39-42: The test contains unreachable return statements immediately
after t.Fatal (e.g., in the nil check for ns where code calls t.Fatal("expected
non-nil namespace but got nil") then `return`); remove the redundant `return`
or, if the `return` was added to appease static analyzers, keep it but add a
short clarifying comment like `// appease static analysis` next to the `return`;
apply the same change to the other occurrence around the ns-related check later
in the file (the block at lines similar to 121-124).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c976c110-dc88-455d-ab2c-185b8095ee7a
⛔ Files ignored due to path filters (2)
apps/evm/go.sumis excluded by!**/*.sumapps/testapp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.github/workflows/docker-build-push.yml.github/workflows/release-apps.ymlapps/evm/go.modapps/testapp/go.modnode/execution_test.gopkg/da/types/namespace_test.go
| replace ( | ||
| github.com/evstack/ev-node => ../../ | ||
| github.com/evstack/ev-node/execution/evm => ../../execution/evm | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
mod = Path("apps/evm/go.mod")
module_dir = mod.parent.resolve()
for raw in mod.read_text().splitlines():
line = raw.strip()
if "=>" not in line or line.startswith("//"):
continue
left, right = [part.strip() for part in line.split("=>", 1)]
if left.startswith("replace "):
target = right.split()[0]
elif left.startswith("github.com/"):
target = right.split()[0]
else:
continue
if target.startswith("."):
resolved = (module_dir / target).resolve()
escapes = module_dir not in (resolved, *resolved.parents)
print(f"{mod}: {target} -> {resolved}")
print(f"escapes module dir: {escapes}")
PYRepository: evstack/ev-node
Length of output: 227
🏁 Script executed:
# Check if go.work exists in repo root
ls -la go.work 2>/dev/null || echo "go.work not found"
# Check the actual module path in apps/evm/go.mod
head -5 apps/evm/go.mod
# Check Go version requirement
head -3 go.modRepository: evstack/ev-node
Length of output: 186
🏁 Script executed:
# Search for any references to go.work or workspace setup
rg "go.work" --type md --type txt --type sh .
# Check if there are any build/CI configurations that might show intended usage
fd -t f -e yml -e yaml -e Makefile | head -20
# Look for any documentation about building apps/evm standalone
rg "apps/evm" --type md . -A 3Repository: evstack/ev-node
Length of output: 1538
These local replace directives break standalone releases of apps/evm.
The RELEASE.md documents apps/evm as a standalone module released via go get github.com/evstack/ev-node/apps/evm@v0.3.0. When users obtain this module, the ../../ and ../../execution/evm paths will not exist, making the module unusable.
Move these replacements to a go.work file in the repository root to manage local monorepo development without shipping broken module files. Go 1.25.6 fully supports workspaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/evm/go.mod` around lines 5 - 8, The apps/evm module contains local
replace directives ("replace github.com/evstack/ev-node => ../../" and "replace
github.com/evstack/ev-node/execution/evm => ../../execution/evm") that break
standalone releases; remove these replace lines from the apps/evm go.mod and
instead add equivalent replace entries into a repo-root go.work workspace file
(using Go workspaces supported in Go 1.25.6) so local monorepo development works
but published modules remain usable via go get.
| go 1.25.6 | ||
|
|
||
| //replace github.com/evstack/ev-node => ../../. | ||
| replace github.com/evstack/ev-node => ../../. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
mod = Path("apps/testapp/go.mod")
module_dir = mod.parent.resolve()
for raw in mod.read_text().splitlines():
line = raw.strip()
if "=>" not in line or line.startswith("//"):
continue
left, right = [part.strip() for part in line.split("=>", 1)]
if left.startswith("replace "):
target = right.split()[0]
elif left.startswith("github.com/"):
target = right.split()[0]
else:
continue
if target.startswith("."):
resolved = (module_dir / target).resolve()
escapes = module_dir not in (resolved, *resolved.parents)
print(f"{mod}: {target} -> {resolved}")
print(f"escapes module dir: {escapes}")
PYRepository: evstack/ev-node
Length of output: 134
Do not commit a relative replace in this nested module.
apps/testapp is its own module. A versioned checkout of github.com/evstack/ev-node/apps/testapp will not contain the target at ../../., breaking standalone builds of this module outside a full monorepo checkout. Use go.work for local wiring instead of shipping relative replace directives in published go.mod files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/testapp/go.mod` at line 5, The go.mod in apps/testapp contains a
relative replace directive ("replace github.com/evstack/ev-node => ../../.")
which must be removed because apps/testapp is a standalone module; delete that
replace line from the module's go.mod and instead wire local module replacements
via a workspace file (go.work) at the monorepo root when developing locally so
published/checked-out versions of github.com/evstack/ev-node/apps/testapp do not
depend on a relative path.
2 participants
Overview
Badger was seeing an increasing amount of big vlog entry due to this optimistic save (small win for crash recovery).
This avoids badger to go crazy.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Documentation