Skip to content

fix: scope the rescue to the guarded call in run strategies - #836

Merged
bolshakov merged 1 commit into
bolshakov:developfrom
snowyukitty:fix/run-strategy-rescue-scope
Jul 22, 2026
Merged

bolshakov merged 1 commit into
bolshakov:developfrom
snowyukitty:fix/run-strategy-rescue-scope

Conversation

@snowyukitty

Copy link
Copy Markdown

Fixes #766.

Problem

GreenRunStrategy#execute and YellowRunStrategy#execute wrapped the strategy's own post-success bookkeeping inside the same begin/rescue region as the user's guarded code.call. When a bookkeeping call raised on the success path, the exception was indistinguishable from a failure of the caller's code:

  • it was passed to error_tracking_policy.track?,
  • recorded via record_error / record_recovery_probe_failure, corrupting the success/failure metrics that drive tripping and recovery, and
  • when a fallback was configured, the fallback was invoked with this synthetic error while the real, successful result was silently discarded.

Concretely:

  • Green — the implicit begin covered code.call and the following record_success.
  • Yellow — the recovery block covered enter_recovery (before the call) and record_recovery_probe_success (after it), not just code.call.

Fix

Scope the rescue to code.call only and run the success bookkeeping in an else clause; for yellow, run enter_recovery ahead of the guarded region as well. A bookkeeping/recovery failure on a successful call now surfaces as itself instead of being reclassified as a tracked run failure. The recovery lock is still released by the existing ensure in with_recovery_lock. No method signatures or RBS changed.

Tests

Added unit regressions that make a bookkeeping collaborator raise on the success path (as suggested in the issue):

  • green — a raising record_success must surface the error, and must not consult track?, call record_failure, or invoke the fallback.
  • yellow — the same for a raising record_recovery_probe_success and for a raising enter_recovery; both also assert the recovery lock is still released.

Each fails on the current code (the strategy swallows the error and returns the fallback value) and passes with the fix. The domain strategy files stay at 100% line coverage.

Validated with bundle exec rspec (712 examples, 0 failures), STOPLIGHT_DATA_STORE=Memory and STOPLIGHT_DATA_STORE=Redis bundle exec cucumber (62 scenarios each), bundle exec standardrb, and bundle exec steep check — all green.

@github-actions

github-actions Bot commented Jul 21, 2026 •

Copy link
Copy Markdown

Review price tag

🟢 68 effective lines — about 8–20 min (down from 9–22 min) of focused review (based on 200–500 lines/hour). This is within the range where reviewers find the most issues per line, and small changes usually receive feedback the fastest.

Why these numbers?

These minutes are what careful defect-finding costs at 200–500 lines/hour — the rate review studies report, not how long a skim takes. "Effective lines" already exclude generated files and lockfiles. Treat the rates and the 200/400 thresholds as guardrails, not laws.

@github-actions github-actions Bot added the proquo: small ProQuo review price tier: small label Jul 21, 2026
@bolshakov

Copy link
Copy Markdown
Owner

Thanks for the thorough fix here! I appreciate that you added tests to enter_recovery in yellow..

A few small things I'd like cleaned up before merging:

  1. Drop the "Regression for Rescue scope in GreenRunStrategy#execute and YellowRunStrategy#execute swallows the strategy's own post-success bookkeeping #766" comment blocks in both spec. They restate the issue history rather than an invariant
    the code owns. That kind of comments age now very well and rather liability.

  2. Fold the "still releases the recovery lock" tests into their sibling test in the two new yellow contexts, rather than separate it blocks - that matches how the rest of the file already handles it (e.g. the existing failure-branch tests assert lock release inline with the primary assertion). Two standalone examples for this is more than we need.

  3. The allow(request_tracker).to receive(:record_success) stub in the "when entering recovery raises" context is dead - enter_recovery raises before code.call (and therefore before record_recovery_probe_success) is ever reached, so that line can go.

None of this is a correctness concern -- the fix itself is right. Just want to tidy up the tests before we merge.

GreenRunStrategy#execute and YellowRunStrategy#execute wrapped the
strategy's own post-success bookkeeping inside the same begin/rescue
region as the user's guarded code.call. A bookkeeping failure on a
successful call was therefore indistinguishable from a failure of the
caller's code: it was passed to error_tracking_policy.track?, recorded
via record_error / record_recovery_probe_failure (corrupting the
success/failure metrics that drive tripping and recovery), and, when a
fallback was configured, the fallback was invoked with this synthetic
error while the real, successful result was silently discarded.

Scope the rescue to code.call only and run the success bookkeeping in
an else clause; for yellow, run enter_recovery ahead of the guarded
region as well. The recovery lock is still released by the existing
ensure in with_recovery_lock.

Fixes #766.
@snowyukitty
snowyukitty force-pushed the fix/run-strategy-rescue-scope branch from e5028eb to 2f4b8c3 Compare July 21, 2026 21:43
@snowyukitty

Copy link
Copy Markdown
Author

Thanks for the review! Addressed all three:

  1. Dropped the Regression for #766 comment blocks in both specs.
  2. Folded the lock-release assertions into the primary examples in the two new yellow contexts, matching how the existing failure-branch tests assert release inline.
  3. Removed the dead record_success stub from the "when entering recovery raises" context — enter_recovery raises before the probe is ever reached.

Each test still fails on the pre-fix code and passes with the fix; full suite, standardrb, and steep are green.

@bolshakov

Copy link
Copy Markdown
Owner

Thank you, great job!

@bolshakov
bolshakov merged commit 6e7b84d into bolshakov:develop Jul 22, 2026
34 checks passed
@bolshakov bolshakov mentioned this pull request Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

proquo: small ProQuo review price tier: small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rescue scope in GreenRunStrategy#execute and YellowRunStrategy#execute swallows the strategy's own post-success bookkeeping

2 participants