Repository navigation
fix: scope the rescue to the guarded call in run strategies - #836
Conversation
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. |
|
Thanks for the thorough fix here! I appreciate that you added tests to A few small things I'd like cleaned up before merging:
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.
e5028eb to
2f4b8c3
Compare
|
Thanks for the review! Addressed all three:
Each test still fails on the pre-fix code and passes with the fix; full suite, |
|
Thank you, great job! |
Fixes #766.
Problem
GreenRunStrategy#executeandYellowRunStrategy#executewrapped the strategy's own post-success bookkeeping inside the samebegin/rescueregion as the user's guardedcode.call. When a bookkeeping call raised on the success path, the exception was indistinguishable from a failure of the caller's code:error_tracking_policy.track?,record_error/record_recovery_probe_failure, corrupting the success/failure metrics that drive tripping and recovery, andConcretely:
begincoveredcode.calland the followingrecord_success.enter_recovery(before the call) andrecord_recovery_probe_success(after it), not justcode.call.Fix
Scope the
rescuetocode.callonly and run the success bookkeeping in anelseclause; for yellow, runenter_recoveryahead 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 existingensureinwith_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):
record_successmust surface the error, and must not consulttrack?, callrecord_failure, or invoke the fallback.record_recovery_probe_successand for a raisingenter_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=MemoryandSTOPLIGHT_DATA_STORE=Redis bundle exec cucumber(62 scenarios each),bundle exec standardrb, andbundle exec steep check— all green.