feat: implement phase4 tier3 log-noise and fallback hardening
- commit
- bf5b0f61fae13a734997aa61ac1cdcbcaee1f416
- author
- brice · · 2026-02-07 05:44 UTC
- parent
- faa8558
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
10 files · +743 -30
M internal/application/port/updater.go +4 -0
| @@ -3,10 +3,14 @@ package port | |||
| 3 | 3 | ||
| 4 | 4 | import ( | |
| 5 | 5 | "context" | |
| 6 | + | "errors" | |
| 6 | 7 | ||
| 7 | 8 | "github.com/bnema/dumber/internal/domain/entity" | |
| 8 | 9 | ) | |
| 9 | 10 | ||
| 11 | + | // ErrUpdateCheckTransient indicates a temporary update-check failure that should not be treated as actionable. | |
| 12 | + | var ErrUpdateCheckTransient = errors.New("transient update check failure") | |
| 13 | + | ||
| 10 | 14 | // UpdateChecker checks for available updates from a remote source. | |
| 11 | 15 | type UpdateChecker interface { | |
| 12 | 16 | // CheckForUpdate compares the current version with the latest available release. | |
| @@ -3,10 +3,14 @@ package port | |||||
| 3 | 3 | ||||
| 4 | import ( | 4 | import ( | ||
| 5 | "context" | 5 | "context" | ||
| 6 | + | "errors" | |||
| 6 | 7 | ||||
| 7 | "github.com/bnema/dumber/internal/domain/entity" | 8 | "github.com/bnema/dumber/internal/domain/entity" | ||
| 8 | ) | 9 | ) | ||
| 9 | 10 | ||||
| 11 | + | // ErrUpdateCheckTransient indicates a temporary update-check failure that should not be treated as actionable. | |||
| 12 | + | var ErrUpdateCheckTransient = errors.New("transient update check failure") | |||
| 13 | + | ||||
| 10 | // UpdateChecker checks for available updates from a remote source. | 14 | // UpdateChecker checks for available updates from a remote source. | ||
| 11 | type UpdateChecker interface { | 15 | type UpdateChecker interface { | ||
| 12 | // CheckForUpdate compares the current version with the latest available release. | 16 | // CheckForUpdate compares the current version with the latest available release. | ||
M internal/application/usecase/check_update.go +10 -0
| @@ -2,6 +2,7 @@ package usecase | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "errors" | |
| 5 | 6 | ||
| 6 | 7 | "github.com/bnema/dumber/internal/application/port" | |
| 7 | 8 | "github.com/bnema/dumber/internal/domain/build" | |
| @@ -53,6 +54,15 @@ func (uc *CheckUpdateUseCase) Execute(ctx context.Context, _ CheckUpdateInput) ( | |||
| 53 | 54 | ||
| 54 | 55 | info, err := uc.checker.CheckForUpdate(ctx, uc.buildInfo.Version) | |
| 55 | 56 | if err != nil { | |
| 57 | + | if errors.Is(err, port.ErrUpdateCheckTransient) { | |
| 58 | + | log.Debug().Err(err).Msg("transient update check failure") | |
| 59 | + | return &CheckUpdateOutput{ | |
| 60 | + | UpdateAvailable: false, | |
| 61 | + | CanAutoUpdate: false, | |
| 62 | + | CurrentVersion: uc.buildInfo.Version, | |
| 63 | + | LatestVersion: uc.buildInfo.Version, | |
| 64 | + | }, nil | |
| 65 | + | } | |
| 56 | 66 | log.Warn().Err(err).Msg("update check failed") | |
| 57 | 67 | return nil, err | |
| 58 | 68 | } | |
| @@ -2,6 +2,7 @@ package usecase | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "errors" | |||
| 5 | 6 | ||||
| 6 | "github.com/bnema/dumber/internal/application/port" | 7 | "github.com/bnema/dumber/internal/application/port" | ||
| 7 | "github.com/bnema/dumber/internal/domain/build" | 8 | "github.com/bnema/dumber/internal/domain/build" | ||
| @@ -53,6 +54,15 @@ func (uc *CheckUpdateUseCase) Execute(ctx context.Context, _ CheckUpdateInput) ( | |||||
| 53 | 54 | ||||
| 54 | info, err := uc.checker.CheckForUpdate(ctx, uc.buildInfo.Version) | 55 | info, err := uc.checker.CheckForUpdate(ctx, uc.buildInfo.Version) | ||
| 55 | if err != nil { | 56 | if err != nil { | ||
| 57 | + | if errors.Is(err, port.ErrUpdateCheckTransient) { | |||
| 58 | + | log.Debug().Err(err).Msg("transient update check failure") | |||
| 59 | + | return &CheckUpdateOutput{ | |||
| 60 | + | UpdateAvailable: false, | |||
| 61 | + | CanAutoUpdate: false, | |||
| 62 | + | CurrentVersion: uc.buildInfo.Version, | |||
| 63 | + | LatestVersion: uc.buildInfo.Version, | |||
| 64 | + | }, nil | |||
| 65 | + | } | |||
| 56 | log.Warn().Err(err).Msg("update check failed") | 66 | log.Warn().Err(err).Msg("update check failed") | ||
| 57 | return nil, err | 67 | return nil, err | ||
| 58 | } | 68 | } | ||
A internal/application/usecase/check_update_test.go +88 -0
| @@ -0,0 +1,88 @@ | |||
| 1 | + | package usecase | |
| 2 | + | ||
| 3 | + | import ( | |
| 4 | + | "context" | |
| 5 | + | "errors" | |
| 6 | + | "testing" | |
| 7 | + | ||
| 8 | + | "github.com/bnema/dumber/internal/application/port" | |
| 9 | + | "github.com/bnema/dumber/internal/domain/build" | |
| 10 | + | "github.com/bnema/dumber/internal/domain/entity" | |
| 11 | + | ) | |
| 12 | + | ||
| 13 | + | func TestCheckUpdateUseCase_TransientErrorReturnsNoUpdate(t *testing.T) { | |
| 14 | + | uc := NewCheckUpdateUseCase( | |
| 15 | + | stubUpdateChecker{err: port.ErrUpdateCheckTransient}, | |
| 16 | + | stubUpdateApplier{}, | |
| 17 | + | build.Info{Version: "1.2.3"}, | |
| 18 | + | ) | |
| 19 | + | ||
| 20 | + | out, err := uc.Execute(context.Background(), CheckUpdateInput{}) | |
| 21 | + | if err != nil { | |
| 22 | + | t.Fatalf("Execute() unexpected error: %v", err) | |
| 23 | + | } | |
| 24 | + | if out.UpdateAvailable { | |
| 25 | + | t.Fatal("UpdateAvailable = true, want false") | |
| 26 | + | } | |
| 27 | + | if out.CurrentVersion != "1.2.3" || out.LatestVersion != "1.2.3" { | |
| 28 | + | t.Fatalf("unexpected versions: current=%q latest=%q", out.CurrentVersion, out.LatestVersion) | |
| 29 | + | } | |
| 30 | + | } | |
| 31 | + | ||
| 32 | + | func TestCheckUpdateUseCase_NonTransientErrorPropagates(t *testing.T) { | |
| 33 | + | uc := NewCheckUpdateUseCase( | |
| 34 | + | stubUpdateChecker{err: errors.New("boom")}, | |
| 35 | + | stubUpdateApplier{}, | |
| 36 | + | build.Info{Version: "1.2.3"}, | |
| 37 | + | ) | |
| 38 | + | ||
| 39 | + | _, err := uc.Execute(context.Background(), CheckUpdateInput{}) | |
| 40 | + | if err == nil { | |
| 41 | + | t.Fatal("expected error, got nil") | |
| 42 | + | } | |
| 43 | + | if err.Error() != "boom" { | |
| 44 | + | t.Fatalf("unexpected error: %v", err) | |
| 45 | + | } | |
| 46 | + | } | |
| 47 | + | ||
| 48 | + | type stubUpdateChecker struct { | |
| 49 | + | info *entity.UpdateInfo | |
| 50 | + | err error | |
| 51 | + | } | |
| 52 | + | ||
| 53 | + | func (s stubUpdateChecker) CheckForUpdate(context.Context, string) (*entity.UpdateInfo, error) { | |
| 54 | + | if s.err != nil { | |
| 55 | + | return nil, s.err | |
| 56 | + | } | |
| 57 | + | return s.info, nil | |
| 58 | + | } | |
| 59 | + | ||
| 60 | + | type stubUpdateApplier struct{} | |
| 61 | + | ||
| 62 | + | func (stubUpdateApplier) CanSelfUpdate(context.Context) bool { | |
| 63 | + | return false | |
| 64 | + | } | |
| 65 | + | ||
| 66 | + | func (stubUpdateApplier) SelfUpdateBlockedReason(context.Context) port.SelfUpdateBlockedReason { | |
| 67 | + | return port.SelfUpdateBlockedNotWritable | |
| 68 | + | } | |
| 69 | + | ||
| 70 | + | func (stubUpdateApplier) GetBinaryPath() (string, error) { | |
| 71 | + | return "", nil | |
| 72 | + | } | |
| 73 | + | ||
| 74 | + | func (stubUpdateApplier) StageUpdate(context.Context, string) error { | |
| 75 | + | return nil | |
| 76 | + | } | |
| 77 | + | ||
| 78 | + | func (stubUpdateApplier) HasStagedUpdate(context.Context) bool { | |
| 79 | + | return false | |
| 80 | + | } | |
| 81 | + | ||
| 82 | + | func (stubUpdateApplier) ApplyOnExit(context.Context) (string, error) { | |
| 83 | + | return "", nil | |
| 84 | + | } | |
| 85 | + | ||
| 86 | + | func (stubUpdateApplier) ClearStagedUpdate(context.Context) error { | |
| 87 | + | return nil | |
| 88 | + | } | |
| @@ -0,0 +1,88 @@ | |||||
| 1 | + | package usecase | |||
| 2 | + | ||||
| 3 | + | import ( | |||
| 4 | + | "context" | |||
| 5 | + | "errors" | |||
| 6 | + | "testing" | |||
| 7 | + | ||||
| 8 | + | "github.com/bnema/dumber/internal/application/port" | |||
| 9 | + | "github.com/bnema/dumber/internal/domain/build" | |||
| 10 | + | "github.com/bnema/dumber/internal/domain/entity" | |||
| 11 | + | ) | |||
| 12 | + | ||||
| 13 | + | func TestCheckUpdateUseCase_TransientErrorReturnsNoUpdate(t *testing.T) { | |||
| 14 | + | uc := NewCheckUpdateUseCase( | |||
| 15 | + | stubUpdateChecker{err: port.ErrUpdateCheckTransient}, | |||
| 16 | + | stubUpdateApplier{}, | |||
| 17 | + | build.Info{Version: "1.2.3"}, | |||
| 18 | + | ) | |||
| 19 | + | ||||
| 20 | + | out, err := uc.Execute(context.Background(), CheckUpdateInput{}) | |||
| 21 | + | if err != nil { | |||
| 22 | + | t.Fatalf("Execute() unexpected error: %v", err) | |||
| 23 | + | } | |||
| 24 | + | if out.UpdateAvailable { | |||
| 25 | + | t.Fatal("UpdateAvailable = true, want false") | |||
| 26 | + | } | |||
| 27 | + | if out.CurrentVersion != "1.2.3" || out.LatestVersion != "1.2.3" { | |||
| 28 | + | t.Fatalf("unexpected versions: current=%q latest=%q", out.CurrentVersion, out.LatestVersion) | |||
| 29 | + | } | |||
| 30 | + | } | |||
| 31 | + | ||||
| 32 | + | func TestCheckUpdateUseCase_NonTransientErrorPropagates(t *testing.T) { | |||
| 33 | + | uc := NewCheckUpdateUseCase( | |||
| 34 | + | stubUpdateChecker{err: errors.New("boom")}, | |||
| 35 | + | stubUpdateApplier{}, | |||
| 36 | + | build.Info{Version: "1.2.3"}, | |||
| 37 | + | ) | |||
| 38 | + | ||||
| 39 | + | _, err := uc.Execute(context.Background(), CheckUpdateInput{}) | |||
| 40 | + | if err == nil { | |||
| 41 | + | t.Fatal("expected error, got nil") | |||
| 42 | + | } | |||
| 43 | + | if err.Error() != "boom" { | |||
| 44 | + | t.Fatalf("unexpected error: %v", err) | |||
| 45 | + | } | |||
| 46 | + | } | |||
| 47 | + | ||||
| 48 | + | type stubUpdateChecker struct { | |||
| 49 | + | info *entity.UpdateInfo | |||
| 50 | + | err error | |||
| 51 | + | } | |||
| 52 | + | ||||
| 53 | + | func (s stubUpdateChecker) CheckForUpdate(context.Context, string) (*entity.UpdateInfo, error) { | |||
| 54 | + | if s.err != nil { | |||
| 55 | + | return nil, s.err | |||
| 56 | + | } | |||
| 57 | + | return s.info, nil | |||
| 58 | + | } | |||
| 59 | + | ||||
| 60 | + | type stubUpdateApplier struct{} | |||
| 61 | + | ||||
| 62 | + | func (stubUpdateApplier) CanSelfUpdate(context.Context) bool { | |||
| 63 | + | return false | |||
| 64 | + | } | |||
| 65 | + | ||||
| 66 | + | func (stubUpdateApplier) SelfUpdateBlockedReason(context.Context) port.SelfUpdateBlockedReason { | |||
| 67 | + | return port.SelfUpdateBlockedNotWritable | |||
| 68 | + | } | |||
| 69 | + | ||||
| 70 | + | func (stubUpdateApplier) GetBinaryPath() (string, error) { | |||
| 71 | + | return "", nil | |||
| 72 | + | } | |||
| 73 | + | ||||
| 74 | + | func (stubUpdateApplier) StageUpdate(context.Context, string) error { | |||
| 75 | + | return nil | |||
| 76 | + | } | |||
| 77 | + | ||||
| 78 | + | func (stubUpdateApplier) HasStagedUpdate(context.Context) bool { | |||
| 79 | + | return false | |||
| 80 | + | } | |||
| 81 | + | ||||
| 82 | + | func (stubUpdateApplier) ApplyOnExit(context.Context) (string, error) { | |||
| 83 | + | return "", nil | |||
| 84 | + | } | |||
| 85 | + | ||||
| 86 | + | func (stubUpdateApplier) ClearStagedUpdate(context.Context) error { | |||
| 87 | + | return nil | |||
| 88 | + | } | |||
M internal/infrastructure/updater/github.go +146 -7
| @@ -12,6 +12,8 @@ import ( | |||
| 12 | 12 | "errors" | |
| 13 | 13 | "fmt" | |
| 14 | 14 | "io" | |
| 15 | + | "math/rand" | |
| 16 | + | "net" | |
| 15 | 17 | "net/http" | |
| 16 | 18 | "net/url" | |
| 17 | 19 | "os" | |
| @@ -22,6 +24,7 @@ import ( | |||
| 22 | 24 | "strings" | |
| 23 | 25 | "time" | |
| 24 | 26 | ||
| 27 | + | "github.com/bnema/dumber/internal/application/port" | |
| 25 | 28 | "github.com/bnema/dumber/internal/domain/entity" | |
| 26 | 29 | "github.com/bnema/dumber/internal/infrastructure/env" | |
| 27 | 30 | "github.com/bnema/dumber/internal/logging" | |
| @@ -58,6 +61,18 @@ const ( | |||
| 58 | 61 | ||
| 59 | 62 | // Allowed GitHub download host. | |
| 60 | 63 | allowedDownloadHost = "github.com" | |
| 64 | + | ||
| 65 | + | // Maximum number of attempts for retryable network requests. | |
| 66 | + | maxRetryAttempts = 3 | |
| 67 | + | ||
| 68 | + | // Base delay used for exponential backoff between retries. | |
| 69 | + | retryBaseDelay = 250 * time.Millisecond | |
| 70 | + | ||
| 71 | + | // Maximum delay cap for exponential backoff between retries. | |
| 72 | + | retryMaxDelay = 2 * time.Second | |
| 73 | + | ||
| 74 | + | // Max random jitter added to each retry backoff. | |
| 75 | + | retryJitterMax = 200 * time.Millisecond | |
| 61 | 76 | ) | |
| 62 | 77 | ||
| 63 | 78 | // githubRelease represents the GitHub API response for a release. | |
| @@ -70,13 +85,17 @@ type githubRelease struct { | |||
| 70 | 85 | ||
| 71 | 86 | // GitHubChecker implements UpdateChecker using the GitHub API. | |
| 72 | 87 | type GitHubChecker struct { | |
| 73 | - | client *http.Client | |
| 88 | + | client *http.Client | |
| 89 | + | randInt63 func(n int64) int64 | |
| 90 | + | sleep func(ctx context.Context, d time.Duration) error | |
| 74 | 91 | } | |
| 75 | 92 | ||
| 76 | 93 | // NewGitHubChecker creates a new GitHub-based update checker. | |
| 77 | 94 | func NewGitHubChecker() *GitHubChecker { | |
| 78 | 95 | return &GitHubChecker{ | |
| 79 | - | client: &http.Client{Timeout: apiTimeout}, | |
| 96 | + | client: &http.Client{Timeout: apiTimeout}, | |
| 97 | + | randInt63: rand.Int63n, | |
| 98 | + | sleep: waitForBackoff, | |
| 80 | 99 | } | |
| 81 | 100 | } | |
| 82 | 101 | ||
| @@ -115,13 +134,21 @@ func (g *GitHubChecker) CheckForUpdate(ctx context.Context, currentVersion strin | |||
| 115 | 134 | req.Header.Set("Accept", "application/vnd.github.v3+json") | |
| 116 | 135 | req.Header.Set("User-Agent", "dumber-browser/"+currentVersion) | |
| 117 | 136 | ||
| 118 | - | resp, err := g.client.Do(req) | |
| 137 | + | resp, err := g.doRequestWithRetry(ctx, req) | |
| 119 | 138 | if err != nil { | |
| 139 | + | if isRetryableRequestError(err) { | |
| 140 | + | log.Debug().Err(err).Msg("transient update check failure; skipping check") | |
| 141 | + | return nil, fmt.Errorf("%w: %w", port.ErrUpdateCheckTransient, err) | |
| 142 | + | } | |
| 120 | 143 | return nil, fmt.Errorf("failed to fetch latest release: %w", err) | |
| 121 | 144 | } | |
| 122 | 145 | defer func() { _ = resp.Body.Close() }() | |
| 123 | 146 | ||
| 124 | 147 | if resp.StatusCode != http.StatusOK { | |
| 148 | + | if isRetryableStatus(resp.StatusCode) { | |
| 149 | + | log.Debug().Int("status", resp.StatusCode).Msg("transient update check API status; skipping check") | |
| 150 | + | return nil, fmt.Errorf("%w: github API returned status %d", port.ErrUpdateCheckTransient, resp.StatusCode) | |
| 151 | + | } | |
| 125 | 152 | return nil, fmt.Errorf("github API returned status %d", resp.StatusCode) | |
| 126 | 153 | } | |
| 127 | 154 | ||
| @@ -214,14 +241,100 @@ func compareVersions(v1, v2 string) int { | |||
| 214 | 241 | ||
| 215 | 242 | // GitHubDownloader implements UpdateDownloader for GitHub releases. | |
| 216 | 243 | type GitHubDownloader struct { | |
| 217 | - | client *http.Client | |
| 244 | + | client *http.Client | |
| 245 | + | randInt63 func(n int64) int64 | |
| 246 | + | sleep func(ctx context.Context, d time.Duration) error | |
| 218 | 247 | } | |
| 219 | 248 | ||
| 220 | 249 | // NewGitHubDownloader creates a new GitHub release downloader. | |
| 221 | 250 | func NewGitHubDownloader() *GitHubDownloader { | |
| 222 | 251 | return &GitHubDownloader{ | |
| 223 | - | client: &http.Client{Timeout: downloadTimeout}, | |
| 252 | + | client: &http.Client{Timeout: downloadTimeout}, | |
| 253 | + | randInt63: rand.Int63n, | |
| 254 | + | sleep: waitForBackoff, | |
| 255 | + | } | |
| 256 | + | } | |
| 257 | + | ||
| 258 | + | func isRetryableStatus(status int) bool { | |
| 259 | + | if status == http.StatusForbidden || status == http.StatusTooManyRequests || status == http.StatusRequestTimeout { | |
| 260 | + | return true | |
| 261 | + | } | |
| 262 | + | return status >= http.StatusInternalServerError | |
| 263 | + | } | |
| 264 | + | ||
| 265 | + | func isRetryableRequestError(err error) bool { | |
| 266 | + | if err == nil { | |
| 267 | + | return false | |
| 268 | + | } | |
| 269 | + | if errors.Is(err, context.Canceled) { | |
| 270 | + | return false | |
| 271 | + | } | |
| 272 | + | if errors.Is(err, context.DeadlineExceeded) { | |
| 273 | + | return true | |
| 274 | + | } | |
| 275 | + | var netErr net.Error | |
| 276 | + | return errors.As(err, &netErr) && netErr.Timeout() | |
| 277 | + | } | |
| 278 | + | ||
| 279 | + | func waitForBackoff(ctx context.Context, d time.Duration) error { | |
| 280 | + | timer := time.NewTimer(d) | |
| 281 | + | defer timer.Stop() | |
| 282 | + | ||
| 283 | + | select { | |
| 284 | + | case <-ctx.Done(): | |
| 285 | + | return ctx.Err() | |
| 286 | + | case <-timer.C: | |
| 287 | + | return nil | |
| 288 | + | } | |
| 289 | + | } | |
| 290 | + | ||
| 291 | + | func retryDelayForAttempt(attempt int, randInt63 func(n int64) int64) time.Duration { | |
| 292 | + | if attempt < 1 { | |
| 293 | + | attempt = 1 | |
| 294 | + | } | |
| 295 | + | delay := retryBaseDelay | |
| 296 | + | for i := 1; i < attempt; i++ { | |
| 297 | + | if delay >= retryMaxDelay { | |
| 298 | + | delay = retryMaxDelay | |
| 299 | + | break | |
| 300 | + | } | |
| 301 | + | delay *= 2 | |
| 302 | + | if delay > retryMaxDelay { | |
| 303 | + | delay = retryMaxDelay | |
| 304 | + | } | |
| 305 | + | } | |
| 306 | + | ||
| 307 | + | if randInt63 != nil && retryJitterMax > 0 { | |
| 308 | + | delay += time.Duration(randInt63(int64(retryJitterMax) + 1)) | |
| 224 | 309 | } | |
| 310 | + | ||
| 311 | + | return delay | |
| 312 | + | } | |
| 313 | + | ||
| 314 | + | func (g *GitHubChecker) doRequestWithRetry(ctx context.Context, req *http.Request) (*http.Response, error) { | |
| 315 | + | for attempt := 1; attempt <= maxRetryAttempts; attempt++ { | |
| 316 | + | resp, err := g.client.Do(req) | |
| 317 | + | if err != nil { | |
| 318 | + | if !isRetryableRequestError(err) || attempt == maxRetryAttempts { | |
| 319 | + | return nil, err | |
| 320 | + | } | |
| 321 | + | if waitErr := g.sleep(ctx, retryDelayForAttempt(attempt, g.randInt63)); waitErr != nil { | |
| 322 | + | return nil, waitErr | |
| 323 | + | } | |
| 324 | + | continue | |
| 325 | + | } | |
| 326 | + | ||
| 327 | + | if !isRetryableStatus(resp.StatusCode) || attempt == maxRetryAttempts { | |
| 328 | + | return resp, nil | |
| 329 | + | } | |
| 330 | + | ||
| 331 | + | _ = resp.Body.Close() | |
| 332 | + | if waitErr := g.sleep(ctx, retryDelayForAttempt(attempt, g.randInt63)); waitErr != nil { | |
| 333 | + | return nil, waitErr | |
| 334 | + | } | |
| 335 | + | } | |
| 336 | + | ||
| 337 | + | return nil, fmt.Errorf("request failed after retries") | |
| 225 | 338 | } | |
| 226 | 339 | ||
| 227 | 340 | // validateDownloadURL ensures the URL is a valid GitHub releases URL. | |
| @@ -257,7 +370,7 @@ func (g *GitHubDownloader) fetchChecksums(ctx context.Context) (map[string]strin | |||
| 257 | 370 | } | |
| 258 | 371 | req.Header.Set("User-Agent", "dumber-browser") | |
| 259 | 372 | ||
| 260 | - | resp, err := g.client.Do(req) | |
| 373 | + | resp, err := g.doRequestWithRetry(ctx, req) | |
| 261 | 374 | if err != nil { | |
| 262 | 375 | return nil, fmt.Errorf("failed to fetch checksums: %w", err) | |
| 263 | 376 | } | |
| @@ -362,7 +475,7 @@ func (g *GitHubDownloader) Download(ctx context.Context, downloadURL, destDir st | |||
| 362 | 475 | ||
| 363 | 476 | log.Debug().Str("url", downloadURL).Msg("downloading update") | |
| 364 | 477 | ||
| 365 | - | resp, err := g.client.Do(req) | |
| 478 | + | resp, err := g.doRequestWithRetry(ctx, req) | |
| 366 | 479 | if err != nil { | |
| 367 | 480 | return "", fmt.Errorf("failed to download: %w", err) | |
| 368 | 481 | } | |
| @@ -423,6 +536,32 @@ func (g *GitHubDownloader) Download(ctx context.Context, downloadURL, destDir st | |||
| 423 | 536 | return archivePath, nil | |
| 424 | 537 | } | |
| 425 | 538 | ||
| 539 | + | func (g *GitHubDownloader) doRequestWithRetry(ctx context.Context, req *http.Request) (*http.Response, error) { | |
| 540 | + | for attempt := 1; attempt <= maxRetryAttempts; attempt++ { | |
| 541 | + | resp, err := g.client.Do(req) | |
| 542 | + | if err != nil { | |
| 543 | + | if !isRetryableRequestError(err) || attempt == maxRetryAttempts { | |
| 544 | + | return nil, err | |
| 545 | + | } | |
| 546 | + | if waitErr := g.sleep(ctx, retryDelayForAttempt(attempt, g.randInt63)); waitErr != nil { | |
| 547 | + | return nil, waitErr | |
| 548 | + | } | |
| 549 | + | continue | |
| 550 | + | } | |
| 551 | + | ||
| 552 | + | if !isRetryableStatus(resp.StatusCode) || attempt == maxRetryAttempts { | |
| 553 | + | return resp, nil | |
| 554 | + | } | |
| 555 | + | ||
| 556 | + | _ = resp.Body.Close() | |
| 557 | + | if waitErr := g.sleep(ctx, retryDelayForAttempt(attempt, g.randInt63)); waitErr != nil { | |
| 558 | + | return nil, waitErr | |
| 559 | + | } | |
| 560 | + | } | |
| 561 | + | ||
| 562 | + | return nil, fmt.Errorf("request failed after retries") | |
| 563 | + | } | |
| 564 | + | ||
| 426 | 565 | // sanitizeTarPath validates and sanitizes a tar header name to prevent path traversal. | |
| 427 | 566 | func sanitizeTarPath(name, destDir string) (string, error) { | |
| 428 | 567 | // Clean the path to remove any . or .. components. | |
| @@ -12,6 +12,8 @@ import ( | |||||
| 12 | "errors" | 12 | "errors" | ||
| 13 | "fmt" | 13 | "fmt" | ||
| 14 | "io" | 14 | "io" | ||
| 15 | + | "math/rand" | |||
| 16 | + | "net" | |||
| 15 | "net/http" | 17 | "net/http" | ||
| 16 | "net/url" | 18 | "net/url" | ||
| 17 | "os" | 19 | "os" | ||
| @@ -22,6 +24,7 @@ import ( | |||||
| 22 | "strings" | 24 | "strings" | ||
| 23 | "time" | 25 | "time" | ||
| 24 | 26 | ||||
| 27 | + | "github.com/bnema/dumber/internal/application/port" | |||
| 25 | "github.com/bnema/dumber/internal/domain/entity" | 28 | "github.com/bnema/dumber/internal/domain/entity" | ||
| 26 | "github.com/bnema/dumber/internal/infrastructure/env" | 29 | "github.com/bnema/dumber/internal/infrastructure/env" | ||
| 27 | "github.com/bnema/dumber/internal/logging" | 30 | "github.com/bnema/dumber/internal/logging" | ||
| @@ -58,6 +61,18 @@ const ( | |||||
| 58 | 61 | ||||
| 59 | // Allowed GitHub download host. | 62 | // Allowed GitHub download host. | ||
| 60 | allowedDownloadHost = "github.com" | 63 | allowedDownloadHost = "github.com" | ||
| 64 | + | ||||
| 65 | + | // Maximum number of attempts for retryable network requests. | |||
| 66 | + | maxRetryAttempts = 3 | |||
| 67 | + | ||||
| 68 | + | // Base delay used for exponential backoff between retries. | |||
| 69 | + | retryBaseDelay = 250 * time.Millisecond | |||
| 70 | + | ||||
| 71 | + | // Maximum delay cap for exponential backoff between retries. | |||
| 72 | + | retryMaxDelay = 2 * time.Second | |||
| 73 | + | ||||
| 74 | + | // Max random jitter added to each retry backoff. | |||
| 75 | + | retryJitterMax = 200 * time.Millisecond | |||
| 61 | ) | 76 | ) | ||
| 62 | 77 | ||||
| 63 | // githubRelease represents the GitHub API response for a release. | 78 | // githubRelease represents the GitHub API response for a release. | ||
| @@ -70,13 +85,17 @@ type githubRelease struct { | |||||
| 70 | 85 | ||||
| 71 | // GitHubChecker implements UpdateChecker using the GitHub API. | 86 | // GitHubChecker implements UpdateChecker using the GitHub API. | ||
| 72 | type GitHubChecker struct { | 87 | type GitHubChecker struct { | ||
| 73 | - | client *http.Client | 88 | + | client *http.Client |
| 89 | + | randInt63 func(n int64) int64 | |||
| 90 | + | sleep func(ctx context.Context, d time.Duration) error | |||
| 74 | } | 91 | } | ||
| 75 | 92 | ||||
| 76 | // NewGitHubChecker creates a new GitHub-based update checker. | 93 | // NewGitHubChecker creates a new GitHub-based update checker. | ||
| 77 | func NewGitHubChecker() *GitHubChecker { | 94 | func NewGitHubChecker() *GitHubChecker { | ||
| 78 | return &GitHubChecker{ | 95 | return &GitHubChecker{ | ||
| 79 | - | client: &http.Client{Timeout: apiTimeout}, | 96 | + | client: &http.Client{Timeout: apiTimeout}, |
| 97 | + | randInt63: rand.Int63n, | |||
| 98 | + | sleep: waitForBackoff, | |||
| 80 | } | 99 | } | ||
| 81 | } | 100 | } | ||
| 82 | 101 | ||||
| @@ -115,13 +134,21 @@ func (g *GitHubChecker) CheckForUpdate(ctx context.Context, currentVersion strin | |||||
| 115 | req.Header.Set("Accept", "application/vnd.github.v3+json") | 134 | req.Header.Set("Accept", "application/vnd.github.v3+json") | ||
| 116 | req.Header.Set("User-Agent", "dumber-browser/"+currentVersion) | 135 | req.Header.Set("User-Agent", "dumber-browser/"+currentVersion) | ||
| 117 | 136 | ||||
| 118 | - | resp, err := g.client.Do(req) | 137 | + | resp, err := g.doRequestWithRetry(ctx, req) |
| 119 | if err != nil { | 138 | if err != nil { | ||
| 139 | + | if isRetryableRequestError(err) { | |||
| 140 | + | log.Debug().Err(err).Msg("transient update check failure; skipping check") | |||
| 141 | + | return nil, fmt.Errorf("%w: %w", port.ErrUpdateCheckTransient, err) | |||
| 142 | + | } | |||
| 120 | return nil, fmt.Errorf("failed to fetch latest release: %w", err) | 143 | return nil, fmt.Errorf("failed to fetch latest release: %w", err) | ||
| 121 | } | 144 | } | ||
| 122 | defer func() { _ = resp.Body.Close() }() | 145 | defer func() { _ = resp.Body.Close() }() | ||
| 123 | 146 | ||||
| 124 | if resp.StatusCode != http.StatusOK { | 147 | if resp.StatusCode != http.StatusOK { | ||
| 148 | + | if isRetryableStatus(resp.StatusCode) { | |||
| 149 | + | log.Debug().Int("status", resp.StatusCode).Msg("transient update check API status; skipping check") | |||
| 150 | + | return nil, fmt.Errorf("%w: github API returned status %d", port.ErrUpdateCheckTransient, resp.StatusCode) | |||
| 151 | + | } | |||
| 125 | return nil, fmt.Errorf("github API returned status %d", resp.StatusCode) | 152 | return nil, fmt.Errorf("github API returned status %d", resp.StatusCode) | ||
| 126 | } | 153 | } | ||
| 127 | 154 | ||||
| @@ -214,14 +241,100 @@ func compareVersions(v1, v2 string) int { | |||||
| 214 | 241 | ||||
| 215 | // GitHubDownloader implements UpdateDownloader for GitHub releases. | 242 | // GitHubDownloader implements UpdateDownloader for GitHub releases. | ||
| 216 | type GitHubDownloader struct { | 243 | type GitHubDownloader struct { | ||
| 217 | - | client *http.Client | 244 | + | client *http.Client |
| 245 | + | randInt63 func(n int64) int64 | |||
| 246 | + | sleep func(ctx context.Context, d time.Duration) error | |||
| 218 | } | 247 | } | ||
| 219 | 248 | ||||
| 220 | // NewGitHubDownloader creates a new GitHub release downloader. | 249 | // NewGitHubDownloader creates a new GitHub release downloader. | ||
| 221 | func NewGitHubDownloader() *GitHubDownloader { | 250 | func NewGitHubDownloader() *GitHubDownloader { | ||
| 222 | return &GitHubDownloader{ | 251 | return &GitHubDownloader{ | ||
| 223 | - | client: &http.Client{Timeout: downloadTimeout}, | 252 | + | client: &http.Client{Timeout: downloadTimeout}, |
| 253 | + | randInt63: rand.Int63n, | |||
| 254 | + | sleep: waitForBackoff, | |||
| 255 | + | } | |||
| 256 | + | } | |||
| 257 | + | ||||
| 258 | + | func isRetryableStatus(status int) bool { | |||
| 259 | + | if status == http.StatusForbidden || status == http.StatusTooManyRequests || status == http.StatusRequestTimeout { | |||
| 260 | + | return true | |||
| 261 | + | } | |||
| 262 | + | return status >= http.StatusInternalServerError | |||
| 263 | + | } | |||
| 264 | + | ||||
| 265 | + | func isRetryableRequestError(err error) bool { | |||
| 266 | + | if err == nil { | |||
| 267 | + | return false | |||
| 268 | + | } | |||
| 269 | + | if errors.Is(err, context.Canceled) { | |||
| 270 | + | return false | |||
| 271 | + | } | |||
| 272 | + | if errors.Is(err, context.DeadlineExceeded) { | |||
| 273 | + | return true | |||
| 274 | + | } | |||
| 275 | + | var netErr net.Error | |||
| 276 | + | return errors.As(err, &netErr) && netErr.Timeout() | |||
| 277 | + | } | |||
| 278 | + | ||||
| 279 | + | func waitForBackoff(ctx context.Context, d time.Duration) error { | |||
| 280 | + | timer := time.NewTimer(d) | |||
| 281 | + | defer timer.Stop() | |||
| 282 | + | ||||
| 283 | + | select { | |||
| 284 | + | case <-ctx.Done(): | |||
| 285 | + | return ctx.Err() | |||
| 286 | + | case <-timer.C: | |||
| 287 | + | return nil | |||
| 288 | + | } | |||
| 289 | + | } | |||
| 290 | + | ||||
| 291 | + | func retryDelayForAttempt(attempt int, randInt63 func(n int64) int64) time.Duration { | |||
| 292 | + | if attempt < 1 { | |||
| 293 | + | attempt = 1 | |||
| 294 | + | } | |||
| 295 | + | delay := retryBaseDelay | |||
| 296 | + | for i := 1; i < attempt; i++ { | |||
| 297 | + | if delay >= retryMaxDelay { | |||
| 298 | + | delay = retryMaxDelay | |||
| 299 | + | break | |||
| 300 | + | } | |||
| 301 | + | delay *= 2 | |||
| 302 | + | if delay > retryMaxDelay { | |||
| 303 | + | delay = retryMaxDelay | |||
| 304 | + | } | |||
| 305 | + | } | |||
| 306 | + | ||||
| 307 | + | if randInt63 != nil && retryJitterMax > 0 { | |||
| 308 | + | delay += time.Duration(randInt63(int64(retryJitterMax) + 1)) | |||
| 224 | } | 309 | } | ||
| 310 | + | ||||
| 311 | + | return delay | |||
| 312 | + | } | |||
| 313 | + | ||||
| 314 | + | func (g *GitHubChecker) doRequestWithRetry(ctx context.Context, req *http.Request) (*http.Response, error) { | |||
| 315 | + | for attempt := 1; attempt <= maxRetryAttempts; attempt++ { | |||
| 316 | + | resp, err := g.client.Do(req) | |||
| 317 | + | if err != nil { | |||
| 318 | + | if !isRetryableRequestError(err) || attempt == maxRetryAttempts { | |||
| 319 | + | return nil, err | |||
| 320 | + | } | |||
| 321 | + | if waitErr := g.sleep(ctx, retryDelayForAttempt(attempt, g.randInt63)); waitErr != nil { | |||
| 322 | + | return nil, waitErr | |||
| 323 | + | } | |||
| 324 | + | continue | |||
| 325 | + | } | |||
| 326 | + | ||||
| 327 | + | if !isRetryableStatus(resp.StatusCode) || attempt == maxRetryAttempts { | |||
| 328 | + | return resp, nil | |||
| 329 | + | } | |||
| 330 | + | ||||
| 331 | + | _ = resp.Body.Close() | |||
| 332 | + | if waitErr := g.sleep(ctx, retryDelayForAttempt(attempt, g.randInt63)); waitErr != nil { | |||
| 333 | + | return nil, waitErr | |||
| 334 | + | } | |||
| 335 | + | } | |||
| 336 | + | ||||
| 337 | + | return nil, fmt.Errorf("request failed after retries") | |||
| 225 | } | 338 | } | ||
| 226 | 339 | ||||
| 227 | // validateDownloadURL ensures the URL is a valid GitHub releases URL. | 340 | // validateDownloadURL ensures the URL is a valid GitHub releases URL. | ||
| @@ -257,7 +370,7 @@ func (g *GitHubDownloader) fetchChecksums(ctx context.Context) (map[string]strin | |||||
| 257 | } | 370 | } | ||
| 258 | req.Header.Set("User-Agent", "dumber-browser") | 371 | req.Header.Set("User-Agent", "dumber-browser") | ||
| 259 | 372 | ||||
| 260 | - | resp, err := g.client.Do(req) | 373 | + | resp, err := g.doRequestWithRetry(ctx, req) |
| 261 | if err != nil { | 374 | if err != nil { | ||
| 262 | return nil, fmt.Errorf("failed to fetch checksums: %w", err) | 375 | return nil, fmt.Errorf("failed to fetch checksums: %w", err) | ||
| 263 | } | 376 | } | ||
| @@ -362,7 +475,7 @@ func (g *GitHubDownloader) Download(ctx context.Context, downloadURL, destDir st | |||||
| 362 | 475 | ||||
| 363 | log.Debug().Str("url", downloadURL).Msg("downloading update") | 476 | log.Debug().Str("url", downloadURL).Msg("downloading update") | ||
| 364 | 477 | ||||
| 365 | - | resp, err := g.client.Do(req) | 478 | + | resp, err := g.doRequestWithRetry(ctx, req) |
| 366 | if err != nil { | 479 | if err != nil { | ||
| 367 | return "", fmt.Errorf("failed to download: %w", err) | 480 | return "", fmt.Errorf("failed to download: %w", err) | ||
| 368 | } | 481 | } | ||
| @@ -423,6 +536,32 @@ func (g *GitHubDownloader) Download(ctx context.Context, downloadURL, destDir st | |||||
| 423 | return archivePath, nil | 536 | return archivePath, nil | ||
| 424 | } | 537 | } | ||
| 425 | 538 | ||||
| 539 | + | func (g *GitHubDownloader) doRequestWithRetry(ctx context.Context, req *http.Request) (*http.Response, error) { | |||
| 540 | + | for attempt := 1; attempt <= maxRetryAttempts; attempt++ { | |||
| 541 | + | resp, err := g.client.Do(req) | |||
| 542 | + | if err != nil { | |||
| 543 | + | if !isRetryableRequestError(err) || attempt == maxRetryAttempts { | |||
| 544 | + | return nil, err | |||
| 545 | + | } | |||
| 546 | + | if waitErr := g.sleep(ctx, retryDelayForAttempt(attempt, g.randInt63)); waitErr != nil { | |||
| 547 | + | return nil, waitErr | |||
| 548 | + | } | |||
| 549 | + | continue | |||
| 550 | + | } | |||
| 551 | + | ||||
| 552 | + | if !isRetryableStatus(resp.StatusCode) || attempt == maxRetryAttempts { | |||
| 553 | + | return resp, nil | |||
| 554 | + | } | |||
| 555 | + | ||||
| 556 | + | _ = resp.Body.Close() | |||
| 557 | + | if waitErr := g.sleep(ctx, retryDelayForAttempt(attempt, g.randInt63)); waitErr != nil { | |||
| 558 | + | return nil, waitErr | |||
| 559 | + | } | |||
| 560 | + | } | |||
| 561 | + | ||||
| 562 | + | return nil, fmt.Errorf("request failed after retries") | |||
| 563 | + | } | |||
| 564 | + | ||||
| 426 | // sanitizeTarPath validates and sanitizes a tar header name to prevent path traversal. | 565 | // sanitizeTarPath validates and sanitizes a tar header name to prevent path traversal. | ||
| 427 | func sanitizeTarPath(name, destDir string) (string, error) { | 566 | func sanitizeTarPath(name, destDir string) (string, error) { | ||
| 428 | // Clean the path to remove any . or .. components. | 567 | // Clean the path to remove any . or .. components. | ||
M internal/infrastructure/updater/github_test.go +141 -0
| @@ -1,12 +1,20 @@ | |||
| 1 | 1 | package updater | |
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | + | "context" | |
| 4 | 5 | "crypto/sha256" | |
| 5 | 6 | "encoding/hex" | |
| 7 | + | "errors" | |
| 8 | + | "io" | |
| 9 | + | "net/http" | |
| 6 | 10 | "os" | |
| 7 | 11 | "path/filepath" | |
| 8 | 12 | "strings" | |
| 13 | + | "sync/atomic" | |
| 9 | 14 | "testing" | |
| 15 | + | "time" | |
| 16 | + | ||
| 17 | + | "github.com/bnema/dumber/internal/application/port" | |
| 10 | 18 | ) | |
| 11 | 19 | ||
| 12 | 20 | func TestCompareVersions(t *testing.T) { | |
| @@ -355,7 +363,140 @@ func TestSizeLimits(t *testing.T) { | |||
| 355 | 363 | } | |
| 356 | 364 | } | |
| 357 | 365 | ||
| 366 | + | func TestRetryDelayForAttempt(t *testing.T) { | |
| 367 | + | tests := []struct { | |
| 368 | + | name string | |
| 369 | + | attempt int | |
| 370 | + | want time.Duration | |
| 371 | + | }{ | |
| 372 | + | {name: "first attempt", attempt: 1, want: retryBaseDelay}, | |
| 373 | + | {name: "second attempt", attempt: 2, want: retryBaseDelay * 2}, | |
| 374 | + | {name: "third attempt", attempt: 3, want: retryBaseDelay * 4}, | |
| 375 | + | {name: "capped attempt", attempt: 6, want: retryMaxDelay}, | |
| 376 | + | } | |
| 377 | + | ||
| 378 | + | for _, tt := range tests { | |
| 379 | + | t.Run(tt.name, func(t *testing.T) { | |
| 380 | + | got := retryDelayForAttempt(tt.attempt, func(_ int64) int64 { return 0 }) | |
| 381 | + | if got != tt.want { | |
| 382 | + | t.Fatalf("retryDelayForAttempt(%d) = %v, want %v", tt.attempt, got, tt.want) | |
| 383 | + | } | |
| 384 | + | }) | |
| 385 | + | } | |
| 386 | + | } | |
| 387 | + | ||
| 388 | + | func TestRetryDelayForAttemptAddsJitter(t *testing.T) { | |
| 389 | + | got := retryDelayForAttempt(1, func(_ int64) int64 { return int64(50 * time.Millisecond) }) | |
| 390 | + | want := retryBaseDelay + (50 * time.Millisecond) | |
| 391 | + | if got != want { | |
| 392 | + | t.Fatalf("retryDelayForAttempt with jitter = %v, want %v", got, want) | |
| 393 | + | } | |
| 394 | + | } | |
| 395 | + | ||
| 396 | + | func TestGitHubCheckerDoRequestWithRetry_RetryableStatus(t *testing.T) { | |
| 397 | + | var calls int32 | |
| 398 | + | tr := roundTripFunc(func(_ *http.Request) (*http.Response, error) { | |
| 399 | + | attempt := atomic.AddInt32(&calls, 1) | |
| 400 | + | if attempt < maxRetryAttempts { | |
| 401 | + | return testResponse(http.StatusForbidden, "forbidden"), nil | |
| 402 | + | } | |
| 403 | + | return testResponse(http.StatusOK, `{"tag_name":"v1.2.3"}`), nil | |
| 404 | + | }) | |
| 405 | + | ||
| 406 | + | checker := NewGitHubChecker() | |
| 407 | + | checker.client = &http.Client{Transport: tr} | |
| 408 | + | checker.randInt63 = func(_ int64) int64 { return 0 } | |
| 409 | + | checker.sleep = func(context.Context, time.Duration) error { return nil } | |
| 410 | + | ||
| 411 | + | req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, githubAPIURL, http.NoBody) | |
| 412 | + | if err != nil { | |
| 413 | + | t.Fatalf("failed to create request: %v", err) | |
| 414 | + | } | |
| 415 | + | ||
| 416 | + | resp, err := checker.doRequestWithRetry(context.Background(), req) | |
| 417 | + | if err != nil { | |
| 418 | + | t.Fatalf("doRequestWithRetry() unexpected error: %v", err) | |
| 419 | + | } | |
| 420 | + | defer func() { _ = resp.Body.Close() }() | |
| 421 | + | ||
| 422 | + | if resp.StatusCode != http.StatusOK { | |
| 423 | + | t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusOK) | |
| 424 | + | } | |
| 425 | + | if atomic.LoadInt32(&calls) != maxRetryAttempts { | |
| 426 | + | t.Fatalf("calls = %d, want %d", calls, maxRetryAttempts) | |
| 427 | + | } | |
| 428 | + | } | |
| 429 | + | ||
| 430 | + | func TestGitHubCheckerCheckForUpdate_TransientError(t *testing.T) { | |
| 431 | + | var calls int32 | |
| 432 | + | tr := roundTripFunc(func(_ *http.Request) (*http.Response, error) { | |
| 433 | + | atomic.AddInt32(&calls, 1) | |
| 434 | + | return testResponse(http.StatusForbidden, "forbidden"), nil | |
| 435 | + | }) | |
| 436 | + | ||
| 437 | + | checker := NewGitHubChecker() | |
| 438 | + | checker.client = &http.Client{Transport: tr} | |
| 439 | + | checker.randInt63 = func(_ int64) int64 { return 0 } | |
| 440 | + | checker.sleep = func(context.Context, time.Duration) error { return nil } | |
| 441 | + | ||
| 442 | + | _, err := checker.CheckForUpdate(context.Background(), "1.0.0") | |
| 443 | + | if err == nil { | |
| 444 | + | t.Fatal("expected error, got nil") | |
| 445 | + | } | |
| 446 | + | if !errors.Is(err, port.ErrUpdateCheckTransient) { | |
| 447 | + | t.Fatalf("expected transient error, got %v", err) | |
| 448 | + | } | |
| 449 | + | if atomic.LoadInt32(&calls) != maxRetryAttempts { | |
| 450 | + | t.Fatalf("calls = %d, want %d", calls, maxRetryAttempts) | |
| 451 | + | } | |
| 452 | + | } | |
| 453 | + | ||
| 454 | + | func TestGitHubDownloaderDoRequestWithRetry_NoRetryOnClientError(t *testing.T) { | |
| 455 | + | var calls int32 | |
| 456 | + | tr := roundTripFunc(func(_ *http.Request) (*http.Response, error) { | |
| 457 | + | atomic.AddInt32(&calls, 1) | |
| 458 | + | return testResponse(http.StatusNotFound, "not found"), nil | |
| 459 | + | }) | |
| 460 | + | ||
| 461 | + | downloader := NewGitHubDownloader() | |
| 462 | + | downloader.client = &http.Client{Transport: tr} | |
| 463 | + | downloader.randInt63 = func(_ int64) int64 { return 0 } | |
| 464 | + | downloader.sleep = func(context.Context, time.Duration) error { return nil } | |
| 465 | + | ||
| 466 | + | req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, checksumsURLTemplate, http.NoBody) | |
| 467 | + | if err != nil { | |
| 468 | + | t.Fatalf("failed to create request: %v", err) | |
| 469 | + | } | |
| 470 | + | ||
| 471 | + | resp, err := downloader.doRequestWithRetry(context.Background(), req) | |
| 472 | + | if err != nil { | |
| 473 | + | t.Fatalf("doRequestWithRetry() unexpected error: %v", err) | |
| 474 | + | } | |
| 475 | + | defer func() { _ = resp.Body.Close() }() | |
| 476 | + | ||
| 477 | + | if resp.StatusCode != http.StatusNotFound { | |
| 478 | + | t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusNotFound) | |
| 479 | + | } | |
| 480 | + | if atomic.LoadInt32(&calls) != 1 { | |
| 481 | + | t.Fatalf("calls = %d, want 1", calls) | |
| 482 | + | } | |
| 483 | + | } | |
| 484 | + | ||
| 358 | 485 | // contains checks if substr is in s (helper for error message checks). | |
| 359 | 486 | func contains(s, substr string) bool { | |
| 360 | 487 | return strings.Contains(s, substr) | |
| 361 | 488 | } | |
| 489 | + | ||
| 490 | + | type roundTripFunc func(*http.Request) (*http.Response, error) | |
| 491 | + | ||
| 492 | + | func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { | |
| 493 | + | return f(req) | |
| 494 | + | } | |
| 495 | + | ||
| 496 | + | func testResponse(status int, body string) *http.Response { | |
| 497 | + | return &http.Response{ | |
| 498 | + | StatusCode: status, | |
| 499 | + | Body: io.NopCloser(strings.NewReader(body)), | |
| 500 | + | Header: make(http.Header), | |
| 501 | + | } | |
| 502 | + | } | |
| @@ -1,12 +1,20 @@ | |||||
| 1 | package updater | 1 | package updater | ||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | + | "context" | |||
| 4 | "crypto/sha256" | 5 | "crypto/sha256" | ||
| 5 | "encoding/hex" | 6 | "encoding/hex" | ||
| 7 | + | "errors" | |||
| 8 | + | "io" | |||
| 9 | + | "net/http" | |||
| 6 | "os" | 10 | "os" | ||
| 7 | "path/filepath" | 11 | "path/filepath" | ||
| 8 | "strings" | 12 | "strings" | ||
| 13 | + | "sync/atomic" | |||
| 9 | "testing" | 14 | "testing" | ||
| 15 | + | "time" | |||
| 16 | + | ||||
| 17 | + | "github.com/bnema/dumber/internal/application/port" | |||
| 10 | ) | 18 | ) | ||
| 11 | 19 | ||||
| 12 | func TestCompareVersions(t *testing.T) { | 20 | func TestCompareVersions(t *testing.T) { | ||
| @@ -355,7 +363,140 @@ func TestSizeLimits(t *testing.T) { | |||||
| 355 | } | 363 | } | ||
| 356 | } | 364 | } | ||
| 357 | 365 | ||||
| 366 | + | func TestRetryDelayForAttempt(t *testing.T) { | |||
| 367 | + | tests := []struct { | |||
| 368 | + | name string | |||
| 369 | + | attempt int | |||
| 370 | + | want time.Duration | |||
| 371 | + | }{ | |||
| 372 | + | {name: "first attempt", attempt: 1, want: retryBaseDelay}, | |||
| 373 | + | {name: "second attempt", attempt: 2, want: retryBaseDelay * 2}, | |||
| 374 | + | {name: "third attempt", attempt: 3, want: retryBaseDelay * 4}, | |||
| 375 | + | {name: "capped attempt", attempt: 6, want: retryMaxDelay}, | |||
| 376 | + | } | |||
| 377 | + | ||||
| 378 | + | for _, tt := range tests { | |||
| 379 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 380 | + | got := retryDelayForAttempt(tt.attempt, func(_ int64) int64 { return 0 }) | |||
| 381 | + | if got != tt.want { | |||
| 382 | + | t.Fatalf("retryDelayForAttempt(%d) = %v, want %v", tt.attempt, got, tt.want) | |||
| 383 | + | } | |||
| 384 | + | }) | |||
| 385 | + | } | |||
| 386 | + | } | |||
| 387 | + | ||||
| 388 | + | func TestRetryDelayForAttemptAddsJitter(t *testing.T) { | |||
| 389 | + | got := retryDelayForAttempt(1, func(_ int64) int64 { return int64(50 * time.Millisecond) }) | |||
| 390 | + | want := retryBaseDelay + (50 * time.Millisecond) | |||
| 391 | + | if got != want { | |||
| 392 | + | t.Fatalf("retryDelayForAttempt with jitter = %v, want %v", got, want) | |||
| 393 | + | } | |||
| 394 | + | } | |||
| 395 | + | ||||
| 396 | + | func TestGitHubCheckerDoRequestWithRetry_RetryableStatus(t *testing.T) { | |||
| 397 | + | var calls int32 | |||
| 398 | + | tr := roundTripFunc(func(_ *http.Request) (*http.Response, error) { | |||
| 399 | + | attempt := atomic.AddInt32(&calls, 1) | |||
| 400 | + | if attempt < maxRetryAttempts { | |||
| 401 | + | return testResponse(http.StatusForbidden, "forbidden"), nil | |||
| 402 | + | } | |||
| 403 | + | return testResponse(http.StatusOK, `{"tag_name":"v1.2.3"}`), nil | |||
| 404 | + | }) | |||
| 405 | + | ||||
| 406 | + | checker := NewGitHubChecker() | |||
| 407 | + | checker.client = &http.Client{Transport: tr} | |||
| 408 | + | checker.randInt63 = func(_ int64) int64 { return 0 } | |||
| 409 | + | checker.sleep = func(context.Context, time.Duration) error { return nil } | |||
| 410 | + | ||||
| 411 | + | req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, githubAPIURL, http.NoBody) | |||
| 412 | + | if err != nil { | |||
| 413 | + | t.Fatalf("failed to create request: %v", err) | |||
| 414 | + | } | |||
| 415 | + | ||||
| 416 | + | resp, err := checker.doRequestWithRetry(context.Background(), req) | |||
| 417 | + | if err != nil { | |||
| 418 | + | t.Fatalf("doRequestWithRetry() unexpected error: %v", err) | |||
| 419 | + | } | |||
| 420 | + | defer func() { _ = resp.Body.Close() }() | |||
| 421 | + | ||||
| 422 | + | if resp.StatusCode != http.StatusOK { | |||
| 423 | + | t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusOK) | |||
| 424 | + | } | |||
| 425 | + | if atomic.LoadInt32(&calls) != maxRetryAttempts { | |||
| 426 | + | t.Fatalf("calls = %d, want %d", calls, maxRetryAttempts) | |||
| 427 | + | } | |||
| 428 | + | } | |||
| 429 | + | ||||
| 430 | + | func TestGitHubCheckerCheckForUpdate_TransientError(t *testing.T) { | |||
| 431 | + | var calls int32 | |||
| 432 | + | tr := roundTripFunc(func(_ *http.Request) (*http.Response, error) { | |||
| 433 | + | atomic.AddInt32(&calls, 1) | |||
| 434 | + | return testResponse(http.StatusForbidden, "forbidden"), nil | |||
| 435 | + | }) | |||
| 436 | + | ||||
| 437 | + | checker := NewGitHubChecker() | |||
| 438 | + | checker.client = &http.Client{Transport: tr} | |||
| 439 | + | checker.randInt63 = func(_ int64) int64 { return 0 } | |||
| 440 | + | checker.sleep = func(context.Context, time.Duration) error { return nil } | |||
| 441 | + | ||||
| 442 | + | _, err := checker.CheckForUpdate(context.Background(), "1.0.0") | |||
| 443 | + | if err == nil { | |||
| 444 | + | t.Fatal("expected error, got nil") | |||
| 445 | + | } | |||
| 446 | + | if !errors.Is(err, port.ErrUpdateCheckTransient) { | |||
| 447 | + | t.Fatalf("expected transient error, got %v", err) | |||
| 448 | + | } | |||
| 449 | + | if atomic.LoadInt32(&calls) != maxRetryAttempts { | |||
| 450 | + | t.Fatalf("calls = %d, want %d", calls, maxRetryAttempts) | |||
| 451 | + | } | |||
| 452 | + | } | |||
| 453 | + | ||||
| 454 | + | func TestGitHubDownloaderDoRequestWithRetry_NoRetryOnClientError(t *testing.T) { | |||
| 455 | + | var calls int32 | |||
| 456 | + | tr := roundTripFunc(func(_ *http.Request) (*http.Response, error) { | |||
| 457 | + | atomic.AddInt32(&calls, 1) | |||
| 458 | + | return testResponse(http.StatusNotFound, "not found"), nil | |||
| 459 | + | }) | |||
| 460 | + | ||||
| 461 | + | downloader := NewGitHubDownloader() | |||
| 462 | + | downloader.client = &http.Client{Transport: tr} | |||
| 463 | + | downloader.randInt63 = func(_ int64) int64 { return 0 } | |||
| 464 | + | downloader.sleep = func(context.Context, time.Duration) error { return nil } | |||
| 465 | + | ||||
| 466 | + | req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, checksumsURLTemplate, http.NoBody) | |||
| 467 | + | if err != nil { | |||
| 468 | + | t.Fatalf("failed to create request: %v", err) | |||
| 469 | + | } | |||
| 470 | + | ||||
| 471 | + | resp, err := downloader.doRequestWithRetry(context.Background(), req) | |||
| 472 | + | if err != nil { | |||
| 473 | + | t.Fatalf("doRequestWithRetry() unexpected error: %v", err) | |||
| 474 | + | } | |||
| 475 | + | defer func() { _ = resp.Body.Close() }() | |||
| 476 | + | ||||
| 477 | + | if resp.StatusCode != http.StatusNotFound { | |||
| 478 | + | t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusNotFound) | |||
| 479 | + | } | |||
| 480 | + | if atomic.LoadInt32(&calls) != 1 { | |||
| 481 | + | t.Fatalf("calls = %d, want 1", calls) | |||
| 482 | + | } | |||
| 483 | + | } | |||
| 484 | + | ||||
| 358 | // contains checks if substr is in s (helper for error message checks). | 485 | // contains checks if substr is in s (helper for error message checks). | ||
| 359 | func contains(s, substr string) bool { | 486 | func contains(s, substr string) bool { | ||
| 360 | return strings.Contains(s, substr) | 487 | return strings.Contains(s, substr) | ||
| 361 | } | 488 | } | ||
| 489 | + | ||||
| 490 | + | type roundTripFunc func(*http.Request) (*http.Response, error) | |||
| 491 | + | ||||
| 492 | + | func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { | |||
| 493 | + | return f(req) | |||
| 494 | + | } | |||
| 495 | + | ||||
| 496 | + | func testResponse(status int, body string) *http.Response { | |||
| 497 | + | return &http.Response{ | |||
| 498 | + | StatusCode: status, | |||
| 499 | + | Body: io.NopCloser(strings.NewReader(body)), | |||
| 500 | + | Header: make(http.Header), | |||
| 501 | + | } | |||
| 502 | + | } | |||
M internal/infrastructure/webkit/webview.go +146 -15
| @@ -163,6 +163,9 @@ type WebView struct { | |||
| 163 | 163 | // asyncCallbacks keeps references to async JS callbacks to prevent GC | |
| 164 | 164 | asyncCallbacks []interface{} | |
| 165 | 165 | ||
| 166 | + | // runJSErrorStats aggregates repeated non-fatal RunJavaScript errors by domain+signature. | |
| 167 | + | runJSErrorStats map[string]runJSErrorStat | |
| 168 | + | ||
| 166 | 169 | // findController is cached to prevent GC from collecting the Go wrapper | |
| 167 | 170 | findController *findControllerAdapter | |
| 168 | 171 | findControllerOnce sync.Once | |
| @@ -171,10 +174,19 @@ type WebView struct { | |||
| 171 | 174 | backForwardListSignalID uint32 | |
| 172 | 175 | } | |
| 173 | 176 | ||
| 177 | + | type runJSErrorStat struct { | |
| 178 | + | count uint64 | |
| 179 | + | lastLog time.Time | |
| 180 | + | } | |
| 181 | + | ||
| 174 | 182 | const ( | |
| 175 | 183 | terminatePolicyAuto = "auto" | |
| 176 | 184 | terminatePolicyAlways = "always" | |
| 177 | 185 | terminatePolicyNever = "never" | |
| 186 | + | ||
| 187 | + | runJSNonFatalLogInterval = 30 * time.Second | |
| 188 | + | runJSAggregateLogEvery = 20 | |
| 189 | + | runJSUnknown = "unknown" | |
| 178 | 190 | ) | |
| 179 | 191 | ||
| 180 | 192 | type findControllerAdapter struct { | |
| @@ -309,10 +321,11 @@ func NewWebView(ctx context.Context, wkCtx *WebKitContext, settings *SettingsMan | |||
| 309 | 321 | } | |
| 310 | 322 | ||
| 311 | 323 | wv := &WebView{ | |
| 312 | - | inner: inner, | |
| 313 | - | ucm: inner.GetUserContentManager(), | |
| 314 | - | logger: log.With().Str("component", "webview").Logger(), | |
| 315 | - | signalIDs: make([]uint32, 0, 4), | |
| 324 | + | inner: inner, | |
| 325 | + | ucm: inner.GetUserContentManager(), | |
| 326 | + | logger: log.With().Str("component", "webview").Logger(), | |
| 327 | + | signalIDs: make([]uint32, 0, 4), | |
| 328 | + | runJSErrorStats: make(map[string]runJSErrorStat), | |
| 316 | 329 | } | |
| 317 | 330 | ||
| 318 | 331 | // Register in global registry | |
| @@ -358,11 +371,12 @@ func NewWebViewWithRelated(ctx context.Context, parent *WebView, settings *Setti | |||
| 358 | 371 | Msg("related webview created, checking pointers") | |
| 359 | 372 | ||
| 360 | 373 | wv := &WebView{ | |
| 361 | - | inner: inner, | |
| 362 | - | isRelated: true, // Shares web process with parent - must not terminate process on destroy | |
| 363 | - | ucm: inner.GetUserContentManager(), | |
| 364 | - | logger: log.With().Str("component", "webview-popup").Logger(), | |
| 365 | - | signalIDs: make([]uint32, 0, 6), | |
| 374 | + | inner: inner, | |
| 375 | + | isRelated: true, // Shares web process with parent - must not terminate process on destroy | |
| 376 | + | ucm: inner.GetUserContentManager(), | |
| 377 | + | logger: log.With().Str("component", "webview-popup").Logger(), | |
| 378 | + | signalIDs: make([]uint32, 0, 6), | |
| 379 | + | runJSErrorStats: make(map[string]runJSErrorStat), | |
| 366 | 380 | } | |
| 367 | 381 | ||
| 368 | 382 | wv.id = globalRegistry.register(wv) | |
| @@ -1447,6 +1461,90 @@ func shouldTerminateWebProcess(policy string, isRelated, navigationActive bool) | |||
| 1447 | 1461 | } | |
| 1448 | 1462 | } | |
| 1449 | 1463 | ||
| 1464 | + | func classifyRunJSEvaluateError(err error) (nonFatal bool, signature string) { | |
| 1465 | + | if err == nil { | |
| 1466 | + | return false, "evaluate_error" | |
| 1467 | + | } | |
| 1468 | + | msg := strings.ToLower(strings.TrimSpace(err.Error())) | |
| 1469 | + | signature = "evaluate_error:" + normalizeRunJSErrorSignature(msg) | |
| 1470 | + | switch { | |
| 1471 | + | case strings.Contains(msg, "canceled"), | |
| 1472 | + | strings.Contains(msg, "cancel"), | |
| 1473 | + | strings.Contains(msg, "javascript execution context"), | |
| 1474 | + | strings.Contains(msg, "execution context was destroyed"), | |
| 1475 | + | strings.Contains(msg, "document unloaded"): | |
| 1476 | + | return true, signature | |
| 1477 | + | default: | |
| 1478 | + | return false, signature | |
| 1479 | + | } | |
| 1480 | + | } | |
| 1481 | + | ||
| 1482 | + | func normalizeRunJSErrorSignature(msg string) string { | |
| 1483 | + | msg = strings.ToLower(strings.TrimSpace(msg)) | |
| 1484 | + | if msg == "" { | |
| 1485 | + | return "empty" | |
| 1486 | + | } | |
| 1487 | + | fields := strings.Fields(msg) | |
| 1488 | + | if len(fields) == 0 { | |
| 1489 | + | return "empty" | |
| 1490 | + | } | |
| 1491 | + | return strings.Join(fields, " ") | |
| 1492 | + | } | |
| 1493 | + | ||
| 1494 | + | func (wv *WebView) runJSDomain() string { | |
| 1495 | + | wv.mu.RLock() | |
| 1496 | + | currentURI := strings.TrimSpace(wv.uri) | |
| 1497 | + | wv.mu.RUnlock() | |
| 1498 | + | if currentURI == "" { | |
| 1499 | + | return runJSUnknown | |
| 1500 | + | } | |
| 1501 | + | parsed, err := url.Parse(currentURI) | |
| 1502 | + | if err != nil { | |
| 1503 | + | return runJSUnknown | |
| 1504 | + | } | |
| 1505 | + | host := strings.ToLower(strings.TrimSpace(parsed.Hostname())) | |
| 1506 | + | if host == "" { | |
| 1507 | + | return runJSUnknown | |
| 1508 | + | } | |
| 1509 | + | return host | |
| 1510 | + | } | |
| 1511 | + | ||
| 1512 | + | func (wv *WebView) shouldLogRunJSError(domain, signature string, nonFatal bool, now time.Time) (bool, uint64) { | |
| 1513 | + | if wv == nil { | |
| 1514 | + | return true, 1 | |
| 1515 | + | } | |
| 1516 | + | if now.IsZero() { | |
| 1517 | + | now = time.Now() | |
| 1518 | + | } | |
| 1519 | + | if domain == "" { | |
| 1520 | + | domain = runJSUnknown | |
| 1521 | + | } | |
| 1522 | + | if signature == "" { | |
| 1523 | + | signature = runJSUnknown | |
| 1524 | + | } | |
| 1525 | + | key := domain + "|" + signature | |
| 1526 | + | ||
| 1527 | + | wv.mu.Lock() | |
| 1528 | + | if wv.runJSErrorStats == nil { | |
| 1529 | + | wv.runJSErrorStats = make(map[string]runJSErrorStat) | |
| 1530 | + | } | |
| 1531 | + | stat := wv.runJSErrorStats[key] | |
| 1532 | + | stat.count++ | |
| 1533 | + | count := stat.count | |
| 1534 | + | ||
| 1535 | + | shouldLog := true | |
| 1536 | + | if nonFatal { | |
| 1537 | + | shouldLog = count == 1 || count%runJSAggregateLogEvery == 0 || now.Sub(stat.lastLog) >= runJSNonFatalLogInterval | |
| 1538 | + | } | |
| 1539 | + | if shouldLog { | |
| 1540 | + | stat.lastLog = now | |
| 1541 | + | } | |
| 1542 | + | wv.runJSErrorStats[key] = stat | |
| 1543 | + | wv.mu.Unlock() | |
| 1544 | + | ||
| 1545 | + | return shouldLog, count | |
| 1546 | + | } | |
| 1547 | + | ||
| 1450 | 1548 | // RunJavaScript executes script in the specified world (empty for main world). | |
| 1451 | 1549 | // This is fire-and-forget: it does not block and errors are logged asynchronously. | |
| 1452 | 1550 | // Safe to call from any context including GTK signal handlers. | |
| @@ -1461,25 +1559,58 @@ func (wv *WebView) RunJavaScript(ctx context.Context, script, worldName string) | |||
| 1461 | 1559 | log := logging.FromContext(ctx) | |
| 1462 | 1560 | ||
| 1463 | 1561 | cb := gio.AsyncReadyCallback(func(_ uintptr, resPtr uintptr, _ uintptr) { | |
| 1562 | + | domain := wv.runJSDomain() | |
| 1464 | 1563 | if resPtr == 0 { | |
| 1465 | - | log.Warn().Uint64("webview_id", uint64(wv.id)).Msg("RunJavaScript: nil async result") | |
| 1564 | + | signature := "nil_async_result" | |
| 1565 | + | shouldLog, count := wv.shouldLogRunJSError(domain, signature, true, time.Now()) | |
| 1566 | + | if shouldLog { | |
| 1567 | + | log.Debug(). | |
| 1568 | + | Uint64("webview_id", uint64(wv.id)). | |
| 1569 | + | Str("domain", domain). | |
| 1570 | + | Str("signature", signature). | |
| 1571 | + | Uint64("repeat_count", count). | |
| 1572 | + | Msg("RunJavaScript: non-fatal nil async result") | |
| 1573 | + | } | |
| 1466 | 1574 | return | |
| 1467 | 1575 | } | |
| 1468 | 1576 | ||
| 1469 | 1577 | res := &gio.AsyncResultBase{Ptr: resPtr} | |
| 1470 | 1578 | value, err := wv.inner.EvaluateJavascriptFinish(res) | |
| 1471 | 1579 | if err != nil { | |
| 1472 | - | log.Warn().Err(err).Uint64("webview_id", uint64(wv.id)).Msg("RunJavaScript: failed") | |
| 1580 | + | nonFatal, signature := classifyRunJSEvaluateError(err) | |
| 1581 | + | shouldLog, count := wv.shouldLogRunJSError(domain, signature, nonFatal, time.Now()) | |
| 1582 | + | if shouldLog { | |
| 1583 | + | ev := log.Warn() | |
| 1584 | + | msg := "RunJavaScript: failed" | |
| 1585 | + | if nonFatal { | |
| 1586 | + | ev = log.Debug() | |
| 1587 | + | msg = "RunJavaScript: non-fatal failure" | |
| 1588 | + | } | |
| 1589 | + | ev.Err(err). | |
| 1590 | + | Uint64("webview_id", uint64(wv.id)). | |
| 1591 | + | Str("domain", domain). | |
| 1592 | + | Str("signature", signature). | |
| 1593 | + | Uint64("repeat_count", count). | |
| 1594 | + | Msg(msg) | |
| 1595 | + | } | |
| 1473 | 1596 | return | |
| 1474 | 1597 | } | |
| 1475 | 1598 | ||
| 1476 | 1599 | if value != nil { | |
| 1477 | 1600 | if jscCtx := value.GetContext(); jscCtx != nil { | |
| 1478 | 1601 | if exc := jscCtx.GetException(); exc != nil { | |
| 1479 | - | log.Warn(). | |
| 1480 | - | Str("exception", exc.GetMessage()). | |
| 1481 | - | Uint64("webview_id", uint64(wv.id)). | |
| 1482 | - | Msg("RunJavaScript: JS exception") | |
| 1602 | + | exceptionMessage := strings.TrimSpace(exc.GetMessage()) | |
| 1603 | + | signature := "js_exception:" + normalizeRunJSErrorSignature(exceptionMessage) | |
| 1604 | + | shouldLog, count := wv.shouldLogRunJSError(domain, signature, true, time.Now()) | |
| 1605 | + | if shouldLog { | |
| 1606 | + | log.Debug(). | |
| 1607 | + | Str("exception", exceptionMessage). | |
| 1608 | + | Uint64("webview_id", uint64(wv.id)). | |
| 1609 | + | Str("domain", domain). | |
| 1610 | + | Str("signature", signature). | |
| 1611 | + | Uint64("repeat_count", count). | |
| 1612 | + | Msg("RunJavaScript: JS exception") | |
| 1613 | + | } | |
| 1483 | 1614 | } | |
| 1484 | 1615 | } | |
| 1485 | 1616 | } | |
| @@ -163,6 +163,9 @@ type WebView struct { | |||||
| 163 | // asyncCallbacks keeps references to async JS callbacks to prevent GC | 163 | // asyncCallbacks keeps references to async JS callbacks to prevent GC | ||
| 164 | asyncCallbacks []interface{} | 164 | asyncCallbacks []interface{} | ||
| 165 | 165 | ||||
| 166 | + | // runJSErrorStats aggregates repeated non-fatal RunJavaScript errors by domain+signature. | |||
| 167 | + | runJSErrorStats map[string]runJSErrorStat | |||
| 168 | + | ||||
| 166 | // findController is cached to prevent GC from collecting the Go wrapper | 169 | // findController is cached to prevent GC from collecting the Go wrapper | ||
| 167 | findController *findControllerAdapter | 170 | findController *findControllerAdapter | ||
| 168 | findControllerOnce sync.Once | 171 | findControllerOnce sync.Once | ||
| @@ -171,10 +174,19 @@ type WebView struct { | |||||
| 171 | backForwardListSignalID uint32 | 174 | backForwardListSignalID uint32 | ||
| 172 | } | 175 | } | ||
| 173 | 176 | ||||
| 177 | + | type runJSErrorStat struct { | |||
| 178 | + | count uint64 | |||
| 179 | + | lastLog time.Time | |||
| 180 | + | } | |||
| 181 | + | ||||
| 174 | const ( | 182 | const ( | ||
| 175 | terminatePolicyAuto = "auto" | 183 | terminatePolicyAuto = "auto" | ||
| 176 | terminatePolicyAlways = "always" | 184 | terminatePolicyAlways = "always" | ||
| 177 | terminatePolicyNever = "never" | 185 | terminatePolicyNever = "never" | ||
| 186 | + | ||||
| 187 | + | runJSNonFatalLogInterval = 30 * time.Second | |||
| 188 | + | runJSAggregateLogEvery = 20 | |||
| 189 | + | runJSUnknown = "unknown" | |||
| 178 | ) | 190 | ) | ||
| 179 | 191 | ||||
| 180 | type findControllerAdapter struct { | 192 | type findControllerAdapter struct { | ||
| @@ -309,10 +321,11 @@ func NewWebView(ctx context.Context, wkCtx *WebKitContext, settings *SettingsMan | |||||
| 309 | } | 321 | } | ||
| 310 | 322 | ||||
| 311 | wv := &WebView{ | 323 | wv := &WebView{ | ||
| 312 | - | inner: inner, | 324 | + | inner: inner, |
| 313 | - | ucm: inner.GetUserContentManager(), | 325 | + | ucm: inner.GetUserContentManager(), |
| 314 | - | logger: log.With().Str("component", "webview").Logger(), | 326 | + | logger: log.With().Str("component", "webview").Logger(), |
| 315 | - | signalIDs: make([]uint32, 0, 4), | 327 | + | signalIDs: make([]uint32, 0, 4), |
| 328 | + | runJSErrorStats: make(map[string]runJSErrorStat), | |||
| 316 | } | 329 | } | ||
| 317 | 330 | ||||
| 318 | // Register in global registry | 331 | // Register in global registry | ||
| @@ -358,11 +371,12 @@ func NewWebViewWithRelated(ctx context.Context, parent *WebView, settings *Setti | |||||
| 358 | Msg("related webview created, checking pointers") | 371 | Msg("related webview created, checking pointers") | ||
| 359 | 372 | ||||
| 360 | wv := &WebView{ | 373 | wv := &WebView{ | ||
| 361 | - | inner: inner, | 374 | + | inner: inner, |
| 362 | - | isRelated: true, // Shares web process with parent - must not terminate process on destroy | 375 | + | isRelated: true, // Shares web process with parent - must not terminate process on destroy |
| 363 | - | ucm: inner.GetUserContentManager(), | 376 | + | ucm: inner.GetUserContentManager(), |
| 364 | - | logger: log.With().Str("component", "webview-popup").Logger(), | 377 | + | logger: log.With().Str("component", "webview-popup").Logger(), |
| 365 | - | signalIDs: make([]uint32, 0, 6), | 378 | + | signalIDs: make([]uint32, 0, 6), |
| 379 | + | runJSErrorStats: make(map[string]runJSErrorStat), | |||
| 366 | } | 380 | } | ||
| 367 | 381 | ||||
| 368 | wv.id = globalRegistry.register(wv) | 382 | wv.id = globalRegistry.register(wv) | ||
| @@ -1447,6 +1461,90 @@ func shouldTerminateWebProcess(policy string, isRelated, navigationActive bool) | |||||
| 1447 | } | 1461 | } | ||
| 1448 | } | 1462 | } | ||
| 1449 | 1463 | ||||
| 1464 | + | func classifyRunJSEvaluateError(err error) (nonFatal bool, signature string) { | |||
| 1465 | + | if err == nil { | |||
| 1466 | + | return false, "evaluate_error" | |||
| 1467 | + | } | |||
| 1468 | + | msg := strings.ToLower(strings.TrimSpace(err.Error())) | |||
| 1469 | + | signature = "evaluate_error:" + normalizeRunJSErrorSignature(msg) | |||
| 1470 | + | switch { | |||
| 1471 | + | case strings.Contains(msg, "canceled"), | |||
| 1472 | + | strings.Contains(msg, "cancel"), | |||
| 1473 | + | strings.Contains(msg, "javascript execution context"), | |||
| 1474 | + | strings.Contains(msg, "execution context was destroyed"), | |||
| 1475 | + | strings.Contains(msg, "document unloaded"): | |||
| 1476 | + | return true, signature | |||
| 1477 | + | default: | |||
| 1478 | + | return false, signature | |||
| 1479 | + | } | |||
| 1480 | + | } | |||
| 1481 | + | ||||
| 1482 | + | func normalizeRunJSErrorSignature(msg string) string { | |||
| 1483 | + | msg = strings.ToLower(strings.TrimSpace(msg)) | |||
| 1484 | + | if msg == "" { | |||
| 1485 | + | return "empty" | |||
| 1486 | + | } | |||
| 1487 | + | fields := strings.Fields(msg) | |||
| 1488 | + | if len(fields) == 0 { | |||
| 1489 | + | return "empty" | |||
| 1490 | + | } | |||
| 1491 | + | return strings.Join(fields, " ") | |||
| 1492 | + | } | |||
| 1493 | + | ||||
| 1494 | + | func (wv *WebView) runJSDomain() string { | |||
| 1495 | + | wv.mu.RLock() | |||
| 1496 | + | currentURI := strings.TrimSpace(wv.uri) | |||
| 1497 | + | wv.mu.RUnlock() | |||
| 1498 | + | if currentURI == "" { | |||
| 1499 | + | return runJSUnknown | |||
| 1500 | + | } | |||
| 1501 | + | parsed, err := url.Parse(currentURI) | |||
| 1502 | + | if err != nil { | |||
| 1503 | + | return runJSUnknown | |||
| 1504 | + | } | |||
| 1505 | + | host := strings.ToLower(strings.TrimSpace(parsed.Hostname())) | |||
| 1506 | + | if host == "" { | |||
| 1507 | + | return runJSUnknown | |||
| 1508 | + | } | |||
| 1509 | + | return host | |||
| 1510 | + | } | |||
| 1511 | + | ||||
| 1512 | + | func (wv *WebView) shouldLogRunJSError(domain, signature string, nonFatal bool, now time.Time) (bool, uint64) { | |||
| 1513 | + | if wv == nil { | |||
| 1514 | + | return true, 1 | |||
| 1515 | + | } | |||
| 1516 | + | if now.IsZero() { | |||
| 1517 | + | now = time.Now() | |||
| 1518 | + | } | |||
| 1519 | + | if domain == "" { | |||
| 1520 | + | domain = runJSUnknown | |||
| 1521 | + | } | |||
| 1522 | + | if signature == "" { | |||
| 1523 | + | signature = runJSUnknown | |||
| 1524 | + | } | |||
| 1525 | + | key := domain + "|" + signature | |||
| 1526 | + | ||||
| 1527 | + | wv.mu.Lock() | |||
| 1528 | + | if wv.runJSErrorStats == nil { | |||
| 1529 | + | wv.runJSErrorStats = make(map[string]runJSErrorStat) | |||
| 1530 | + | } | |||
| 1531 | + | stat := wv.runJSErrorStats[key] | |||
| 1532 | + | stat.count++ | |||
| 1533 | + | count := stat.count | |||
| 1534 | + | ||||
| 1535 | + | shouldLog := true | |||
| 1536 | + | if nonFatal { | |||
| 1537 | + | shouldLog = count == 1 || count%runJSAggregateLogEvery == 0 || now.Sub(stat.lastLog) >= runJSNonFatalLogInterval | |||
| 1538 | + | } | |||
| 1539 | + | if shouldLog { | |||
| 1540 | + | stat.lastLog = now | |||
| 1541 | + | } | |||
| 1542 | + | wv.runJSErrorStats[key] = stat | |||
| 1543 | + | wv.mu.Unlock() | |||
| 1544 | + | ||||
| 1545 | + | return shouldLog, count | |||
| 1546 | + | } | |||
| 1547 | + | ||||
| 1450 | // RunJavaScript executes script in the specified world (empty for main world). | 1548 | // RunJavaScript executes script in the specified world (empty for main world). | ||
| 1451 | // This is fire-and-forget: it does not block and errors are logged asynchronously. | 1549 | // This is fire-and-forget: it does not block and errors are logged asynchronously. | ||
| 1452 | // Safe to call from any context including GTK signal handlers. | 1550 | // Safe to call from any context including GTK signal handlers. | ||
| @@ -1461,25 +1559,58 @@ func (wv *WebView) RunJavaScript(ctx context.Context, script, worldName string) | |||||
| 1461 | log := logging.FromContext(ctx) | 1559 | log := logging.FromContext(ctx) | ||
| 1462 | 1560 | ||||
| 1463 | cb := gio.AsyncReadyCallback(func(_ uintptr, resPtr uintptr, _ uintptr) { | 1561 | cb := gio.AsyncReadyCallback(func(_ uintptr, resPtr uintptr, _ uintptr) { | ||
| 1562 | + | domain := wv.runJSDomain() | |||
| 1464 | if resPtr == 0 { | 1563 | if resPtr == 0 { | ||
| 1465 | - | log.Warn().Uint64("webview_id", uint64(wv.id)).Msg("RunJavaScript: nil async result") | 1564 | + | signature := "nil_async_result" |
| 1565 | + | shouldLog, count := wv.shouldLogRunJSError(domain, signature, true, time.Now()) | |||
| 1566 | + | if shouldLog { | |||
| 1567 | + | log.Debug(). | |||
| 1568 | + | Uint64("webview_id", uint64(wv.id)). | |||
| 1569 | + | Str("domain", domain). | |||
| 1570 | + | Str("signature", signature). | |||
| 1571 | + | Uint64("repeat_count", count). | |||
| 1572 | + | Msg("RunJavaScript: non-fatal nil async result") | |||
| 1573 | + | } | |||
| 1466 | return | 1574 | return | ||
| 1467 | } | 1575 | } | ||
| 1468 | 1576 | ||||
| 1469 | res := &gio.AsyncResultBase{Ptr: resPtr} | 1577 | res := &gio.AsyncResultBase{Ptr: resPtr} | ||
| 1470 | value, err := wv.inner.EvaluateJavascriptFinish(res) | 1578 | value, err := wv.inner.EvaluateJavascriptFinish(res) | ||
| 1471 | if err != nil { | 1579 | if err != nil { | ||
| 1472 | - | log.Warn().Err(err).Uint64("webview_id", uint64(wv.id)).Msg("RunJavaScript: failed") | 1580 | + | nonFatal, signature := classifyRunJSEvaluateError(err) |
| 1581 | + | shouldLog, count := wv.shouldLogRunJSError(domain, signature, nonFatal, time.Now()) | |||
| 1582 | + | if shouldLog { | |||
| 1583 | + | ev := log.Warn() | |||
| 1584 | + | msg := "RunJavaScript: failed" | |||
| 1585 | + | if nonFatal { | |||
| 1586 | + | ev = log.Debug() | |||
| 1587 | + | msg = "RunJavaScript: non-fatal failure" | |||
| 1588 | + | } | |||
| 1589 | + | ev.Err(err). | |||
| 1590 | + | Uint64("webview_id", uint64(wv.id)). | |||
| 1591 | + | Str("domain", domain). | |||
| 1592 | + | Str("signature", signature). | |||
| 1593 | + | Uint64("repeat_count", count). | |||
| 1594 | + | Msg(msg) | |||
| 1595 | + | } | |||
| 1473 | return | 1596 | return | ||
| 1474 | } | 1597 | } | ||
| 1475 | 1598 | ||||
| 1476 | if value != nil { | 1599 | if value != nil { | ||
| 1477 | if jscCtx := value.GetContext(); jscCtx != nil { | 1600 | if jscCtx := value.GetContext(); jscCtx != nil { | ||
| 1478 | if exc := jscCtx.GetException(); exc != nil { | 1601 | if exc := jscCtx.GetException(); exc != nil { | ||
| 1479 | - | log.Warn(). | 1602 | + | exceptionMessage := strings.TrimSpace(exc.GetMessage()) |
| 1480 | - | Str("exception", exc.GetMessage()). | 1603 | + | signature := "js_exception:" + normalizeRunJSErrorSignature(exceptionMessage) |
| 1481 | - | Uint64("webview_id", uint64(wv.id)). | 1604 | + | shouldLog, count := wv.shouldLogRunJSError(domain, signature, true, time.Now()) |
| 1482 | - | Msg("RunJavaScript: JS exception") | 1605 | + | if shouldLog { |
| 1606 | + | log.Debug(). | |||
| 1607 | + | Str("exception", exceptionMessage). | |||
| 1608 | + | Uint64("webview_id", uint64(wv.id)). | |||
| 1609 | + | Str("domain", domain). | |||
| 1610 | + | Str("signature", signature). | |||
| 1611 | + | Uint64("repeat_count", count). | |||
| 1612 | + | Msg("RunJavaScript: JS exception") | |||
| 1613 | + | } | |||
| 1483 | } | 1614 | } | ||
| 1484 | } | 1615 | } | ||
| 1485 | } | 1616 | } | ||
A internal/infrastructure/webkit/webview_runjavascript_test.go +89 -0
| @@ -0,0 +1,89 @@ | |||
| 1 | + | package webkit | |
| 2 | + | ||
| 3 | + | import ( | |
| 4 | + | "errors" | |
| 5 | + | "testing" | |
| 6 | + | "time" | |
| 7 | + | ||
| 8 | + | "github.com/stretchr/testify/assert" | |
| 9 | + | ) | |
| 10 | + | ||
| 11 | + | func TestClassifyRunJSEvaluateError(t *testing.T) { | |
| 12 | + | t.Run("non-fatal canceled", func(t *testing.T) { | |
| 13 | + | nonFatal, signature := classifyRunJSEvaluateError(errors.New("Operation was canceled")) | |
| 14 | + | assert.True(t, nonFatal) | |
| 15 | + | assert.Equal(t, "evaluate_error:operation was canceled", signature) | |
| 16 | + | }) | |
| 17 | + | ||
| 18 | + | t.Run("non-fatal context destroyed", func(t *testing.T) { | |
| 19 | + | nonFatal, signature := classifyRunJSEvaluateError(errors.New("JavaScript execution context was destroyed")) | |
| 20 | + | assert.True(t, nonFatal) | |
| 21 | + | assert.Equal(t, "evaluate_error:javascript execution context was destroyed", signature) | |
| 22 | + | }) | |
| 23 | + | ||
| 24 | + | t.Run("fatal unknown", func(t *testing.T) { | |
| 25 | + | nonFatal, signature := classifyRunJSEvaluateError(errors.New("segmentation fault in JS runtime")) | |
| 26 | + | assert.False(t, nonFatal) | |
| 27 | + | assert.Equal(t, "evaluate_error:segmentation fault in js runtime", signature) | |
| 28 | + | }) | |
| 29 | + | } | |
| 30 | + | ||
| 31 | + | func TestNormalizeRunJSErrorSignature(t *testing.T) { | |
| 32 | + | assert.Equal(t, "empty", normalizeRunJSErrorSignature(" \n\t ")) | |
| 33 | + | assert.Equal(t, "foo bar baz", normalizeRunJSErrorSignature(" Foo BAR\tbaz ")) | |
| 34 | + | } | |
| 35 | + | ||
| 36 | + | func TestWebViewRunJSDomain(t *testing.T) { | |
| 37 | + | wv := &WebView{uri: "https://Sub.Example.com/path?q=1"} | |
| 38 | + | assert.Equal(t, "sub.example.com", wv.runJSDomain()) | |
| 39 | + | ||
| 40 | + | wv = &WebView{uri: "not a uri"} | |
| 41 | + | assert.Equal(t, "unknown", wv.runJSDomain()) | |
| 42 | + | } | |
| 43 | + | ||
| 44 | + | func TestShouldLogRunJSError_NonFatalAggregation(t *testing.T) { | |
| 45 | + | wv := &WebView{} | |
| 46 | + | now := time.Now() | |
| 47 | + | ||
| 48 | + | shouldLog, count := wv.shouldLogRunJSError("example.com", "sig", true, now) | |
| 49 | + | assert.True(t, shouldLog) | |
| 50 | + | assert.Equal(t, uint64(1), count) | |
| 51 | + | ||
| 52 | + | shouldLog, count = wv.shouldLogRunJSError("example.com", "sig", true, now.Add(10*time.Second)) | |
| 53 | + | assert.False(t, shouldLog) | |
| 54 | + | assert.Equal(t, uint64(2), count) | |
| 55 | + | ||
| 56 | + | shouldLog, count = wv.shouldLogRunJSError("example.com", "sig", true, now.Add(31*time.Second)) | |
| 57 | + | assert.True(t, shouldLog) | |
| 58 | + | assert.Equal(t, uint64(3), count) | |
| 59 | + | } | |
| 60 | + | ||
| 61 | + | func TestShouldLogRunJSError_NonFatalLogsEveryN(t *testing.T) { | |
| 62 | + | wv := &WebView{} | |
| 63 | + | base := time.Now() | |
| 64 | + | for i := 1; i < runJSAggregateLogEvery; i++ { | |
| 65 | + | shouldLog, _ := wv.shouldLogRunJSError("example.com", "sig", true, base) | |
| 66 | + | if i == 1 { | |
| 67 | + | assert.True(t, shouldLog) | |
| 68 | + | continue | |
| 69 | + | } | |
| 70 | + | assert.False(t, shouldLog) | |
| 71 | + | } | |
| 72 | + | ||
| 73 | + | shouldLog, count := wv.shouldLogRunJSError("example.com", "sig", true, base) | |
| 74 | + | assert.True(t, shouldLog) | |
| 75 | + | assert.Equal(t, uint64(runJSAggregateLogEvery), count) | |
| 76 | + | } | |
| 77 | + | ||
| 78 | + | func TestShouldLogRunJSError_FatalAlwaysLogs(t *testing.T) { | |
| 79 | + | wv := &WebView{} | |
| 80 | + | base := time.Now() | |
| 81 | + | ||
| 82 | + | shouldLog, count := wv.shouldLogRunJSError("example.com", "fatal-sig", false, base) | |
| 83 | + | assert.True(t, shouldLog) | |
| 84 | + | assert.Equal(t, uint64(1), count) | |
| 85 | + | ||
| 86 | + | shouldLog, count = wv.shouldLogRunJSError("example.com", "fatal-sig", false, base) | |
| 87 | + | assert.True(t, shouldLog) | |
| 88 | + | assert.Equal(t, uint64(2), count) | |
| 89 | + | } | |
| @@ -0,0 +1,89 @@ | |||||
| 1 | + | package webkit | |||
| 2 | + | ||||
| 3 | + | import ( | |||
| 4 | + | "errors" | |||
| 5 | + | "testing" | |||
| 6 | + | "time" | |||
| 7 | + | ||||
| 8 | + | "github.com/stretchr/testify/assert" | |||
| 9 | + | ) | |||
| 10 | + | ||||
| 11 | + | func TestClassifyRunJSEvaluateError(t *testing.T) { | |||
| 12 | + | t.Run("non-fatal canceled", func(t *testing.T) { | |||
| 13 | + | nonFatal, signature := classifyRunJSEvaluateError(errors.New("Operation was canceled")) | |||
| 14 | + | assert.True(t, nonFatal) | |||
| 15 | + | assert.Equal(t, "evaluate_error:operation was canceled", signature) | |||
| 16 | + | }) | |||
| 17 | + | ||||
| 18 | + | t.Run("non-fatal context destroyed", func(t *testing.T) { | |||
| 19 | + | nonFatal, signature := classifyRunJSEvaluateError(errors.New("JavaScript execution context was destroyed")) | |||
| 20 | + | assert.True(t, nonFatal) | |||
| 21 | + | assert.Equal(t, "evaluate_error:javascript execution context was destroyed", signature) | |||
| 22 | + | }) | |||
| 23 | + | ||||
| 24 | + | t.Run("fatal unknown", func(t *testing.T) { | |||
| 25 | + | nonFatal, signature := classifyRunJSEvaluateError(errors.New("segmentation fault in JS runtime")) | |||
| 26 | + | assert.False(t, nonFatal) | |||
| 27 | + | assert.Equal(t, "evaluate_error:segmentation fault in js runtime", signature) | |||
| 28 | + | }) | |||
| 29 | + | } | |||
| 30 | + | ||||
| 31 | + | func TestNormalizeRunJSErrorSignature(t *testing.T) { | |||
| 32 | + | assert.Equal(t, "empty", normalizeRunJSErrorSignature(" \n\t ")) | |||
| 33 | + | assert.Equal(t, "foo bar baz", normalizeRunJSErrorSignature(" Foo BAR\tbaz ")) | |||
| 34 | + | } | |||
| 35 | + | ||||
| 36 | + | func TestWebViewRunJSDomain(t *testing.T) { | |||
| 37 | + | wv := &WebView{uri: "https://Sub.Example.com/path?q=1"} | |||
| 38 | + | assert.Equal(t, "sub.example.com", wv.runJSDomain()) | |||
| 39 | + | ||||
| 40 | + | wv = &WebView{uri: "not a uri"} | |||
| 41 | + | assert.Equal(t, "unknown", wv.runJSDomain()) | |||
| 42 | + | } | |||
| 43 | + | ||||
| 44 | + | func TestShouldLogRunJSError_NonFatalAggregation(t *testing.T) { | |||
| 45 | + | wv := &WebView{} | |||
| 46 | + | now := time.Now() | |||
| 47 | + | ||||
| 48 | + | shouldLog, count := wv.shouldLogRunJSError("example.com", "sig", true, now) | |||
| 49 | + | assert.True(t, shouldLog) | |||
| 50 | + | assert.Equal(t, uint64(1), count) | |||
| 51 | + | ||||
| 52 | + | shouldLog, count = wv.shouldLogRunJSError("example.com", "sig", true, now.Add(10*time.Second)) | |||
| 53 | + | assert.False(t, shouldLog) | |||
| 54 | + | assert.Equal(t, uint64(2), count) | |||
| 55 | + | ||||
| 56 | + | shouldLog, count = wv.shouldLogRunJSError("example.com", "sig", true, now.Add(31*time.Second)) | |||
| 57 | + | assert.True(t, shouldLog) | |||
| 58 | + | assert.Equal(t, uint64(3), count) | |||
| 59 | + | } | |||
| 60 | + | ||||
| 61 | + | func TestShouldLogRunJSError_NonFatalLogsEveryN(t *testing.T) { | |||
| 62 | + | wv := &WebView{} | |||
| 63 | + | base := time.Now() | |||
| 64 | + | for i := 1; i < runJSAggregateLogEvery; i++ { | |||
| 65 | + | shouldLog, _ := wv.shouldLogRunJSError("example.com", "sig", true, base) | |||
| 66 | + | if i == 1 { | |||
| 67 | + | assert.True(t, shouldLog) | |||
| 68 | + | continue | |||
| 69 | + | } | |||
| 70 | + | assert.False(t, shouldLog) | |||
| 71 | + | } | |||
| 72 | + | ||||
| 73 | + | shouldLog, count := wv.shouldLogRunJSError("example.com", "sig", true, base) | |||
| 74 | + | assert.True(t, shouldLog) | |||
| 75 | + | assert.Equal(t, uint64(runJSAggregateLogEvery), count) | |||
| 76 | + | } | |||
| 77 | + | ||||
| 78 | + | func TestShouldLogRunJSError_FatalAlwaysLogs(t *testing.T) { | |||
| 79 | + | wv := &WebView{} | |||
| 80 | + | base := time.Now() | |||
| 81 | + | ||||
| 82 | + | shouldLog, count := wv.shouldLogRunJSError("example.com", "fatal-sig", false, base) | |||
| 83 | + | assert.True(t, shouldLog) | |||
| 84 | + | assert.Equal(t, uint64(1), count) | |||
| 85 | + | ||||
| 86 | + | shouldLog, count = wv.shouldLogRunJSError("example.com", "fatal-sig", false, base) | |||
| 87 | + | assert.True(t, shouldLog) | |||
| 88 | + | assert.Equal(t, uint64(2), count) | |||
| 89 | + | } | |||
M internal/ui/adapter/favicon.go +66 -4
| @@ -12,6 +12,7 @@ import ( | |||
| 12 | 12 | "github.com/jwijenbergh/puregotk/v4/gdk" | |
| 13 | 13 | "github.com/jwijenbergh/puregotk/v4/gio" | |
| 14 | 14 | "github.com/jwijenbergh/puregotk/v4/glib" | |
| 15 | + | "github.com/rs/zerolog" | |
| 15 | 16 | ) | |
| 16 | 17 | ||
| 17 | 18 | // FaviconAdapter bridges the domain FaviconService to GTK by providing | |
| @@ -21,6 +22,8 @@ type FaviconAdapter struct { | |||
| 21 | 22 | faviconDB *webkit.FaviconDatabase | |
| 22 | 23 | textureCache map[string]*gdk.Texture | |
| 23 | 24 | mu sync.RWMutex | |
| 25 | + | warnMu sync.Mutex | |
| 26 | + | warnCounts map[string]int | |
| 24 | 27 | } | |
| 25 | 28 | ||
| 26 | 29 | // NewFaviconAdapter creates a new FaviconAdapter. | |
| @@ -30,6 +33,7 @@ func NewFaviconAdapter(service *favicon.Service, faviconDB *webkit.FaviconDataba | |||
| 30 | 33 | service: service, | |
| 31 | 34 | faviconDB: faviconDB, | |
| 32 | 35 | textureCache: make(map[string]*gdk.Texture), | |
| 36 | + | warnCounts: make(map[string]int), | |
| 33 | 37 | } | |
| 34 | 38 | } | |
| 35 | 39 | ||
| @@ -195,6 +199,9 @@ func (a *FaviconAdapter) StoreFromWebKitWithOrigin( | |||
| 195 | 199 | // Uses glib.IdleAdd to defer disk I/O until GTK main loop is idle, avoiding UI blocking. | |
| 196 | 200 | func (a *FaviconAdapter) saveFaviconToDisk(ctx context.Context, domain string, texture *gdk.Texture) { | |
| 197 | 201 | log := logging.FromContext(ctx) | |
| 202 | + | cacheDirWarnKey := "cache-dir:" + domain | |
| 203 | + | savePNGWarnKey := "save-png:" + domain | |
| 204 | + | sizedPNGWarnKey := "sized-png:" + domain | |
| 198 | 205 | ||
| 199 | 206 | // Check if PNG save is needed before scheduling idle callback | |
| 200 | 207 | pngPath := a.service.DiskPathPNG(domain) | |
| @@ -203,8 +210,12 @@ func (a *FaviconAdapter) saveFaviconToDisk(ctx context.Context, domain string, t | |||
| 203 | 210 | if needsPNGSave { | |
| 204 | 211 | // Ensure cache directory exists (cheap check, do it now) | |
| 205 | 212 | if err := a.service.EnsureCacheDir(); err != nil { | |
| 206 | - | log.Warn().Err(err).Str("domain", domain).Msg("failed to create favicon cache dir") | |
| 213 | + | a.logWarningDedup(ctx, cacheDirWarnKey, err, func(e *zerolog.Event) { | |
| 214 | + | e.Str("domain", domain).Msg("failed to create favicon cache dir") | |
| 215 | + | }) | |
| 207 | 216 | needsPNGSave = false | |
| 217 | + | } else { | |
| 218 | + | a.clearWarningDedup(cacheDirWarnKey) | |
| 208 | 219 | } | |
| 209 | 220 | } | |
| 210 | 221 | ||
| @@ -213,15 +224,21 @@ func (a *FaviconAdapter) saveFaviconToDisk(ctx context.Context, domain string, t | |||
| 213 | 224 | // texture.SaveToPng must run on main thread but we defer it to avoid blocking | |
| 214 | 225 | cb := glib.SourceFunc(func(_ uintptr) bool { | |
| 215 | 226 | if ok := texture.SaveToPng(pngPath); !ok { | |
| 216 | - | log.Warn().Str("domain", domain).Str("path", pngPath).Msg("failed to save favicon PNG") | |
| 227 | + | a.logWarningDedup(ctx, savePNGWarnKey, nil, func(e *zerolog.Event) { | |
| 228 | + | e.Str("domain", domain).Str("path", pngPath).Msg("failed to save favicon PNG") | |
| 229 | + | }) | |
| 217 | 230 | } else { | |
| 231 | + | a.clearWarningDedup(savePNGWarnKey) | |
| 218 | 232 | log.Debug().Str("domain", domain).Str("path", pngPath).Msg("saved favicon PNG") | |
| 219 | 233 | ||
| 220 | 234 | // Create normalized sized copy for dmenu/fuzzel (async) | |
| 221 | 235 | go func() { | |
| 222 | 236 | if err := a.service.EnsureSizedPNG(ctx, domain, favicon.NormalizedIconSize); err != nil { | |
| 223 | - | log.Warn().Err(err).Str("domain", domain).Msg("failed to create sized PNG") | |
| 237 | + | a.logWarningDedup(ctx, sizedPNGWarnKey, err, func(e *zerolog.Event) { | |
| 238 | + | e.Str("domain", domain).Msg("failed to create sized PNG") | |
| 239 | + | }) | |
| 224 | 240 | } else { | |
| 241 | + | a.clearWarningDedup(sizedPNGWarnKey) | |
| 225 | 242 | log.Debug().Str("domain", domain).Msg("created sized PNG") | |
| 226 | 243 | } | |
| 227 | 244 | }() | |
| @@ -318,13 +335,58 @@ func (a *FaviconAdapter) ensureSizedPNG(ctx context.Context, domain string) { | |||
| 318 | 335 | if hasPNG && !hasSized { | |
| 319 | 336 | log.Debug().Str("domain", domain).Msg("creating sized PNG") | |
| 320 | 337 | if err := a.service.EnsureSizedPNG(ctx, domain, favicon.NormalizedIconSize); err != nil { | |
| 321 | - | log.Warn().Err(err).Str("domain", domain).Msg("failed to create sized PNG") | |
| 338 | + | a.logWarningDedup(ctx, "sized-png:"+domain, err, func(e *zerolog.Event) { | |
| 339 | + | e.Str("domain", domain).Msg("failed to create sized PNG") | |
| 340 | + | }) | |
| 322 | 341 | } else { | |
| 342 | + | a.clearWarningDedup("sized-png:" + domain) | |
| 323 | 343 | log.Debug().Str("domain", domain).Msg("sized PNG created successfully") | |
| 324 | 344 | } | |
| 325 | 345 | } | |
| 326 | 346 | } | |
| 327 | 347 | ||
| 348 | + | func (a *FaviconAdapter) shouldLogWarningDedup(key string) (bool, int) { | |
| 349 | + | a.warnMu.Lock() | |
| 350 | + | defer a.warnMu.Unlock() | |
| 351 | + | ||
| 352 | + | count := a.warnCounts[key] + 1 | |
| 353 | + | a.warnCounts[key] = count | |
| 354 | + | ||
| 355 | + | return count == 1, count - 1 | |
| 356 | + | } | |
| 357 | + | ||
| 358 | + | func (a *FaviconAdapter) clearWarningDedup(key string) { | |
| 359 | + | a.warnMu.Lock() | |
| 360 | + | delete(a.warnCounts, key) | |
| 361 | + | a.warnMu.Unlock() | |
| 362 | + | } | |
| 363 | + | ||
| 364 | + | func (a *FaviconAdapter) logWarningDedup( | |
| 365 | + | ctx context.Context, | |
| 366 | + | key string, | |
| 367 | + | err error, | |
| 368 | + | warnFn func(*zerolog.Event), | |
| 369 | + | ) { | |
| 370 | + | log := logging.FromContext(ctx) | |
| 371 | + | logWarning, suppressed := a.shouldLogWarningDedup(key) | |
| 372 | + | if logWarning { | |
| 373 | + | event := log.Warn() | |
| 374 | + | if err != nil { | |
| 375 | + | event = event.Err(err) | |
| 376 | + | } | |
| 377 | + | warnFn(event) | |
| 378 | + | return | |
| 379 | + | } | |
| 380 | + | ||
| 381 | + | if suppressed == 1 { | |
| 382 | + | event := log.Debug() | |
| 383 | + | if err != nil { | |
| 384 | + | event = event.Err(err) | |
| 385 | + | } | |
| 386 | + | event.Str("dedupe_key", key).Msg("suppressing repeated favicon warning") | |
| 387 | + | } | |
| 388 | + | } | |
| 389 | + | ||
| 328 | 390 | // getOrCreateLogoTexture returns the app logo texture, creating it if needed. | |
| 329 | 391 | // The texture is cached under the InternalDomain key. | |
| 330 | 392 | func (a *FaviconAdapter) getOrCreateLogoTexture() *gdk.Texture { | |
| @@ -12,6 +12,7 @@ import ( | |||||
| 12 | "github.com/jwijenbergh/puregotk/v4/gdk" | 12 | "github.com/jwijenbergh/puregotk/v4/gdk" | ||
| 13 | "github.com/jwijenbergh/puregotk/v4/gio" | 13 | "github.com/jwijenbergh/puregotk/v4/gio" | ||
| 14 | "github.com/jwijenbergh/puregotk/v4/glib" | 14 | "github.com/jwijenbergh/puregotk/v4/glib" | ||
| 15 | + | "github.com/rs/zerolog" | |||
| 15 | ) | 16 | ) | ||
| 16 | 17 | ||||
| 17 | // FaviconAdapter bridges the domain FaviconService to GTK by providing | 18 | // FaviconAdapter bridges the domain FaviconService to GTK by providing | ||
| @@ -21,6 +22,8 @@ type FaviconAdapter struct { | |||||
| 21 | faviconDB *webkit.FaviconDatabase | 22 | faviconDB *webkit.FaviconDatabase | ||
| 22 | textureCache map[string]*gdk.Texture | 23 | textureCache map[string]*gdk.Texture | ||
| 23 | mu sync.RWMutex | 24 | mu sync.RWMutex | ||
| 25 | + | warnMu sync.Mutex | |||
| 26 | + | warnCounts map[string]int | |||
| 24 | } | 27 | } | ||
| 25 | 28 | ||||
| 26 | // NewFaviconAdapter creates a new FaviconAdapter. | 29 | // NewFaviconAdapter creates a new FaviconAdapter. | ||
| @@ -30,6 +33,7 @@ func NewFaviconAdapter(service *favicon.Service, faviconDB *webkit.FaviconDataba | |||||
| 30 | service: service, | 33 | service: service, | ||
| 31 | faviconDB: faviconDB, | 34 | faviconDB: faviconDB, | ||
| 32 | textureCache: make(map[string]*gdk.Texture), | 35 | textureCache: make(map[string]*gdk.Texture), | ||
| 36 | + | warnCounts: make(map[string]int), | |||
| 33 | } | 37 | } | ||
| 34 | } | 38 | } | ||
| 35 | 39 | ||||
| @@ -195,6 +199,9 @@ func (a *FaviconAdapter) StoreFromWebKitWithOrigin( | |||||
| 195 | // Uses glib.IdleAdd to defer disk I/O until GTK main loop is idle, avoiding UI blocking. | 199 | // Uses glib.IdleAdd to defer disk I/O until GTK main loop is idle, avoiding UI blocking. | ||
| 196 | func (a *FaviconAdapter) saveFaviconToDisk(ctx context.Context, domain string, texture *gdk.Texture) { | 200 | func (a *FaviconAdapter) saveFaviconToDisk(ctx context.Context, domain string, texture *gdk.Texture) { | ||
| 197 | log := logging.FromContext(ctx) | 201 | log := logging.FromContext(ctx) | ||
| 202 | + | cacheDirWarnKey := "cache-dir:" + domain | |||
| 203 | + | savePNGWarnKey := "save-png:" + domain | |||
| 204 | + | sizedPNGWarnKey := "sized-png:" + domain | |||
| 198 | 205 | ||||
| 199 | // Check if PNG save is needed before scheduling idle callback | 206 | // Check if PNG save is needed before scheduling idle callback | ||
| 200 | pngPath := a.service.DiskPathPNG(domain) | 207 | pngPath := a.service.DiskPathPNG(domain) | ||
| @@ -203,8 +210,12 @@ func (a *FaviconAdapter) saveFaviconToDisk(ctx context.Context, domain string, t | |||||
| 203 | if needsPNGSave { | 210 | if needsPNGSave { | ||
| 204 | // Ensure cache directory exists (cheap check, do it now) | 211 | // Ensure cache directory exists (cheap check, do it now) | ||
| 205 | if err := a.service.EnsureCacheDir(); err != nil { | 212 | if err := a.service.EnsureCacheDir(); err != nil { | ||
| 206 | - | log.Warn().Err(err).Str("domain", domain).Msg("failed to create favicon cache dir") | 213 | + | a.logWarningDedup(ctx, cacheDirWarnKey, err, func(e *zerolog.Event) { |
| 214 | + | e.Str("domain", domain).Msg("failed to create favicon cache dir") | |||
| 215 | + | }) | |||
| 207 | needsPNGSave = false | 216 | needsPNGSave = false | ||
| 217 | + | } else { | |||
| 218 | + | a.clearWarningDedup(cacheDirWarnKey) | |||
| 208 | } | 219 | } | ||
| 209 | } | 220 | } | ||
| 210 | 221 | ||||
| @@ -213,15 +224,21 @@ func (a *FaviconAdapter) saveFaviconToDisk(ctx context.Context, domain string, t | |||||
| 213 | // texture.SaveToPng must run on main thread but we defer it to avoid blocking | 224 | // texture.SaveToPng must run on main thread but we defer it to avoid blocking | ||
| 214 | cb := glib.SourceFunc(func(_ uintptr) bool { | 225 | cb := glib.SourceFunc(func(_ uintptr) bool { | ||
| 215 | if ok := texture.SaveToPng(pngPath); !ok { | 226 | if ok := texture.SaveToPng(pngPath); !ok { | ||
| 216 | - | log.Warn().Str("domain", domain).Str("path", pngPath).Msg("failed to save favicon PNG") | 227 | + | a.logWarningDedup(ctx, savePNGWarnKey, nil, func(e *zerolog.Event) { |
| 228 | + | e.Str("domain", domain).Str("path", pngPath).Msg("failed to save favicon PNG") | |||
| 229 | + | }) | |||
| 217 | } else { | 230 | } else { | ||
| 231 | + | a.clearWarningDedup(savePNGWarnKey) | |||
| 218 | log.Debug().Str("domain", domain).Str("path", pngPath).Msg("saved favicon PNG") | 232 | log.Debug().Str("domain", domain).Str("path", pngPath).Msg("saved favicon PNG") | ||
| 219 | 233 | ||||
| 220 | // Create normalized sized copy for dmenu/fuzzel (async) | 234 | // Create normalized sized copy for dmenu/fuzzel (async) | ||
| 221 | go func() { | 235 | go func() { | ||
| 222 | if err := a.service.EnsureSizedPNG(ctx, domain, favicon.NormalizedIconSize); err != nil { | 236 | if err := a.service.EnsureSizedPNG(ctx, domain, favicon.NormalizedIconSize); err != nil { | ||
| 223 | - | log.Warn().Err(err).Str("domain", domain).Msg("failed to create sized PNG") | 237 | + | a.logWarningDedup(ctx, sizedPNGWarnKey, err, func(e *zerolog.Event) { |
| 238 | + | e.Str("domain", domain).Msg("failed to create sized PNG") | |||
| 239 | + | }) | |||
| 224 | } else { | 240 | } else { | ||
| 241 | + | a.clearWarningDedup(sizedPNGWarnKey) | |||
| 225 | log.Debug().Str("domain", domain).Msg("created sized PNG") | 242 | log.Debug().Str("domain", domain).Msg("created sized PNG") | ||
| 226 | } | 243 | } | ||
| 227 | }() | 244 | }() | ||
| @@ -318,13 +335,58 @@ func (a *FaviconAdapter) ensureSizedPNG(ctx context.Context, domain string) { | |||||
| 318 | if hasPNG && !hasSized { | 335 | if hasPNG && !hasSized { | ||
| 319 | log.Debug().Str("domain", domain).Msg("creating sized PNG") | 336 | log.Debug().Str("domain", domain).Msg("creating sized PNG") | ||
| 320 | if err := a.service.EnsureSizedPNG(ctx, domain, favicon.NormalizedIconSize); err != nil { | 337 | if err := a.service.EnsureSizedPNG(ctx, domain, favicon.NormalizedIconSize); err != nil { | ||
| 321 | - | log.Warn().Err(err).Str("domain", domain).Msg("failed to create sized PNG") | 338 | + | a.logWarningDedup(ctx, "sized-png:"+domain, err, func(e *zerolog.Event) { |
| 339 | + | e.Str("domain", domain).Msg("failed to create sized PNG") | |||
| 340 | + | }) | |||
| 322 | } else { | 341 | } else { | ||
| 342 | + | a.clearWarningDedup("sized-png:" + domain) | |||
| 323 | log.Debug().Str("domain", domain).Msg("sized PNG created successfully") | 343 | log.Debug().Str("domain", domain).Msg("sized PNG created successfully") | ||
| 324 | } | 344 | } | ||
| 325 | } | 345 | } | ||
| 326 | } | 346 | } | ||
| 327 | 347 | ||||
| 348 | + | func (a *FaviconAdapter) shouldLogWarningDedup(key string) (bool, int) { | |||
| 349 | + | a.warnMu.Lock() | |||
| 350 | + | defer a.warnMu.Unlock() | |||
| 351 | + | ||||
| 352 | + | count := a.warnCounts[key] + 1 | |||
| 353 | + | a.warnCounts[key] = count | |||
| 354 | + | ||||
| 355 | + | return count == 1, count - 1 | |||
| 356 | + | } | |||
| 357 | + | ||||
| 358 | + | func (a *FaviconAdapter) clearWarningDedup(key string) { | |||
| 359 | + | a.warnMu.Lock() | |||
| 360 | + | delete(a.warnCounts, key) | |||
| 361 | + | a.warnMu.Unlock() | |||
| 362 | + | } | |||
| 363 | + | ||||
| 364 | + | func (a *FaviconAdapter) logWarningDedup( | |||
| 365 | + | ctx context.Context, | |||
| 366 | + | key string, | |||
| 367 | + | err error, | |||
| 368 | + | warnFn func(*zerolog.Event), | |||
| 369 | + | ) { | |||
| 370 | + | log := logging.FromContext(ctx) | |||
| 371 | + | logWarning, suppressed := a.shouldLogWarningDedup(key) | |||
| 372 | + | if logWarning { | |||
| 373 | + | event := log.Warn() | |||
| 374 | + | if err != nil { | |||
| 375 | + | event = event.Err(err) | |||
| 376 | + | } | |||
| 377 | + | warnFn(event) | |||
| 378 | + | return | |||
| 379 | + | } | |||
| 380 | + | ||||
| 381 | + | if suppressed == 1 { | |||
| 382 | + | event := log.Debug() | |||
| 383 | + | if err != nil { | |||
| 384 | + | event = event.Err(err) | |||
| 385 | + | } | |||
| 386 | + | event.Str("dedupe_key", key).Msg("suppressing repeated favicon warning") | |||
| 387 | + | } | |||
| 388 | + | } | |||
| 389 | + | ||||
| 328 | // getOrCreateLogoTexture returns the app logo texture, creating it if needed. | 390 | // getOrCreateLogoTexture returns the app logo texture, creating it if needed. | ||
| 329 | // The texture is cached under the InternalDomain key. | 391 | // The texture is cached under the InternalDomain key. | ||
| 330 | func (a *FaviconAdapter) getOrCreateLogoTexture() *gdk.Texture { | 392 | func (a *FaviconAdapter) getOrCreateLogoTexture() *gdk.Texture { | ||
A internal/ui/adapter/favicon_test.go +51 -0
| @@ -0,0 +1,51 @@ | |||
| 1 | + | package adapter | |
| 2 | + | ||
| 3 | + | import "testing" | |
| 4 | + | ||
| 5 | + | func TestFaviconWarningDedup_FirstAndRepeated(t *testing.T) { | |
| 6 | + | adapter := NewFaviconAdapter(nil, nil) | |
| 7 | + | ||
| 8 | + | first, suppressed := adapter.shouldLogWarningDedup("save-png:example.com") | |
| 9 | + | if !first { | |
| 10 | + | t.Fatalf("expected first warning to be logged") | |
| 11 | + | } | |
| 12 | + | if suppressed != 0 { | |
| 13 | + | t.Fatalf("expected no suppressed count on first warning, got %d", suppressed) | |
| 14 | + | } | |
| 15 | + | ||
| 16 | + | first, suppressed = adapter.shouldLogWarningDedup("save-png:example.com") | |
| 17 | + | if first { | |
| 18 | + | t.Fatalf("expected repeated warning to be suppressed") | |
| 19 | + | } | |
| 20 | + | if suppressed != 1 { | |
| 21 | + | t.Fatalf("expected suppressed count to be 1, got %d", suppressed) | |
| 22 | + | } | |
| 23 | + | ||
| 24 | + | first, suppressed = adapter.shouldLogWarningDedup("save-png:example.com") | |
| 25 | + | if first { | |
| 26 | + | t.Fatalf("expected repeated warning to remain suppressed") | |
| 27 | + | } | |
| 28 | + | if suppressed != 2 { | |
| 29 | + | t.Fatalf("expected suppressed count to be 2, got %d", suppressed) | |
| 30 | + | } | |
| 31 | + | } | |
| 32 | + | ||
| 33 | + | func TestFaviconWarningDedup_ClearResetsState(t *testing.T) { | |
| 34 | + | adapter := NewFaviconAdapter(nil, nil) | |
| 35 | + | key := "sized-png:example.com" | |
| 36 | + | ||
| 37 | + | first, _ := adapter.shouldLogWarningDedup(key) | |
| 38 | + | if !first { | |
| 39 | + | t.Fatalf("expected first warning to be logged") | |
| 40 | + | } | |
| 41 | + | ||
| 42 | + | adapter.clearWarningDedup(key) | |
| 43 | + | ||
| 44 | + | first, suppressed := adapter.shouldLogWarningDedup(key) | |
| 45 | + | if !first { | |
| 46 | + | t.Fatalf("expected warning to log again after clear") | |
| 47 | + | } | |
| 48 | + | if suppressed != 0 { | |
| 49 | + | t.Fatalf("expected suppressed count reset after clear, got %d", suppressed) | |
| 50 | + | } | |
| 51 | + | } | |
| @@ -0,0 +1,51 @@ | |||||
| 1 | + | package adapter | |||
| 2 | + | ||||
| 3 | + | import "testing" | |||
| 4 | + | ||||
| 5 | + | func TestFaviconWarningDedup_FirstAndRepeated(t *testing.T) { | |||
| 6 | + | adapter := NewFaviconAdapter(nil, nil) | |||
| 7 | + | ||||
| 8 | + | first, suppressed := adapter.shouldLogWarningDedup("save-png:example.com") | |||
| 9 | + | if !first { | |||
| 10 | + | t.Fatalf("expected first warning to be logged") | |||
| 11 | + | } | |||
| 12 | + | if suppressed != 0 { | |||
| 13 | + | t.Fatalf("expected no suppressed count on first warning, got %d", suppressed) | |||
| 14 | + | } | |||
| 15 | + | ||||
| 16 | + | first, suppressed = adapter.shouldLogWarningDedup("save-png:example.com") | |||
| 17 | + | if first { | |||
| 18 | + | t.Fatalf("expected repeated warning to be suppressed") | |||
| 19 | + | } | |||
| 20 | + | if suppressed != 1 { | |||
| 21 | + | t.Fatalf("expected suppressed count to be 1, got %d", suppressed) | |||
| 22 | + | } | |||
| 23 | + | ||||
| 24 | + | first, suppressed = adapter.shouldLogWarningDedup("save-png:example.com") | |||
| 25 | + | if first { | |||
| 26 | + | t.Fatalf("expected repeated warning to remain suppressed") | |||
| 27 | + | } | |||
| 28 | + | if suppressed != 2 { | |||
| 29 | + | t.Fatalf("expected suppressed count to be 2, got %d", suppressed) | |||
| 30 | + | } | |||
| 31 | + | } | |||
| 32 | + | ||||
| 33 | + | func TestFaviconWarningDedup_ClearResetsState(t *testing.T) { | |||
| 34 | + | adapter := NewFaviconAdapter(nil, nil) | |||
| 35 | + | key := "sized-png:example.com" | |||
| 36 | + | ||||
| 37 | + | first, _ := adapter.shouldLogWarningDedup(key) | |||
| 38 | + | if !first { | |||
| 39 | + | t.Fatalf("expected first warning to be logged") | |||
| 40 | + | } | |||
| 41 | + | ||||
| 42 | + | adapter.clearWarningDedup(key) | |||
| 43 | + | ||||
| 44 | + | first, suppressed := adapter.shouldLogWarningDedup(key) | |||
| 45 | + | if !first { | |||
| 46 | + | t.Fatalf("expected warning to log again after clear") | |||
| 47 | + | } | |||
| 48 | + | if suppressed != 0 { | |||
| 49 | + | t.Fatalf("expected suppressed count reset after clear, got %d", suppressed) | |||
| 50 | + | } | |||
| 51 | + | } | |||
M internal/ui/coordinator/content.go +2 -4
| @@ -928,11 +928,9 @@ func (c *ContentCoordinator) PreloadCachedFavicon(ctx context.Context, paneID en | |||
| 928 | 928 | ||
| 929 | 929 | // Check memory and disk cache (no external fetch) | |
| 930 | 930 | texture := c.faviconAdapter.PreloadFromCache(uri) | |
| 931 | - | if texture == nil { | |
| 932 | - | return | |
| 933 | - | } | |
| 934 | 931 | ||
| 935 | - | // Update stacked pane favicon if applicable | |
| 932 | + | // Update stacked pane favicon if applicable. | |
| 933 | + | // A nil texture triggers the default icon fallback, which avoids stale favicons. | |
| 936 | 934 | _, wsView := c.getActiveWS() | |
| 937 | 935 | if wsView != nil { | |
| 938 | 936 | tr := wsView.TreeRenderer() | |
| @@ -928,11 +928,9 @@ func (c *ContentCoordinator) PreloadCachedFavicon(ctx context.Context, paneID en | |||||
| 928 | 928 | ||||
| 929 | // Check memory and disk cache (no external fetch) | 929 | // Check memory and disk cache (no external fetch) | ||
| 930 | texture := c.faviconAdapter.PreloadFromCache(uri) | 930 | texture := c.faviconAdapter.PreloadFromCache(uri) | ||
| 931 | - | if texture == nil { | |||
| 932 | - | return | |||
| 933 | - | } | |||
| 934 | 931 | ||||
| 935 | - | // Update stacked pane favicon if applicable | 932 | + | // Update stacked pane favicon if applicable. |
| 933 | + | // A nil texture triggers the default icon fallback, which avoids stale favicons. | |||
| 936 | _, wsView := c.getActiveWS() | 934 | _, wsView := c.getActiveWS() | ||
| 937 | if wsView != nil { | 935 | if wsView != nil { | ||
| 938 | tr := wsView.TreeRenderer() | 936 | tr := wsView.TreeRenderer() | ||