From 7f299e56c33052d25750c89d0504fb2027c6b837 Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Thu, 20 Aug 2026 12:59:53 -0400 Subject: [PATCH 1/2] feat(ratelimit): pace every Snipe-IT request from the API's own headers Syncs were tripping 429s and then crawling through backoff. Three reasons, all fixed here by moving the rate-limit machinery into go-snipeit and consuming it: 1. Nothing read the rate-limit headers Snipe-IT sends on every response (X-Ratelimit-Limit/-Remaining/-Reset). The only signal acted on was the 429 itself, after the fact. 2. Licenses ran on a second, unlimited HTTP client. Asset traffic was paced while license traffic was not, and neither knew what the other had spent. 3. The one limiter that existed was a fixed 2 req/s, off by default in practice, and unrelated to the instance's actual plan allowance. - sync.rate_limit is now a plan name: basic (120/min), small_business (240/min, default), or dedicated (no client-side limit). The legacy booleans still parse. An unknown name fails validation instead of silently disabling limiting. - snipe.New builds go-snipeit's AdaptiveRateLimiter from the plan, which tightens as X-Ratelimit-Remaining drains and holds requests until the window resets. Remaining budget is logged per response, at warn once a quarter of the window is left. - LicenseClient now borrows the shared client's connection instead of dialing its own, so both spend one budget through one limiter. Its hand-rolled HTTP layer, retry loop, and Retry-After parsing are gone; the SDK's licenses and seats service does the work. - The local retry429 wrapper is gone too. go-snipeit retries 429s for every method, retries 5xx/transport failures only for idempotent ones (plus PATCH, whose bodies here are absolute), and clamps server-provided waits. Verified against campus-students: "snipe-it rate limit limit=240 remaining=238 resets_in=48s". go.mod points go-snipeit at the fork until michellepellon/go-snipeit#9 and #10 merge; drop the replace afterwards. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016PFsj8aMUiWqyi7ViXmozp --- README.md | 4 +- cmd/license_setup.go | 7 +- cmd/licenses.go | 5 +- cmd/setup.go | 2 +- cmd/sync.go | 2 +- cmd/test.go | 2 +- config/config.go | 44 ++++- config/config_test.go | 51 ++++++ go.mod | 2 + go.sum | 4 +- settings.example.yaml | 5 +- snipe/client.go | 214 ++++++++-------------- snipe/client_test.go | 51 +++++- snipe/licenses.go | 407 +++++++++-------------------------------- snipe/licenses_test.go | 68 +++---- 15 files changed, 355 insertions(+), 513 deletions(-) diff --git a/README.md b/README.md index 72aa78d..7456298 100644 --- a/README.md +++ b/README.md @@ -255,7 +255,7 @@ sync: - **Model & manufacturer:** the Snipe-IT model is auto-created from the `model` string. ChromeOS has no separate vendor field, so the manufacturer is derived from the **first token** of the model (e.g. `Lenovo` from `Lenovo 300e Chromebook`), resolved against `snipe_it.manufacturer_ids` (lowercased vendor → ID), auto-created if absent, or `snipe_it.default_manufacturer_id` as a fallback. - **Custom-field rejection retry:** if Snipe-IT rejects fields with "not available on this Asset Model's fieldset", the bad keys are stripped and the PATCH is retried once so the rest applies. Re-run `setup` to fix the underlying fieldset. - **Cache:** every fetch writes `.cache/devices.json` (ChromeOS devices) and `.cache/users.json` (the Snipe-IT user list used for checkout matching); `--use-cache` replays both without re-paginating the APIs (device raw JSON is restored so gjson mapping still works). Models and manufacturers are always fetched fresh, since they're created during syncs. -- **Rate limiting:** Snipe-IT writes go through a token-bucket limiter (`sync.rate_limit: true`). +- **Rate limiting:** every Snipe-IT request — assets *and* licenses — goes through one adaptive limiter, sized by the plan named in `sync.rate_limit`: `basic` (120 req/min), `small_business` (240 req/min, the default), or `dedicated` (no client-side limit). The limiter also reads the API's `X-Ratelimit-Limit`/`-Remaining`/`-Reset` headers on every response and slows down as the window drains, so a shared token or a busy instance throttles this tool instead of tripping 429s. Legacy `true`/`false` values still parse as `small_business`/`dedicated`. ## Configuration reference @@ -265,7 +265,7 @@ See [`settings.example.yaml`](settings.example.yaml) for a fully-commented templ google: # credentials_file, impersonate_subject, customer_id, projection, org_unit_path, query snipe_it: # url, api_key, default_status_id, default_category_id, default_manufacturer_id, # custom_fieldset_id, status_map, manufacturer_ids -sync: # dry_run, force, rate_limit, concurrency (default 8; 1=serial), update_only, use_cache, +sync: # dry_run, force, rate_limit (basic|small_business|dedicated), concurrency (default 8; 1=serial), update_only, use_cache, # cache_dir, set_name, name_template, asset_tag.template, field_mapping (managed by setup), # checkout {...} licenses: # enabled, default_license_category_id, chrome {...}, workspace {...} diff --git a/cmd/license_setup.go b/cmd/license_setup.go index 7c746be..c412664 100644 --- a/cmd/license_setup.go +++ b/cmd/license_setup.go @@ -62,8 +62,11 @@ func runLicensesSetup(cmd *cobra.Command, args []string) error { if catName == "" { catName = "Software Licenses" } - lc := snipe.NewLicenseClient(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, false, licLog) - id, err := lc.EnsureLicenseCategory(cmd.Context(), catName) + sc, err := snipe.New(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, false, string(cfg.Sync.RateLimit), licLog) + if err != nil { + return err + } + id, err := snipe.NewLicenseClient(sc).EnsureLicenseCategory(cmd.Context(), catName) if err != nil { return fmt.Errorf("creating license category: %w", err) } diff --git a/cmd/licenses.go b/cmd/licenses.go index e878794..257f7d7 100644 --- a/cmd/licenses.go +++ b/cmd/licenses.go @@ -53,11 +53,12 @@ func runLicensesSync(cmd *cobra.Command, args []string) error { cfg.Sync.UseCache = cfg.Sync.UseCache || licUseCache // asset lookups via the existing go-snipeit-backed client - sc, err := snipe.New(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, cfg.Sync.DryRun, cfg.Sync.RateLimit, snipeLog) + sc, err := snipe.New(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, cfg.Sync.DryRun, string(cfg.Sync.RateLimit), snipeLog) if err != nil { return err } - lc := snipe.NewLicenseClient(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, cfg.Sync.DryRun, licLog) + // The license client shares sc's connection, so both spend one rate-limit budget. + lc := snipe.NewLicenseClient(sc) engine := licensesync.New(lc, licLog, licensesync.WithConcurrency(cfg.Sync.Concurrency)) scopes := config.EffectiveLicenseScopes(cfg) diff --git a/cmd/setup.go b/cmd/setup.go index 3c2ab1d..ce82e5d 100644 --- a/cmd/setup.go +++ b/cmd/setup.go @@ -27,7 +27,7 @@ func runSetup(cmd *cobra.Command, args []string) error { if err != nil { return err } - sc, err := snipe.New(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, setupDryRun, cfg.Sync.RateLimit, snipeLog) + sc, err := snipe.New(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, setupDryRun, string(cfg.Sync.RateLimit), snipeLog) if err != nil { return err } diff --git a/cmd/sync.go b/cmd/sync.go index f54265b..2d554d0 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -71,7 +71,7 @@ func runSync(cmd *cobra.Command, args []string) error { cfg.Sync.Concurrency = syncConcurrency } - sc, err := snipe.New(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, cfg.Sync.DryRun, cfg.Sync.RateLimit, snipeLog) + sc, err := snipe.New(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, cfg.Sync.DryRun, string(cfg.Sync.RateLimit), snipeLog) if err != nil { return err } diff --git a/cmd/test.go b/cmd/test.go index ac2f3e8..e1a9029 100644 --- a/cmd/test.go +++ b/cmd/test.go @@ -31,7 +31,7 @@ func runTest(cmd *cobra.Command, args []string) error { } googleLog.WithField("customer_id", customer).Warn("google admin sdk: OK") - sc, err := snipe.New(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, true, cfg.Sync.RateLimit, snipeLog) + sc, err := snipe.New(cfg.SnipeIT.URL, cfg.SnipeIT.APIKey, true, string(cfg.Sync.RateLimit), snipeLog) if err != nil { return err } diff --git a/config/config.go b/config/config.go index c18d24c..838649b 100644 --- a/config/config.go +++ b/config/config.go @@ -41,7 +41,7 @@ type SnipeITConfig struct { type SyncConfig struct { DryRun bool `yaml:"dry_run"` Force bool `yaml:"force"` - RateLimit bool `yaml:"rate_limit"` + RateLimit RateLimitSetting `yaml:"rate_limit"` UpdateOnly bool `yaml:"update_only"` UseCache bool `yaml:"use_cache"` CacheDir string `yaml:"cache_dir"` @@ -54,6 +54,40 @@ type SyncConfig struct { Concurrency int `yaml:"concurrency"` } +// RateLimitSetting names the Snipe-IT plan whose request budget the client +// should pace itself against. Snipe-IT Cloud publishes a per-minute allowance +// per plan, and the client tightens further from the X-Ratelimit-* headers the +// API returns on every response. +// +// Accepted values: "basic" (120/min), "small_business" (240/min), "dedicated" +// (unmetered). The legacy booleans still parse: true is small_business, +// false is dedicated (i.e. no client-side limiting). +type RateLimitSetting string + +const ( + RateLimitBasic RateLimitSetting = "basic" + RateLimitSmallBusiness RateLimitSetting = "small_business" + RateLimitDedicated RateLimitSetting = "dedicated" +) + +// UnmarshalYAML accepts the plan names and the pre-preset booleans. +func (r *RateLimitSetting) UnmarshalYAML(value *yaml.Node) error { + raw := strings.TrimSpace(value.Value) + switch strings.ToLower(raw) { + case "true", "yes", "on": + *r = RateLimitSmallBusiness + return nil + case "false", "no", "off": + *r = RateLimitDedicated + return nil + case "": + *r = "" + return nil + } + *r = RateLimitSetting(strings.ToLower(strings.ReplaceAll(raw, "-", "_"))) + return nil +} + type AssetTagConfig struct { Template string `yaml:"template"` } @@ -237,6 +271,9 @@ func (c *Config) applyDefaults() { if c.Sync.Concurrency == 0 { c.Sync.Concurrency = 8 } + if c.Sync.RateLimit == "" { + c.Sync.RateLimit = RateLimitSmallBusiness + } } // Validate fails fast on missing required fields and bad enum values. @@ -256,6 +293,11 @@ func (c *Config) Validate() error { if c.SnipeIT.APIKey == "" { return fmt.Errorf("snipe_it.api_key is required") } + switch c.Sync.RateLimit { + case RateLimitBasic, RateLimitSmallBusiness, RateLimitDedicated: + default: + return fmt.Errorf("sync.rate_limit must be one of basic, small_business, dedicated, got %q", c.Sync.RateLimit) + } if c.SnipeIT.DefaultStatusID == 0 { return fmt.Errorf("snipe_it.default_status_id is required") } diff --git a/config/config_test.go b/config/config_test.go index 775d51d..66b8526 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -5,6 +5,8 @@ import ( "path/filepath" "reflect" "testing" + + "gopkg.in/yaml.v3" ) func writeTemp(t *testing.T, body string) string { @@ -262,3 +264,52 @@ func TestDefaultScopesCoverDirectoryUsers(t *testing.T) { t.Errorf("configured scopes = %v, want them left as %v", c.Google.Scopes, custom) } } +// The plan name drives the client's pacing, and the pre-preset booleans must +// keep working for configs written before the presets existed. +func TestRateLimitSettingParsing(t *testing.T) { + cases := map[string]RateLimitSetting{ + "basic": RateLimitBasic, + "small_business": RateLimitSmallBusiness, + "small-business": RateLimitSmallBusiness, + "Dedicated": RateLimitDedicated, + "true": RateLimitSmallBusiness, + "false": RateLimitDedicated, + } + for in, want := range cases { + var got struct { + RateLimit RateLimitSetting `yaml:"rate_limit"` + } + if err := yaml.Unmarshal([]byte("rate_limit: "+in), &got); err != nil { + t.Fatalf("%s: %v", in, err) + } + if got.RateLimit != want { + t.Errorf("rate_limit: %s parsed as %q, want %q", in, got.RateLimit, want) + } + } +} + +func TestRateLimitSettingDefaultsAndValidates(t *testing.T) { + c := &Config{} + c.applyDefaults() + if c.Sync.RateLimit != RateLimitSmallBusiness { + t.Errorf("default rate_limit = %q, want small_business", c.Sync.RateLimit) + } + + c = validConfigForRateLimit() + c.Sync.RateLimit = "enterprise" + if err := c.Validate(); err == nil { + t.Error("an unknown plan name must fail validation rather than silently disabling limiting") + } +} + +// validConfigForRateLimit returns a config that passes Validate, so the test +// above fails only on the rate-limit field. +func validConfigForRateLimit() *Config { + c := &Config{} + c.Google.CredentialsFile = "creds.json" + c.Google.ImpersonateSubject = "admin@example.com" + c.SnipeIT.URL = "https://snipe.example.com" + c.SnipeIT.APIKey = "key" + c.applyDefaults() + return c +} diff --git a/go.mod b/go.mod index 3b238d9..86fe743 100644 --- a/go.mod +++ b/go.mod @@ -42,3 +42,5 @@ require ( google.golang.org/grpc v1.81.1 // indirect google.golang.org/protobuf v1.36.11 // indirect ) + +replace github.com/michellepellon/go-snipeit => github.com/CampusTech/go-snipeit v0.0.0-20260820165655-4da368d81bee diff --git a/go.sum b/go.sum index cb07a92..ce60641 100644 --- a/go.sum +++ b/go.sum @@ -4,6 +4,8 @@ cloud.google.com/go/auth/oauth2adapt v0.2.8 h1:keo8NaayQZ6wimpNSmW5OPc283g65QNIi cloud.google.com/go/auth/oauth2adapt v0.2.8/go.mod h1:XQ9y31RkqZCcwJWNSx2Xvric3RrU88hAYYbjDWYDL+c= cloud.google.com/go/compute/metadata v0.9.0 h1:pDUj4QMoPejqq20dK0Pg2N4yG9zIkYGdBtwLoEkH9Zs= cloud.google.com/go/compute/metadata v0.9.0/go.mod h1:E0bWwX5wTnLPedCKqk3pJmVgCBSM6qQI1yTBdEb3C10= +github.com/CampusTech/go-snipeit v0.0.0-20260820165655-4da368d81bee h1:O6dyLwurRmW4JVgTVOzQALKmz83Uw9B1Z1iBUxdLmRo= +github.com/CampusTech/go-snipeit v0.0.0-20260820165655-4da368d81bee/go.mod h1:N5ro1zf9aciff0dTslglVIuP/xGWxYVuZzkFWvSi08U= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= @@ -37,8 +39,6 @@ github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= -github.com/michellepellon/go-snipeit v0.0.0-20260618143325-14ded9c8bf9f h1:wPhw7FwV7Q8msNvyCLANa1SeFhLpNn6kJmsd9bQ5qlI= -github.com/michellepellon/go-snipeit v0.0.0-20260618143325-14ded9c8bf9f/go.mod h1:N5ro1zf9aciff0dTslglVIuP/xGWxYVuZzkFWvSi08U= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= diff --git a/settings.example.yaml b/settings.example.yaml index c403dc0..02aac12 100644 --- a/settings.example.yaml +++ b/settings.example.yaml @@ -39,7 +39,10 @@ snipe_it: sync: dry_run: false - rate_limit: true # token-bucket limit on Snipe-IT writes + # Snipe-IT plan whose request budget to pace against: basic (120/min), + # small_business (240/min), or dedicated (no client-side limit). The client + # tightens further from the API's X-Ratelimit-* response headers. + rate_limit: small_business concurrency: 8 # parallel Snipe-IT workers; 1 = serial update_only: false set_name: false diff --git a/snipe/client.go b/snipe/client.go index 011ab87..9cbea24 100644 --- a/snipe/client.go +++ b/snipe/client.go @@ -120,75 +120,78 @@ func (l *snipeLogger) LogResponse(method, url string, statusCode int, body []byt // applied — the same default used by the other CampusTech 2snipe tools. // When dryRun is true, every mutating method returns ErrDryRun before any HTTP // request is made. -func New(url, apiKey string, dryRun, rateLimit bool, logger *logrus.Logger) (*Client, error) { +// +// ratePlan names the Snipe-IT plan whose request budget the client paces +// itself against ("basic", "small_business", "dedicated"); an empty value +// means small_business. The plan only sets the ceiling — go-snipeit's adaptive +// limiter tightens further from the X-Ratelimit-* headers Snipe-IT returns on +// every response, so a shared token or a busy instance slows this client down +// instead of pushing it into 429s. +func New(url, apiKey string, dryRun bool, ratePlan string, logger *logrus.Logger) (*Client, error) { if logger == nil { logger = logrus.New() } baseURL := strings.TrimRight(url, "/") - opts := &snipeit.ClientOptions{ - Logger: &snipeLogger{logger: logger}, - DisableRetries: true, // retry429 handles retries at our layer + if ratePlan == "" { + ratePlan = "small_business" + } + preset, ok := snipeit.PresetByName(ratePlan) + if !ok { + return nil, fmt.Errorf("unknown snipe-it rate limit plan %q (want basic, small_business, or dedicated)", ratePlan) } - if rateLimit { - opts.RateLimiter = snipeit.NewTokenBucketRateLimiter(2, 5) + + opts := &snipeit.ClientOptions{ + Logger: &snipeLogger{logger: logger}, + RateLimiter: preset.Limiter(), + RetryPolicy: retryPolicy(), + OnRateLimit: rateLimitLogger(logger), } sc, err := snipeit.NewClientWithOptions(baseURL, apiKey, opts) if err != nil { return nil, fmt.Errorf("creating snipe-it client: %w", err) } + logger.WithFields(logrus.Fields{ + "plan": preset.Name, "requests_per_minute": preset.RequestsPerMinute, + }).Debug("snipe-it rate limit plan") return &Client{sc: sc, dryRun: dryRun, logger: logger}, nil } -// retry429 runs fn and retries transient failures — HTTP 429 (honoring -// Retry-After), HTTP 5xx, and network/connection errors — with backoff -// (Retry-After when present and non-negative, else exponential from 500 ms ×2 -// capped at 30 s), up to 6 attempts total. Context cancellation/deadline and -// any non-transient (e.g. 4xx) error are returned immediately. go-snipeit's own -// retry policy is disabled (see New's DisableRetries) so this layer owns all -// retry behavior — replacing the SDK's 429+5xx+connection retries. -func (c *Client) retry429(ctx context.Context, op string, fn func() (*http.Response, error)) error { - const maxAttempts = 6 - backoff := 500 * time.Millisecond - for attempt := 1; ; attempt++ { - resp, err := fn() - retryable := false +// maxBackoff caps the retry backoff and any server-provided Retry-After, so a +// single large (or malformed-but-numeric) value can't stall a sync for hours. +const maxBackoff = 30 * time.Second + +// retryPolicy is go-snipeit's default policy plus PATCH, which is safe to +// replay here because every PATCH this package sends is an absolute update +// (asset fields, license seats), never a relative mutation. +func retryPolicy() *snipeit.RetryPolicy { + p := snipeit.DefaultRetryPolicy() + p.MaxRetries = 5 + p.MaxBackoff = maxBackoff + p.RetryMethods = map[string]bool{ + http.MethodGet: true, + http.MethodHead: true, + http.MethodPut: true, + http.MethodDelete: true, + http.MethodPatch: true, + } + return p +} + +// rateLimitLogger reports the server's remaining budget: at debug normally, and +// at warn once a quarter of the window's allowance is left, which is the point +// where a long sync is at risk of being throttled. +func rateLimitLogger(logger *logrus.Logger) func(snipeit.RateLimit) { + return func(rl snipeit.RateLimit) { + f := logrus.Fields{"limit": rl.Limit, "remaining": rl.Remaining, "resets_in": rl.Reset.String()} switch { - case resp != nil && (resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode >= 500): - retryable = true - case resp == nil && err != nil && - !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded): - retryable = true // transient network/connection error - } - if !retryable { - return err - } - if attempt >= maxAttempts { - if err != nil { - return fmt.Errorf("%s: failed after %d attempts: %w", op, maxAttempts, err) - } - return fmt.Errorf("%s: failed after %d attempts (HTTP %d)", op, maxAttempts, resp.StatusCode) - } - wait := backoff - if resp != nil { - // retryAfterDuration (snipe/licenses.go) honors a numeric Retry-After and clamps - // it to maxBackoff so one large/malformed value can't stall the sync for hours. - if d, ok := retryAfterDuration(resp.Header); ok { - wait = d - } - } - c.logger.WithFields(logrus.Fields{"op": op, "attempt": attempt, "wait": wait.String()}). - Warn("snipe request failed (429/5xx/transient); backing off") - // Cancel-aware backoff: a Ctrl-C (SIGINT/SIGTERM) cancels ctx so we abort the - // sleep promptly instead of waiting out the full Retry-After/exponential wait. - select { - case <-ctx.Done(): - return ctx.Err() - case <-time.After(wait): - } - if backoff *= 2; backoff > maxBackoff { - backoff = maxBackoff + case rl.Exhausted(): + logger.WithFields(f).Warn("snipe-it rate limit exhausted; waiting for the window to reset") + case rl.Limit > 0 && rl.Remaining*4 <= rl.Limit: + logger.WithFields(f).Warn("snipe-it rate limit budget running low") + default: + logger.WithFields(f).Debug("snipe-it rate limit") } } } @@ -206,12 +209,7 @@ func (c *Client) Ping() (string, error) { // GetAssetBySerial looks up assets by serial. Snipe's /byserial endpoint does a // partial search, so this filters to exact case-insensitive matches. func (c *Client) GetAssetBySerial(ctx context.Context, serial string) ([]Asset, error) { - var resp *snipeit.AssetsResponse - err := c.retry429(ctx, "get asset by serial", func() (*http.Response, error) { - r, httpResp, e := c.sc.Assets.GetAssetBySerialContext(ctx, serial) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.Assets.GetAssetBySerialContext(ctx, serial) if err != nil { return nil, fmt.Errorf("looking up serial %s: %w", serial, err) } @@ -254,18 +252,14 @@ func (c *Client) listAssetsPaged(ctx context.Context, status string) ([]Asset, e if status != "" { u += "&status=" + url.QueryEscape(status) } - var resp snipeit.AssetsResponse - err := c.retry429(ctx, "list assets", func() (*http.Response, error) { - resp = snipeit.AssetsResponse{} - req, e := c.sc.NewRequest(http.MethodGet, u, nil) - if e != nil { - return nil, e - } - return c.sc.DoContext(ctx, req, &resp) - }) + req, err := c.sc.NewRequest(http.MethodGet, u, nil) if err != nil { return nil, fmt.Errorf("listing assets (status=%q): %w", status, err) } + var resp snipeit.AssetsResponse + if _, err := c.sc.DoContext(ctx, req, &resp); err != nil { + return nil, fmt.Errorf("listing assets (status=%q): %w", status, err) + } for _, a := range resp.Rows { out = append(out, fromSnipeAsset(a)) } @@ -288,12 +282,7 @@ func (c *Client) CreateAsset(ctx context.Context, a Asset) (Asset, error) { return Asset{}, ErrDryRun } sa := toSnipeAsset(a) - var resp *snipeit.AssetCreateResponse - err := c.retry429(ctx, "create asset", func() (*http.Response, error) { - r, httpResp, e := c.sc.Assets.CreateContext(ctx, sa) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.Assets.CreateContext(ctx, sa) if err != nil { return Asset{}, fmt.Errorf("creating asset: %w", err) } @@ -318,11 +307,7 @@ func (c *Client) CreateAsset(ctx context.Context, a Asset) (Asset, error) { delete(cleaned, k) } sa.CustomFields = cleaned - err = c.retry429(ctx, "create asset (field retry)", func() (*http.Response, error) { - r, httpResp, e := c.sc.Assets.CreateContext(ctx, sa) - resp = r - return httpResp, e - }) + resp, _, err = c.sc.Assets.CreateContext(ctx, sa) if err != nil { return Asset{}, fmt.Errorf("creating asset (retry): %w", err) } @@ -343,12 +328,7 @@ func (c *Client) PatchAsset(ctx context.Context, id int, a Asset) (Asset, error) return Asset{}, ErrDryRun } sa := toSnipeAsset(a) - var resp *snipeit.AssetCreateResponse - err := c.retry429(ctx, "patch asset", func() (*http.Response, error) { - r, httpResp, e := c.sc.Assets.PatchContext(ctx, id, sa) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.Assets.PatchContext(ctx, id, sa) if err != nil { return Asset{}, fmt.Errorf("updating asset %d: %w", id, err) } @@ -373,11 +353,7 @@ func (c *Client) PatchAsset(ctx context.Context, id int, a Asset) (Asset, error) delete(cleaned, k) } sa.CustomFields = cleaned - err = c.retry429(ctx, "patch asset (field retry)", func() (*http.Response, error) { - r, httpResp, e := c.sc.Assets.PatchContext(ctx, id, sa) - resp = r - return httpResp, e - }) + resp, _, err = c.sc.Assets.PatchContext(ctx, id, sa) if err != nil { return Asset{}, fmt.Errorf("updating asset %d (retry): %w", id, err) } @@ -399,12 +375,7 @@ func (c *Client) CheckoutAssetToUser(ctx context.Context, assetID, userID int) e "checkout_to_type": "user", "assigned_user": userID, } - var resp *snipeit.AssetCreateResponse - err := c.retry429(ctx, "checkout asset", func() (*http.Response, error) { - r, httpResp, e := c.sc.Assets.CheckoutContext(ctx, assetID, body) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.Assets.CheckoutContext(ctx, assetID, body) if err != nil { return fmt.Errorf("checking out asset %d to user %d: %w", assetID, userID, err) } @@ -421,14 +392,7 @@ func (c *Client) CheckinAsset(ctx context.Context, assetID int) error { if c.dryRun { return ErrDryRun } - var resp *snipeit.AssetCreateResponse - var savedHTTPResp *http.Response - err := c.retry429(ctx, "checkin asset", func() (*http.Response, error) { - r, httpResp, e := c.sc.Assets.CheckinContext(ctx, assetID, map[string]any{}) - resp = r - savedHTTPResp = httpResp - return httpResp, e - }) + resp, savedHTTPResp, err := c.sc.Assets.CheckinContext(ctx, assetID, map[string]any{}) if err != nil { // go-snipeit types the checkin response's payload.model as an object, // but Snipe-IT returns it as a string, so the SUCCESS body fails to @@ -457,12 +421,7 @@ func (c *Client) ListAllModels(ctx context.Context) ([]Model, error) { offset := 0 const limit = 500 for { - var resp *snipeit.ModelsResponse - err := c.retry429(ctx, "list models", func() (*http.Response, error) { - r, httpResp, e := c.sc.Models.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.Models.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) if err != nil { return nil, fmt.Errorf("listing models: %w", err) } @@ -482,12 +441,7 @@ func (c *Client) CreateModel(ctx context.Context, m Model) (Model, error) { if c.dryRun { return Model{}, ErrDryRun } - var resp *snipeit.ModelResponse - err := c.retry429(ctx, "create model", func() (*http.Response, error) { - r, httpResp, e := c.sc.Models.CreateContext(ctx, toSnipeModel(m)) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.Models.CreateContext(ctx, toSnipeModel(m)) if err != nil { return Model{}, fmt.Errorf("creating model: %w", err) } @@ -503,12 +457,7 @@ func (c *Client) ListAllManufacturers(ctx context.Context) ([]Manufacturer, erro offset := 0 const limit = 500 for { - var resp *snipeit.ManufacturersResponse - err := c.retry429(ctx, "list manufacturers", func() (*http.Response, error) { - r, httpResp, e := c.sc.Manufacturers.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.Manufacturers.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) if err != nil { return nil, fmt.Errorf("listing manufacturers: %w", err) } @@ -535,12 +484,7 @@ func (c *Client) ListAllStatusLabels(ctx context.Context) ([]StatusLabel, error) offset := 0 const limit = 500 for { - var resp *snipeit.StatusLabelsResponse - err := c.retry429(ctx, "list status labels", func() (*http.Response, error) { - r, httpResp, e := c.sc.StatusLabels.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.StatusLabels.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) if err != nil { return nil, fmt.Errorf("listing status labels: %w", err) } @@ -562,12 +506,7 @@ func (c *Client) CreateManufacturer(ctx context.Context, name string) (Manufactu } m := snipeit.Manufacturer{} m.Name = name - var resp *snipeit.ManufacturerResponse - err := c.retry429(ctx, "create manufacturer", func() (*http.Response, error) { - r, httpResp, e := c.sc.Manufacturers.CreateContext(ctx, m) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.Manufacturers.CreateContext(ctx, m) if err != nil { return Manufacturer{}, fmt.Errorf("creating manufacturer: %w", err) } @@ -583,12 +522,7 @@ func (c *Client) ListAllUsers(ctx context.Context) ([]User, error) { offset := 0 const limit = 500 for { - var resp *snipeit.UsersResponse - err := c.retry429(ctx, "list users", func() (*http.Response, error) { - r, httpResp, e := c.sc.Users.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) - resp = r - return httpResp, e - }) + resp, _, err := c.sc.Users.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) if err != nil { return nil, fmt.Errorf("listing users: %w", err) } diff --git a/snipe/client_test.go b/snipe/client_test.go index a44e4dd..2ab3c0c 100644 --- a/snipe/client_test.go +++ b/snipe/client_test.go @@ -13,7 +13,7 @@ import ( ) func TestDryRunBlocksCreate(t *testing.T) { - c, err := New("https://snipe.invalid", "key", true /*dryRun*/, false, logrus.New()) + c, err := New("https://snipe.invalid", "key", true /*dryRun*/, "dedicated", logrus.New()) if err != nil { t.Fatal(err) } @@ -36,7 +36,7 @@ func TestCreateAssetRetriesOn429(t *testing.T) { _, _ = w.Write([]byte(`{"status":"success","payload":{"id":7,"asset_tag":"A","serial":"S"}}`)) })) defer srv.Close() - c, err := New(srv.URL, "k", false, false, logrus.New()) + c, err := New(srv.URL, "k", false, "dedicated", logrus.New()) if err != nil { t.Fatal(err) } @@ -52,7 +52,33 @@ func TestCreateAssetRetriesOn429(t *testing.T) { } } -func TestCreateAssetRetriesOn5xx(t *testing.T) { +// A create that fails with a 5xx may already have landed server-side, so it +// must NOT be replayed — a retry would risk a duplicate asset. Reads and +// absolute updates are still retried. +func TestCreateAssetNotRetriedOn5xx(t *testing.T) { + var n int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&n, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(503) + _, _ = w.Write([]byte(`{"status":"error","messages":"unavailable"}`)) + })) + defer srv.Close() + c, err := New(srv.URL, "k", false, "dedicated", logrus.New()) + if err != nil { + t.Fatal(err) + } + if _, err := c.CreateAsset(context.Background(), Asset{Serial: "S", ModelID: 1, StatusID: 1}); err == nil { + t.Fatal("expected the 503 to surface instead of being retried") + } + if got := atomic.LoadInt32(&n); got != 1 { + t.Fatalf("POST sent %d times, want 1 (a create must not be replayed after a 5xx)", got) + } +} + +// A PATCH carries an absolute update here, so replaying it is safe and a +// transient 5xx should not fail the sync. +func TestPatchAssetRetriesOn5xx(t *testing.T) { var n int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -64,11 +90,11 @@ func TestCreateAssetRetriesOn5xx(t *testing.T) { _, _ = w.Write([]byte(`{"status":"success","payload":{"id":8,"asset_tag":"A","serial":"S"}}`)) })) defer srv.Close() - c, err := New(srv.URL, "k", false, false, logrus.New()) + c, err := New(srv.URL, "k", false, "dedicated", logrus.New()) if err != nil { t.Fatal(err) } - a, err := c.CreateAsset(context.Background(), Asset{Serial: "S", ModelID: 1, StatusID: 1}) + a, err := c.PatchAsset(context.Background(), 8, Asset{StatusID: 2}) if err != nil { t.Fatalf("expected success after 503 retry, got %v", err) } @@ -80,6 +106,19 @@ func TestCreateAssetRetriesOn5xx(t *testing.T) { } } +// The plan name selects the pace; an unknown one is a config error, not a +// silently unlimited client. +func TestNewRejectsUnknownRatePlan(t *testing.T) { + if _, err := New("https://snipe.invalid", "k", true, "enterprise", logrus.New()); err == nil { + t.Fatal("expected an unknown rate limit plan to be rejected") + } + for _, plan := range []string{"", "basic", "small_business", "dedicated"} { + if _, err := New("https://snipe.invalid", "k", true, plan, logrus.New()); err != nil { + t.Errorf("plan %q: %v", plan, err) + } + } +} + func TestListAllAssetsPaginates(t *testing.T) { page1 := `{"total":2,"rows":[{"id":1,"asset_tag":"A1","serial":"S1"}]}` page2 := `{"total":2,"rows":[{"id":2,"asset_tag":"A2","serial":"S2"}]}` @@ -101,7 +140,7 @@ func TestListAllAssetsPaginates(t *testing.T) { } })) defer srv.Close() - c, err := New(srv.URL, "k", false, false, logrus.New()) + c, err := New(srv.URL, "k", false, "dedicated", logrus.New()) if err != nil { t.Fatal(err) } diff --git a/snipe/licenses.go b/snipe/licenses.go index eb707bf..60f0cc9 100644 --- a/snipe/licenses.go +++ b/snipe/licenses.go @@ -1,17 +1,12 @@ package snipe import ( - "bytes" "context" - "encoding/json" - "errors" "fmt" - "io" - "net/http" - "strconv" "strings" "time" + snipeit "github.com/michellepellon/go-snipeit" "github.com/sirupsen/logrus" ) @@ -36,158 +31,28 @@ type LicenseSeat struct { AssignedAssetID int // 0 if not assigned to an asset } -// LicenseClient talks to Snipe-IT licenses/seats directly (go-snipeit has no support). +// LicenseClient manages Snipe-IT licenses and their seats. +// +// It borrows the shared *Client's go-snipeit connection, so license traffic is +// paced by the same rate limiter and retried by the same policy as asset +// traffic — the two used to run on separate HTTP clients and each spend the +// instance's request budget without knowing about the other. type LicenseClient struct { - baseURL string - apiKey string - dryRun bool - http *http.Client - log *logrus.Logger + sc *snipeit.Client + dryRun bool + log *logrus.Logger } -func NewLicenseClient(url, apiKey string, dryRun bool, logger *logrus.Logger) *LicenseClient { - if logger == nil { - logger = logrus.New() - } - return &LicenseClient{ - baseURL: strings.TrimRight(url, "/"), - apiKey: apiKey, - dryRun: dryRun, - http: &http.Client{Timeout: 30 * time.Second}, - log: logger, - } +// NewLicenseClient returns a license client that shares c's connection, +// rate limiter, and dry-run setting. +func NewLicenseClient(c *Client) *LicenseClient { + return &LicenseClient{sc: c.sc, dryRun: c.dryRun, log: c.logger} } -// snipeResp is the {status, messages, payload} envelope returned by mutating endpoints. -type snipeResp struct { - Status string `json:"status"` - Messages json.RawMessage `json:"messages"` - Payload json.RawMessage `json:"payload"` -} - -// check2xx returns a descriptive error for non-2xx responses, including the body, -// so an auth/rate-limit/validation failure (401/429/422) is not lost as an opaque -// JSON-unmarshal error. -func check2xx(status int, raw []byte, what string) error { - if status < 200 || status >= 300 { - return fmt.Errorf("%s: HTTP %d: %s", what, status, strings.TrimSpace(string(raw))) - } - return nil -} - -// do issues an authenticated request to /api/v1 and returns the raw body, so -// list ({total,rows}) and mutation ({status,payload}) callers each decode what they need. -// Mutating callers must check c.dryRun BEFORE calling do. -// -// It retries on rate-limiting and transient failures (honoring Retry-After, then an -// exponential backoff, both capped at maxBackoff). A 429 is always retried — it was -// rate-limited and never processed. A 5xx or transient network error is retried only for -// idempotent methods. CONTRACT: every PATCH caller in this package MUST send an -// absolute/idempotent body (updateLicense, patchSeat, EnsureSeats all do); do not add a -// relative-mutation PATCH without making do() skip retries for it. POST (license/category -// create) is never retried on 5xx/network errors so a create that may have succeeded is not -// replayed. -func (c *LicenseClient) do(ctx context.Context, method, path string, body any) ([]byte, int, error) { - var bodyBytes []byte - if body != nil { - b, err := json.Marshal(body) - if err != nil { - return nil, 0, err - } - bodyBytes = b - } - idempotent := method != http.MethodPost - const maxAttempts = 6 - backoff := 500 * time.Millisecond - - for attempt := 1; ; attempt++ { - var rdr io.Reader - if bodyBytes != nil { - rdr = bytes.NewReader(bodyBytes) - } - // The caller-supplied ctx (rooted at a signal.NotifyContext in cmd) lets a Ctrl-C - // abort the in-flight request and any backoff sleep instead of hard-killing the run. - req, err := http.NewRequestWithContext(ctx, method, c.baseURL+"/api/v1"+path, rdr) - if err != nil { - return nil, 0, err - } - req.Header.Set("Authorization", "Bearer "+c.apiKey) - req.Header.Set("Accept", "application/json") - req.Header.Set("Content-Type", "application/json") - c.log.WithFields(logrus.Fields{"method": method, "path": path, "attempt": attempt}).Debug("snipe license request") - - var ( - data []byte - status int - wait time.Duration - retry bool - reason string - ) - resp, err := c.http.Do(req) - if err != nil { - // A cancelled/deadline-exceeded ctx is not a transient failure — abort - // immediately rather than retrying a request the caller no longer wants. - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - return nil, 0, err - } - if !idempotent || attempt >= maxAttempts { - return nil, 0, fmt.Errorf("%s %s: after %d attempt(s): %w", method, path, attempt, err) - } - retry, wait, reason = true, backoff, err.Error() - } else { - var rerr error - data, rerr = io.ReadAll(resp.Body) - _ = resp.Body.Close() - if rerr != nil { - return data, resp.StatusCode, rerr - } - status = resp.StatusCode - if (status == http.StatusTooManyRequests || (status >= 500 && idempotent)) && attempt < maxAttempts { - retry, wait, reason = true, backoff, fmt.Sprintf("HTTP %d", status) - if d, ok := retryAfterDuration(resp.Header); ok { - wait = d - } - } - } - if !retry { - return data, status, nil - } - c.log.WithFields(logrus.Fields{"method": method, "path": path, "attempt": attempt, "wait": wait.String(), "reason": reason}). - Warn("snipe license request failed (429/5xx/transient); backing off") - // Cancel-aware backoff: a Ctrl-C (SIGINT/SIGTERM) cancels ctx so we abort the - // sleep promptly instead of waiting out the full Retry-After/exponential wait. - select { - case <-ctx.Done(): - return nil, 0, ctx.Err() - case <-time.After(wait): - } - if backoff *= 2; backoff > maxBackoff { - backoff = maxBackoff - } - } -} - -// maxBackoff caps both the exponential retry backoff and any server-provided Retry-After, so -// a single large (or malformed-but-numeric) Retry-After can't stall the whole sync for hours. -const maxBackoff = 30 * time.Second - -// retryAfterDuration parses a Retry-After header given in whole seconds (delta-seconds form -// only; an HTTP-date value is treated as absent and falls back to exponential backoff). The -// bool reports whether a valid value was present (a present "0" means retry immediately, -// distinct from no header at all). The returned wait is clamped to maxBackoff. -func retryAfterDuration(h http.Header) (time.Duration, bool) { - ra := strings.TrimSpace(h.Get("Retry-After")) - if ra == "" { - return 0, false - } - if secs, err := strconv.Atoi(ra); err == nil && secs >= 0 { - d := time.Duration(secs) * time.Second - if d > maxBackoff { - d = maxBackoff - } - return d, true - } - return 0, false +// apiErr turns a non-success envelope into an error carrying the API's message, +// so a validation/permission failure is not lost as a bare "not success". +func apiErr(what string, r snipeit.Response) error { + return fmt.Errorf("%s: %s", what, r.Message.String()) } // EnsureLicenseCategory finds a Snipe-IT category of type "license" by name @@ -196,22 +61,8 @@ func (c *LicenseClient) EnsureLicenseCategory(ctx context.Context, name string) offset := 0 const limit = 100 for { - raw, status, err := c.do(ctx, http.MethodGet, fmt.Sprintf("/categories?limit=%d&offset=%d", limit, offset), nil) + page, _, err := c.sc.Categories.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) if err != nil { - return 0, err - } - if err := check2xx(status, raw, "listing categories"); err != nil { - return 0, err - } - var page struct { - Total int `json:"total"` - Rows []struct { - ID int `json:"id"` - Name string `json:"name"` - CategoryType string `json:"category_type"` - } `json:"rows"` - } - if err := json.Unmarshal(raw, &page); err != nil { return 0, fmt.Errorf("listing categories: %w", err) } for _, r := range page.Rows { @@ -227,27 +78,17 @@ func (c *LicenseClient) EnsureLicenseCategory(ctx context.Context, name string) if c.dryRun { return 0, ErrDryRun } - raw, status, err := c.do(ctx, http.MethodPost, "/categories", map[string]any{"name": name, "category_type": "license"}) + created, _, err := c.sc.Categories.CreateContext(ctx, snipeit.Category{ + CommonFields: snipeit.CommonFields{Name: name}, + CategoryType: "license", + }) if err != nil { - return 0, err - } - if err := check2xx(status, raw, fmt.Sprintf("creating license category %q", name)); err != nil { - return 0, err - } - var r snipeResp - if err := json.Unmarshal(raw, &r); err != nil { return 0, fmt.Errorf("creating license category %q: %w", name, err) } - if r.Status != "success" { - return 0, fmt.Errorf("creating license category %q: %s", name, string(r.Messages)) + if created.Status != "success" { + return 0, apiErr(fmt.Sprintf("creating license category %q", name), created.Response) } - var p struct { - ID int `json:"id"` - } - if err := json.Unmarshal(r.Payload, &p); err != nil { - return 0, fmt.Errorf("parsing created category %q: %w", name, err) - } - return p.ID, nil + return created.Payload.ID, nil } // ListLicenses returns all licenses (paginated). @@ -256,26 +97,12 @@ func (c *LicenseClient) ListLicenses(ctx context.Context) ([]License, error) { offset := 0 const limit = 100 for { - raw, status, err := c.do(ctx, http.MethodGet, fmt.Sprintf("/licenses?limit=%d&offset=%d", limit, offset), nil) + page, _, err := c.sc.Licenses.ListContext(ctx, &snipeit.ListOptions{Limit: limit, Offset: offset}) if err != nil { - return nil, err - } - if err := check2xx(status, raw, "listing licenses"); err != nil { - return nil, err - } - var page struct { - Total int `json:"total"` - Rows []struct { - ID int `json:"id"` - Name string `json:"name"` - Seats int `json:"seats"` - } `json:"rows"` - } - if err := json.Unmarshal(raw, &page); err != nil { return nil, fmt.Errorf("listing licenses: %w", err) } - for _, r := range page.Rows { - out = append(out, License{ID: r.ID, Name: r.Name, Seats: r.Seats}) + for _, l := range page.Rows { + out = append(out, License{ID: l.ID, Name: l.Name, Seats: l.Seats}) } if len(page.Rows) == 0 || len(out) >= page.Total { break @@ -285,32 +112,38 @@ func (c *LicenseClient) ListLicenses(ctx context.Context) ([]License, error) { return out, nil } -// updateLicense PATCHes the mutable fields of an existing license so config changes -// (cost, category, reassignable, expiration) propagate on re-sync. config is source of truth. -func (c *LicenseClient) updateLicense(ctx context.Context, id int, spec LicenseSpec) error { - body := map[string]any{ - "purchase_cost": spec.CostPerSeat, - "category_id": spec.CategoryID, - "reassignable": spec.Reassignable, - } +// toSnipeLicense renders a spec as the license body Snipe-IT expects. seats is +// passed separately because create and update bound it differently. +func toSnipeLicense(spec LicenseSpec, seats int) snipeit.License { + l := snipeit.License{ + CommonFields: snipeit.CommonFields{Name: spec.Name}, + Seats: seats, + CategoryID: spec.CategoryID, + Reassignable: snipeit.FlexBool(spec.Reassignable), + PurchaseCost: fmt.Sprintf("%.2f", spec.CostPerSeat), + } + // A nil date leaves the stored expiration alone; a zero one clears it, which + // is what an emptied config value must do. if spec.ExpirationDate != "" { - body["expiration_date"] = spec.ExpirationDate + if t, err := time.Parse("2006-01-02", spec.ExpirationDate); err == nil { + l.ExpirationDate = &snipeit.SnipeTime{Time: t} + } } else { - body["expiration_date"] = nil + l.ExpirationDate = &snipeit.SnipeTime{} } - raw, status, err := c.do(ctx, http.MethodPatch, fmt.Sprintf("/licenses/%d", id), body) + return l +} + +// updateLicense PATCHes the mutable fields of an existing license so config changes +// (cost, category, reassignable, expiration) propagate on re-sync. config is source of truth. +func (c *LicenseClient) updateLicense(ctx context.Context, id int, spec LicenseSpec) error { + // Seats are grown by EnsureSeats in bounded steps; leave them out here. + resp, _, err := c.sc.Licenses.UpdateContext(ctx, id, toSnipeLicense(spec, 0)) if err != nil { - return err - } - if err := check2xx(status, raw, fmt.Sprintf("updating license %d", id)); err != nil { - return err - } - var r snipeResp - if err := json.Unmarshal(raw, &r); err != nil { return fmt.Errorf("updating license %d: %w", id, err) } - if r.Status != "success" { - return fmt.Errorf("updating license %d: %s", id, string(r.Messages)) + if resp.Status != "success" { + return apiErr(fmt.Sprintf("updating license %d", id), resp.Response) } return nil } @@ -335,40 +168,17 @@ func (c *LicenseClient) EnsureLicense(ctx context.Context, spec LicenseSpec) (Li if c.dryRun { return License{}, ErrDryRun } - body := map[string]any{ - "name": spec.Name, - // On create the license has 0 seats, so Snipe-IT's limit_change rule bounds the - // seats field to 1..maxSeatsPerChange. Clamp here; EnsureSeats grows the rest in steps. - "seats": min(max(spec.Seats, 1), maxSeatsPerChange), - "category_id": spec.CategoryID, - "reassignable": spec.Reassignable, - "purchase_cost": spec.CostPerSeat, - } - if spec.ExpirationDate != "" { - body["expiration_date"] = spec.ExpirationDate - } - raw, status, err := c.do(ctx, http.MethodPost, "/licenses", body) + // On create the license has 0 seats, so Snipe-IT's limit_change rule bounds the + // seats field to 1..maxSeatsPerChange. Clamp here; EnsureSeats grows the rest in steps. + seats := min(max(spec.Seats, 1), maxSeatsPerChange) + created, _, err := c.sc.Licenses.CreateContext(ctx, toSnipeLicense(spec, seats)) if err != nil { - return License{}, err - } - if err := check2xx(status, raw, fmt.Sprintf("creating license %q", spec.Name)); err != nil { - return License{}, err - } - var r snipeResp - if err := json.Unmarshal(raw, &r); err != nil { return License{}, fmt.Errorf("creating license %q: %w", spec.Name, err) } - if r.Status != "success" { - return License{}, fmt.Errorf("creating license %q: %s", spec.Name, string(r.Messages)) - } - var p struct { - ID int `json:"id"` - Name string `json:"name"` - Seats int `json:"seats"` - } - if err := json.Unmarshal(r.Payload, &p); err != nil { - return License{}, fmt.Errorf("parsing created license %q: %w", spec.Name, err) + if created.Status != "success" { + return License{}, apiErr(fmt.Sprintf("creating license %q", spec.Name), created.Response) } + p := created.Payload return License{ID: p.ID, Name: p.Name, Seats: p.Seats}, nil } @@ -378,26 +188,8 @@ func (c *LicenseClient) ListSeats(ctx context.Context, licenseID int) ([]License offset := 0 const limit = 500 for { - raw, status, err := c.do(ctx, http.MethodGet, fmt.Sprintf("/licenses/%d/seats?limit=%d&offset=%d", licenseID, limit, offset), nil) + page, _, err := c.sc.Licenses.ListSeatsContext(ctx, licenseID, &snipeit.ListOptions{Limit: limit, Offset: offset}) if err != nil { - return nil, err - } - if err := check2xx(status, raw, fmt.Sprintf("listing seats for license %d", licenseID)); err != nil { - return nil, err - } - var page struct { - Total int `json:"total"` - Rows []struct { - ID int `json:"id"` - AssignedUser *struct { - ID int `json:"id"` - } `json:"assigned_user"` - AssignedAsset *struct { - ID int `json:"id"` - } `json:"assigned_asset"` - } `json:"rows"` - } - if err := json.Unmarshal(raw, &page); err != nil { return nil, fmt.Errorf("listing seats for license %d: %w", licenseID, err) } for _, s := range page.Rows { @@ -418,41 +210,40 @@ func (c *LicenseClient) ListSeats(ctx context.Context, licenseID int) ([]License return out, nil } -func (c *LicenseClient) patchSeat(ctx context.Context, licenseID, seatID int, body map[string]any) error { - raw, status, err := c.do(ctx, http.MethodPatch, fmt.Sprintf("/licenses/%d/seats/%d", licenseID, seatID), body) - if err != nil { - return err - } - if err := check2xx(status, raw, fmt.Sprintf("seat %d on license %d", seatID, licenseID)); err != nil { - return err - } - var r snipeResp - if err := json.Unmarshal(raw, &r); err != nil { - return fmt.Errorf("seat %d on license %d: %w", seatID, licenseID, err) - } - if r.Status != "success" { - return fmt.Errorf("seat %d on license %d: %s", seatID, licenseID, string(r.Messages)) - } - return nil -} - func (c *LicenseClient) CheckoutSeatToUser(ctx context.Context, licenseID, seatID, userID int) error { if c.dryRun { return ErrDryRun } - return c.patchSeat(ctx, licenseID, seatID, map[string]any{"assigned_to": userID}) + resp, _, err := c.sc.Licenses.CheckoutSeatToUserContext(ctx, licenseID, seatID, userID) + return seatResult(err, resp, licenseID, seatID) } + func (c *LicenseClient) CheckoutSeatToAsset(ctx context.Context, licenseID, seatID, assetID int) error { if c.dryRun { return ErrDryRun } - return c.patchSeat(ctx, licenseID, seatID, map[string]any{"asset_id": assetID}) + resp, _, err := c.sc.Licenses.CheckoutSeatToAssetContext(ctx, licenseID, seatID, assetID) + return seatResult(err, resp, licenseID, seatID) } + func (c *LicenseClient) CheckinSeat(ctx context.Context, licenseID, seatID int) error { if c.dryRun { return ErrDryRun } - return c.patchSeat(ctx, licenseID, seatID, map[string]any{"assigned_to": nil, "asset_id": nil}) + resp, _, err := c.sc.Licenses.CheckinSeatContext(ctx, licenseID, seatID) + return seatResult(err, resp, licenseID, seatID) +} + +// seatResult folds a seat call's transport error and API envelope into one error. +func seatResult(err error, resp *snipeit.Response, licenseID, seatID int) error { + what := fmt.Sprintf("seat %d on license %d", seatID, licenseID) + if err != nil { + return fmt.Errorf("%s: %w", what, err) + } + if resp.Status != "success" { + return apiErr(what, *resp) + } + return nil } // maxSeatsPerChange mirrors Snipe-IT's `limit_change:10000` rule on a license's seats field @@ -485,38 +276,22 @@ func (c *LicenseClient) EnsureSeats(ctx context.Context, licenseID, total int) e // patchLicenseSeats sets the license's seat total in one request. The caller must keep each // change within maxSeatsPerChange of the current seat count. func (c *LicenseClient) patchLicenseSeats(ctx context.Context, licenseID, seats int) error { - raw, status, err := c.do(ctx, http.MethodPatch, fmt.Sprintf("/licenses/%d", licenseID), map[string]any{"seats": seats}) + what := fmt.Sprintf("growing license %d seats to %d", licenseID, seats) + resp, _, err := c.sc.Licenses.UpdateContext(ctx, licenseID, snipeit.License{Seats: seats}) if err != nil { - return err + return fmt.Errorf("%s: %w", what, err) } - if err := check2xx(status, raw, fmt.Sprintf("growing license %d seats to %d", licenseID, seats)); err != nil { - return err - } - var r snipeResp - if err := json.Unmarshal(raw, &r); err != nil { - return fmt.Errorf("growing license %d seats to %d: %w", licenseID, seats, err) - } - if r.Status != "success" { - return fmt.Errorf("growing license %d seats to %d: %s", licenseID, seats, string(r.Messages)) + if resp.Status != "success" { + return apiErr(what, resp.Response) } return nil } -// licenseSeatCount returns a license's current seat total via GET /licenses/{id}, which Snipe -// returns as the bare license object. +// licenseSeatCount returns a license's current seat total. func (c *LicenseClient) licenseSeatCount(ctx context.Context, licenseID int) (int, error) { - raw, status, err := c.do(ctx, http.MethodGet, fmt.Sprintf("/licenses/%d", licenseID), nil) + lic, _, err := c.sc.Licenses.GetContext(ctx, licenseID) if err != nil { - return 0, err - } - if err := check2xx(status, raw, fmt.Sprintf("reading license %d", licenseID)); err != nil { - return 0, err - } - var lic struct { - Seats int `json:"seats"` - } - if err := json.Unmarshal(raw, &lic); err != nil { - return 0, fmt.Errorf("reading license %d seats: %w", licenseID, err) + return 0, fmt.Errorf("reading license %d: %w", licenseID, err) } return lic.Seats, nil } diff --git a/snipe/licenses_test.go b/snipe/licenses_test.go index 61a785d..a9509ed 100644 --- a/snipe/licenses_test.go +++ b/snipe/licenses_test.go @@ -15,29 +15,20 @@ import ( "github.com/sirupsen/logrus" ) -func TestRetryAfterDurationParsesAndClamps(t *testing.T) { - cases := []struct { - in string - want time.Duration - ok bool - }{ - {"", 0, false}, - {"5", 5 * time.Second, true}, - {"0", 0, true}, // present zero => retry immediately (distinct from absent) - {"999999", maxBackoff, true}, // must clamp to the cap, not sleep for days - {"-3", 0, false}, - {"banana", 0, false}, // HTTP-date / garbage => treated as absent - } - for _, c := range cases { - h := http.Header{} - h.Set("Retry-After", c.in) - got, ok := retryAfterDuration(h) - if got != c.want || ok != c.ok { - t.Errorf("retryAfterDuration(%q) = (%v, %v), want (%v, %v)", c.in, got, ok, c.want, c.ok) - } +// newTestLicenseClient builds a license client over the shared Snipe client, as +// production does, so both use one connection and one rate limiter. +func newTestLicenseClient(t *testing.T, url string, dryRun bool) *LicenseClient { + t.Helper() + c, err := New(url, "k", dryRun, "dedicated", logrus.New()) + if err != nil { + t.Fatal(err) } + return NewLicenseClient(c) } +// Retry-After parsing and clamping now live in go-snipeit (ParseRateLimit and +// RetryPolicy.MaxBackoff) and are covered by its tests. + func TestLicenseClientRetriesThenSucceeds(t *testing.T) { var calls int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -49,7 +40,7 @@ func TestLicenseClientRetriesThenSucceeds(t *testing.T) { _, _ = w.Write([]byte(`{"status":"success"}`)) })) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) // CheckoutSeatToAsset issues a single PATCH through do(); it must ride out the 429s. if err := c.CheckoutSeatToAsset(context.Background(), 1, 2, 3); err != nil { t.Fatalf("CheckoutSeatToAsset should retry past 429s and succeed, got %v", err) @@ -67,7 +58,7 @@ func TestLicenseClientGivesUpAfterPersistent429(t *testing.T) { w.WriteHeader(http.StatusTooManyRequests) })) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) err := c.CheckoutSeatToAsset(context.Background(), 1, 2, 3) if err == nil || !strings.Contains(err.Error(), "429") { t.Fatalf("want a 429 error after exhausting retries, got %v", err) @@ -100,7 +91,7 @@ func TestLicenseClientRetriesDroppedConnection(t *testing.T) { _, _ = w.Write([]byte(`{"status":"success"}`)) })) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) // CheckoutSeatToAsset issues a PATCH (idempotent); the dropped first connection must be // retried and the second attempt must succeed. if err := c.CheckoutSeatToAsset(context.Background(), 3, 3045, 999); err != nil { @@ -122,7 +113,7 @@ func TestLicenseClientRetries5xxOnGet(t *testing.T) { _, _ = w.Write([]byte(`{"total":0,"rows":[]}`)) })) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) if _, err := c.ListLicenses(context.Background()); err != nil { t.Fatalf("ListLicenses should retry a 5xx and succeed, got %v", err) } @@ -132,7 +123,7 @@ func TestLicenseClientRetries5xxOnGet(t *testing.T) { } func TestLicenseClientDryRunSentinel(t *testing.T) { - c := NewLicenseClient("https://snipe.invalid", "key", true /*dryRun*/, logrus.New()) + c := newTestLicenseClient(t, "https://snipe.invalid", true) // EnsureSeats is a pure mutator: in dry-run it must return ErrDryRun before any HTTP. if err := c.EnsureSeats(context.Background(), 1, 5); !errors.Is(err, ErrDryRun) { t.Fatalf("EnsureSeats dry-run = %v, want ErrDryRun", err) @@ -164,7 +155,7 @@ func TestEnsureLicenseClampsCreateSeats(t *testing.T) { _, _ = w.Write([]byte(`{"status":"success","payload":{"id":7,"name":"Big","seats":10000}}`)) })) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) if _, err := c.EnsureLicense(context.Background(), LicenseSpec{Name: "Big", CategoryID: 1, Seats: 13000}); err != nil { t.Fatalf("EnsureLicense: %v", err) } @@ -197,7 +188,7 @@ func TestEnsureSeatsStepsPastChangeLimit(t *testing.T) { _, _ = w.Write([]byte(`{"status":"success"}`)) })) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) if err := c.EnsureSeats(context.Background(), 7, 25000); err != nil { t.Fatalf("EnsureSeats: %v", err) } @@ -218,15 +209,15 @@ func TestEnsureLicenseSurfacesHTTPError(t *testing.T) { _, _ = w.Write([]byte(`{"total":0,"rows":[]}`)) // empty list so create is attempted })) defer srv.Close() - c := NewLicenseClient(srv.URL, "key", false /*not dry-run*/, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) _, err := c.EnsureLicense(context.Background(), LicenseSpec{Name: "X", CategoryID: 1, Seats: 1}) - if err == nil || !strings.Contains(err.Error(), "HTTP 422") { + if err == nil || !strings.Contains(err.Error(), "422") { t.Fatalf("want HTTP 422 error, got %v", err) } } func TestSeatMutatorsDryRun(t *testing.T) { - c := NewLicenseClient("https://snipe.invalid", "k", true /*dryRun*/, logrus.New()) + c := newTestLicenseClient(t, "https://snipe.invalid", true) if err := c.CheckoutSeatToUser(context.Background(), 1, 2, 3); !errors.Is(err, ErrDryRun) { t.Fatalf("CheckoutSeatToUser = %v", err) } @@ -249,7 +240,7 @@ func TestListSeatsParsesAssignments(t *testing.T) { _, _ = w.Write([]byte(body)) })) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) seats, err := c.ListSeats(context.Background(), 42) if err != nil { t.Fatal(err) @@ -278,7 +269,7 @@ func TestEnsureLicenseDryRunSkipsCreate(t *testing.T) { _, _ = w.Write([]byte(`{"total":0,"rows":[]}`)) // empty license list })) defer srv.Close() - c := NewLicenseClient(srv.URL, "key", true /*dryRun*/, logrus.New()) + c := newTestLicenseClient(t, srv.URL, true) _, err := c.EnsureLicense(context.Background(), LicenseSpec{Name: "X", CategoryID: 1, Seats: 1}) if !errors.Is(err, ErrDryRun) { t.Fatalf("EnsureLicense dry-run = %v, want ErrDryRun", err) @@ -303,7 +294,7 @@ func TestEnsureLicenseCategoryCreates(t *testing.T) { }) srv := httptest.NewServer(mux) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) id, err := c.EnsureLicenseCategory(context.Background(), "Software Licenses") if err != nil { t.Fatal(err) @@ -328,7 +319,7 @@ func TestEnsureLicenseCategoryFindsExisting(t *testing.T) { }) srv := httptest.NewServer(mux) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) id, err := c.EnsureLicenseCategory(context.Background(), "software licenses") // case-insensitive if err != nil { t.Fatal(err) @@ -353,7 +344,7 @@ func TestEnsureLicenseUpdatesExisting(t *testing.T) { }) srv := httptest.NewServer(mux) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false /*not dry-run*/, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) lic, err := c.EnsureLicense(context.Background(), LicenseSpec{Name: "X", CostPerSeat: 9.99, CategoryID: 2, Reassignable: true, Seats: 3}) if err != nil { t.Fatal(err) @@ -364,8 +355,9 @@ func TestEnsureLicenseUpdatesExisting(t *testing.T) { if patched == nil { t.Fatal("existing license was not updated (no PATCH issued)") } - if patched["purchase_cost"] != 9.99 { - t.Errorf("purchase_cost = %v, want 9.99", patched["purchase_cost"]) + // Snipe-IT takes the cost as a formatted string on write. + if patched["purchase_cost"] != "9.99" { + t.Errorf("purchase_cost = %v, want \"9.99\"", patched["purchase_cost"]) } } @@ -392,7 +384,7 @@ func TestLicenseClientCancelAbortsBackoff(t *testing.T) { } })) defer srv.Close() - c := NewLicenseClient(srv.URL, "k", false, logrus.New()) + c := newTestLicenseClient(t, srv.URL, false) // Cancel only once the first 429 has been returned and the client has had a moment to // enter the ~1s Retry-After backoff sleep. From ee68509af9c5bb4d33b1db67598d28602e59128f Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Thu, 20 Aug 2026 13:37:31 -0400 Subject: [PATCH 2/2] fix(ratelimit): stop warning on every response about the remaining budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adaptive limiter deliberately paces right up against the plan's cap, so "budget running low" at a quarter of the window fired on nearly every response — 226 times in a single sync, burying the run's real output. It now warns only below a tenth of the window, or when the budget is spent, and at most once per window since the budget refills on that cadence. Also picks up go-snipeit's fix for Retry-After on successful responses: Snipe-IT sends the header on every response, and treating it as binding parked the limiter for a full window after each request (~60s per page). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016PFsj8aMUiWqyi7ViXmozp --- config/config_test.go | 1 + go.mod | 2 +- go.sum | 4 ++-- snipe/client.go | 43 +++++++++++++++++++++++++++++++--------- snipe/client_test.go | 46 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 84 insertions(+), 12 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 66b8526..d935ff6 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -264,6 +264,7 @@ func TestDefaultScopesCoverDirectoryUsers(t *testing.T) { t.Errorf("configured scopes = %v, want them left as %v", c.Google.Scopes, custom) } } + // The plan name drives the client's pacing, and the pre-preset booleans must // keep working for configs written before the presets existed. func TestRateLimitSettingParsing(t *testing.T) { diff --git a/go.mod b/go.mod index 86fe743..a59a0cc 100644 --- a/go.mod +++ b/go.mod @@ -43,4 +43,4 @@ require ( google.golang.org/protobuf v1.36.11 // indirect ) -replace github.com/michellepellon/go-snipeit => github.com/CampusTech/go-snipeit v0.0.0-20260820165655-4da368d81bee +replace github.com/michellepellon/go-snipeit => github.com/CampusTech/go-snipeit v0.0.0-20260820171155-0f2936b38d11 diff --git a/go.sum b/go.sum index ce60641..915a60d 100644 --- a/go.sum +++ b/go.sum @@ -4,8 +4,8 @@ cloud.google.com/go/auth/oauth2adapt v0.2.8 h1:keo8NaayQZ6wimpNSmW5OPc283g65QNIi cloud.google.com/go/auth/oauth2adapt v0.2.8/go.mod h1:XQ9y31RkqZCcwJWNSx2Xvric3RrU88hAYYbjDWYDL+c= cloud.google.com/go/compute/metadata v0.9.0 h1:pDUj4QMoPejqq20dK0Pg2N4yG9zIkYGdBtwLoEkH9Zs= cloud.google.com/go/compute/metadata v0.9.0/go.mod h1:E0bWwX5wTnLPedCKqk3pJmVgCBSM6qQI1yTBdEb3C10= -github.com/CampusTech/go-snipeit v0.0.0-20260820165655-4da368d81bee h1:O6dyLwurRmW4JVgTVOzQALKmz83Uw9B1Z1iBUxdLmRo= -github.com/CampusTech/go-snipeit v0.0.0-20260820165655-4da368d81bee/go.mod h1:N5ro1zf9aciff0dTslglVIuP/xGWxYVuZzkFWvSi08U= +github.com/CampusTech/go-snipeit v0.0.0-20260820171155-0f2936b38d11 h1:GAhi9vEZe4/hLBB/h/bOTfhrT/PDEvWqPKvYEI1tqgk= +github.com/CampusTech/go-snipeit v0.0.0-20260820171155-0f2936b38d11/go.mod h1:N5ro1zf9aciff0dTslglVIuP/xGWxYVuZzkFWvSi08U= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= diff --git a/snipe/client.go b/snipe/client.go index 9cbea24..c06f09e 100644 --- a/snipe/client.go +++ b/snipe/client.go @@ -16,6 +16,7 @@ import ( "net/http" "net/url" "strings" + "sync" "time" snipeit "github.com/michellepellon/go-snipeit" @@ -179,23 +180,47 @@ func retryPolicy() *snipeit.RetryPolicy { return p } -// rateLimitLogger reports the server's remaining budget: at debug normally, and -// at warn once a quarter of the window's allowance is left, which is the point -// where a long sync is at risk of being throttled. +// rateLimitLogger reports the server's remaining budget. Every response carries +// it, and the limiter deliberately runs near the cap, so the routine case is +// debug-level; a warning fires only when the budget is nearly gone (a tenth of +// the window left) or spent, and at most once per window so a long sync doesn't +// bury its real output under thousands of identical lines. func rateLimitLogger(logger *logrus.Logger) func(snipeit.RateLimit) { + var mu sync.Mutex + var nextWarn time.Time + shouldWarn := func(now time.Time) bool { + mu.Lock() + defer mu.Unlock() + if now.Before(nextWarn) { + return false + } + nextWarn = now.Add(warnInterval) + return true + } return func(rl snipeit.RateLimit) { f := logrus.Fields{"limit": rl.Limit, "remaining": rl.Remaining, "resets_in": rl.Reset.String()} - switch { - case rl.Exhausted(): - logger.WithFields(f).Warn("snipe-it rate limit exhausted; waiting for the window to reset") - case rl.Limit > 0 && rl.Remaining*4 <= rl.Limit: - logger.WithFields(f).Warn("snipe-it rate limit budget running low") - default: + low := rl.Limit > 0 && rl.Remaining*10 <= rl.Limit + if !rl.Exhausted() && !low { + logger.WithFields(f).Debug("snipe-it rate limit") + return + } + if !shouldWarn(time.Now()) { logger.WithFields(f).Debug("snipe-it rate limit") + return + } + if rl.Exhausted() { + logger.WithFields(f).Warn("snipe-it rate limit exhausted; waiting for the window to reset") + return } + logger.WithFields(f).Warn("snipe-it rate limit budget nearly spent") } } +// warnInterval is the minimum gap between rate-limit warnings — one window's +// worth, since the budget refills on that cadence and re-warning inside a +// window says nothing new. +const warnInterval = time.Minute + // Ping fetches one record to verify the API key works and returns a short // status string. Used by the connectivity-check command. func (c *Client) Ping() (string, error) { diff --git a/snipe/client_test.go b/snipe/client_test.go index 2ab3c0c..c10b8c5 100644 --- a/snipe/client_test.go +++ b/snipe/client_test.go @@ -6,9 +6,12 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" "sync/atomic" "testing" + "time" + snipeit "github.com/michellepellon/go-snipeit" "github.com/sirupsen/logrus" ) @@ -153,3 +156,46 @@ func TestListAllAssetsPaginates(t *testing.T) { t.Fatalf("paging failed: %+v", assets) } } + +// The limiter runs near the cap by design, so a healthy budget must stay at +// debug and a low one must warn at most once per window — a long sync otherwise +// buries its real output under thousands of identical lines. +func TestRateLimitLoggerWarnsSparingly(t *testing.T) { + log := logrus.New() + log.SetLevel(logrus.DebugLevel) + var mu sync.Mutex + warns := 0 + log.AddHook(&countingHook{mu: &mu, n: &warns}) + report := rateLimitLogger(log) + + for i := 0; i < 50; i++ { + report(snipeit.RateLimit{Valid: true, Limit: 240, Remaining: 200, Reset: 30 * time.Second}) + } + if warns != 0 { + t.Fatalf("healthy budget produced %d warnings, want 0", warns) + } + + for i := 0; i < 50; i++ { + report(snipeit.RateLimit{Valid: true, Limit: 240, Remaining: 5, Reset: 30 * time.Second}) + } + if warns != 1 { + t.Fatalf("low budget produced %d warnings, want 1 per window", warns) + } +} + +// countingHook counts warn-and-above entries. +type countingHook struct { + mu *sync.Mutex + n *int +} + +func (h *countingHook) Levels() []logrus.Level { + return []logrus.Level{logrus.WarnLevel, logrus.ErrorLevel, logrus.FatalLevel, logrus.PanicLevel} +} + +func (h *countingHook) Fire(*logrus.Entry) error { + h.mu.Lock() + defer h.mu.Unlock() + *h.n++ + return nil +}