Skip to content

@W-23735204 Add circuit breaker around data store reads - #639

Draft
npeternel-sf wants to merge 1 commit into
mainfrom
W-23735204-circuit-breaker
Draft

npeternel-sf wants to merge 1 commit into
mainfrom
W-23735204-circuit-breaker

Conversation

@npeternel-sf

Copy link
Copy Markdown
Contributor

Summary

Wraps DataStore.getEntry reads in a per-warm-container, in-memory circuit breaker so a container stops hammering DynamoDB when the table is failing or throttling. Builds on the adaptive-retry + throttle-aware work (W-23735205) and the sharding work (W-23735176).

Even with adaptive retries and bounded timeouts, storefront-next keeps issuing DAL reads during a sustained throttling event — paying per-call latency/cost and adding load to a saturated table. The breaker converts a run of slow repeated failures into fast, cheap ones and gives the table room to recover.

How it works

  • States: closed → open → half-open. Closed counts failures (throttles weighted heavier, reusing the existing isThrottlingError signal). At threshold it opens and fails fast for a cooldown without calling DynamoDB; after the cooldown it admits a bounded half-open probe burst — success closes it, any failure re-opens it.
  • Fail-fast when open: throws DataStoreServiceError immediately. The DAL client's existing application-level API fallback then serves correct data (the API is the source of truth; DAL is a read-scaling copy).
  • Misses are healthy: DataStoreNotFoundError (a miss) never trips the breaker; a completed send counts as success even on a miss. Normal miss/hit traffic can't open it.
  • Per warm container: state lives in memory and rides the same warm-container reuse as the memoized DynamoDB client. Cold starts begin closed. Not fleet-coordinated (by design).
  • Concurrency-safe probing: half-open admits only up to the probe budget, so concurrent reads on one container can't stampede a still-saturated table.

Telemetry

Emitted only on state transitions (log-quota conscious), via MRT internal structured logging:

  • Opening → logMRTError (error level — the backend is failing).
  • Probing / recovery → new info-level logMRTEvent, so recovery doesn't trip error-based alerting.

Configuration

One env var kill switch: MRT_DATA_STORE_CIRCUIT_BREAKER_DISABLED (true/1 to disable; unset = active). Threshold, throttle weight, cooldown, and half-open probe count are engineering-tuned internal constants, not incident-time dials.

Backward compatibility

getEntry(key) signature and return shape unchanged. Default behavior unchanged when the breaker never trips. CircuitBreaker is kept internal (not exported from the package barrel).

Testing

  • New CircuitBreaker unit tests: trip, throttle weighting, open/half-open/recovery, re-open on probe failure, concurrent-probe budget, mixed fail/success non-trip, transition callbacks.
  • New getEntry integration tests: fail-fast with zero DynamoDB calls when open, failures accumulate to open, throttle weighting, miss doesn't trip, recovery closes, concurrent reads straddling half-open admit one probe, telemetry emitted, kill switch bypass (true and 1).
  • 581 passing; typecheck, lint, and prettier clean for @salesforce/mrt-utilities.

Work item: W-23735204

Wrap DataStore.getEntry in a per-warm-container in-memory circuit breaker
(closed -> open -> half-open) that sheds load when DynamoDB is failing or
throttling: after a run of failures (throttles weighted heavier) the breaker
opens and fails fast for a cooldown without calling DynamoDB, letting the
client's application-level API fallback serve reads, then admits a bounded
probe burst to recover.

- Misses (DataStoreNotFoundError) count as healthy, never trip the breaker.
- Telemetry only on state transitions: open via logMRTError, recovery via a
  new info-level logMRTEvent so recovery doesn't trip error alerting.
- Kill switch: MRT_DATA_STORE_CIRCUIT_BREAKER_DISABLED. Threshold/cooldown/
  probe count are internal constants.
- CircuitBreaker kept internal (not exported from the package barrel).
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.

2 participants