Startup retry loop for nbxplorer connection#1083
Conversation
Avoids an `arkd-wallet` crash by ensuring nbxplorer can serve RPCs before continuing with a connection. Retries 30 times at 5 second intervals for a total of 2.5 minutes, leaving the orchestrator to restart afterward.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe NBXplorer service initializer now implements retry logic for establishing connectivity. Package constants define maximum retries and backoff intervals; the ChangesNBXplorer Connection Initialization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Approve — Startup retry loop for nbxplorer connection
Scope: Infrastructure startup code only. Not protocol-critical. No VTXO/signing/forfeit/round/exit paths touched.
What it does: Wraps the GetBitcoinStatus call in New() with a retry loop — 30 attempts × 5s = 2.5 min max wait — so arkd-wallet doesn't crash-loop when nbxplorer takes a moment to come up.
Assessment
The change is correct, minimal, and solves a real operational problem. The retry logic is straightforward with no subtle bugs.
Nits (non-blocking)
-
No context propagation in retry loop —
service.go:77: The loop usescontext.Background()andtime.Sleep(). If the process receives a shutdown signal during startup, it'll block up to 2.5 minutes before giving up. IdeallyNew()would accept acontext.Context(or at least the retry sleep would use atime.After/selectpattern). This is a pre-existing design limitation sinceNew()never took a context, so not blocking this PR on it — but worth a follow-up. -
No test coverage — There are zero
*_test.gofiles underpkg/arkd-wallet/core/infrastructure/nbxplorer/. The retry logic is simple enough to review by inspection, but a unit test with a mock HTTP server that fails N times then succeeds would be nice. Again, non-blocking given the simplicity. -
Fixed interval vs backoff — 5s fixed interval is fine for "waiting for a dependent service to start." No need for exponential backoff here.
Verified
- Constants are reasonable (30 retries × 5s = 2.5 min)
- Loop terminates correctly on both success and max retries
- Error message on final failure includes attempt count
- Warning log on each retry includes attempt number and error
-
statusis properly used after the loop (linesvc.minRelayTxFee = status.MinRelayTxFee) - Single caller in
config.go:126— no API change, no downstream breakage - No cross-repo impact (internal infrastructure, unexported behavior change)
LGTM. Ship it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go (1)
77-78: 🏗️ Heavy liftConsider accepting a context parameter to enable cancellation during initialization.
The retry loop uses
context.Background(), which creates an uncancellable 2.5-minute initialization window. If the service needs to shut down during this period, it must wait for all retries to complete.Accepting a context parameter would enable:
- Graceful shutdown during startup
- Configurable timeouts per deployment environment
- Easier testing with context cancellation
Suggested approach
Modify the function signature to accept a context:
-func New(rawURL string) (ports.Nbxplorer, error) { +func New(ctx context.Context, rawURL string) (ports.Nbxplorer, error) { rawURL = strings.TrimSuffix(rawURL, "/") // ... URL validation ... for attempt := 1; attempt <= nbxplorerMaxRetries; attempt++ { - status, err = svc.GetBitcoinStatus(context.Background()) + status, err = svc.GetBitcoinStatus(ctx) if err == nil { break } + + if ctx.Err() != nil { + return nil, fmt.Errorf("context cancelled during nbxplorer initialization: %w", ctx.Err()) + }This requires updating call sites (e.g., in
pkg/arkd-wallet/config/config.go).🤖 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 `@pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go` around lines 77 - 78, Change the initialization function that contains the retry loop (the function in pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go that calls svc.GetBitcoinStatus and uses nbxplorerMaxRetries) to accept a context.Context parameter instead of using context.Background(); replace context.Background() with the passed ctx when calling svc.GetBitcoinStatus, check ctx.Done()/ctx.Err() inside the retry loop and return early with a canceled error if context is canceled, and propagate the new ctx parameter to all callers (e.g., update the call site in pkg/arkd-wallet/config/config.go) so startup can be canceled or timed out.
🤖 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.
Nitpick comments:
In `@pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go`:
- Around line 77-78: Change the initialization function that contains the retry
loop (the function in pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go
that calls svc.GetBitcoinStatus and uses nbxplorerMaxRetries) to accept a
context.Context parameter instead of using context.Background(); replace
context.Background() with the passed ctx when calling svc.GetBitcoinStatus,
check ctx.Done()/ctx.Err() inside the retry loop and return early with a
canceled error if context is canceled, and propagate the new ctx parameter to
all callers (e.g., update the call site in pkg/arkd-wallet/config/config.go) so
startup can be canceled or timed out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcd375f5-ed94-400a-a488-512d604c6bc4
📒 Files selected for processing (1)
pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go
2 participants
Avoids an
arkd-walletcrash and restart by ensuringnbxplorercan serve RPCs before continuing with a connection. Retries 30 times at 5 second intervals for a total of 2.5 minutes, leaving the orchestrator to restart afterward.Summary by CodeRabbit