fix(bootstrap): address review comments - mutex, context, docs
- commit
- 326c2f61a597a3cf4f709f19d4ff17c871770fc5
- author
- bnema · · 2025-12-27 21:25 UTC
- parent
- 725d1f4
- signature
- GPG Unverified
2B82207D370127A0The key is unknown
6 files · +38 -3
M internal/application/port/database.go +4 -0
| @@ -8,6 +8,10 @@ import ( | |||
| 8 | 8 | ||
| 9 | 9 | // DatabaseProvider provides access to the database connection. | |
| 10 | 10 | // Implementations may initialize the database lazily on first access. | |
| 11 | + | // | |
| 12 | + | // Currently used by the lazy repository infrastructure (sqlite.LazyDB and lazy_repos.go), | |
| 13 | + | // which is kept for potential future optimization scenarios where deferring database | |
| 14 | + | // initialization past first paint provides latency benefits. | |
| 11 | 15 | type DatabaseProvider interface { | |
| 12 | 16 | // DB returns the database connection, initializing it if necessary. | |
| 13 | 17 | // Returns an error if initialization fails. | |
| @@ -8,6 +8,10 @@ import ( | |||||
| 8 | 8 | ||||
| 9 | // DatabaseProvider provides access to the database connection. | 9 | // DatabaseProvider provides access to the database connection. | ||
| 10 | // Implementations may initialize the database lazily on first access. | 10 | // Implementations may initialize the database lazily on first access. | ||
| 11 | + | // | |||
| 12 | + | // Currently used by the lazy repository infrastructure (sqlite.LazyDB and lazy_repos.go), | |||
| 13 | + | // which is kept for potential future optimization scenarios where deferring database | |||
| 14 | + | // initialization past first paint provides latency benefits. | |||
| 11 | type DatabaseProvider interface { | 15 | type DatabaseProvider interface { | ||
| 12 | // DB returns the database connection, initializing it if necessary. | 16 | // DB returns the database connection, initializing it if necessary. | ||
| 13 | // Returns an error if initialization fails. | 17 | // Returns an error if initialization fails. | ||
M internal/bootstrap/gui.go +4 -0
| @@ -206,6 +206,10 @@ func OpenDatabase(ctx context.Context) (*DatabaseResult, error) { | |||
| 206 | 206 | ||
| 207 | 207 | // CreateLazyDatabase creates a lazy database provider that defers initialization. | |
| 208 | 208 | // The database is initialized on first access, allowing the UI to render faster. | |
| 209 | + | // | |
| 210 | + | // Currently unused: the application uses OpenDatabase with RunParallelDBWebKit for | |
| 211 | + | // eager initialization. This function is kept for potential future use when lazy | |
| 212 | + | // initialization past first paint becomes beneficial. | |
| 209 | 213 | func CreateLazyDatabase() (*sqlite.LazyDB, error) { | |
| 210 | 214 | dbPath, err := config.GetDatabaseFile() | |
| 211 | 215 | if err != nil { | |
| @@ -206,6 +206,10 @@ func OpenDatabase(ctx context.Context) (*DatabaseResult, error) { | |||||
| 206 | 206 | ||||
| 207 | // CreateLazyDatabase creates a lazy database provider that defers initialization. | 207 | // CreateLazyDatabase creates a lazy database provider that defers initialization. | ||
| 208 | // The database is initialized on first access, allowing the UI to render faster. | 208 | // The database is initialized on first access, allowing the UI to render faster. | ||
| 209 | + | // | |||
| 210 | + | // Currently unused: the application uses OpenDatabase with RunParallelDBWebKit for | |||
| 211 | + | // eager initialization. This function is kept for potential future use when lazy | |||
| 212 | + | // initialization past first paint becomes beneficial. | |||
| 209 | func CreateLazyDatabase() (*sqlite.LazyDB, error) { | 213 | func CreateLazyDatabase() (*sqlite.LazyDB, error) { | ||
| 210 | dbPath, err := config.GetDatabaseFile() | 214 | dbPath, err := config.GetDatabaseFile() | ||
| 211 | if err != nil { | 215 | if err != nil { | ||
M internal/bootstrap/session.go +7 -3
| @@ -199,8 +199,9 @@ func unlockAndClose(f *os.File) error { | |||
| 199 | 199 | ||
| 200 | 200 | // runSessionCleanupAsync performs stale session cleanup and old session pruning | |
| 201 | 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 cancelled. | |
| 202 | 203 | func runSessionCleanupAsync( | |
| 203 | - | ctx context.Context, | |
| 204 | + | _ context.Context, | |
| 204 | 205 | sessionUC *usecase.ManageSessionUseCase, | |
| 205 | 206 | cleanupUC *usecase.CleanupSessionsUseCase, | |
| 206 | 207 | cfg *config.Config, | |
| @@ -208,9 +209,12 @@ func runSessionCleanupAsync( | |||
| 208 | 209 | log *zerolog.Logger, | |
| 209 | 210 | ) { | |
| 210 | 211 | go func() { | |
| 212 | + | // Use detached context: cleanup should complete regardless of startup context lifecycle. | |
| 213 | + | bgCtx := context.Background() | |
| 214 | + | ||
| 211 | 215 | // End stale active sessions (orphaned from crashed processes) | |
| 212 | 216 | if lockDir != "" { | |
| 213 | - | if err := endStaleActiveBrowserSessions(ctx, sessionUC, lockDir, log); err != nil { | |
| 217 | + | if err := endStaleActiveBrowserSessions(bgCtx, sessionUC, lockDir, log); err != nil { | |
| 214 | 218 | if log != nil { | |
| 215 | 219 | log.Warn().Err(err).Msg("background: failed to mark stale sessions ended") | |
| 216 | 220 | } | |
| @@ -218,7 +222,7 @@ func runSessionCleanupAsync( | |||
| 218 | 222 | } | |
| 219 | 223 | ||
| 220 | 224 | // Clean up old exited sessions based on config limits | |
| 221 | - | cleanupOutput, err := cleanupUC.Execute(ctx, usecase.CleanupSessionsInput{ | |
| 225 | + | cleanupOutput, err := cleanupUC.Execute(bgCtx, usecase.CleanupSessionsInput{ | |
| 222 | 226 | MaxExitedSessions: cfg.Session.MaxExitedSessions, | |
| 223 | 227 | MaxExitedSessionAgeDays: cfg.Session.MaxExitedSessionAgeDays, | |
| 224 | 228 | }) | |
| @@ -199,8 +199,9 @@ func unlockAndClose(f *os.File) error { | |||||
| 199 | 199 | ||||
| 200 | // runSessionCleanupAsync performs stale session cleanup and old session pruning | 200 | // runSessionCleanupAsync performs stale session cleanup and old session pruning | ||
| 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 cancelled. | |||
| 202 | func runSessionCleanupAsync( | 203 | func runSessionCleanupAsync( | ||
| 203 | - | ctx context.Context, | 204 | + | _ context.Context, |
| 204 | sessionUC *usecase.ManageSessionUseCase, | 205 | sessionUC *usecase.ManageSessionUseCase, | ||
| 205 | cleanupUC *usecase.CleanupSessionsUseCase, | 206 | cleanupUC *usecase.CleanupSessionsUseCase, | ||
| 206 | cfg *config.Config, | 207 | cfg *config.Config, | ||
| @@ -208,9 +209,12 @@ func runSessionCleanupAsync( | |||||
| 208 | log *zerolog.Logger, | 209 | log *zerolog.Logger, | ||
| 209 | ) { | 210 | ) { | ||
| 210 | go func() { | 211 | go func() { | ||
| 212 | + | // Use detached context: cleanup should complete regardless of startup context lifecycle. | |||
| 213 | + | bgCtx := context.Background() | |||
| 214 | + | ||||
| 211 | // End stale active sessions (orphaned from crashed processes) | 215 | // End stale active sessions (orphaned from crashed processes) | ||
| 212 | if lockDir != "" { | 216 | if lockDir != "" { | ||
| 213 | - | if err := endStaleActiveBrowserSessions(ctx, sessionUC, lockDir, log); err != nil { | 217 | + | if err := endStaleActiveBrowserSessions(bgCtx, sessionUC, lockDir, log); err != nil { |
| 214 | if log != nil { | 218 | if log != nil { | ||
| 215 | log.Warn().Err(err).Msg("background: failed to mark stale sessions ended") | 219 | log.Warn().Err(err).Msg("background: failed to mark stale sessions ended") | ||
| 216 | } | 220 | } | ||
| @@ -218,7 +222,7 @@ func runSessionCleanupAsync( | |||||
| 218 | } | 222 | } | ||
| 219 | 223 | ||||
| 220 | // Clean up old exited sessions based on config limits | 224 | // Clean up old exited sessions based on config limits | ||
| 221 | - | cleanupOutput, err := cleanupUC.Execute(ctx, usecase.CleanupSessionsInput{ | 225 | + | cleanupOutput, err := cleanupUC.Execute(bgCtx, usecase.CleanupSessionsInput{ |
| 222 | MaxExitedSessions: cfg.Session.MaxExitedSessions, | 226 | MaxExitedSessions: cfg.Session.MaxExitedSessions, | ||
| 223 | MaxExitedSessionAgeDays: cfg.Session.MaxExitedSessionAgeDays, | 227 | MaxExitedSessionAgeDays: cfg.Session.MaxExitedSessionAgeDays, | ||
| 224 | }) | 228 | }) | ||
M internal/bootstrap/timing.go +3 -0
| @@ -52,7 +52,10 @@ func (t *StartupTimer) MarkDuration(phase string, d time.Duration) { | |||
| 52 | 52 | } | |
| 53 | 53 | ||
| 54 | 54 | // Total returns the total elapsed time since timer creation. | |
| 55 | + | // Thread-safe: uses mutex for memory visibility guarantees. | |
| 55 | 56 | func (t *StartupTimer) Total() time.Duration { | |
| 57 | + | t.mu.Lock() | |
| 58 | + | defer t.mu.Unlock() | |
| 56 | 59 | return time.Since(t.start) | |
| 57 | 60 | } | |
| 58 | 61 | ||
| @@ -52,7 +52,10 @@ func (t *StartupTimer) MarkDuration(phase string, d time.Duration) { | |||||
| 52 | } | 52 | } | ||
| 53 | 53 | ||||
| 54 | // Total returns the total elapsed time since timer creation. | 54 | // Total returns the total elapsed time since timer creation. | ||
| 55 | + | // Thread-safe: uses mutex for memory visibility guarantees. | |||
| 55 | func (t *StartupTimer) Total() time.Duration { | 56 | func (t *StartupTimer) Total() time.Duration { | ||
| 57 | + | t.mu.Lock() | |||
| 58 | + | defer t.mu.Unlock() | |||
| 56 | return time.Since(t.start) | 59 | return time.Since(t.start) | ||
| 57 | } | 60 | } | ||
| 58 | 61 | ||||
M internal/infrastructure/persistence/sqlite/lazy_db.go +5 -0
| @@ -13,6 +13,11 @@ import ( | |||
| 13 | 13 | // LazyDB implements port.DatabaseProvider with lazy initialization. | |
| 14 | 14 | // The database connection is created on first access, deferring the ~300-400ms | |
| 15 | 15 | // WASM compilation and migration overhead until actually needed. | |
| 16 | + | // | |
| 17 | + | // Currently unused: the application uses eager initialization via OpenDatabase | |
| 18 | + | // in RunParallelDBWebKit. This implementation is kept for potential future use | |
| 19 | + | // in scenarios where deferring database initialization past first paint provides | |
| 20 | + | // additional latency benefits, or for CLI commands that may not need DB access. | |
| 16 | 21 | type LazyDB struct { | |
| 17 | 22 | dbPath string | |
| 18 | 23 | db *sql.DB | |
| @@ -13,6 +13,11 @@ import ( | |||||
| 13 | // LazyDB implements port.DatabaseProvider with lazy initialization. | 13 | // LazyDB implements port.DatabaseProvider with lazy initialization. | ||
| 14 | // The database connection is created on first access, deferring the ~300-400ms | 14 | // The database connection is created on first access, deferring the ~300-400ms | ||
| 15 | // WASM compilation and migration overhead until actually needed. | 15 | // WASM compilation and migration overhead until actually needed. | ||
| 16 | + | // | |||
| 17 | + | // Currently unused: the application uses eager initialization via OpenDatabase | |||
| 18 | + | // in RunParallelDBWebKit. This implementation is kept for potential future use | |||
| 19 | + | // in scenarios where deferring database initialization past first paint provides | |||
| 20 | + | // additional latency benefits, or for CLI commands that may not need DB access. | |||
| 16 | type LazyDB struct { | 21 | type LazyDB struct { | ||
| 17 | dbPath string | 22 | dbPath string | ||
| 18 | db *sql.DB | 23 | db *sql.DB | ||
M internal/infrastructure/persistence/sqlite/lazy_repos.go +15 -0
| @@ -1,3 +1,18 @@ | |||
| 1 | + | // Package sqlite provides SQLite implementations of domain repositories. | |
| 2 | + | // | |
| 3 | + | // # Lazy Repository Infrastructure | |
| 4 | + | // | |
| 5 | + | // This file contains lazy-loading repository wrappers that defer database initialization | |
| 6 | + | // until first access. While currently unused (the application uses eager initialization | |
| 7 | + | // via OpenDatabase in RunParallelDBWebKit), this infrastructure is kept for potential | |
| 8 | + | // future optimization scenarios: | |
| 9 | + | // | |
| 10 | + | // - CLI commands that may not need database access | |
| 11 | + | // - Further startup latency reduction by deferring DB init past first paint | |
| 12 | + | // - Testing with mock DatabaseProvider implementations | |
| 13 | + | // | |
| 14 | + | // The lazy wrappers implement the same repository interfaces as their eager counterparts, | |
| 15 | + | // making them drop-in replacements when lazy initialization becomes beneficial. | |
| 1 | 16 | package sqlite | |
| 2 | 17 | ||
| 3 | 18 | import ( | |
| @@ -1,3 +1,18 @@ | |||||
| 1 | + | // Package sqlite provides SQLite implementations of domain repositories. | |||
| 2 | + | // | |||
| 3 | + | // # Lazy Repository Infrastructure | |||
| 4 | + | // | |||
| 5 | + | // This file contains lazy-loading repository wrappers that defer database initialization | |||
| 6 | + | // until first access. While currently unused (the application uses eager initialization | |||
| 7 | + | // via OpenDatabase in RunParallelDBWebKit), this infrastructure is kept for potential | |||
| 8 | + | // future optimization scenarios: | |||
| 9 | + | // | |||
| 10 | + | // - CLI commands that may not need database access | |||
| 11 | + | // - Further startup latency reduction by deferring DB init past first paint | |||
| 12 | + | // - Testing with mock DatabaseProvider implementations | |||
| 13 | + | // | |||
| 14 | + | // The lazy wrappers implement the same repository interfaces as their eager counterparts, | |||
| 15 | + | // making them drop-in replacements when lazy initialization becomes beneficial. | |||
| 1 | package sqlite | 16 | package sqlite | ||
| 2 | 17 | ||||
| 3 | import ( | 18 | import ( | ||