Repository navigation
Add opt-in retry for caller-declared transient failures - #7
Merged
Merged
Conversation
Runs were single-shot, so a transient blip (nav timeout, connection reset, 5xx mid-restart) became a failed report or a raised ScriptError — a false alarm. Add an opt-in retry where the caller declares which conditions are retryable. - retries: (default 0) — additional attempts; 0 preserves current behaviour - retry_on: — array of named conditions (:load_error, :script_error, :server_error, :client_error, :network_error) or a predicate proc; default [:load_error, :script_error, :server_error] - retry_backoff_ms: (default 250) — exponential backoff; slot released while waiting Symbol form is conservative: a run is retried only when every reason it failed is a declared condition. Console/assertion errors are never retryable. Classification lives in the pure RetryPolicy module. Bump to 0.5.0; ADR 0021; README/configuration/error-handling docs.
bigtiger
force-pushed
the
jr-opt-in-retries
branch
from
June 5, 2026 21:10
d3c2deb to
48421fe
Compare
retry_backoff_ms validated the base at 30_000ms, but the exponential delay base_ms * 2**(attempt-1) was unclamped. A valid config of retries: 10, retry_backoff_ms: 30_000 produced a single ~4.3h sleep (~8.5h cumulative), contradicting the documented 30_000ms cap. Clamp each computed wait to MAX_RETRY_BACKOFF_MS and clarify the docs that the per-wait — not just the base — is capped.
Review follow-ups on the retry feature: - run_with_retries captured ScriptError separately from the report path, duplicating the backoff call and the terminal-condition check. Capture a retryable ScriptError as the attempt's outcome so both paths share one decision (re-raise if the outcome is an exception, else return the report). - validate_retry_on! rejected a bare symbol even though RetryPolicy.retryable? wraps retry_on in Array(...), so :server_error and [:server_error] behave identically. Accept a bare condition symbol to match.
Mechanical, behaviour-preserving fixes from `rubocop -a` once the house style (double quotes) was configured: string-literal style, hash alignment, line length, indentation, semicolons, block delimiters, arguments forwarding. No logic changes; full suite still green (292 examples).
Wire RuboCop in as an enforced check so lint regressions fail CI: - .rubocop.yml: set TargetRubyVersion 3.2 and EnforcedStyle double_quotes for the string cops (matches the existing house style — eliminates ~1060 false 'offenses'); exclude specs from Metrics/BlockLength. Metrics numeric limits are inherited from the generated baseline; the curated client.rb exemptions are kept. - .rubocop_todo.yml: generated baseline grandfathering the offenses that remain after safe autocorrect (Metrics, a few Lint/Style), to be burned down over time. New code must not add to it. - ci.yml: add a RuboCop job (github format) alongside the unit suite. - README: document `bundle exec rubocop` and refresh example counts (292 / 310).
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
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.
Why
Users reported Perchfall flagging failures caused by "small timing errors" — a navigation that timed out by a hair, a connection reset, a server returning 5xx mid-restart. Runs were single-shot, so a one-off blip became a failed report (
report.ok? == false) or a raisedScriptError— a false alarm.What
Opt-in retry where the caller declares which conditions are retryable. Off by default —
retries: 0preserves today's single-shot behaviour exactly.Options
retries:0Nretries =N+1attempts). Capped at 10.retry_on:[:load_error, :script_error, :server_error]retry_backoff_ms:2500disables. Slot released while waiting.Retryable conditions
:load_errorstatus: "error"):script_errorErrors::ScriptError(process failed):server_error:client_error:network_errornet::ERR_*JavaScript/console (assertion) errors are never retryable — they're real defects, not timing blips, and aren't in the valid set.
Key safety property
The symbol form is conservative: a run is retried only when every reason it failed is a declared condition. A transient 5xx that also carries a console error is not retried — the assertion failure would never clear, so it would just fail again after burning the backoff budget.
ConcurrencyLimitError,InvocationError,ParseError, andArgumentErrorare never auto-retried.Implementation
Perchfall::RetryPolicymodule classifies an outcome's failure reasons and decides retryability — testable with plainReportobjects and exceptions, no browser/process needed.Clientgains the options, an injectedsleeper:(for fast deterministic tests), and arun_with_retriesloop. Backoff sleeps outside the concurrency slot; each:query_bustretry uses a fresh_pf=timestamp.run!benefits automatically — raisesPageLoadErroronly after retries are exhausted.Docs / metadata
Version →
0.5.0, CHANGELOG, new ADR 0021, README,docs/configuration.md,docs/error-handling.md.Non-goals
Report(keeps the value object + JSON schema untouched) — easy follow-up if a "passed after N retries" signal is wanted.Testing
All 288 unit specs green (
306withRUN_JS_SPECS=true); no browser or Node required. New coverage: reason classification, all-reasons-opted-in rule, proc form, ScriptError retry, never-retry of non-transient exceptions, exponential backoff timing, and validation.