From 2c543be48125869c302debf9c5adbf3984c91617 Mon Sep 17 00:00:00 2001 From: Calmcacil B <716671+calmcacil@users.noreply.github.com> Date: Mon, 1 Jun 2026 22:09:37 +0200 Subject: [PATCH] fix: resolve calendar field shadowing, Retry-After header, log ordering, CHANGELOG Fixes from second architecture review: - Remove shadowing WatchMode/WatchSeconds fields from calendar.ToolConfig (the outer fields silently shadowed embedded core.CommonConfig.Watch and core.CommonConfig.WatchSeconds; renamed cfg.WatchMode -> cfg.Watch) - Add HTTP Retry-After header support to DoRequest (was only parsing JSON body for retryAfter, ignoring the standard header; now prefers the header and falls back to body parsing) - Replace slogx.Default().Warn() with fmt.Fprintf in LoadAndValidate (startup warming was emitted before logger was configured, causing text-only format even under --json) - Fix CHANGELOG incorrectly claiming NewTransportClient was removed --- CHANGELOG.md | 1 - cmd/media-calendar/main.go | 2 +- internal/calendar/calendar.go | 3 +-- internal/calendar/render.go | 6 ++--- internal/cmdutil/cmdutil.go | 4 +-- internal/feed/client.go | 8 +++--- internal/httputil/client.go | 44 ++++++++++++++++++++++++-------- internal/httputil/client_test.go | 27 ++++++++++++++++++-- 8 files changed, 70 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9bc84e..bffad10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,7 +32,6 @@ Architecture refactor — no user-facing API changes. - Hard-coded 10 MiB body cap in `DoRequest`. - Duplicated theme/flag/validation code in every `main.go`. - Duplicated timeout/no-color/theme parsing in every `BuildToolConfig`. -- `NewTransportClient` (dead code). ### Fixed - Error messages now consistently go to stderr (via slog). diff --git a/cmd/media-calendar/main.go b/cmd/media-calendar/main.go index b3a7001..5d2a9e3 100644 --- a/cmd/media-calendar/main.go +++ b/cmd/media-calendar/main.go @@ -34,7 +34,7 @@ func main() { cfg.NoColor = cu.NoColor cfg.Theme = cu.Theme cfg.JSONOutput = cu.JSONFlag() - cfg.WatchMode = cu.Watch + cfg.Watch = cu.Watch cfg.WatchSeconds = cu.WatchSeconds cfg.Debug = cu.Debug cfg.NoBanner = *noBanner diff --git a/internal/calendar/calendar.go b/internal/calendar/calendar.go index d8f8d4f..1c44a8c 100644 --- a/internal/calendar/calendar.go +++ b/internal/calendar/calendar.go @@ -94,8 +94,6 @@ type ToolConfig struct { RadarrInstances []config.ArrInstance Days int DaysPast int - WatchMode bool - WatchSeconds int NoBanner bool Filter string MonitoredOnly bool @@ -149,6 +147,7 @@ func BuildToolConfig(tk *config.ToolkitConfig) ToolConfig { cfg.Timeout = 10 * time.Second cfg.Days = 1 cfg.WatchSeconds = 300 + cfg.Watch = false return cfg } diff --git a/internal/calendar/render.go b/internal/calendar/render.go index f1f1f35..ae0c254 100644 --- a/internal/calendar/render.go +++ b/internal/calendar/render.go @@ -80,7 +80,7 @@ func dataAgent(ctx context.Context, cfg ToolConfig, ch chan<- dataEvent) { case <-ctx.Done(): return } - if !cfg.WatchMode { + if !cfg.Watch { return } if err != nil { @@ -123,7 +123,7 @@ func runWithSubagents(ctx context.Context, cfg ToolConfig, p *colors.Palette) er termWidth = re.width case de := <-dataCh: if de.err != nil { - if !cfg.WatchMode { + if !cfg.Watch { return de.err } fmt.Fprintf(os.Stderr, "WARNING: %v (retrying)\n", de.err) @@ -137,7 +137,7 @@ func runWithSubagents(ctx context.Context, cfg ToolConfig, p *colors.Palette) er fmt.Print(colors.ClearScreen + colors.HomeCursor) renderCalendar(cfg, items, issues, termWidth, p) - if !cfg.WatchMode { + if !cfg.Watch { return nil } diff --git a/internal/cmdutil/cmdutil.go b/internal/cmdutil/cmdutil.go index 97559ba..9b100f4 100644 --- a/internal/cmdutil/cmdutil.go +++ b/internal/cmdutil/cmdutil.go @@ -125,11 +125,11 @@ func (c *Common) JSONFlag() bool { func LoadAndValidate() *config.ToolkitConfig { tk, err := config.LoadToolkitConfig() if err != nil { - slogx.Default().Warn("config not loaded, using defaults", "err", err) + fmt.Fprintf(os.Stderr, "WARN: config not loaded, using defaults: %v\n", err) } if tk != nil { if err := tk.Validate(); err != nil { - slogx.Default().Warn("config validation", "err", err) + fmt.Fprintf(os.Stderr, "WARN: config validation: %v\n", err) } } return tk diff --git a/internal/feed/client.go b/internal/feed/client.go index 6fdd562..91ec3cb 100644 --- a/internal/feed/client.go +++ b/internal/feed/client.go @@ -74,7 +74,7 @@ func fetchSonarrHistory(ctx context.Context, client *httputil.Client, inst confi endpoint := fmt.Sprintf("%s/api/v3/history/since?date=%s&includeEpisode=true&includeSeries=true", inst.URL, sinceStr) headers := map[string]string{"X-Api-Key": inst.APIKey} - body, status, err := client.DoRequest(ctx, "GET", endpoint, headers, nil) + body, status, _, err := client.DoRequest(ctx, "GET", endpoint, headers, nil) if err != nil { return nil, err } @@ -126,7 +126,7 @@ func enrichSonarrSubtitles(ctx context.Context, client *httputil.Client, inst co } headers := map[string]string{"X-Api-Key": inst.APIKey} - body, status, err := client.DoRequest(ctx, "GET", endpoint, headers, nil) + body, status, _, err := client.DoRequest(ctx, "GET", endpoint, headers, nil) if err != nil || status != http.StatusOK { return } @@ -155,7 +155,7 @@ func fetchRadarrHistory(ctx context.Context, client *httputil.Client, inst confi endpoint := fmt.Sprintf("%s/api/v3/history/since?date=%s&includeMovie=true", inst.URL, sinceStr) headers := map[string]string{"X-Api-Key": inst.APIKey} - body, status, err := client.DoRequest(ctx, "GET", endpoint, headers, nil) + body, status, _, err := client.DoRequest(ctx, "GET", endpoint, headers, nil) if err != nil { return nil, err } @@ -207,7 +207,7 @@ func enrichRadarrSubtitles(ctx context.Context, client *httputil.Client, inst co } headers := map[string]string{"X-Api-Key": inst.APIKey} - body, status, err := client.DoRequest(ctx, "GET", endpoint, headers, nil) + body, status, _, err := client.DoRequest(ctx, "GET", endpoint, headers, nil) if err != nil || status != http.StatusOK { return } diff --git a/internal/httputil/client.go b/internal/httputil/client.go index 3fcc71d..f6edbd4 100644 --- a/internal/httputil/client.go +++ b/internal/httputil/client.go @@ -80,36 +80,58 @@ func jitter(d time.Duration) time.Duration { } // DoRequest performs an HTTP request and returns the raw body bytes and HTTP status code. -func (c *Client) DoRequest(ctx context.Context, method, url string, headers map[string]string, body io.Reader) ([]byte, int, error) { +func (c *Client) DoRequest(ctx context.Context, method, url string, headers map[string]string, body io.Reader) ([]byte, int, time.Duration, error) { req, err := http.NewRequestWithContext(ctx, method, url, body) if err != nil { - return nil, 0, err + return nil, 0, 0, err } for k, v := range headers { req.Header.Set(k, v) } resp, err := c.Do(req) if err != nil { - return nil, 0, err + return nil, 0, 0, err } defer resp.Body.Close() + + retryAfter := parseRetryAfterHeader(resp) + maxSize := c.MaxBodySize if maxSize <= 0 { maxSize = 10 * 1024 * 1024 } data, err := io.ReadAll(io.LimitReader(resp.Body, maxSize+1)) if err != nil { - return nil, resp.StatusCode, err + return nil, resp.StatusCode, retryAfter, err } if int64(len(data)) > maxSize { - return nil, resp.StatusCode, fmt.Errorf("response body too large (exceeds %d bytes)", maxSize) + return nil, resp.StatusCode, retryAfter, fmt.Errorf("response body too large (exceeds %d bytes)", maxSize) + } + return data, resp.StatusCode, retryAfter, nil +} + +func parseRetryAfterHeader(resp *http.Response) time.Duration { + if resp == nil { + return 0 + } + h := resp.Header.Get("Retry-After") + if h == "" { + return 0 } - return data, resp.StatusCode, nil + if s, err := strconv.Atoi(h); err == nil && s > 0 { + return time.Duration(s) * time.Second + } + if t, err := time.Parse(time.RFC1123, h); err == nil { + if d := time.Until(t); d > 0 { + return d + } + } + return 0 } // DoJSON performs an HTTP request and decodes the JSON response into result. func (c *Client) DoJSON(ctx context.Context, method, url string, headers map[string]string, body io.Reader, result interface{}) error { - data, status, err := c.DoRequest(ctx, method, url, headers, body) + data, status, _, err := c.DoRequest(ctx, method, url, headers, body) if err != nil { return err } @@ -126,7 +148,7 @@ func (c *Client) DoJSON(ctx context.Context, method, url string, headers map[str // DoXML performs an HTTP request and decodes the XML response into result. func (c *Client) DoXML(ctx context.Context, method, url string, headers map[string]string, body io.Reader, result interface{}) error { - data, status, err := c.DoRequest(ctx, method, url, headers, body) + data, status, _, err := c.DoRequest(ctx, method, url, headers, body) if err != nil { return err } @@ -172,11 +194,13 @@ type responseHandler func([]byte, int) error func (c *Client) doWithRetry(ctx context.Context, method, url string, headers map[string]string, body io.Reader, fn responseHandler, rc RetryConfig) error { backoff := rc.InitialBackoff for attempt := 1; attempt <= rc.MaxAttempts; attempt++ { - data, status, err := c.DoRequest(ctx, method, url, headers, body) + data, status, retryAfter, err := c.DoRequest(ctx, method, url, headers, body) if err == nil { if rc.shouldRetry(status) && attempt < rc.MaxAttempts { if rc.RespectRetryAfter { - if ra := parseRetryAfter(string(data)); ra > 0 && ra < rc.MaxBackoff { + if retryAfter > 0 && retryAfter < rc.MaxBackoff { + backoff = retryAfter + } else if ra := parseRetryAfter(string(data)); ra > 0 && ra < rc.MaxBackoff { backoff = ra } } diff --git a/internal/httputil/client_test.go b/internal/httputil/client_test.go index 318ff10..81d462e 100644 --- a/internal/httputil/client_test.go +++ b/internal/httputil/client_test.go @@ -33,7 +33,7 @@ func TestDoRequest(t *testing.T) { ctx := context.Background() client := NewClient(5 * time.Second) - body, status, err := client.DoRequest(ctx, "GET", server.URL, nil, nil) + body, status, _, err := client.DoRequest(ctx, "GET", server.URL, nil, nil) if err != nil { t.Fatalf("DoRequest error: %v", err) } @@ -121,7 +121,7 @@ func TestLargeBodyRejection(t *testing.T) { ctx := context.Background() client := NewClient(5 * time.Second) - _, _, err := client.DoRequest(ctx, "GET", server.URL, nil, nil) + _, _, _, err := client.DoRequest(ctx, "GET", server.URL, nil, nil) if err == nil { t.Fatal("Expected error for large body, got nil") } @@ -129,3 +129,26 @@ func TestLargeBodyRejection(t *testing.T) { t.Errorf("Error should mention too large, got: %v", err) } } + +func TestRetryAfterFromHeader(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "30") + w.WriteHeader(http.StatusTooManyRequests) + w.Write([]byte(`{}`)) + })) + defer server.Close() + + ctx := context.Background() + client := NewClient(5 * time.Second) + + _, status, retryAfter, err := client.DoRequest(ctx, "GET", server.URL, nil, nil) + if err != nil { + t.Fatalf("DoRequest error: %v", err) + } + if status != http.StatusTooManyRequests { + t.Errorf("status = %d, want 429", status) + } + if retryAfter != 30*time.Second { + t.Errorf("retryAfter = %v, want 30s", retryAfter) + } +}