fix(cli/operator): convert logger.Fatal goroutines to errgroup (#2867) - #2897
fix(cli/operator): convert logger.Fatal goroutines to errgroup (#2867)#2897Roy-blox wants to merge 3 commits into
Conversation
…ated errors Resolves #2867. All long-lived goroutines in start() now run under a shared errgroup so the first failure cancels the rest and surfaces as a return value from start() rather than os.Exit via logger.Fatal. Changes: - node.go start(): wrap with errgroup.WithContext, replace Fatal goroutines with g.Go calls, use gctx throughout, return g.Wait() at the end - prober.go startHealthProber(): return error instead of Fatal on unhealthy probe - eventsync.go syncContractEvents(): return (EventSyncer, func() error, error); the ongoing-sync start-func is g.Go'd by start() instead of spawned internally Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
QA Report: staging deploy ✅
1. Commit verifiedAll 4 nodes started on commit 2. Duty executionDuties completing at round 1 on all nodes: 3. ErrorsOnly benign P2P gossip noise ( 4. Metrics
Consensus at 6-13ms across duty types — normal for stage. 5. errgroup-specific checks
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if err != nil { | ||
| return fmt.Errorf("%s: %w", componentsUnhealthyErrorMsg, err) | ||
| } |
There was a problem hiding this comment.
If gctx is canceled while ProbeAll is in flight (e.g., an OS signal arrives mid-probe, or another errgroup goroutine fails), probeCtx is immediately derived-canceled, so ProbeAll returns a context.Canceled-wrapped error. Because there's no ctx.Err() != nil guard before the return fmt.Errorf(...), startHealthProber returns a non-nil error to the errgroup. g.Wait() propagates it to start(), which returns it to start_node.go's logger.Fatal("could not start node", ...) — a misleading message for what was a clean shutdown.
In the old design, startHealthProber was a fire-and-forget goroutine; when operatorNode.Start returned nil on normal shutdown, start() returned nil immediately without waiting for the prober. In the new design, g.Wait() blocks until all goroutines finish, so the prober's stale context error can now determine the final return value.
The ongoingSync closure in eventsync.go correctly guards against this pattern with !errors.Is(err, context.Canceled). The same guard is needed here.
| if err != nil { | |
| return fmt.Errorf("%s: %w", componentsUnhealthyErrorMsg, err) | |
| } | |
| if err != nil { | |
| if ctx.Err() != nil { | |
| return nil | |
| } | |
| return fmt.Errorf("%s: %w", componentsUnhealthyErrorMsg, err) | |
| } |
| ongoingSync = func() error { | ||
| err := eventSyncer.SyncOngoing(ctx, ongoingFromBlock) | ||
| if err != nil && !errors.Is(err, context.Canceled) { | ||
| logger.Fatal("failed syncing ongoing registry events", | ||
| zap.Uint64("last_processed_block", lastProcessedBlock), | ||
| zap.Error(err), | ||
| ) | ||
| return fmt.Errorf("failed syncing ongoing registry events (from_block=%d): %w", ongoingFromBlock, err) | ||
| } | ||
| }() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
context.DeadlineExceeded slips through the cancellation guard
The closure only filters context.Canceled, but not context.DeadlineExceeded. If the parent n.ctx (or any ancestor) ever carries a deadline, gctx inherits it, SyncOngoing returns an error wrapping context.DeadlineExceeded, and the closure returns a non-nil error to the errgroup — causing logger.Fatal("could not start node: failed syncing ongoing registry events...") on what was a normal deadline expiry. The more idiomatic guard is ctx.Err() != nil, which covers both cancellation and deadline exhaustion.
| ongoingSync = func() error { | |
| err := eventSyncer.SyncOngoing(ctx, ongoingFromBlock) | |
| if err != nil && !errors.Is(err, context.Canceled) { | |
| logger.Fatal("failed syncing ongoing registry events", | |
| zap.Uint64("last_processed_block", lastProcessedBlock), | |
| zap.Error(err), | |
| ) | |
| return fmt.Errorf("failed syncing ongoing registry events (from_block=%d): %w", ongoingFromBlock, err) | |
| } | |
| }() | |
| return nil | |
| } | |
| ongoingSync = func() error { | |
| err := eventSyncer.SyncOngoing(ctx, ongoingFromBlock) | |
| if err != nil && ctx.Err() == nil { | |
| return fmt.Errorf("failed syncing ongoing registry events (from_block=%d): %w", ongoingFromBlock, err) | |
| } | |
| return nil | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…tsync - prober.go startHealthProber: return nil when ctx.Err() != nil so a probe failure that races normal shutdown does not produce a misleading Fatal log - eventsync.go ongoingSync: use ctx.Err()==nil instead of narrower !errors.Is(err,context.Canceled) to also cover context.DeadlineExceeded - prober_test.go: three tests covering the happy path, unhealthy probe, and the ctx-cancel-during-probe race - codecov.yml: ignore node.go and eventsync.go (bootstrap/integration-level code, not unit-testable in isolation) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Greptile's concerns — addressed in follow-up commit 1. The concern is correct: if a component's Fixed: added 2. The original |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| g.Go(func() error { return startHealthProber(gctx, n.logger, healthProber) }) | ||
|
|
||
| if _, err := n.metadataSyncer.SyncAll(n.ctx); err != nil { | ||
| if _, err := n.metadataSyncer.SyncAll(gctx); err != nil { |
There was a problem hiding this comment.
What do you think about these early returns racing a backgrounded failure?
Once ongoingSync/prober/metrics are g.Go'd, a failure cancels gctx, so this path returns failed to sync metadata on startup: context canceled while the real cause sits in the un-waited errgroup — the funnel Fatal then misattributes the crash.
Maybe return errors.Join(err, g.Wait()) on these early returns so the originating error wins?
|
|
||
| return nil | ||
| g.Go(func() error { | ||
| if err := n.operatorNode.Start(gctx); err != nil { |
There was a problem hiding this comment.
Should we consider that operatorNode.Start still calls logger.Fatal internally (WS serve loop, operator/node.go:445)?
Running inside this g.Go, it os.Exits past the errgroup and Close() on a mid-run WS failure — the same pattern #2867 set out to remove, just one layer down.
Worth a follow-up, or threading that serveErr out like metrics/API do?
| cancel() // cancel before starting — simulates shutdown racing a probe | ||
|
|
||
| prober := hprobe.NewHealthProber(zap.NewNop()) | ||
| prober.AddComponent("cl", simpleComponent{err: errors.New("broken")}, 100*time.Millisecond, 0, 0) |
There was a problem hiding this comment.
Might be worth strengthening this case — the stub's Healthy ignores ctx and returns the same error regardless, so the test passes via the guard but doesn't actually prove it distinguishes a real probe failure from a cancel.
A component that respects ctx would return context.Canceled (swallowed to nil in probeComponent), so ProbeAll returns nil and the guard is never reached. A ctx-respecting stub would exercise the realistic teardown race the comment describes.
| ongoingSync = func() error { | ||
| err := eventSyncer.SyncOngoing(ctx, ongoingFromBlock) | ||
| if err != nil && ctx.Err() == nil { | ||
| return fmt.Errorf("failed syncing ongoing registry events (from_block=%d): %w", ongoingFromBlock, err) |
There was a problem hiding this comment.
Seems that the old Fatal logged last_processed_block while this now logs from_block (= last_processed_block + 1).
Minor, but operators grepping for last_processed_block on this failure won't find it anymore — maybe include both fields?
2 participants
Summary
Closes #2867.
All
logger.Fatalcalls inside background goroutines incli/operatorare replaced with errgroup-propagated errors. The first service failure now cancels the rest and surfaces as a return value fromstart(), which already ends at the singlelogger.Fatal("could not start node", ...)instart_node.go.Changes:
node.gostart(): wrap witherrgroup.WithContext(n.ctx), replace all 4 Fatal goroutines (metrics bind/serve, API bind/serve) withreturn err/g.Go(...), usegctxthroughout all ctx-accepting calls, registeroperatorNode.Startin the group, returng.Wait()prober.gostartHealthProber(): signature→ error; unhealthy probe now returnsfmt.Errorf(...)instead of callinglogger.Fataleventsync.gosyncContractEvents(): return(*EventSyncer, func() error, error); ongoing-sync goroutine + Fatal removed; caller receives a start-func itg.GosTest plan
logger.Fatalcalls remaining in background goroutines incli/operator🤖 Generated with Claude Code