[arandomogg]

Summary

CircuitBreaker::new(5, Duration::from_secs(60)) was hardcoded at every construction site, so tuning a threshold during an incident meant a code change and a redeploy. This adds environment-variable-driven construction following the pattern already used by middleware::timeout::timeout_layer_from_env() and middleware::body_limit::body_limit_layer_from_env(), with the current hardcoded values as defaults.

closes #406

What changed

src/services/circuit_breaker.rs

• DEFAULT_FAILURE_THRESHOLD (5) and DEFAULT_RECOVERY_TIMEOUT_SECS (60) constants — the previously hardcoded values.

• CircuitBreakerConfig { failure_threshold, recovery_timeout } with a Default impl.

• CircuitBreakerConfig::from_env() reads the global CIRCUIT_BREAKER_FAILURE_THRESHOLD / CIRCUIT_BREAKER_RECOVERY_TIMEOUT_SECS.

• CircuitBreakerConfig::from_env_prefixed(prefix) layers a per-dependency override on top. For prefix = "DB" the resolution order per field is:

  1. DB_CIRCUIT_BREAKER_FAILURE_THRESHOLD / DB_CIRCUIT_BREAKER_RECOVERY_TIMEOUT_SECS
  2. CIRCUIT_BREAKER_FAILURE_THRESHOLD / CIRCUIT_BREAKER_RECOVERY_TIMEOUT_SECS
  3. the compiled-in defaults

  Each field resolves independently, so a per-dependency threshold composes with a globally set recovery window.

• CircuitBreaker::from_config(), ::from_env(), ::from_env_prefixed(), plus Default and the failure_threshold() / recovery_timeout() accessors used by the tests.

Bad input never blocks startup: unparsable values and a 0 failure threshold (which would report the circuit open before a single request) are logged at warn and skipped, falling through to the next source in the chain. A 0 recovery timeout is accepted, since it legitimately means "probe immediately". Resolved values are logged at debug.

Call sites

• src/main.rs resolves CircuitBreakerConfig::from_env_prefixed("DB") once and uses it for both the connect-time breaker inside connect_with_retry (previously the literals 5 and 60 passed as arguments) and AppState::db_circuit_breaker, so the two can no longer drift apart.
• src/services/stellar_service.rs builds the Horizon breaker with CircuitBreaker::from_env_prefixed("HORIZON").
• Test AppState builders (src/ws.rs, src/tests/integration_test_setup.rs, tests/common/mod.rs, tests/team_test.rs, tests/tips_test.rs) use CircuitBreaker::default() — same values as before and deliberately not env-dependent, so the suite stays deterministic.

The gateway::api_gateway breaker is a different type with its own success-threshold parameter and is untouched.

Behaviour

Defaults are unchanged: with no environment variables set, every breaker is still 5 failures / 60 seconds. CircuitBreaker::new keeps its existing signature, so no existing caller had to change.

Riding out a flaky upstream window is now a config change:

HORIZON_CIRCUIT_BREAKER_FAILURE_THRESHOLD=20
HORIZON_CIRCUIT_BREAKER_RECOVERY_TIMEOUT_SECS=15

Tests

Ten new unit tests in src/services/circuit_breaker.rs cover defaults, global vars, prefix precedence, independent per-field resolution, case-insensitive prefixes, unparsable values, an invalid prefixed value falling through to the global one, zero-threshold rejection (both global and prefixed), zero recovery timeout, and an env-configured breaker actually tripping at its configured threshold. They serialize on a mutex and clear the variables before and after, since std::env is process-global.

cargo fmt --all is clean, and cargo clippy --all-targets --all-features reports no lint against any of the changed code. Note that the repo-wide clippy gate is already failing independently of this PR: the same command with -D warnings (as CI runs it) exits 101 with 188 errors on unmodified main at 2a91dcd, all pre-existing dead-code and unused-import warnings. cargo test --lib leaves only the three failures that main also has locally (db::transaction::tests::savepoint_recovery_works, db::transaction::tests::transaction_rollback_works — both need a live database — and the ws::integration::ping_timeout_disconnects_after_missed_pongs timing flake).

One unrelated test fix

src/telemetry/sampler.rs — its four tests race on the process-global OTEL_SAMPLE_RATIO (one clearing it while another has just set it), which already failed intermittently on main: 2 of 12 runs of cargo test --lib telemetry::sampler at 2a91dcd, with no changes applied. The tests added here perturb scheduling enough that it surfaced on most runs, so rather than ship a PR that reddens CI I serialized them with the same mutex-guard idiom. 12 of 12 green afterwards. Happy to split this into its own PR if you would rather keep this one single-purpose.

Docs

.env.example gains a commented circuit-breaker block, and docs/DEPLOYMENT.md gains the four variables in the environment table plus a "Tuning Circuit Breakers" section documenting the resolution order and the incident-response workflow.

[Christopherdominic]

Thanks for picking this up, @arandomogg — this is assigned to you via the GrantFox bot for issue #406.

Before this can be merged, all required CI checks need to pass. Currently failing:

• ❌ clippy
• ❌ test

Currently passing:

• ✅ fmt
• ✅ sqlx
• ✅ docker

Please push a fix and make sure the full CI suite is green — I'll take another look once it is. Thanks for the contribution!

[arandomogg]

#406 — circuit breaker environment configuration. @Bonizozo this is ready for review. One thing needs your action first: the latest run on this branch is sitting in action_required, so the workflow will not start until you approve it. Nothing has executed against the new commit yet.

A note on the two red jobs, because neither was caused by this PR.

test — fixed here. The seven failures in advanced_integration_tests were all the same thing: duplicate key value violates unique constraint "creators_username_key" while each test created its own creator. Cargo runs the test functions in a binary on parallel threads, and they all share one database, but TestContext::cleanup called common::cleanup_test_db, which TRUNCATEs creators and tips outright. A test finishing mid-run therefore deleted the rows every other in-flight test was still using, and with fixed usernames shared across tests an insert would race a neighbour's truncate and collide on the unique index. This PR gives each TestContext a random namespace, derives every creator username and email from it, and deletes only that namespace's rows on cleanup instead of truncating shared tables. The tests keep their parallelism and no longer depend on each other's data or on leftovers from a previous run. Callers that read the username back out of the create response, such as tip_flows::test_bulk_tip_processing, are unaffected.

clippy — pre-existing, and not something this PR should carry. cargo clippy --all-targets --all-features -- -D warnings currently reports roughly 2400 lint instances across 98 files, about 1700 of them dead_code and 470 unused_imports. None are in the code this PR touches. For what it is worth on the history: the gate arrived in #356, which was merged with no checks reported on its own branch, so it never ran against itself; the pipeline's first execution was the push to main three seconds after that merge, and it failed. Every CI run in this repository so far has failed, on main included.

I deliberately did not paper over that. Dropping -D warnings to force a green tick would have been a one-line change, but it weakens the gate for everyone and that is your call, not mine. Clearing the debt properly is a real piece of work that deserves its own PR rather than riding along inside an unrelated change. So clippy will still be red here, exactly as red as it is on main.

Happy to open that cleanup PR separately if you want it, and equally happy to split the test-isolation commit out of this one if you would rather keep this PR to a single concern.