fix: address race condition in LazyDB and add context cancellation in omnibox
- commit
- be07ce6be536189b4a21412d00fe0be719d759ff
- author
- brice · · 2025-12-28 08:31 UTC
- parent
- 24d74a4
- signature
- unsigned
3 files · +52 -16
M internal/bootstrap/session.go +4 -0
| @@ -89,6 +89,10 @@ func StartBrowserSession( | |||
| 89 | 89 | persistErr error | |
| 90 | 90 | ) | |
| 91 | 91 | ||
| 92 | + | // persistFn ensures the session is saved exactly once. Using sync.Once is intentional: | |
| 93 | + | // if the first persist fails, we cache the error and return it on subsequent calls | |
| 94 | + | // rather than retrying. This prevents duplicate session records and ensures consistent | |
| 95 | + | // error handling during shutdown sequences where persistFn may be called multiple times. | |
| 92 | 96 | persistFn := func(persistCtx context.Context) error { | |
| 93 | 97 | persistMu.Do(func() { | |
| 94 | 98 | saveCtx := corelogging.WithContext(context.Background(), logger) | |
| @@ -89,6 +89,10 @@ func StartBrowserSession( | |||||
| 89 | persistErr error | 89 | persistErr error | ||
| 90 | ) | 90 | ) | ||
| 91 | 91 | ||||
| 92 | + | // persistFn ensures the session is saved exactly once. Using sync.Once is intentional: | |||
| 93 | + | // if the first persist fails, we cache the error and return it on subsequent calls | |||
| 94 | + | // rather than retrying. This prevents duplicate session records and ensures consistent | |||
| 95 | + | // error handling during shutdown sequences where persistFn may be called multiple times. | |||
| 92 | persistFn := func(persistCtx context.Context) error { | 96 | persistFn := func(persistCtx context.Context) error { | ||
| 93 | persistMu.Do(func() { | 97 | persistMu.Do(func() { | ||
| 94 | saveCtx := corelogging.WithContext(context.Background(), logger) | 98 | saveCtx := corelogging.WithContext(context.Background(), logger) | ||
M internal/infrastructure/persistence/sqlite/lazy_db.go +18 -6
| @@ -42,18 +42,30 @@ func (l *LazyDB) DB(ctx context.Context) (*sql.DB, error) { | |||
| 42 | 42 | log := logging.FromContext(ctx) | |
| 43 | 43 | log.Debug().Str("path", l.dbPath).Msg("lazy database initialization starting") | |
| 44 | 44 | ||
| 45 | - | l.db, l.err = NewConnection(ctx, l.dbPath) | |
| 46 | - | if l.err != nil { | |
| 47 | - | log.Error().Err(l.err).Msg("lazy database initialization failed") | |
| 45 | + | db, err := NewConnection(ctx, l.dbPath) | |
| 46 | + | ||
| 47 | + | // Acquire lock before assigning to ensure IsInitialized() sees | |
| 48 | + | // a consistent state when reading l.db under RLock. | |
| 49 | + | l.mu.Lock() | |
| 50 | + | l.db = db | |
| 51 | + | l.err = err | |
| 52 | + | l.mu.Unlock() | |
| 53 | + | ||
| 54 | + | if err != nil { | |
| 55 | + | log.Error().Err(err).Msg("lazy database initialization failed") | |
| 48 | 56 | } else { | |
| 49 | 57 | log.Debug().Msg("lazy database initialized successfully") | |
| 50 | 58 | } | |
| 51 | 59 | }) | |
| 52 | 60 | ||
| 53 | - | if l.err != nil { | |
| 54 | - | return nil, fmt.Errorf("database initialization failed: %w", l.err) | |
| 61 | + | l.mu.RLock() | |
| 62 | + | db, err := l.db, l.err | |
| 63 | + | l.mu.RUnlock() | |
| 64 | + | ||
| 65 | + | if err != nil { | |
| 66 | + | return nil, fmt.Errorf("database initialization failed: %w", err) | |
| 55 | 67 | } | |
| 56 | - | return l.db, nil | |
| 68 | + | return db, nil | |
| 57 | 69 | } | |
| 58 | 70 | ||
| 59 | 71 | // Close closes the database connection if it was initialized. | |
| @@ -42,18 +42,30 @@ func (l *LazyDB) DB(ctx context.Context) (*sql.DB, error) { | |||||
| 42 | log := logging.FromContext(ctx) | 42 | log := logging.FromContext(ctx) | ||
| 43 | log.Debug().Str("path", l.dbPath).Msg("lazy database initialization starting") | 43 | log.Debug().Str("path", l.dbPath).Msg("lazy database initialization starting") | ||
| 44 | 44 | ||||
| 45 | - | l.db, l.err = NewConnection(ctx, l.dbPath) | 45 | + | db, err := NewConnection(ctx, l.dbPath) |
| 46 | - | if l.err != nil { | 46 | + | |
| 47 | - | log.Error().Err(l.err).Msg("lazy database initialization failed") | 47 | + | // Acquire lock before assigning to ensure IsInitialized() sees |
| 48 | + | // a consistent state when reading l.db under RLock. | |||
| 49 | + | l.mu.Lock() | |||
| 50 | + | l.db = db | |||
| 51 | + | l.err = err | |||
| 52 | + | l.mu.Unlock() | |||
| 53 | + | ||||
| 54 | + | if err != nil { | |||
| 55 | + | log.Error().Err(err).Msg("lazy database initialization failed") | |||
| 48 | } else { | 56 | } else { | ||
| 49 | log.Debug().Msg("lazy database initialized successfully") | 57 | log.Debug().Msg("lazy database initialized successfully") | ||
| 50 | } | 58 | } | ||
| 51 | }) | 59 | }) | ||
| 52 | 60 | ||||
| 53 | - | if l.err != nil { | 61 | + | l.mu.RLock() |
| 54 | - | return nil, fmt.Errorf("database initialization failed: %w", l.err) | 62 | + | db, err := l.db, l.err |
| 63 | + | l.mu.RUnlock() | |||
| 64 | + | ||||
| 65 | + | if err != nil { | |||
| 66 | + | return nil, fmt.Errorf("database initialization failed: %w", err) | |||
| 55 | } | 67 | } | ||
| 56 | - | return l.db, nil | 68 | + | return db, nil |
| 57 | } | 69 | } | ||
| 58 | 70 | ||||
| 59 | // Close closes the database connection if it was initialized. | 71 | // Close closes the database connection if it was initialized. | ||
M internal/ui/component/omnibox.go +30 -10
| @@ -606,16 +606,26 @@ func (o *Omnibox) performSearch() { | |||
| 606 | 606 | favCh := make(chan map[string]struct{}, 1) | |
| 607 | 607 | ||
| 608 | 608 | go func() { | |
| 609 | - | searchInput := usecase.SearchInput{ | |
| 610 | - | Query: query, | |
| 611 | - | Limit: OmniboxListDefaults.MaxResults, | |
| 609 | + | select { | |
| 610 | + | case <-ctx.Done(): | |
| 611 | + | searchCh <- searchResult{nil, ctx.Err()} | |
| 612 | + | default: | |
| 613 | + | searchInput := usecase.SearchInput{ | |
| 614 | + | Query: query, | |
| 615 | + | Limit: OmniboxListDefaults.MaxResults, | |
| 616 | + | } | |
| 617 | + | output, err := o.historyUC.Search(ctx, searchInput) | |
| 618 | + | searchCh <- searchResult{output, err} | |
| 612 | 619 | } | |
| 613 | - | output, err := o.historyUC.Search(ctx, searchInput) | |
| 614 | - | searchCh <- searchResult{output, err} | |
| 615 | 620 | }() | |
| 616 | 621 | ||
| 617 | 622 | go func() { | |
| 618 | - | favCh <- o.getFavoriteURLs(ctx) | |
| 623 | + | select { | |
| 624 | + | case <-ctx.Done(): | |
| 625 | + | favCh <- nil | |
| 626 | + | default: | |
| 627 | + | favCh <- o.getFavoriteURLs(ctx) | |
| 628 | + | } | |
| 619 | 629 | }() | |
| 620 | 630 | ||
| 621 | 631 | // Wait for both results | |
| @@ -668,13 +678,23 @@ func (o *Omnibox) loadInitialHistory() { | |||
| 668 | 678 | favCh := make(chan map[string]struct{}, 1) | |
| 669 | 679 | ||
| 670 | 680 | go func() { | |
| 671 | - | // TODO: Implement GetMostVisited in use case if needed | |
| 672 | - | results, err := o.historyUC.GetRecent(ctx, OmniboxListDefaults.MaxResults, 0) | |
| 673 | - | historyCh <- historyResult{results, err} | |
| 681 | + | select { | |
| 682 | + | case <-ctx.Done(): | |
| 683 | + | historyCh <- historyResult{nil, ctx.Err()} | |
| 684 | + | default: | |
| 685 | + | // TODO: Implement GetMostVisited in use case if needed | |
| 686 | + | results, err := o.historyUC.GetRecent(ctx, OmniboxListDefaults.MaxResults, 0) | |
| 687 | + | historyCh <- historyResult{results, err} | |
| 688 | + | } | |
| 674 | 689 | }() | |
| 675 | 690 | ||
| 676 | 691 | go func() { | |
| 677 | - | favCh <- o.getFavoriteURLs(ctx) | |
| 692 | + | select { | |
| 693 | + | case <-ctx.Done(): | |
| 694 | + | favCh <- nil | |
| 695 | + | default: | |
| 696 | + | favCh <- o.getFavoriteURLs(ctx) | |
| 697 | + | } | |
| 678 | 698 | }() | |
| 679 | 699 | ||
| 680 | 700 | // Wait for both results | |
| @@ -606,16 +606,26 @@ func (o *Omnibox) performSearch() { | |||||
| 606 | favCh := make(chan map[string]struct{}, 1) | 606 | favCh := make(chan map[string]struct{}, 1) | ||
| 607 | 607 | ||||
| 608 | go func() { | 608 | go func() { | ||
| 609 | - | searchInput := usecase.SearchInput{ | 609 | + | select { |
| 610 | - | Query: query, | 610 | + | case <-ctx.Done(): |
| 611 | - | Limit: OmniboxListDefaults.MaxResults, | 611 | + | searchCh <- searchResult{nil, ctx.Err()} |
| 612 | + | default: | |||
| 613 | + | searchInput := usecase.SearchInput{ | |||
| 614 | + | Query: query, | |||
| 615 | + | Limit: OmniboxListDefaults.MaxResults, | |||
| 616 | + | } | |||
| 617 | + | output, err := o.historyUC.Search(ctx, searchInput) | |||
| 618 | + | searchCh <- searchResult{output, err} | |||
| 612 | } | 619 | } | ||
| 613 | - | output, err := o.historyUC.Search(ctx, searchInput) | |||
| 614 | - | searchCh <- searchResult{output, err} | |||
| 615 | }() | 620 | }() | ||
| 616 | 621 | ||||
| 617 | go func() { | 622 | go func() { | ||
| 618 | - | favCh <- o.getFavoriteURLs(ctx) | 623 | + | select { |
| 624 | + | case <-ctx.Done(): | |||
| 625 | + | favCh <- nil | |||
| 626 | + | default: | |||
| 627 | + | favCh <- o.getFavoriteURLs(ctx) | |||
| 628 | + | } | |||
| 619 | }() | 629 | }() | ||
| 620 | 630 | ||||
| 621 | // Wait for both results | 631 | // Wait for both results | ||
| @@ -668,13 +678,23 @@ func (o *Omnibox) loadInitialHistory() { | |||||
| 668 | favCh := make(chan map[string]struct{}, 1) | 678 | favCh := make(chan map[string]struct{}, 1) | ||
| 669 | 679 | ||||
| 670 | go func() { | 680 | go func() { | ||
| 671 | - | // TODO: Implement GetMostVisited in use case if needed | 681 | + | select { |
| 672 | - | results, err := o.historyUC.GetRecent(ctx, OmniboxListDefaults.MaxResults, 0) | 682 | + | case <-ctx.Done(): |
| 673 | - | historyCh <- historyResult{results, err} | 683 | + | historyCh <- historyResult{nil, ctx.Err()} |
| 684 | + | default: | |||
| 685 | + | // TODO: Implement GetMostVisited in use case if needed | |||
| 686 | + | results, err := o.historyUC.GetRecent(ctx, OmniboxListDefaults.MaxResults, 0) | |||
| 687 | + | historyCh <- historyResult{results, err} | |||
| 688 | + | } | |||
| 674 | }() | 689 | }() | ||
| 675 | 690 | ||||
| 676 | go func() { | 691 | go func() { | ||
| 677 | - | favCh <- o.getFavoriteURLs(ctx) | 692 | + | select { |
| 693 | + | case <-ctx.Done(): | |||
| 694 | + | favCh <- nil | |||
| 695 | + | default: | |||
| 696 | + | favCh <- o.getFavoriteURLs(ctx) | |||
| 697 | + | } | |||
| 678 | }() | 698 | }() | ||
| 679 | 699 | ||||
| 680 | // Wait for both results | 700 | // Wait for both results | ||