0 releases 36 tags

RSS feed

• v1.7.8 4e147072f0

  Compare

  metrics: stop counting client-cancelled requests as server errors (#1255)

  Ghost released this 2026-05-05 05:58:05 -04:00 | 108 commits to main since this release

  Summary

  • Remap recorded status to 499 (NGINX-style "client closed") when
    ctx.Context().Err() is context.Canceled after the handler returns
  • Skip the http_errors_total increment for 499s so the availability
    metric reflects server-visible errors only

  Why

  Under scraper-driven load, clients with short timeouts paginate
  /v0.1/servers, give up while PG is busy, and close the TCP connection.
  The handler's DB iteration returns context canceled, the handler
  converts that to huma.Error500InternalServerError, and tries to write
  a response to the now-closed socket. NGINX records 499 (client closed)
  and never delivers anything to the client — but the registry's
  middleware records status_code=500 and bumps
  mcp_registry_http_errors_total.

  That's what's been firing the Availability dropped below 95% alert
  during the daily 17:00 UTC bursts. Recent prod numbers from one pod
  (~19h uptime):

  mcp_registry_http_errors_total{path="/v0.1/servers", status_code="500"} 7396
  mcp_registry_http_errors_total{path="/v0.1/servers/{serverName}/versions", status_code="500"} 283
  mcp_registry_http_errors_total{path="/v0/servers", status_code="500"} 352
  

  …against zero 5xx in NGINX ingress logs over the same window. The alert
  was correct given the data it had; the data was misclassifying client
  cancellations as server errors.

  What this does

  In internal/api/router/router.go (the metric middleware): after
  next(ctx), check ctx.Context().Err(). If it's context.Canceled,
  override statusCode to 499 and skip the error counter. Requests
  counter and duration histogram still record so the cancellation rate
  stays visible.

  context.DeadlineExceeded is intentionally not remapped — that
  would indicate a server-side timeout if we ever add per-request
  deadlines, and should still count as a server error.

  What this does not do

  • Doesn't reduce the underlying load (ServiceNow + notion catalog
    scrapers driving the daily peak — separate work, see the updated_since
    outreach we're doing).
  • Doesn't change handler behaviour — handlers still call
    huma.Error500… on context.Canceled. We could short-circuit earlier
    (return without writing) to save the work, but that's a bigger refactor
    and the metric fix unblocks the alert immediately.

  Companion change (no code, just a Grafana annotation tweak)

  The Availability dropped below 95% alert (UID bexrc60etvhmoa)
  currently has the annotation:

  No. of 5xx are increased from acceptable limit i.e. 5% of total
  throughput

  The query is on mcp_registry_http_errors_total, not 5xx specifically.
  After this change the count is meaningful again, but the wording is
  still misleading. Suggest updating to:

  Internal error rate exceeded 5% of total throughput

  (no PromQL change required.)

  Test plan

  • go test ./internal/api/router/... — new test passes
  • golangci-lint run ./internal/... — clean
  • After merge, watch a daily 17:00 UTC burst —
    mcp_registry_http_errors_total{status_code="500", path="/v0.1/servers"} should stay flat while
    mcp_registry_http_requests_total{status_code="499", path="/v0.1/servers"} grows. Availability alert should not fire on
    cancellation-only bursts.

  🤖 Generated with Claude Code

  Co-authored-by: Claude Opus 4.7 (1M context) noreply@anthropic.com

  Downloads

  • Source code (ZIP)
    1 download
  • Source code (TAR.GZ)
    1 download