Switch outbound HTTP to urllib3; expose per-stage timing

[prim-8]

Summary

Replaces the milter's outbound HTTP layer (urllib.request) with a shared
urllib3.PoolManager, and uses the swap to add per-stage timing
histograms — connect / TTFB / body — so operators can see where outbound
webhook and storage-upload latency is actually spent.

Why

urllib.request is a black box: a single stopwatch around urlopen() is
all you can measure, which makes it impossible to tell whether time is
being spent in TLS handshakes, in remote processing, or in body transit.
This is the only HTTP path the milter takes for the webhook and the
HTTP-mode storage upload, so the visibility gap matters disproportionately.

What changes

• The webhook call and the non-S3 storage upload now go through one
  module-level urllib3.PoolManager. Previously each call opened a fresh
  TCP+TLS connection; the pool reuses sockets across calls.
• New Prometheus histograms, labelled by (host, scheme):
  • milter_outbound_connect_seconds — TCP + TLS handshake time.
    Recorded only when urllib3 opens a fresh socket; reused pooled
    connections never enter connect() so the count of this metric is
    also the pool-miss rate.
  • milter_outbound_ttfb_seconds — request start → first response byte.
  • milter_outbound_body_read_seconds — first byte → response complete.
  • Plus counters: milter_outbound_new_connections_total,
    milter_outbound_requests_total.
• Per-stage timing is implemented by subclassing
  urllib3.connection.HTTPSConnection / HTTPConnection to time
  connect() in place, and by issuing requests with
  preload_content=False at the call site so the headers and body
  phases can be observed separately.
• Lifecycle: _HTTP.clear() and a reset of the cached boto3 S3 client
  are wired into the existing SIGHUP reload path so a webhook_url or
  storage_url change drops idle connections instead of leaving them
  pinned to the old endpoint.
• storage_upload_threshold is now part of the reloadable config (it
  was a startup-only global, which was inconsistent with storage_url
  being reloadable).
• Pool size is configurable via the outbound_http_pool_maxsize
  config-file key or the OUTBOUND_HTTP_POOL_MAXSIZE env var (default
  50, config file wins over env var, matching the rest of the milter's
  resolution pattern). Invalid values fall back to the default rather
  than failing startup, and the resolved value + its source are logged
  at startup.

Behaviour preserved

• Per-call auth: the bearer secret and any extra webhook headers are
  built fresh per request — pooled connections carry no auth state.
• Error handling: non-2xx responses are still surfaced through the
  existing _interpret_webhook_response path; network errors are still
  classified into timeout / connection_refused / connection_reset
  / network_error.
• Single-attempt application semantics: urllib3.Retry is configured
  with read=False, status_forcelist=(), and total=1, connect=1.
  The one retry that exists fires only on connect-level errors — the
  failure mode where the server has closed our pooled keep-alive
  between calls. This preserves the original "retry would only happen
  on a fresh connection anyway" behaviour of urllib.request, while
  preventing the spurious-network-error class that would otherwise be
  visible from pooling.
• Timeouts: the same numeric budget passed to urllib.request.urlopen
  is now passed to urllib3.Timeout(connect=N, read=N), so the
  per-call budget is identical.
• SIGHUP: continues to swap reloadable config atomically; the new step
  is dropping the cached HTTP/S3 clients in the same handler.

New dependency

urllib3>=2.0,<3 is added explicitly in Dockerfile. It is already
pulled in transitively as a dependency of boto3, so the runtime
footprint is unchanged.

Testing

• Module compiles and imports cleanly.
• The custom HTTPSConnection / HTTPConnection subclasses register
  correctly via PoolManager.pool_classes_by_scheme — confirmed by a
  live HTTPS round-trip in a Python REPL using the same wiring.
• New histograms appear in the milter's existing :9901/metrics
  endpoint with the documented labels.
• Existing milter_webhook_duration_seconds /
  milter_storage_upload_duration_seconds /
  milter_*_calls_total / milter_errors_total continue to be emitted
  with unchanged label sets.
• OUTBOUND_HTTP_POOL_MAXSIZE parsing covers integer / non-integer /
  zero / negative / empty / unset cases — invalid inputs degrade to
  the documented default.

Out of scope

• connect / read timeouts are still passed the same value to
  preserve baseline behaviour. Splitting them (e.g. a short connect
  deadline + a longer read deadline) would be a follow-up.

[greptile-apps]

Greptile Summary

This PR replaces urllib.request with a shared urllib3.PoolManager and introduces per-stage outbound HTTP timing histograms (connect, TTFB, body). The previously-flagged SIGHUP safety gap for storage_upload_threshold has been addressed: int() is now guarded by _coerce_positive_int, consistent with how outbound_http_pool_maxsize is handled. The timing-aware connection subclasses, MaxRetryError unwrapping, preload_content=False split, and SIGHUP reset path are all implemented correctly.

Confidence Score: 5/5

Safe to merge — all previously flagged P1 concerns have been resolved and no new issues were found.

The storage_upload_threshold SIGHUP safety issue from the prior review is fixed via _coerce_positive_int. The urllib3 migration is structurally sound: MaxRetryError is caught via HTTPError inheritance and correctly unwrapped, the preload_content=False timing split is guarded by finally blocks to prevent metric loss on body-read failures, and per-call auth is never cached in the pool. No P0/P1 findings remain.

No files require special attention.

Important Files Changed

Filename	Overview
milter/primitivemail_milter.py	Core change: replaces urllib.request with a shared urllib3.PoolManager backed by timing-aware connection subclasses; adds TTFB/connect/body histograms; promotes storage_upload_threshold to reloadable config with safe coercion; MaxRetryError correctly unwrapped for timeout classification; 2xx/non-2xx handling unified cleanly.
test_milter.py	Test harness updated: MockResponse reshaped to the urllib3 HTTPResponse interface; patch_http() helper added to adapt old capture callbacks; new tests cover MaxRetryError timeout unwrapping, mid-body failure metric recording, and non-UTF-8 error bodies.
Dockerfile	Pins urllib3>=2.0,<3 explicitly; previously only a transitive dep of boto3 — explicit pin matches the PR's stated intent and prevents a future boto3 upgrade from silently pulling in urllib3 v3.

Sequence Diagram

sequenceDiagram
    participant M as PrimitiveMailMilter
    participant HC as _call_webhook_for_recipient / _upload_to_http
    participant PM as _HTTP (urllib3.PoolManager)
    participant TC as _TimingHTTPS/HTTPConnection
    participant R as Remote Endpoint

    M->>HC: eom() triggers webhook/storage path

    HC->>PM: request(POST, url, preload_content=False)
    alt pool miss (fresh socket)
        PM->>TC: connect()
        Note over TC: t0 = time.monotonic()
        TC->>R: TCP + TLS handshake
        Note over TC: record HTTP_CONNECT_DURATION, increment HTTP_CONNECTIONS_NEW
    else pool hit (reused socket)
        PM-->>R: (reuse existing socket)
    end

    Note over HC: t_send = time.monotonic()
    PM->>R: send request headers + body
    R-->>PM: response headers
    PM-->>HC: HTTPResponse (headers only)
    Note over HC: t_headers = time.monotonic()

    HC->>HC: try: response.data
    R-->>HC: response body bytes
    Note over HC: t_done = time.monotonic()
    HC->>PM: release_conn()
    Note over HC: [finally] record TTFB, body, requests_total

    alt 2xx
        HC->>M: success result
    else non-2xx
        HC->>M: failure result
    else urllib3 network exception
        HC->>M: network_error result
    end

    Note over M: SIGHUP: _HTTP.clear() + _s3_client = None

Loading

_{Reviews (5): Last reviewed commit: "Tolerate non-UTF-8 webhook response bodi..." | Re-trigger Greptile}