fix(syncing): make Syncer shutdown idempotent after start failure - #3418
fix(syncing): make Syncer shutdown idempotent after start failure#3418racequite wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesSyncer lifecycle control
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Syncer
participant ForcedInclusionRetriever
participant DAFollower
participant HeightChannel
participant RaftCallback
Syncer->>Syncer: Capture and clear cancel function
Syncer->>Syncer: Mark lifecycle stopped
Syncer->>ForcedInclusionRetriever: Stop once
Syncer->>DAFollower: Stop once
Syncer->>HeightChannel: Close
Syncer->>RaftCallback: Clear callback
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 (1)
block/internal/syncing/syncer_test.go (1)
1115-1156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the no-restart contract.
The test verifies repeated
Stopcalls and one-time cleanup. It does not verify that a stoppedSyncerrejects a laterStart.As per PR objectives, preserve the behavior that a stopped
Syncercannot be restarted.Suggested assertion
require.NoError(t, s.Stop(t.Context())) require.NoError(t, s.Stop(t.Context())) + require.ErrorContains(t, s.Start(t.Context()), "syncer cannot be restarted after stopping")🤖 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 `@block/internal/syncing/syncer_test.go` around lines 1115 - 1156, Extend TestSyncer_Stop_IsIdempotentAfterStartFailure to call Start again after the syncer has reached syncerLifecycleStopped and assert that it returns the expected error. Preserve the existing repeated-Stop and one-time cleanup assertions, and verify the restart attempt does not alter the stopped state or cleanup counts.
🤖 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 `@block/internal/syncing/syncer.go`:
- Around line 178-190: Serialize Start and Stop through the syncer lifecycle
gate: add explicit starting and stopping states (or an equivalent completion
signal), keep initialization and worker registration under lifecycle protection,
and publish started only after initialization completes. Prevent Start once
stopping or stopped, make concurrent Stop callers wait for the first cleanup
instead of returning early, and protect fiRetriever and daFollower accesses with
lifecycleMu; add a concurrent lifecycle test covering shutdown races and run it
with -race.
---
Nitpick comments:
In `@block/internal/syncing/syncer_test.go`:
- Around line 1115-1156: Extend TestSyncer_Stop_IsIdempotentAfterStartFailure to
call Start again after the syncer has reached syncerLifecycleStopped and assert
that it returns the expected error. Preserve the existing repeated-Stop and
one-time cleanup assertions, and verify the restart attempt does not alter the
stopped state or cleanup counts.
🪄 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 Plus
Run ID: f9469652-1921-4da7-bfba-2e9ca33b0fc1
📒 Files selected for processing (3)
block/internal/syncing/syncer.goblock/internal/syncing/syncer_benchmark_test.goblock/internal/syncing/syncer_test.go
| s.lifecycleMu.Lock() | ||
| switch s.lifecycleState { | ||
| case syncerLifecycleStarted: | ||
| s.lifecycleMu.Unlock() | ||
| return errors.New("syncer already started") | ||
| case syncerLifecycleStopped: | ||
| s.lifecycleMu.Unlock() | ||
| return errors.New("syncer cannot be restarted after stopping") | ||
| } | ||
| ctx, cancel := context.WithCancel(ctx) | ||
| s.ctx, s.cancel = ctx, cancel | ||
| s.lifecycleState = syncerLifecycleStarted | ||
| s.lifecycleMu.Unlock() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Serialize startup and shutdown completion.
At Line 189, Start publishes syncerLifecycleStarted before initialization completes. A concurrent Stop can cancel the context, observe an empty s.wg, and close s.heightInCh. Start can then start s.raftRetriever, create s.daFollower, or launch workers after shutdown. This can cause sends to a closed channel and leave workers outside the shutdown wait.
Stop also reads s.fiRetriever and s.daFollower while Start writes them without lifecycleMu. A second Stop returns at Line 264 before the first cleanup completes.
Add explicit starting and stopping states with a completion signal, or use an equivalent lifecycle gate. Do not allow startup after shutdown begins. Make concurrent Stop callers wait for the first cleanup. Add a concurrent lifecycle test and run it with -race.
As per coding guidelines, protect shared state and prevent goroutine leaks.
Also applies to: 261-271
🤖 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 `@block/internal/syncing/syncer.go` around lines 178 - 190, Serialize Start and
Stop through the syncer lifecycle gate: add explicit starting and stopping
states (or an equivalent completion signal), keep initialization and worker
registration under lifecycle protection, and publish started only after
initialization completes. Prevent Start once stopping or stopped, make
concurrent Stop callers wait for the first cleanup instead of returning early,
and protect fiRetriever and daFollower accesses with lifecycleMu; add a
concurrent lifecycle test covering shutdown races and run it with -race.
Source: Coding guidelines
1 participant
Overview
Syncer.Start()automatically callsStop()when initialization fails. The upper-level component cleanup may subsequently callStop()again.Previously,
Stop()did not clears.cancelor otherwise record that shutdown had completed. The second call therefore reached:and panicked with:
panic: close of closed channelThe explicit lifecycle state ensures that only the first Stop() performs shutdown and closes heightInCh. It also preserves the existing contract that a stopped Syncer cannot be restarted.
Summary by CodeRabbit