Conversation
📝 WalkthroughWalkthroughThe PR adds store synchronization capabilities to the public interface, introduces publisher-mode sync initialization for raft leaders, refactors syncer lifecycle management with mutex-based state tracking, enhances EVM execution recovery against stale inputs, implements raft-aware failover sync branching, and improves E2E testing infrastructure. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 @alpe's task in 2m 56s —— View job Code Review: fix: startup races
SummaryThis PR fixes a real race in the HA startup path: when a fresh raft leader has an empty store, it was blocking in A few issues worth addressing are noted below. Issues1.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3172 +/- ##
==========================================
+ Coverage 60.75% 60.78% +0.02%
==========================================
Files 117 117
Lines 11988 12068 +80
==========================================
+ Hits 7283 7335 +52
- Misses 3886 3906 +20
- Partials 819 827 +8
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/sut_helper.go (1)
189-195:⚠️ Potential issue | 🟡 MinorClose the per-process logfile.
os.Createopens a descriptor that is never closed here. The E2E restart tests can spawn a lot of processes, so this leaks one fd per process and can eventually tripEMFILEor delay log flushing. Please close the file once both reader goroutines finish.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/sut_helper.go` around lines 189 - 195, The per-process logfile returned by os.Create (logfile) is never closed; modify the block that sets errReader and outReader to ensure logfile is closed after the readers finish by adding a sync.WaitGroup and wrapping the reader goroutines so they call wg.Done() when done, then spawn a goroutine that waits on wg.Wait() and calls logfile.Close(); update the code that consumes errReader/outReader (or the code that launches the reader goroutines) to increment the WaitGroup for each reader and to close each reader when finished so the logfile.Close() runs only after both readers have completed (reference symbols: processLogDir(), logfile, errReader, outReader).
🧹 Nitpick comments (1)
execution/evm/execution_reconcile_test.go (1)
122-130: Track Eth RPC mock call counts to harden path guarantees.The Eth mock currently cannot assert whether
HeaderByNumber/GetTxswere invoked. Adding counters would make it easier to lock down expected short-circuit behavior inresume_when_inputs_match.🔍 Suggested enhancement
type mockReconcileEthRPCClient struct{} +type mockReconcileEthRPCClient struct { + headerByNumberCalls int + getTxsCalls int +} -func (mockReconcileEthRPCClient) HeaderByNumber(_ context.Context, _ *big.Int) (*types.Header, error) { +func (m *mockReconcileEthRPCClient) HeaderByNumber(_ context.Context, _ *big.Int) (*types.Header, error) { + m.headerByNumberCalls++ return nil, errors.New("header not found") } -func (mockReconcileEthRPCClient) GetTxs(_ context.Context) ([]string, error) { +func (m *mockReconcileEthRPCClient) GetTxs(_ context.Context) ([]string, error) { + m.getTxsCalls++ return nil, errors.New("unexpected GetTxs call") }- client := &EngineClient{ + ethRPC := &mockReconcileEthRPCClient{} + client := &EngineClient{ engineClient: engineRPC, - ethClient: mockReconcileEthRPCClient{}, + ethClient: ethRPC, store: store, logger: zerolog.Nop(), }+ if spec.expectPayloadID { + require.Equal(t, 0, ethRPC.headerByNumberCalls) + require.Equal(t, 0, ethRPC.getTxsCalls) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/evm/execution_reconcile_test.go` around lines 122 - 130, The mockReconcileEthRPCClient currently just returns errors and cannot assert whether HeaderByNumber or GetTxs were invoked; modify the mockReconcileEthRPCClient to include call-count fields (e.g., headerByNumberCalls, getTxsCalls) and increment them atomically (sync/atomic or a mutex) inside HeaderByNumber and GetTxs, expose read accessors or exported fields so tests can assert counts, then update the resume_when_inputs_match test to assert expected call counts (e.g., HeaderByNumber was called 0/1 and GetTxs 0/1 as appropriate) to harden short-circuit guarantees.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/evm/execution_reconcile_test.go`:
- Around line 59-61: The test uses a map-backed table `specs` and iterates with
`for name, spec := range specs` which yields non-deterministic subtest order;
refactor `specs` into a slice-backed table (e.g. `[]struct { name string; spec
<type> }`) and iterate `for _, tc := range specsSlice { t.Run(tc.name, func(t
*testing.T) { t.Parallel(); spec := tc.spec; ... }) }` so subtest order is
deterministic and the loop-local `spec` is captured correctly; update any
references to `specs` and the `t.Run` closure accordingly.
In `@execution/evm/execution.go`:
- Around line 759-760: The resume fingerprint is ambiguous because hashTxs()
currently hashes raw concatenation of tx bytes; change hashTxs to compute a
length-delimited digest (prefix each transaction with its length using a fixed
or varint encoding, then hash the sequence) so different segmentations/orderings
cannot collide, and update the comparisons that use hashTxs (where
execMeta.TxHash is compared to requestedTxHash alongside
execMeta.Timestamp/timestamp.Unix()) to use the new length-delimited hash
function; also apply the same replacement to the other places that call hashTxs
(the later equality check block referenced around the second usage) so all
resume checks use the unambiguous, length-prefixed hash.
In `@pkg/store/store_test.go`:
- Around line 38-52: The test currently uses two booleans
(syncCalled/closeCalled) in syncingBatchingDatastore which can't detect order;
change the stub to record the call sequence (e.g., a slice of strings or ints)
and have Sync(ctx, key) append "Sync" and Close() append "Close", then update
the test assertions to assert the recorded sequence equals the expected order
(Sync before Close). Update both occurrences that define/instantiate
syncingBatchingDatastore (the methods Sync and Close in
syncingBatchingDatastore) and the corresponding assertions so they check the
exact sequence rather than just both flags being true.
In `@pkg/store/store.go`:
- Around line 34-47: The current close path spawns a goroutine that discards the
result of s.Sync and can return nil after 4s even if Sync or s.db.Close failed
or is still running; change the goroutine to capture both the Sync error and
Close error (e.g., syncErr := s.Sync(syncCtx); closeErr := s.db.Close()), send a
combined/non-nil error on done (prefer returning syncErr if non-nil, else
closeErr), and update the select timeout branch to return a non-nil error (e.g.,
context.DeadlineExceeded or a wrapped error indicating close timed out) instead
of nil so callers cannot reopen while the old store may still be closing. Ensure
you reference the goroutine closure where s.Sync, s.db.Close and the done
channel are used.
In `@pkg/sync/syncer_status_test.go`:
- Around line 22-47: The test spawns goroutines that call status.startOnce and
currently use require.NoError inside each goroutine which can call FailNow only
in that goroutine and cause the main test to hang; instead, create an errors
channel (buffered to the number of workers), have each goroutine send its error
into that channel after calling status.startOnce (referencing the startOnce
call, calls counter, started and release channels, and wg), close the channel
after wg.Wait(), then range over the collected errors in the main goroutine and
use require.NoError (or require.Empty) to assert no worker returned an error;
keep the final checks for calls.Load() == 1 and status.isStarted() after error
checks.
In `@test/e2e/failover_e2e_test.go`:
- Around line 179-180: The restart call is using the global env.SequencerJWT
which can mismatch when oldLeader is node2/node3; replace env.SequencerJWT with
the restarted node's own JWT secret (the per-node JWT selection used in
TestHASequencerRollingRestartE2E) when calling setupRaftSequencerNode for
oldLeader so engine auth matches that node's reth instance (i.e., obtain the JWT
for oldLeader via the same per-node selection you added earlier and pass it
instead of env.SequencerJWT).
---
Outside diff comments:
In `@test/e2e/sut_helper.go`:
- Around line 189-195: The per-process logfile returned by os.Create (logfile)
is never closed; modify the block that sets errReader and outReader to ensure
logfile is closed after the readers finish by adding a sync.WaitGroup and
wrapping the reader goroutines so they call wg.Done() when done, then spawn a
goroutine that waits on wg.Wait() and calls logfile.Close(); update the code
that consumes errReader/outReader (or the code that launches the reader
goroutines) to increment the WaitGroup for each reader and to close each reader
when finished so the logfile.Close() runs only after both readers have completed
(reference symbols: processLogDir(), logfile, errReader, outReader).
---
Nitpick comments:
In `@execution/evm/execution_reconcile_test.go`:
- Around line 122-130: The mockReconcileEthRPCClient currently just returns
errors and cannot assert whether HeaderByNumber or GetTxs were invoked; modify
the mockReconcileEthRPCClient to include call-count fields (e.g.,
headerByNumberCalls, getTxsCalls) and increment them atomically (sync/atomic or
a mutex) inside HeaderByNumber and GetTxs, expose read accessors or exported
fields so tests can assert counts, then update the resume_when_inputs_match test
to assert expected call counts (e.g., HeaderByNumber was called 0/1 and GetTxs
0/1 as appropriate) to harden short-circuit guarantees.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c7b7655-2bb3-4eaf-a6d5-ecbe513092f3
📒 Files selected for processing (17)
.github/workflows/test.ymlexecution/evm/execution.goexecution/evm/execution_reconcile_test.gonode/failover.gopkg/store/cached_store.gopkg/store/store.gopkg/store/store_test.gopkg/store/tracing.gopkg/store/tracing_test.gopkg/store/types.gopkg/sync/sync_service.gopkg/sync/sync_service_test.gopkg/sync/syncer_status.gopkg/sync/syncer_status_test.gotest/e2e/failover_e2e_test.gotest/e2e/sut_helper.gotest/mocks/store.go
| for name, spec := range specs { | ||
| t.Run(name, func(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current nondeterministic table pattern in this test file.
rg -n -C2 'specs := map\[string\]struct|for name, spec := range specs|t\.Run\(' execution/evm/execution_reconcile_test.goRepository: evstack/ev-node
Length of output: 283
Use a slice-backed table to keep subtest execution order deterministic.
Map iteration in Go is randomized, so line 59's for name, spec := range specs produces non-deterministic subtest ordering. Refactor to use a []struct{...} table with a name field to satisfy the test determinism requirement.
Suggested refactor
- specs := map[string]struct {
+ specs := []struct {
+ name string
execMetaTimestamp int64
execMetaTxs [][]byte
requestedTxs [][]byte
requestedTime time.Time
expectFound bool
expectPayloadID bool
expectGetPayloads int
- }{
- "resume_when_inputs_match": {
+ }{
+ {
+ name: "resume_when_inputs_match",
execMetaTimestamp: 1700000012,
execMetaTxs: [][]byte{[]byte("tx-1")},
requestedTxs: [][]byte{[]byte("tx-1")},
requestedTime: time.Unix(1700000012, 0),
expectFound: true,
expectPayloadID: true,
expectGetPayloads: 1,
},
- "ignore_when_timestamp_differs": {
+ {
+ name: "ignore_when_timestamp_differs",
execMetaTimestamp: 1700000010,
execMetaTxs: [][]byte{[]byte("tx-1")},
requestedTxs: [][]byte{[]byte("tx-1")},
requestedTime: time.Unix(1700000012, 0),
expectFound: false,
expectPayloadID: false,
expectGetPayloads: 0,
},
- "ignore_when_txs_differ": {
+ {
+ name: "ignore_when_txs_differ",
execMetaTimestamp: 1700000012,
execMetaTxs: [][]byte{[]byte("tx-old")},
requestedTxs: [][]byte{[]byte("tx-new")},
requestedTime: time.Unix(1700000012, 0),
expectFound: false,
expectPayloadID: false,
expectGetPayloads: 0,
},
}
- for name, spec := range specs {
- t.Run(name, func(t *testing.T) {
+ for _, spec := range specs {
+ t.Run(spec.name, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@execution/evm/execution_reconcile_test.go` around lines 59 - 61, The test
uses a map-backed table `specs` and iterates with `for name, spec := range
specs` which yields non-deterministic subtest order; refactor `specs` into a
slice-backed table (e.g. `[]struct { name string; spec <type> }`) and iterate
`for _, tc := range specsSlice { t.Run(tc.name, func(t *testing.T) {
t.Parallel(); spec := tc.spec; ... }) }` so subtest order is deterministic and
the loop-local `spec` is captured correctly; update any references to `specs`
and the `t.Run` closure accordingly.
| requestedTxHash := hashTxs(txs) | ||
| if execMeta.Timestamp != timestamp.Unix() || !bytes.Equal(execMeta.TxHash, requestedTxHash) { |
There was a problem hiding this comment.
Make the resume fingerprint unambiguous.
hashTxs() hashes the raw byte concatenation, so different ordered transaction slices can share the same digest ([ab,c] vs [a,bc]). The equality check on Line 760 can then resume a payload built from different inputs. Hash a length-delimited encoding instead.
Suggested fix
+import "encoding/binary"
...
func hashTxs(txs [][]byte) []byte {
if len(txs) == 0 {
return nil
}
h := sha256.New()
+ var lenBuf [8]byte
for _, tx := range txs {
- h.Write(tx)
+ binary.LittleEndian.PutUint64(lenBuf[:], uint64(len(tx)))
+ _, _ = h.Write(lenBuf[:])
+ _, _ = h.Write(tx)
}
return h.Sum(nil)
}Also applies to: 1048-1058
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@execution/evm/execution.go` around lines 759 - 760, The resume fingerprint is
ambiguous because hashTxs() currently hashes raw concatenation of tx bytes;
change hashTxs to compute a length-delimited digest (prefix each transaction
with its length using a fixed or varint encoding, then hash the sequence) so
different segmentations/orderings cannot collide, and update the comparisons
that use hashTxs (where execMeta.TxHash is compared to requestedTxHash alongside
execMeta.Timestamp/timestamp.Unix()) to use the new length-delimited hash
function; also apply the same replacement to the other places that call hashTxs
(the later equality check block referenced around the second usage) so all
resume checks use the unambiguous, length-prefixed hash.
| type syncingBatchingDatastore struct { | ||
| ds.Batching | ||
| syncCalled bool | ||
| closeCalled bool | ||
| } | ||
|
|
||
| func (m *syncingBatchingDatastore) Sync(ctx context.Context, key ds.Key) error { | ||
| m.syncCalled = true | ||
| return m.Batching.Sync(ctx, key) | ||
| } | ||
|
|
||
| func (m *syncingBatchingDatastore) Close() error { | ||
| m.closeCalled = true | ||
| return m.Batching.Close() | ||
| } |
There was a problem hiding this comment.
This test doesn't verify the Sync()-before-Close() contract.
Both booleans become true even if the implementation calls Close() first, so the regression this test is named after would still pass. Record the call sequence and assert the exact order.
Suggested fix
+import "sync"
...
type syncingBatchingDatastore struct {
ds.Batching
- syncCalled bool
- closeCalled bool
+ mu sync.Mutex
+ calls []string
}
func (m *syncingBatchingDatastore) Sync(ctx context.Context, key ds.Key) error {
- m.syncCalled = true
+ m.mu.Lock()
+ m.calls = append(m.calls, "sync")
+ m.mu.Unlock()
return m.Batching.Sync(ctx, key)
}
func (m *syncingBatchingDatastore) Close() error {
- m.closeCalled = true
+ m.mu.Lock()
+ m.calls = append(m.calls, "close")
+ m.mu.Unlock()
return m.Batching.Close()
}
...
require.NoError(t, s.Close())
- require.True(t, mock.syncCalled)
- require.True(t, mock.closeCalled)
+ mock.mu.Lock()
+ defer mock.mu.Unlock()
+ require.Equal(t, []string{"sync", "close"}, mock.calls)
}Also applies to: 160-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/store/store_test.go` around lines 38 - 52, The test currently uses two
booleans (syncCalled/closeCalled) in syncingBatchingDatastore which can't detect
order; change the stub to record the call sequence (e.g., a slice of strings or
ints) and have Sync(ctx, key) append "Sync" and Close() append "Close", then
update the test assertions to assert the recorded sequence equals the expected
order (Sync before Close). Update both occurrences that define/instantiate
syncingBatchingDatastore (the methods Sync and Close in
syncingBatchingDatastore) and the corresponding assertions so they check the
exact sequence rather than just both flags being true.
| done := make(chan error, 1) | ||
| go func() { | ||
| syncCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
| defer cancel() | ||
|
|
||
| _ = s.Sync(syncCtx) | ||
| done <- s.db.Close() | ||
| }() | ||
|
|
||
| select { | ||
| case err := <-done: | ||
| return err | ||
| case <-time.After(4 * time.Second): | ||
| return nil |
There was a problem hiding this comment.
Don't turn an incomplete close into a successful one.
This path drops Sync() failures and returns nil after 4 seconds even if the goroutine is still flushing or closing s.db. That lets callers reopen the same store while the old handle is still live and hides the durability failure this change is supposed to prevent.
Suggested fix
func (s *DefaultStore) Close() error {
done := make(chan error, 1)
go func() {
syncCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
defer cancel()
- _ = s.Sync(syncCtx)
- done <- s.db.Close()
+ syncErr := s.Sync(syncCtx)
+ closeErr := s.db.Close()
+
+ switch {
+ case syncErr != nil && closeErr != nil:
+ done <- errors.Join(
+ fmt.Errorf("sync store before close: %w", syncErr),
+ fmt.Errorf("close datastore: %w", closeErr),
+ )
+ case syncErr != nil:
+ done <- fmt.Errorf("sync store before close: %w", syncErr)
+ default:
+ done <- closeErr
+ }
}()
select {
case err := <-done:
return err
case <-time.After(4 * time.Second):
- return nil
+ return fmt.Errorf("closing datastore timed out after 4s")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| done := make(chan error, 1) | |
| go func() { | |
| syncCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | |
| defer cancel() | |
| _ = s.Sync(syncCtx) | |
| done <- s.db.Close() | |
| }() | |
| select { | |
| case err := <-done: | |
| return err | |
| case <-time.After(4 * time.Second): | |
| return nil | |
| done := make(chan error, 1) | |
| go func() { | |
| syncCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | |
| defer cancel() | |
| syncErr := s.Sync(syncCtx) | |
| closeErr := s.db.Close() | |
| switch { | |
| case syncErr != nil && closeErr != nil: | |
| done <- errors.Join( | |
| fmt.Errorf("sync store before close: %w", syncErr), | |
| fmt.Errorf("close datastore: %w", closeErr), | |
| ) | |
| case syncErr != nil: | |
| done <- fmt.Errorf("sync store before close: %w", syncErr) | |
| default: | |
| done <- closeErr | |
| } | |
| }() | |
| select { | |
| case err := <-done: | |
| return err | |
| case <-time.After(4 * time.Second): | |
| return fmt.Errorf("closing datastore timed out after 4s") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/store/store.go` around lines 34 - 47, The current close path spawns a
goroutine that discards the result of s.Sync and can return nil after 4s even if
Sync or s.db.Close failed or is still running; change the goroutine to capture
both the Sync error and Close error (e.g., syncErr := s.Sync(syncCtx); closeErr
:= s.db.Close()), send a combined/non-nil error on done (prefer returning
syncErr if non-nil, else closeErr), and update the select timeout branch to
return a non-nil error (e.g., context.DeadlineExceeded or a wrapped error
indicating close timed out) instead of nil so callers cannot reopen while the
old store may still be closing. Ensure you reference the goroutine closure where
s.Sync, s.db.Close and the done channel are used.
| var calls atomic.Int32 | ||
| started := make(chan struct{}) | ||
| release := make(chan struct{}) | ||
| var wg sync.WaitGroup | ||
|
|
||
| for range 8 { | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| _, err := status.startOnce(func() error { | ||
| if calls.Add(1) == 1 { | ||
| close(started) | ||
| } | ||
| <-release | ||
| return nil | ||
| }) | ||
| require.NoError(t, err) | ||
| }() | ||
| } | ||
|
|
||
| <-started | ||
| close(release) | ||
| wg.Wait() | ||
|
|
||
| require.Equal(t, int32(1), calls.Load()) | ||
| require.True(t, status.isStarted()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file to confirm the code and understand the context
find . -name "syncer_status_test.go" -type fRepository: evstack/ev-node
Length of output: 91
🏁 Script executed:
# Read the test file to see the actual code
cat -n pkg/sync/syncer_status_test.go | head -100Repository: evstack/ev-node
Length of output: 2676
🏁 Script executed:
# Let's also check if there's any custom error collection mechanism or if require is really being used in goroutines
rg "require\.(NoError|Error)" pkg/sync/syncer_status_test.go -B 5 -A 2Repository: evstack/ev-node
Length of output: 729
Use a channel to collect worker errors instead of calling require inside goroutines.
require.NoError (line 38) calls FailNow(), which exits only the spawned goroutine. If an error occurs before the first goroutine closes started, the main test hangs indefinitely on line 42. Collect errors in a channel from each worker and assert after wg.Wait() completes to ensure the test is deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sync/syncer_status_test.go` around lines 22 - 47, The test spawns
goroutines that call status.startOnce and currently use require.NoError inside
each goroutine which can call FailNow only in that goroutine and cause the main
test to hang; instead, create an errors channel (buffered to the number of
workers), have each goroutine send its error into that channel after calling
status.startOnce (referencing the startOnce call, calls counter, started and
release channels, and wg), close the channel after wg.Wait(), then range over
the collected errors in the main goroutine and use require.NoError (or
require.Empty) to assert no worker returned an error; keep the final checks for
calls.Load() == 1 and status.isStarted() after error checks.
| oldDetails := clusterNodes.Details(oldLeader) | ||
| restartedNodeProcess := setupRaftSequencerNode(t, sut, workDir, oldLeader, oldDetails.raftAddr, env.SequencerJWT, env.GenesisHash, env.Endpoints.GetDAAddress(), "", raftCluster, clusterNodes.Details(newLeader).p2pAddr, oldDetails.rpcAddr, oldDetails.p2pAddr, oldDetails.engineURL, oldDetails.ethAddr, false, passphraseFile) | ||
| restartedNodeProcess := setupRaftSequencerNode(t, sut, workDir, oldLeader, oldDetails.raftAddr, env.SequencerJWT, env.GenesisHash, env.Endpoints.GetDAAddress(), "", raftCluster, clusterNodes.Details(newLeader).p2pPeerAddr, oldDetails.rpcAddr, oldDetails.p2pAddr, oldDetails.engineURL, oldDetails.ethAddr, false, passphraseFile) |
There was a problem hiding this comment.
Use the restarted node's own JWT secret here.
oldLeader is elected dynamically, so this branch can restart node2 or node3 with env.SequencerJWT. When that happens the engine auth no longer matches that node's reth instance, and the test either flakes or drops into the later "did not recover on restart" path. Reuse the per-node JWT selection you already added in TestHASequencerRollingRestartE2E.
Suggested fix
oldDetails := clusterNodes.Details(oldLeader)
+ oldLeaderJWT := map[string]string{
+ "node1": env.SequencerJWT,
+ "node2": env.FullNodeJWT,
+ "node3": jwtSecret3,
+ }[oldLeader]
restartedNodeProcess := setupRaftSequencerNode(t, sut, workDir, oldLeader, oldDetails.raftAddr,
- env.SequencerJWT, env.GenesisHash, env.Endpoints.GetDAAddress(), "", raftCluster,
+ oldLeaderJWT, env.GenesisHash, env.Endpoints.GetDAAddress(), "", raftCluster,
clusterNodes.Details(newLeader).p2pPeerAddr, oldDetails.rpcAddr, oldDetails.p2pAddr,
oldDetails.engineURL, oldDetails.ethAddr, false, passphraseFile)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oldDetails := clusterNodes.Details(oldLeader) | |
| restartedNodeProcess := setupRaftSequencerNode(t, sut, workDir, oldLeader, oldDetails.raftAddr, env.SequencerJWT, env.GenesisHash, env.Endpoints.GetDAAddress(), "", raftCluster, clusterNodes.Details(newLeader).p2pAddr, oldDetails.rpcAddr, oldDetails.p2pAddr, oldDetails.engineURL, oldDetails.ethAddr, false, passphraseFile) | |
| restartedNodeProcess := setupRaftSequencerNode(t, sut, workDir, oldLeader, oldDetails.raftAddr, env.SequencerJWT, env.GenesisHash, env.Endpoints.GetDAAddress(), "", raftCluster, clusterNodes.Details(newLeader).p2pPeerAddr, oldDetails.rpcAddr, oldDetails.p2pAddr, oldDetails.engineURL, oldDetails.ethAddr, false, passphraseFile) | |
| oldDetails := clusterNodes.Details(oldLeader) | |
| oldLeaderJWT := map[string]string{ | |
| "node1": env.SequencerJWT, | |
| "node2": env.FullNodeJWT, | |
| "node3": jwtSecret3, | |
| }[oldLeader] | |
| restartedNodeProcess := setupRaftSequencerNode(t, sut, workDir, oldLeader, oldDetails.raftAddr, oldLeaderJWT, env.GenesisHash, env.Endpoints.GetDAAddress(), "", raftCluster, clusterNodes.Details(newLeader).p2pPeerAddr, oldDetails.rpcAddr, oldDetails.p2pAddr, oldDetails.engineURL, oldDetails.ethAddr, false, passphraseFile) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/failover_e2e_test.go` around lines 179 - 180, The restart call is
using the global env.SequencerJWT which can mismatch when oldLeader is
node2/node3; replace env.SequencerJWT with the restarted node's own JWT secret
(the per-node JWT selection used in TestHASequencerRollingRestartE2E) when
calling setupRaftSequencerNode for oldLeader so engine auth matches that node's
reth instance (i.e., obtain the JWT for oldLeader via the same per-node
selection you added earlier and pass it instead of env.SequencerJWT).
1 participant
Overview
E2E HA tests fail sometimes on a race when the leader is waiting for p2p sync complete on a fresh start.
Sync store before closing DB.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests