From 41c5cda2e08ecf8a43719419eb029eea2bdb3cab Mon Sep 17 00:00:00 2001 From: Michael Dorf Date: Wed, 5 Aug 2026 13:47:23 -0700 Subject: [PATCH] feat: return 503 when a goo SPARQL circuit breaker is open goo can shed load when Redis or the triple store is failing (Ship 2, de-fork review D1/D2), but as a library it can only raise Goo::SPARQL::Resilience::CircuitOpenError. Nothing translated that, so an open breaker surfaced as a 500 with a stack trace: indistinguishable from an application bug, and telling the client nothing about retrying. That made D1/D2 implemented but not in force (recorded as D15). config/resilience.rb adds: * a Sinatra handler mapping CircuitOpenError to 503, with Retry-After set from the breaker's own cool-off and Cache-Control: no-store, since Rack::Cache fronts this app and a cached 503 would keep serving the outage after the dependency recovered; * on_state_change wired to the app logger and a New Relic custom event. goo only warns to stderr, which nothing watches, and a load-bearing dependency tripping is page-worthy. Called per transition, not per rejected request, so an outage produces one notice rather than a flood; * on_invalidation_failure wired to a log line and a counter, since a dropped invalidation leaves stale cache entries until that graph's next successful write. Both hooks swallow their own exceptions: reporting must never be the thing that breaks a request. Inert unless OP_SPARQL_CIRCUIT_BREAKER is set, so this changes nothing on its own. Gemfile.lock moves goo to 8a80f1c (development), which is the first revision containing Goo::SPARQL::Resilience. Tests assert 503 rather than 500, the Retry-After and no-store headers, that normal requests are unaffected, and that both hooks are wired and cannot raise. Verified the breaker tests error out without the handler. --- Gemfile.lock | 15 ++++- app.rb | 4 ++ config/resilience.rb | 79 +++++++++++++++++++++++++ test/controllers/test_resilience_503.rb | 79 +++++++++++++++++++++++++ 4 files changed, 174 insertions(+), 3 deletions(-) create mode 100644 config/resilience.rb create mode 100644 test/controllers/test_resilience_503.rb diff --git a/Gemfile.lock b/Gemfile.lock index e6f2fa1f0..092d2bdc8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -8,10 +8,11 @@ GIT GIT remote: https://github.com/ncbo/goo.git - revision: 78ff8608a1d1350de134ce524ccc31befa172abf + revision: 8a80f1cc2c02c26dc2e18653c2c1a19c238dec51 branch: development specs: goo (0.0.2) + activesupport addressable (~> 2.8) pry rdf @@ -19,9 +20,11 @@ GIT rdf-rdfxml rdf-vocab redis + request_store rest-client rsolr sparql-client (= 3.2.2) + stoplight (~> 5.0) uuid GIT @@ -268,7 +271,7 @@ GEM i18n (1.15.2) concurrent-ruby (~> 1.0) io-console (0.8.2) - json (2.21.1) + json (2.21.2) json-canonicalization (0.4.0) json-ld (3.2.5) htmlentities (~> 4.3) @@ -487,6 +490,9 @@ GEM net-sftp (>= 2.1.2) net-ssh (>= 2.8.0) ostruct + stoplight (5.8.2) + concurrent-ruby + zeitwerk systemu (2.6.5) temple (0.10.4) tilt (2.7.0) @@ -512,6 +518,7 @@ GEM crack (>= 0.3.2) hashdiff (>= 0.4.0, < 2.0.0) webrick (1.9.2) + zeitwerk (2.8.3) PLATFORMS aarch64-linux @@ -670,7 +677,7 @@ CHECKSUMS http-cookie (1.1.6) sha256=ba4b82be64de61dc281243dac70e3c382c45142f20268ed9276a3670c93feaa9 i18n (1.15.2) sha256=00f9eb62412fe593b2a65a97daa75300d37abb8f7202ec748e94b6d46a9dd1b5 io-console (0.8.2) sha256=d6e3ae7a7cc7574f4b8893b4fca2162e57a825b223a177b7afa236c5ef9814cc - json (2.21.1) sha256=13a43df75d95641443f5702dff350f237164a9d811ff0f2c2800d4d980220583 + json (2.21.2) sha256=1f1d3b7cf2b3ba1a69beca0bb6db13d5438b80bff3cd54cdaaa620b9b07c1c6a json-canonicalization (0.4.0) sha256=73ea88b68f210d1a09c2116d4cd1ff5a39684c6a409f7ccac70d5b1a426a8bef json-ld (3.2.5) sha256=98b96f1831b0fe9c7d2568a7d43b64f6b8c3f5892d55ccf91640e32a99c273fc json-schema (6.2.0) sha256=e8bff46ed845a22c1ab2bd0d7eccf831c01fe23bb3920caa4c74db4306813666 @@ -774,6 +781,7 @@ CHECKSUMS sinatra-contrib (4.2.1) sha256=10d091c944d268aa910c618ea40a3c3ebe0533e6e32990d84af92235a3d26b4a sparql-client (3.2.2) sha256=d3bac6bea08598dad617c0cdc6d380f9d416ef4ab91ca28309fdf40b93f5a571 sshkit (1.25.0) sha256=c8c6543cdb60f91f1d277306d585dd11b6a064cb44eab0972827e4311ff96744 + stoplight (5.8.2) sha256=8072a7f27a3adf1e9eb27ea635a98bc56a80ca615d07a4d68653f428c7a916d6 systemu (2.6.5) sha256=01f7d014b1453b28e5781e15c4d7d63fc9221c29b174b7aae5253207a75ab33e temple (0.10.4) sha256=b7a1e94b6f09038ab0b6e4fe0126996055da2c38bec53a8a336f075748fff72c tilt (2.7.0) sha256=0d5b9ba69f6a36490c64b0eee9f6e9aad517e20dcc848800a06eb116f08c6ab3 @@ -788,6 +796,7 @@ CHECKSUMS uuid (2.3.9) sha256=aec0cf592053cd6e07c13c1ef94c440aba705f22eb1ee767b39631f2760124d7 webmock (3.26.2) sha256=774556f2ea6371846cca68c01769b2eac0d134492d21f6d0ab5dd643965a4c90 webrick (1.9.2) sha256=beb4a15fc474defed24a3bda4ffd88a490d517c9e4e6118c3edce59e45864131 + zeitwerk (2.8.3) sha256=2c85125a8467ce069e20123d1e709a08955c9d29c118c25b46b7b7fafdbb92e5 BUNDLED WITH 4.0.10 diff --git a/app.rb b/app.rb index 6db21442a..69bf2bf6f 100644 --- a/app.rb +++ b/app.rb @@ -36,6 +36,10 @@ # Inflector setup require_relative "config/inflections" +# Map goo's open-circuit signal to a 503 and route breaker transitions to logging/New Relic. +# Inert unless OP_SPARQL_CIRCUIT_BREAKER is set (goo de-fork review D1/D2/D15). +require_relative "config/resilience" + require 'request_store' # Protection settings diff --git a/config/resilience.rb b/config/resilience.rb new file mode 100644 index 000000000..ca69f37c1 --- /dev/null +++ b/config/resilience.rb @@ -0,0 +1,79 @@ +# Web-layer half of goo's SPARQL circuit breakers (goo de-fork review D1/D2, D15). +# +# goo can shed load when the Redis cache or the triple store is failing, but as a library it can +# only raise: it has no way to produce an HTTP response. It raises +# Goo::SPARQL::Resilience::CircuitOpenError, and translating that into a 503 is this file's job. +# Without the translation an open breaker surfaces as a 500 with a stack trace, which is +# indistinguishable from a real bug and tells a client nothing about retrying. +# +# The breaker itself is off unless OP_SPARQL_CIRCUIT_BREAKER is set in the environment; this file +# is inert until then, so it changes nothing on its own. +# +# Why 503 and not 500: the request failed because a dependency is unavailable, not because the +# request was wrong. 503 is the status clients and load balancers already treat as "retry later", +# and it carries Retry-After. + +if defined?(Goo::SPARQL::Resilience) + + # 503 + Retry-After for a request that hit an open breaker. + # + # Deliberately NOT cached: Rack::Cache sits in front of this app, and caching a shed response + # would keep serving the outage after the dependency recovered. `no-store` also stops any + # intermediary from holding on to it. + error Goo::SPARQL::Resilience::CircuitOpenError do + err = env['sinatra.error'] + retry_after = begin + Goo::SPARQL::Resilience.cool_off + rescue StandardError + 30 + end + + headers 'Retry-After' => retry_after.to_s, 'Cache-Control' => 'no-store' + LOGGER.error("503 (circuit open): #{err.message} #{request.request_method} #{request.path}") + + status 503 + body({ errors: ['A backend dependency is unavailable; the request was not attempted. ' \ + 'Retry shortly.'], + status: 503 }.to_json) + end + + # Breaker state transitions and dropped cache invalidations. goo only warns to stderr, which + # nothing watches; a load-bearing dependency tripping is page-worthy, so route both through the + # app logger and New Relic. + # + # Called from goo at the moment of transition, NOT per rejected request, so this is safe to make + # a real reporting call: an outage produces one notice, not one per request. + Goo::SPARQL::Resilience.on_state_change = lambda do |circuit, from, to, error| + detail = error ? " (#{error.class}: #{error.message})" : '' + message = "SPARQL circuit '#{circuit}' #{from} -> #{to}#{detail}" + + if to.to_s == 'red' + LOGGER.error("ALERT: #{message} -- shedding load for this dependency") + else + LOGGER.info(message) + end + + if defined?(NewRelic::Agent) + NewRelic::Agent.record_custom_event('SparqlCircuitStateChange', + circuit: circuit.to_s, from: from.to_s, to: to.to_s, + error: error&.class&.to_s) + end + rescue StandardError => e + # Reporting must never be the thing that breaks a request. + warn "resilience state-change hook failed: #{e.class}: #{e.message}" + end + + # A dropped invalidation leaves stale cache entries for that graph until its next successful + # write, so it is worth counting even though it cannot fail the request that caused it. + Goo::SPARQL::Resilience.on_invalidation_failure = lambda do |graph_key, error| + LOGGER.warn("SPARQL cache invalidation dropped for #{graph_key}: #{error.class}") + + if defined?(NewRelic::Agent) + NewRelic::Agent.increment_metric('Custom/SPARQL/CacheInvalidationDropped') + end + rescue StandardError => e + warn "resilience invalidation hook failed: #{e.class}: #{e.message}" + end + + puts '(API) >> SPARQL circuit breaker ACTIVE (OP_SPARQL_CIRCUIT_BREAKER)' if Goo::SPARQL::Resilience.enabled? +end diff --git a/test/controllers/test_resilience_503.rb b/test/controllers/test_resilience_503.rb new file mode 100644 index 000000000..f26a27779 --- /dev/null +++ b/test/controllers/test_resilience_503.rb @@ -0,0 +1,79 @@ +require_relative '../test_case' + +# The web-layer half of goo's circuit breakers (goo de-fork review D1/D2/D15): goo raises +# CircuitOpenError when it sheds load, and config/resilience.rb must turn that into a 503 rather +# than letting it surface as a 500. Without this the "fail fast and shed load" design is only half +# implemented, since a 500 tells a client nothing about retrying and reads as an application bug. +# +# The breaker itself is not exercised here (that is goo's test/test_resilience.rb). This asserts +# only the translation, by making a goo query raise the way an open breaker would. +module CircuitOpenInjector + class << self + attr_accessor :armed + end + + def query(*args, **kwargs, &block) + if CircuitOpenInjector.armed + raise Goo::SPARQL::Resilience::CircuitOpenError, "circuit 'goo:redis' is open" + end + + super + end +end + +unless Goo::SPARQL::Client.ancestors.include?(CircuitOpenInjector) + Goo::SPARQL::Client.prepend(CircuitOpenInjector) +end + +class TestResilience503 < TestCase + def teardown + CircuitOpenInjector.armed = false + end + + def test_open_circuit_returns_503_not_500 + CircuitOpenInjector.armed = true + get '/ontologies' + + assert_equal 503, last_response.status, + 'an open breaker must shed load as 503, not look like an application bug (500)' + parsed = MultiJson.load(last_response.body) + assert_equal 503, parsed['status'] + refute_empty parsed['errors'] + end + + def test_503_tells_the_client_when_to_retry_and_is_not_cacheable + CircuitOpenInjector.armed = true + get '/ontologies' + + # Retry-After should reflect the breaker's own cool-off, so clients back off for about as long + # as the breaker stays open rather than guessing. + assert_equal Goo::SPARQL::Resilience.cool_off.to_s, last_response.headers['Retry-After'] + + # Rack::Cache fronts this app; a cached 503 would keep serving the outage after recovery. + assert_includes last_response.headers['Cache-Control'].to_s, 'no-store' + end + + def test_normal_requests_are_unaffected + CircuitOpenInjector.armed = false + get '/ontologies' + + assert_equal 200, last_response.status + refute_equal 'no-store', last_response.headers['Cache-Control'].to_s + end + + # goo calls these at state transitions and when it drops an invalidation. They are the only + # signal that a dependency has tripped, so they must be wired and must never raise: an exception + # here would surface inside whatever request happened to trigger the transition. + def test_reporting_hooks_are_wired_and_cannot_raise + assert_respond_to Goo::SPARQL::Resilience.on_state_change, :call, + 'goo needs a state-change hook or a tripped breaker is invisible' + assert_respond_to Goo::SPARQL::Resilience.on_invalidation_failure, :call + + Goo::SPARQL::Resilience.on_state_change.call('goo:redis', :green, :red, + Redis::CannotConnectError.new('down')) + Goo::SPARQL::Resilience.on_state_change.call('goo:redis', :red, :green, nil) + Goo::SPARQL::Resilience.on_invalidation_failure.call( + 'sparql:graph:http://example.org/g', Goo::SPARQL::Resilience::CircuitOpenError.new('open') + ) + end +end