fix(bootstrap): address copilot review - races, leaks, semantics
- commit
- 494e2a73d2a2589a6637f84288669e747bf09d69
- author
- bnema · · 2025-12-27 21:43 UTC
- parent
- 162340b
- signature
- GPG Unverified
2B82207D370127A0The key is unknown
5 files · +41 -24
M cmd/dumber/main.go +6 -5
| @@ -164,9 +164,9 @@ func initConfig() *config.Config { | |||
| 164 | 164 | ||
| 165 | 165 | func handleParallelInitError(ctx context.Context, err error) { | |
| 166 | 166 | log := logging.FromContext(ctx) | |
| 167 | - | if err, ok := err.(*bootstrap.RuntimeRequirementsError); ok { | |
| 168 | - | err.LogDetails(ctx) | |
| 169 | - | log.Fatal().Err(err). | |
| 167 | + | if runtimeErr, ok := err.(*bootstrap.RuntimeRequirementsError); ok { | |
| 168 | + | runtimeErr.LogDetails(ctx) | |
| 169 | + | log.Fatal().Err(runtimeErr). | |
| 170 | 170 | Str("hint", "Run: dumber doctor (and set runtime.prefix for /opt installs)"). | |
| 171 | 171 | Msg("runtime requirements not met") | |
| 172 | 172 | } | |
| @@ -204,8 +204,9 @@ func setupSignalHandler(ctx context.Context, app *ui.App) { | |||
| 204 | 204 | sigCh := make(chan os.Signal, 1) | |
| 205 | 205 | signal.Notify(sigCh, os.Interrupt, syscall.SIGTERM) | |
| 206 | 206 | go func() { | |
| 207 | - | <-sigCh | |
| 208 | - | log.Info().Msg("received interrupt, quitting") | |
| 207 | + | sig := <-sigCh | |
| 208 | + | signal.Stop(sigCh) | |
| 209 | + | log.Info().Str("signal", sig.String()).Msg("received interrupt, quitting") | |
| 209 | 210 | app.Quit() | |
| 210 | 211 | }() | |
| 211 | 212 | } | |
| @@ -164,9 +164,9 @@ func initConfig() *config.Config { | |||||
| 164 | 164 | ||||
| 165 | func handleParallelInitError(ctx context.Context, err error) { | 165 | func handleParallelInitError(ctx context.Context, err error) { | ||
| 166 | log := logging.FromContext(ctx) | 166 | log := logging.FromContext(ctx) | ||
| 167 | - | if err, ok := err.(*bootstrap.RuntimeRequirementsError); ok { | 167 | + | if runtimeErr, ok := err.(*bootstrap.RuntimeRequirementsError); ok { |
| 168 | - | err.LogDetails(ctx) | 168 | + | runtimeErr.LogDetails(ctx) |
| 169 | - | log.Fatal().Err(err). | 169 | + | log.Fatal().Err(runtimeErr). |
| 170 | Str("hint", "Run: dumber doctor (and set runtime.prefix for /opt installs)"). | 170 | Str("hint", "Run: dumber doctor (and set runtime.prefix for /opt installs)"). | ||
| 171 | Msg("runtime requirements not met") | 171 | Msg("runtime requirements not met") | ||
| 172 | } | 172 | } | ||
| @@ -204,8 +204,9 @@ func setupSignalHandler(ctx context.Context, app *ui.App) { | |||||
| 204 | sigCh := make(chan os.Signal, 1) | 204 | sigCh := make(chan os.Signal, 1) | ||
| 205 | signal.Notify(sigCh, os.Interrupt, syscall.SIGTERM) | 205 | signal.Notify(sigCh, os.Interrupt, syscall.SIGTERM) | ||
| 206 | go func() { | 206 | go func() { | ||
| 207 | - | <-sigCh | 207 | + | sig := <-sigCh |
| 208 | - | log.Info().Msg("received interrupt, quitting") | 208 | + | signal.Stop(sigCh) |
| 209 | + | log.Info().Str("signal", sig.String()).Msg("received interrupt, quitting") | |||
| 209 | app.Quit() | 210 | app.Quit() | ||
| 210 | }() | 211 | }() | ||
| 211 | } | 212 | } | ||
M internal/bootstrap/gui.go +18 -15
| @@ -236,44 +236,47 @@ type ParallelDBWebKitInput struct { | |||
| 236 | 236 | ||
| 237 | 237 | // Note: Database path is resolved via config.GetDatabaseFile() internally. | |
| 238 | 238 | ||
| 239 | + | // dbInitResult holds the result of database initialization for channel communication. | |
| 240 | + | type dbInitResult struct { | |
| 241 | + | db *sql.DB | |
| 242 | + | cleanup func() | |
| 243 | + | err error | |
| 244 | + | } | |
| 245 | + | ||
| 239 | 246 | // RunParallelDBWebKit initializes database in background while WebKit runs on main thread. | |
| 240 | 247 | // Database init happens in goroutine (pure Go/WASM), WebKit must stay on main thread (GTK). | |
| 241 | 248 | // This saves ~150ms by overlapping DB migrations with WebKit context creation. | |
| 242 | 249 | func RunParallelDBWebKit(input ParallelDBWebKitInput) (*ParallelDBWebKitResult, error) { | |
| 243 | 250 | log := logging.FromContext(input.Ctx) | |
| 244 | 251 | ||
| 245 | - | var ( | |
| 246 | - | db *sql.DB | |
| 247 | - | dbCleanup func() | |
| 248 | - | dbErr error | |
| 249 | - | dbDone = make(chan struct{}) | |
| 250 | - | ) | |
| 252 | + | dbCh := make(chan dbInitResult, 1) | |
| 251 | 253 | ||
| 252 | 254 | // Database initialization in background (WASM is thread-safe) | |
| 253 | 255 | go func() { | |
| 254 | - | defer close(dbDone) | |
| 255 | 256 | dbResult, err := OpenDatabase(input.Ctx) | |
| 256 | 257 | if err != nil { | |
| 257 | - | dbErr = err | |
| 258 | + | dbCh <- dbInitResult{err: err} | |
| 258 | 259 | return | |
| 259 | 260 | } | |
| 260 | - | db = dbResult.DB | |
| 261 | - | dbCleanup = dbResult.Cleanup | |
| 261 | + | dbCh <- dbInitResult{ | |
| 262 | + | db: dbResult.DB, | |
| 263 | + | cleanup: dbResult.Cleanup, | |
| 264 | + | } | |
| 262 | 265 | }() | |
| 263 | 266 | ||
| 264 | 267 | // WebKit stack on main thread (GTK requirement) | |
| 265 | 268 | stack := BuildWebKitStack(input.Ctx, input.Config, input.DataDir, input.CacheDir, input.ThemeManager, *log) | |
| 266 | 269 | ||
| 267 | 270 | // Wait for database | |
| 268 | - | <-dbDone | |
| 271 | + | dbRes := <-dbCh | |
| 269 | 272 | ||
| 270 | - | if dbErr != nil { | |
| 271 | - | return nil, fmt.Errorf("database initialization: %w", dbErr) | |
| 273 | + | if dbRes.err != nil { | |
| 274 | + | return nil, fmt.Errorf("database initialization: %w", dbRes.err) | |
| 272 | 275 | } | |
| 273 | 276 | ||
| 274 | 277 | return &ParallelDBWebKitResult{ | |
| 275 | - | DB: db, | |
| 276 | - | DBCleanup: dbCleanup, | |
| 278 | + | DB: dbRes.db, | |
| 279 | + | DBCleanup: dbRes.cleanup, | |
| 277 | 280 | Stack: stack, | |
| 278 | 281 | }, nil | |
| 279 | 282 | } | |
| @@ -236,44 +236,47 @@ type ParallelDBWebKitInput struct { | |||||
| 236 | 236 | ||||
| 237 | // Note: Database path is resolved via config.GetDatabaseFile() internally. | 237 | // Note: Database path is resolved via config.GetDatabaseFile() internally. | ||
| 238 | 238 | ||||
| 239 | + | // dbInitResult holds the result of database initialization for channel communication. | |||
| 240 | + | type dbInitResult struct { | |||
| 241 | + | db *sql.DB | |||
| 242 | + | cleanup func() | |||
| 243 | + | err error | |||
| 244 | + | } | |||
| 245 | + | ||||
| 239 | // RunParallelDBWebKit initializes database in background while WebKit runs on main thread. | 246 | // RunParallelDBWebKit initializes database in background while WebKit runs on main thread. | ||
| 240 | // Database init happens in goroutine (pure Go/WASM), WebKit must stay on main thread (GTK). | 247 | // Database init happens in goroutine (pure Go/WASM), WebKit must stay on main thread (GTK). | ||
| 241 | // This saves ~150ms by overlapping DB migrations with WebKit context creation. | 248 | // This saves ~150ms by overlapping DB migrations with WebKit context creation. | ||
| 242 | func RunParallelDBWebKit(input ParallelDBWebKitInput) (*ParallelDBWebKitResult, error) { | 249 | func RunParallelDBWebKit(input ParallelDBWebKitInput) (*ParallelDBWebKitResult, error) { | ||
| 243 | log := logging.FromContext(input.Ctx) | 250 | log := logging.FromContext(input.Ctx) | ||
| 244 | 251 | ||||
| 245 | - | var ( | 252 | + | dbCh := make(chan dbInitResult, 1) |
| 246 | - | db *sql.DB | |||
| 247 | - | dbCleanup func() | |||
| 248 | - | dbErr error | |||
| 249 | - | dbDone = make(chan struct{}) | |||
| 250 | - | ) | |||
| 251 | 253 | ||||
| 252 | // Database initialization in background (WASM is thread-safe) | 254 | // Database initialization in background (WASM is thread-safe) | ||
| 253 | go func() { | 255 | go func() { | ||
| 254 | - | defer close(dbDone) | |||
| 255 | dbResult, err := OpenDatabase(input.Ctx) | 256 | dbResult, err := OpenDatabase(input.Ctx) | ||
| 256 | if err != nil { | 257 | if err != nil { | ||
| 257 | - | dbErr = err | 258 | + | dbCh <- dbInitResult{err: err} |
| 258 | return | 259 | return | ||
| 259 | } | 260 | } | ||
| 260 | - | db = dbResult.DB | 261 | + | dbCh <- dbInitResult{ |
| 261 | - | dbCleanup = dbResult.Cleanup | 262 | + | db: dbResult.DB, |
| 263 | + | cleanup: dbResult.Cleanup, | |||
| 264 | + | } | |||
| 262 | }() | 265 | }() | ||
| 263 | 266 | ||||
| 264 | // WebKit stack on main thread (GTK requirement) | 267 | // WebKit stack on main thread (GTK requirement) | ||
| 265 | stack := BuildWebKitStack(input.Ctx, input.Config, input.DataDir, input.CacheDir, input.ThemeManager, *log) | 268 | stack := BuildWebKitStack(input.Ctx, input.Config, input.DataDir, input.CacheDir, input.ThemeManager, *log) | ||
| 266 | 269 | ||||
| 267 | // Wait for database | 270 | // Wait for database | ||
| 268 | - | <-dbDone | 271 | + | dbRes := <-dbCh |
| 269 | 272 | ||||
| 270 | - | if dbErr != nil { | 273 | + | if dbRes.err != nil { |
| 271 | - | return nil, fmt.Errorf("database initialization: %w", dbErr) | 274 | + | return nil, fmt.Errorf("database initialization: %w", dbRes.err) |
| 272 | } | 275 | } | ||
| 273 | 276 | ||||
| 274 | return &ParallelDBWebKitResult{ | 277 | return &ParallelDBWebKitResult{ | ||
| 275 | - | DB: db, | 278 | + | DB: dbRes.db, |
| 276 | - | DBCleanup: dbCleanup, | 279 | + | DBCleanup: dbRes.cleanup, |
| 277 | Stack: stack, | 280 | Stack: stack, | ||
| 278 | }, nil | 281 | }, nil | ||
| 279 | } | 282 | } | ||
M internal/bootstrap/session.go +7 -2
| @@ -201,15 +201,20 @@ func unlockAndClose(f *os.File) error { | |||
| 201 | 201 | // in a background goroutine. This avoids blocking startup for non-critical tasks. | |
| 202 | 202 | // Uses context.Background() to ensure cleanup completes even if startup context is canceled. | |
| 203 | 203 | func runSessionCleanupAsync( | |
| 204 | - | _ context.Context, | |
| 204 | + | startupCtx context.Context, | |
| 205 | 205 | sessionUC *usecase.ManageSessionUseCase, | |
| 206 | 206 | cleanupUC *usecase.CleanupSessionsUseCase, | |
| 207 | 207 | cfg *config.Config, | |
| 208 | 208 | lockDir string, | |
| 209 | 209 | log *zerolog.Logger, | |
| 210 | 210 | ) { | |
| 211 | + | // Silence unused parameter warning - startupCtx is intentionally unused. | |
| 212 | + | _ = startupCtx | |
| 213 | + | ||
| 211 | 214 | go func() { | |
| 212 | - | // Use detached context: cleanup should complete regardless of startup context lifecycle. | |
| 215 | + | // Use a detached background context instead of the startup context: | |
| 216 | + | // session cleanup is critical and must run to completion even if the | |
| 217 | + | // startup context is cancelled or times out. | |
| 213 | 218 | bgCtx := context.Background() | |
| 214 | 219 | ||
| 215 | 220 | // End stale active sessions (orphaned from crashed processes) | |
| @@ -201,15 +201,20 @@ func unlockAndClose(f *os.File) error { | |||||
| 201 | // in a background goroutine. This avoids blocking startup for non-critical tasks. | 201 | // in a background goroutine. This avoids blocking startup for non-critical tasks. | ||
| 202 | // Uses context.Background() to ensure cleanup completes even if startup context is canceled. | 202 | // Uses context.Background() to ensure cleanup completes even if startup context is canceled. | ||
| 203 | func runSessionCleanupAsync( | 203 | func runSessionCleanupAsync( | ||
| 204 | - | _ context.Context, | 204 | + | startupCtx context.Context, |
| 205 | sessionUC *usecase.ManageSessionUseCase, | 205 | sessionUC *usecase.ManageSessionUseCase, | ||
| 206 | cleanupUC *usecase.CleanupSessionsUseCase, | 206 | cleanupUC *usecase.CleanupSessionsUseCase, | ||
| 207 | cfg *config.Config, | 207 | cfg *config.Config, | ||
| 208 | lockDir string, | 208 | lockDir string, | ||
| 209 | log *zerolog.Logger, | 209 | log *zerolog.Logger, | ||
| 210 | ) { | 210 | ) { | ||
| 211 | + | // Silence unused parameter warning - startupCtx is intentionally unused. | |||
| 212 | + | _ = startupCtx | |||
| 213 | + | ||||
| 211 | go func() { | 214 | go func() { | ||
| 212 | - | // Use detached context: cleanup should complete regardless of startup context lifecycle. | 215 | + | // Use a detached background context instead of the startup context: |
| 216 | + | // session cleanup is critical and must run to completion even if the | |||
| 217 | + | // startup context is cancelled or times out. | |||
| 213 | bgCtx := context.Background() | 218 | bgCtx := context.Background() | ||
| 214 | 219 | ||||
| 215 | // End stale active sessions (orphaned from crashed processes) | 220 | // End stale active sessions (orphaned from crashed processes) | ||
M internal/infrastructure/persistence/sqlite/connection.go +4 -0
| @@ -82,6 +82,10 @@ func applyPragmas(db *sql.DB) error { | |||
| 82 | 82 | ||
| 83 | 83 | // configurePool sets connection pool parameters optimized for SQLite. | |
| 84 | 84 | // SQLite only supports one writer at a time, so we limit connections. | |
| 85 | + | // | |
| 86 | + | // Connections are configured to never expire or close while idle. This is safe | |
| 87 | + | // because the application's lifecycle matches the database connection's lifecycle: | |
| 88 | + | // the process terminates before the underlying database file is rotated or replaced. | |
| 85 | 89 | func configurePool(db *sql.DB) { | |
| 86 | 90 | db.SetMaxOpenConns(1) // SQLite is single-writer | |
| 87 | 91 | db.SetMaxIdleConns(1) // Keep one connection alive | |
| @@ -82,6 +82,10 @@ func applyPragmas(db *sql.DB) error { | |||||
| 82 | 82 | ||||
| 83 | // configurePool sets connection pool parameters optimized for SQLite. | 83 | // configurePool sets connection pool parameters optimized for SQLite. | ||
| 84 | // SQLite only supports one writer at a time, so we limit connections. | 84 | // SQLite only supports one writer at a time, so we limit connections. | ||
| 85 | + | // | |||
| 86 | + | // Connections are configured to never expire or close while idle. This is safe | |||
| 87 | + | // because the application's lifecycle matches the database connection's lifecycle: | |||
| 88 | + | // the process terminates before the underlying database file is rotated or replaced. | |||
| 85 | func configurePool(db *sql.DB) { | 89 | func configurePool(db *sql.DB) { | ||
| 86 | db.SetMaxOpenConns(1) // SQLite is single-writer | 90 | db.SetMaxOpenConns(1) // SQLite is single-writer | ||
| 87 | db.SetMaxIdleConns(1) // Keep one connection alive | 91 | db.SetMaxIdleConns(1) // Keep one connection alive | ||
M internal/infrastructure/webkit/pool.go +6 -2
| @@ -2,6 +2,7 @@ package webkit | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "errors" | |
| 5 | 6 | "sync" | |
| 6 | 7 | "sync/atomic" | |
| 7 | 8 | "time" | |
| @@ -11,6 +12,9 @@ import ( | |||
| 11 | 12 | "github.com/jwijenbergh/puregotk/v4/glib" | |
| 12 | 13 | ) | |
| 13 | 14 | ||
| 15 | + | // ErrPoolClosed is returned when operations are attempted on a closed pool. | |
| 16 | + | var ErrPoolClosed = errors.New("webview pool is closed") | |
| 17 | + | ||
| 14 | 18 | // FilterApplier applies content filters to a UserContentManager. | |
| 15 | 19 | // This interface decouples the pool from the filtering package. | |
| 16 | 20 | type FilterApplier interface { | |
| @@ -114,7 +118,7 @@ func (p *WebViewPool) Acquire(ctx context.Context) (*WebView, error) { | |||
| 114 | 118 | log := logging.FromContext(ctx) | |
| 115 | 119 | ||
| 116 | 120 | if p.closed.Load() { | |
| 117 | - | return nil, context.Canceled | |
| 121 | + | return nil, ErrPoolClosed | |
| 118 | 122 | } | |
| 119 | 123 | ||
| 120 | 124 | // Try to get from pool first (non-blocking) | |
| @@ -203,7 +207,7 @@ func (p *WebViewPool) Release(ctx context.Context, wv *WebView) { | |||
| 203 | 207 | // Returns error if WebView creation fails; caller should log and continue. | |
| 204 | 208 | func (p *WebViewPool) PrewarmFirst(ctx context.Context) error { | |
| 205 | 209 | if p.closed.Load() { | |
| 206 | - | return context.Canceled | |
| 210 | + | return ErrPoolClosed | |
| 207 | 211 | } | |
| 208 | 212 | if len(p.pool) > 0 { | |
| 209 | 213 | return nil // Already have at least one | |
| @@ -2,6 +2,7 @@ package webkit | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "errors" | |||
| 5 | "sync" | 6 | "sync" | ||
| 6 | "sync/atomic" | 7 | "sync/atomic" | ||
| 7 | "time" | 8 | "time" | ||
| @@ -11,6 +12,9 @@ import ( | |||||
| 11 | "github.com/jwijenbergh/puregotk/v4/glib" | 12 | "github.com/jwijenbergh/puregotk/v4/glib" | ||
| 12 | ) | 13 | ) | ||
| 13 | 14 | ||||
| 15 | + | // ErrPoolClosed is returned when operations are attempted on a closed pool. | |||
| 16 | + | var ErrPoolClosed = errors.New("webview pool is closed") | |||
| 17 | + | ||||
| 14 | // FilterApplier applies content filters to a UserContentManager. | 18 | // FilterApplier applies content filters to a UserContentManager. | ||
| 15 | // This interface decouples the pool from the filtering package. | 19 | // This interface decouples the pool from the filtering package. | ||
| 16 | type FilterApplier interface { | 20 | type FilterApplier interface { | ||
| @@ -114,7 +118,7 @@ func (p *WebViewPool) Acquire(ctx context.Context) (*WebView, error) { | |||||
| 114 | log := logging.FromContext(ctx) | 118 | log := logging.FromContext(ctx) | ||
| 115 | 119 | ||||
| 116 | if p.closed.Load() { | 120 | if p.closed.Load() { | ||
| 117 | - | return nil, context.Canceled | 121 | + | return nil, ErrPoolClosed |
| 118 | } | 122 | } | ||
| 119 | 123 | ||||
| 120 | // Try to get from pool first (non-blocking) | 124 | // Try to get from pool first (non-blocking) | ||
| @@ -203,7 +207,7 @@ func (p *WebViewPool) Release(ctx context.Context, wv *WebView) { | |||||
| 203 | // Returns error if WebView creation fails; caller should log and continue. | 207 | // Returns error if WebView creation fails; caller should log and continue. | ||
| 204 | func (p *WebViewPool) PrewarmFirst(ctx context.Context) error { | 208 | func (p *WebViewPool) PrewarmFirst(ctx context.Context) error { | ||
| 205 | if p.closed.Load() { | 209 | if p.closed.Load() { | ||
| 206 | - | return context.Canceled | 210 | + | return ErrPoolClosed |
| 207 | } | 211 | } | ||
| 208 | if len(p.pool) > 0 { | 212 | if len(p.pool) > 0 { | ||
| 209 | return nil // Already have at least one | 213 | return nil // Already have at least one | ||