Repository navigation
Phase 6a: retry — tests and doubles - #73
Merged
Merged
Conversation
Add Dexpace::Resilience -- Policy (the two-axis classifier consult, the
backoff calculator, the total pacing-header parser over the private
PacingParsers, the retry-count resolver, the recovery-only budget and
RETRY-12's constants), Resend.eligible? (the re-sendability gate over
phase 1's idempotent set and phase 3b's replayability), and the frozen
RetrySettings both retry stacks build from, the first reader of 5a's
Keys::MAX_RETRY_ATTEMPTS -- plus the flat Dexpace::RetryPredicateError.
Widen two earlier-phase files in place: ProtocolError bakes XCUT-5's
retryable_by_status? from 5a's Retryability at construction (phase 4b's
postponement; deliberately not #retryable?, P6-10), and HTTPDate's day
group becomes (\d{1,2}) so RETRY-15's single-digit day parses through the
one RFC 1123 parser (R1); 5a's rejection loop swaps that element for the
bare-date row CFG-31 still refuses.
One calculator, two budget policies: backoff_delay takes no total-timeout
and budget_remaining is a separate function only the recovery engine
names (R6, P6-5). A zero initial delay answers 0.0 before the power is
taken, because 0.0 * Infinity is NaN and [NaN, 8.0].min raises (P6-53).
Widen Pipeline::Cursor with one read-only member, #bundle -- CTX-14's per-call correlation bundle -- seeded by a new optional `bundle:` keyword on Pipeline#call and AsyncPipeline#call (Bundle::NONE when omitted), validated in Cursor.build, threaded through both private drivers' #advance and carried across #fork exactly as #options is. No existing signature moves, and Transport.conforms? still accepts both runtimes because the keyword is optional beside the SPI's three positionals (P6-51). The one consumer is 5b's instrumentation step, on both runtimes: #open_span now takes the cursor's bundle and prefers its tracer factory when the bundle is not NONE, else the step's own keyword, and Tracing.correlate is handed the same bundle, so a populated one pushes its trace and span ids onto the diagnostic context. That is the reconciled precedence's first clause, which phase 5 recorded as having no implementation path; a call that seeds nothing resolves exactly as phase 5 did. The bundle carries no operation name, so the span is still named by the method token (P6-52), and it is not the retry step's HTTP-tracer source (P6-7). Existing tests only widened where the code required it: the cursor's public-method pin gains #bundle, and the two step suites' hand-rolled cursor stand-ins answer #bundle with NONE.
Add Resilience::RetryStep and Resilience::AsyncRetryStep at Stages::RETRY, over the private RetryStepHelpers mixin that holds the decision, the delay resolution, the two override hooks and the trail handling both share. Each drives the downstream chain once per attempt through a fresh Cursor#fork -- the first drive included -- so hop 1 and hop n are one object and each attempt re-executes the chain with fresh per-attempt state (RETRY-44); neither writes cursor state of its own. The synchronous step waits through 5a's cancellable Clock#sleep and checks the token at the top of every attempt. The async step is a trampoline, not a recursion: one Pump per call resumes under a re-arm flag held by a Thread::Mutex across the flip only, so an inline settlement re-enters #resume without growing the stack (the design's recursive sketch overflows at about 1,500 attempts on every supported Ruby); a positive delay goes through Dexpace::Async.delay, which without a Fiber.scheduler raises SeamError synchronously and fails the future with the trail attached (R2's third route), and a zero-length delay continues inline. A settled fatal answering #retryable? is delivered unclassified rather than retried (P6-55). OBS-29's per-attempt group is emitted through the tracer the `http_tracer_factory:` keyword produces once per operation, called with the cursor (R3, P6-7); retries_exhausted fires only when a retryable failure met a spent budget. The interface _HTTPTracer phase 5c declined to declare arrives in the http_tracer sig with the wiring, `context` untyped because the recovery stack hands a Request where the stage stack hands a Cursor. The error-status response is closed before the wait and before any throwable propagates (RETRY-35); no total-timeout is named on this stack (RETRY-28, P6-5).
Add Resilience::RecoveryRetry, chapter 9's first stack (RECOV-17 to RECOV-30 and RECOV-34, which phase 4 postponed), installed as Recovery::Orchestrator's `transport:` decorator rather than as a ResponseChain step (P6-3): Transform#apply carries no request to re-send, and only a transport-decorator position can dispatch its own re-sends while the request chain's stamping stays once per exchange. Sitting below the orchestrator's rescue, a raising transport is retried here as an exception (P6-8), each attempt's response is classified through phase 4b's own Recovery.buffer_error_body then ProtocolError.for_or_nil -- the buffering is what frees the connection before the wait -- and the two terminal shapes are a returned response when the failure was never retryable and a raised throwable otherwise, with every prior failure attached as suppressed (P6-9). This is the only stack with a total-timeout: the budget is a maximum-attempts cap counting the first send as attempt 1 (max_retries plus one, an identity, P6-6) and the time remaining through Policy.budget_remaining, so a delay that would overshoot is suppressed and the last failure surfaced. The HTTP tracer is produced once per operation with the Request, there being no cursor on this stack. The entry file now requires the whole retry layer, the entry test pins the six Resilience constants and the flat error with the private helpers unreachable, and the runtime surface manifest gains the forty-eight rows the phase's public surface adds.
Review round 0 of the phase-6a stack found four behaviours the design states and the code did not fully honour, and one comment nit. The pacing parsers converted an unbounded digit run: a 10 MB header value stalled the retry decision for seconds in String#to_f / #to_i and, past ~309 digits, emitted Ruby's out-of-range warning that the suite's NFR-6 raiser turned into an error Policy#parse_form's rescue swallowed. Both grammars now bound their runs at fifteen digits -- the largest run a Float carries exactly, and 10**15 seconds is thirty million years, so a longer run is RETRY-16's out-of-range value and answers nil without a conversion -- and a 64-byte ceiling in front of every parser keeps the HTTP-date attempt from reading a hostile value either (the longest well-formed form is the 29-byte RFC 1123 date). RetryStep#wait emitted the tracer's attempt_failed outside the RETRY-35 fence, so a tracer that raised left the superseded response open on the sync driver while the async driver closed it. The emission now runs inside the same fence as the delay resolution, so the response is closed before the raise propagates on both drivers. A caller's should_retry answering true retried a downstream CancelledError. Policy.cancellation? walks the cause chain for a CancelledError; Policy.retryable? answers false for one before either branch, and the stage drivers' decision answers :stop for one before the re-sendability gate and before the predicate, so RETRY-23's "never" holds on all three drivers whatever a capability or a predicate says. A negative configured MAX_RETRY_ATTEMPTS raised at RetrySettings.build, so RETRY-41's clamp-and-log was unreachable from any driver. RetrySettings.build takes a logger: keyword, read once at build, and resolves the configured value through Policy.effective_max_retries, so a negative configured value is clamped to the default and the clamp logged as one contained config diagnostic; an explicit negative max_retries: is still refused by RECOV-34. Also: step.rb's class comment cited P6-52 for the no-operation-name statement, which is P6-51's; the RBS mirrors follow each change and the core manifest gains Policy#cancellation? (1004 -> 1005 rows).
Review round 1 of the phase-6a stack found the sync/async drift round 0 closed for attempt_failed still open on the terminal path: RetryStep's settle emitted the tracer's retries_exhausted outside any fence, so a tracer that raised there propagated while the terminal error-status response it had just been handed stayed open, whereas the async pump's finish already ran inside its guarded block and closed it. The trail attachment and the retries_exhausted emission now run inside the same fence as the decision and the delay resolution, so a throwing tracer propagates (OBS-30) with the terminal response closed first on both drivers; a tracer that does not raise leaves the returned response open exactly as before (RETRY-34), and the exception path is untouched. The class comment and the method's comment state the terminal path too.
Add the three doubles phase 6a's retry suites fold over, all under test/support/ and top level like every double since phase 2: ScriptedTransport, which consumes a per-call script of responses, Exceptions and callables so a suite can stage 503, 503, 200 or a run of throwables and assert with assert_same that one request object was re-sent every time; ScriptedAsyncTransport, its SEAM-16 twin over a fresh Completer per call, settling inline by default (the shape that overflowed the plan's recursive pump) or held pending under settle_later: with settle_next! for the cancel-in-flight cases; and the RetryFixtures mixin over RecoveryFixtures -- a request per method, a response over a real ResponseBody so body.closed? is the RETRY-35 assertion, RetryableError and UnretryableError answering XCUT-6's capability, RetryableFatal for RETRY-25, jitter-free settings over a FakeClock, and the RecordingCursor that forwards to the real cursor the driver minted so the fork-for-every-drive rule is asserted against the real runtime. Phase 2's FakeTransport is untouched: seven suites and three doubles require it by name and shape.
Add policy_test.rb (the two-axis classifier consult, the backoff calculator including the 2,000-attempt run that found the NaN at attempt 1,025, the pacing-header parse in every form RETRY-15 names, the retry-count resolver's clamp and its logged diagnostic, and the recovery-only budget), resend_test.rb (the re-sendability gate over phase 1's idempotent set and 3b's replayability, with the streaming and chunked bodies that can never be re-sent) and retry_settings_test.rb (the frozen settings, its validators, the UNSET sentinel and the first read of 5a's Keys::MAX_RETRY_ATTEMPTS under a fake config source). Extend two earlier-phase suites exactly as their postponements said: protocol_error_test.rb gains the nested RetryableTest over the baked retryable_by_status? flag, agreeing with Retryability across 400..599 and deliberately not answering #retryable? (P6-10); http_date_test.rb gains RETRY-15's two single-digit-day tests, one for what the widening admits and one for what it still refuses.
Extend five phase-4c and phase-5b suites with the tests of the one widening Task 8 made. cursor_test.rb's BundleTest: Cursor#bundle is NONE by default, the seeded object at every position and across every fork, refused when not a Bundle, and no writer of any name appears. pipeline_test.rb's and async_pipeline_test.rb's BundleSeedingTest: the `bundle:` keyword seeds the cursor for one call, the empty pipeline still dispatches with no cursor, both runtimes remain a Transport by the duck type, and a non-Bundle is refused before any step runs. step_test.rb's and async_step_test.rb's BundlePrecedenceTest: the reconciled precedence's first clause -- the seeded bundle's tracer factory wins over the step's keyword, the keyword over the constant, a call that seeds nothing resolves exactly as phase 5 did, and the seeded bundle's ids reach the diagnostic context through Tracing.correlate.
Add retry_step_test.rb and async_retry_step_test.rb, both driven through a REAL Pipeline or AsyncPipeline over the scripted transports and a RecordingCursor, so the fork-for-every-drive rule is asserted against the runtime that mints the cursor. The synchronous suite covers eligibility (the two axes, the re-sendability gate no predicate can override), the three terminal paths with the trail attached to the surfaced instance, the delay ladder in RETRY-39's order with the override hook's fall-through logged, the predicate's abort as RetryPredicateError, the budget, cancellation at the top of every attempt and inside the wait, the HTTP-tracer group in OBS-29's order with retries_exhausted only after a retryable failure, and the source guards (no Kernel#sleep, no total-timeout named on this stack). The asynchronous suite adds the trampoline: 2,000 inline-settling attempts complete without growing the stack, a positive delay without a Fiber.scheduler fails the future synchronously with the trail attached and a zero-length delay continues inline (R2), a positive delay under 5a's ParkingScheduler parks a fiber and resumes, a cancelled future aborts at the next boundary while an in-flight attempt is allowed to land, and a settled fatal answering #retryable? is delivered unclassified (P6-55).
Add recovery_retry_test.rb: the classification of each attempt through 4b's own buffer-and-classify primitives with the buffered response what every later line sees, the two terminal shapes (a returned response when the failure was never retryable, a raised throwable otherwise, with the trail attached to the constructed ProtocolError so RETRY-34's cause discrimination holds on this stack), the attempts cap counting the first send as attempt 1, the total-timeout applied as time remaining so no wait can overshoot and a hint past the budget suppresses the wait, the cancellation token at every boundary and inside the wait, the HTTP-tracer group produced once per operation with the Request, and the engine installed as Recovery::Orchestrator's transport: with the request chain's stamping applied once per exchange rather than once per attempt. Add budget_equivalence_test.rb, the sub-phase's convergence point: the three drivers built from ONE RetrySettings and driven against an identical failure sequence exhaust after the same number of wire sends, three under the shared defaults -- max-retries plus one is max-attempts, an identity and not a coincidence (P6-6).
Review round 0's two blocking findings and the test halves of its five should-fix findings, on the tests layer. The three driver suites built their default settings off the live configuration slot, so a host MAX_RETRY_ATTEMPTS changed their answers (=0 broke the recovery suite, =-1 all three). Each suite's Fixtures module now installs a FakeConfigSource env seam in setup and resets the slot in teardown, the shape retry_settings_test.rb and budget_equivalence_test.rb already had; the seven suites run identically with the variable set to 0 and to -1. error/retry_predicate_error.rb, a public lib file, had no test mirror; retry_predicate_error_test.rb pins its shape (StandardError, Dexpace:: Error, flat under Dexpace::, the predicate's raise as #cause). For the code fix on the branch below: policy_test.rb's PacingBoundsTest proves a 10 MB value in every form answers nil in under 50 ms on the parser alone, that a 400-digit run emits no Ruby warning (recorded with WarningCapture, since the suite's raiser and Policy#parse_form's fence had hidden it), the fifteen-digit boundary and the 64-byte ceiling; the '9' * 300 clamp case moves to the out-of-range set and the unexplained retry-after-ms exemption is gone. The at-the-cap jitter test asserts samples on BOTH sides of the cap, which the jitter-then-clip order cannot pass (the reviewer's surviving mutation m02). A tracer raising in attempt_failed is asserted to close the superseded response on both stage drivers. A should_retry answering true is asserted not to retry a downstream CancelledError on both stage drivers, a cancellation wrapped by a retryable error is terminal on all three, and Policy.cancellation? has its own cases. A negative configured MAX_RETRY_ATTEMPTS is asserted clamped and logged at RetrySettings.build, contained, with an explicit negative max_retries: still refused; the Policy method pin gains cancellation?.
Review round 2 of the phase-6a stack: the two guards behind round 1's findings, each seen red on 4.0.6 and 3.2.11. A tracer raising in retries_exhausted now closes the terminal error-status response on both stage drivers: the sync case is the one the fenced settle makes true (round 1's tree leaves the body open) and the async case pins Pump#finish's guarded block, which closed it all along. The async suite's tracer tests move into a TracerFencesTest of their own, because TerminalPathsTest and SharedShapeTest were both at the class-length ceiling with the new case. RETRY-31's "never a blocking sleep" clause was stated and not mechanised: a blocking Clock#sleep inserted beside Async.delay in the pump's wait left the whole async suite green, because every async case runs on a FakeClock whose #sleep records and returns and nothing read it. The inline, parked and no-scheduler cases now assert clock.sleeps empty, and a text scan refuses the token sleep in async_retry_step.rb, whose one wait is Async.delay's.
Wahbeh-Mohammad
added
type:feature
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #22. Second PR of phase 6a's stack — the tests for the code PR below it. Targets that PR's branch; the phase record is the next PR up.
What lands
18 files, +3,638 / −3: seven suites under
gems/dexpace-core/test/dexpace/resilience/mirroringlib/one file per file (pacing_parsers.rbandretry_step_helpers.rbareprivate_constants asserted throughPolicyand the two stage drivers),budget_equivalence_test.rb(theRETRY-14convergence test, nolib/mirror by design),error/retry_predicate_error_test.rb, additions to eight earlier suites (http_date_test.rb's twoRETRY-15cases, 4b'sprotocol_error_test.rbRetryableTest, 4c'scursor_test.rb/pipeline_test.rb/async_pipeline_test.rbbundle cases, 5b'sstep_test.rb/async_step_test.rbBundlePrecedenceTest), and three top-level doubles:ScriptedTransport—.new(script)consuming one entry per call (a callable invoked with the triple, anExceptionraised, anything else returned),#callsof frozen triples soassert_sameon the re-sent request holds; new rather than extending phase 2'sFakeTransport, which seven suites require by shape and is untouched.ScriptedAsyncTransport— the same over a freshCompleterper call, settling inline by default (the shape that overflowed the plan's recursive pump) or held pending undersettle_later:with#settle_next!.RetryFixtures—RetryableError/UnretryableError/RetryableFatal < ::NoMemoryError(theRETRY-25discriminator),RecordingCursor(forwards to the real driver-minted cursor, counts#forkand#call),retry_settingsover aFakeClockwith jitter 0.0. 5c'sDexpace::RecordingHTTPTracer, 5a'sFakeClock/ParkingScheduler/FakeConfigSource, 4b'sRecoveryFixturesconsumed unchanged.Every file's header names the IDs it exercises; every suite over 100 lines is split into nested classes; every thread is joined; nothing uses
assert_nothing_raised; every test that consults the process slot installs aFakeConfigSourceseam and resets it (R0-1 — the suites are identical underMAX_RETRY_ATTEMPTSunset / 0 / −1 / abc).What the suites prove rather than restate: the
RETRY-16negative battery (garbage answersnil, never raises, never warns under-w);RETRY-37's narrowing set; theRETRY-10at-the-cap test discriminating jitter-then-cap from cap-then-jitter (R0-7); the trampoline's depth flat across 2,001 attempts;clock.sleepsempty on every async wait plus a text scan refusingsleepin the async driver (R1-2,RETRY-31); the close of the superseded response recorded before the fake clock's sleep and before a raising tracer propagates (R0-4, R1-1); aCancelledErrornever retried even under a caller'sshould_retry(R0-5);RETRY-25's fatal passthrough on both paths; the fifteenRECOVrows one test each, the 503, 503, 200 sequence among them; theRETRY-14convergence with the sameRetrySettingsdriving both stacks.Verification
bundle exec rakeon Ruby 4.0.6 at this tip: exit 0, all seventeen gates green —test:gems2,285 runs / 65,255 assertions, line coverage 99.98%; honest RuboCop 421 files clean.Known follow-ups from the final review (not blocking)
async_retry_step_test.rb:142:Pump#retry_after's close-before-scheduling order is unpinned (mutation m49 survives equivalently); the sync side's order is pinned.