fix: Start should panic insteal of only print#1371
Merged
Conversation
Contributor
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: foundation/application.go
Line: 214:214
Comment:
with `Start()` now panicking on runner failures (line 292-294), calling it in a goroutine here means panics won't be caught by `Restart()`. this could crash the application unexpectedly instead of returning an error from `Restart()`
```suggestion
startErr := make(chan error, 1)
go func() {
defer func() {
if r := recover(); r != nil {
startErr <- fmt.Errorf("panic during start: %v", r)
}
}()
r.Start()
}()
```
How can I resolve this? If you propose a fix, please make it concise. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.17.x #1371 +/- ##
==========================================
Coverage ? 68.57%
==========================================
Files ? 281
Lines ? 17750
Branches ? 0
==========================================
Hits ? 12172
Misses ? 5075
Partials ? 503 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
devhaozi
approved these changes
Feb 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Greptile Overview
Greptile Summary
Changed
Application.Start()to panic when runners fail instead of just logging errors. The implementation collects all runner errors with thread-safe mutex protection and useserrors.Join()to combine them into a single panic.Key changes:
color.Errorfwith proper logging viaMakeLog()r.cancel()call on runner failure to trigger graceful shutdown of other runnersrunnerWg.Wait()Critical issue identified:
Restart()method (line 214) callsStart()in a goroutine, which means the new panic behavior won't be caught and could crash the application unexpectedly instead of returning an error fromRestart().Confidence Score: 2/5
Restart()method that callsStart()in a goroutine without panic recovery. This means runner failures will crash the entire application instead of being handled byRestart(). The new panic behavior is a breaking change that needs careful consideration of all call sites.foundation/application.goline 214 whereRestart()callsStart()in a goroutineImportant Files Changed
Start()to panic on runner failures instead of just logging. Added error collection with mutex protection and callscancel()on failure. Critical issue:Restart()callsStart()in goroutine which won't catch panics.Sequence Diagram
sequenceDiagram participant Caller participant Application participant Runner1 participant Runner2 participant ErrorCollector participant Logger Caller->>Application: Start() activate Application Note over Application: Initialize error slice & mutex par Run all runners Application->>Runner1: run(runner1) activate Runner1 Runner1->>Runner1: Run() alt Run fails Runner1->>ErrorCollector: Lock & append error Runner1->>Logger: Log error (if available) Runner1->>Application: cancel() Runner1->>Application: runnerWg.Done() else Run succeeds Note over Runner1: Blocking until shutdown end and Application->>Runner2: run(runner2) activate Runner2 Runner2->>Runner2: Run() alt Run fails Runner2->>ErrorCollector: Lock & append error Runner2->>Logger: Log error (if available) Runner2->>Application: cancel() Runner2->>Application: runnerWg.Done() else Run succeeds Note over Runner2: Blocking until shutdown end end Note over Application: Wait for all runners (runnerWg.Wait()) alt Errors collected Application->>Application: panic(errors.Join(errs...)) Application-->>Caller: panic! else No errors Application-->>Caller: return normally end deactivate Runner1 deactivate Runner2 deactivate Application