Skip to content

fix(#1748): bound the LML-client limiter queue with a deadline + circuit breaker - #1883

Open
jakebromberg wants to merge 1 commit into
mainfrom
fix/1748-bound-lml-limiter-queue
Open

fix(#1748): bound the LML-client limiter queue with a deadline + circuit breaker#1883
jakebromberg wants to merge 1 commit into
mainfrom
fix/1748-bound-lml-limiter-queue

Conversation

@jakebromberg

Copy link
Copy Markdown
Member

Summary

The shared LML client's process-wide Semaphore(5) did an unbounded, un-timed acquire() before the HTTP span even opened. Under sustained LML slowness a caller could queue for minutes with no deadline and no visible span — long past STRANDED_TTL_SECONDS (60s), so the row it was holding open would strand anyway. This bounds that admission wait and adds a fast-fail path.

What changed

  • Bounded admission waitSemaphore.acquire(maxWaitMs) rejects with a new LmlSheddedError (extends LmlClientError) once a caller has waited past the queue deadline, instead of blocking indefinitely. FIFO ordering is preserved so a timed-out waiter can't jump the queue.
  • Circuit breakerLmlCircuitBreaker (closed / open / half-open) fast-fails admission after LML_CIRCUIT_BREAKER_THRESHOLD consecutive failures and probes recovery after LML_CIRCUIT_BREAKER_RESET_MS, so a hard-down LML sheds immediately rather than making every caller eat the full deadline.
  • Opt-in, backward-compatiblecreateLmlLimiter gains optional queueDeadlineMs / breaker. Omitting them preserves the original unbounded shape, so backfill/job-level limiters are unchanged. Only the runtime defaultLimiter (interactive path) opts in, wired via LML_LIMITER_QUEUE_DEADLINE_MS / LML_CIRCUIT_BREAKER_THRESHOLD / LML_CIRCUIT_BREAKER_RESET_MS.
  • Shed = leave-for-recovery — a shed throws LmlSheddedError, a subtype of the existing LmlClientError, so every catch arm that already treats an LML failure as "leave the row for the recovery sweep" ([C6] Retune flowsheet-metadata-backfill cron as safety-net (hourly, 15-min grace) #895 backfill cron) needs no change. No new shed-handling branches.
  • CI breaker override.github/workflows/test.yml + dev_env/docker-compose.yml pin LML_CIRCUIT_BREAKER_THRESHOLD high so integration specs that simulate consecutive LML 500s (tests/integration/metadata.spec.js) can't trip the breaker mid-suite (mirrors the BS#955 precedent).

Defaults

Env var Default Rationale
LML_LIMITER_QUEUE_DEADLINE_MS 15000 Total admission wait ceiling, well under STRANDED_TTL_SECONDS=60s so a shed always beats the strand.
LML_CIRCUIT_BREAKER_THRESHOLD 5 Consecutive failures before the breaker opens.
LML_CIRCUIT_BREAKER_RESET_MS 30000 Half-open probe interval.

Testing

  • tests/unit/shared/lml-client/limiter-shed.test.ts (new, 517 lines) — fake-timer coverage of the bounded semaphore (FIFO, deadline rejection), the breaker state machine, the limiter wiring, and an end-to-end path through the real lookupMetadata with mocked fetch.
  • tests/unit/services/lml.client.test.ts — added _resetLmlClientLimitersForTest() to beforeEach (the breaker adds shared mutable state on the process-wide defaultLimiter).
  • Local CI green: prettier --check ✓, typecheck ✓, eslint 0 errors ✓, targeted unit suites 160/160 ✓.

Open questions for review

  1. Defaults15000 / 5 / 30000 are first-pass; the only hard constraint is deadline < STRANDED_TTL_SECONDS. Comfortable with these, or tune?
  2. Breaker failure definition — the breaker currently counts any rejection (timeout, shed, transport, 5xx) as a failure. Intentional (fail-fast on any sustained badness) but worth a sanity check vs. counting only timeouts/5xx.
  3. No shed telemetry span — a shed throws but opens no Sentry span of its own; it surfaces via the caller's existing error handling. A dedicated lml.shed breadcrumb/counter could be a fast follow if we want the shed rate to be first-class queryable.

Scope

+899 / −16 across 8 files. Part of [Epic B #876] (BS↔LML single-coordinator contract) — this is the client-side admission-control slice.

Closes #1748

…uit breaker

The shared LML client's process-wide Semaphore(5) did an unbounded, un-timed
acquire() before the HTTP span even opened, so under sustained LML slowness
callers could queue for minutes with no deadline and no visible span. Add a
bounded admission wait (Semaphore.acquire(maxWaitMs)) and a consecutive-
failure circuit breaker (LmlCircuitBreaker) to createLmlLimiter, wired into
the runtime defaultLimiter via LML_LIMITER_QUEUE_DEADLINE_MS /
LML_CIRCUIT_BREAKER_THRESHOLD / LML_CIRCUIT_BREAKER_RESET_MS. A shed throws
LmlSheddedError (extends LmlClientError), so every existing catch arm that
already treats an LML failure as "leave the row for the recovery sweep"
needs no changes. Backfill/job-level limiters keep the original unbounded
shape by omitting the new opt-in config. CI overrides the breaker threshold
on both surfaces so existing specs that simulate consecutive LML 500s can't
trip it mid-suite.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bound the LML-client limiter queue: total deadline < STRANDED_TTL + circuit breaker (fast-fail, no multi-minute waits)

1 participant