jit: rich-compare inline route re-runs an effectful dunder after an unlatched abort

[youknowone]

Summary

try_walker_inline_user_compareop discards an inline sub-walk result with a bare
Err(DispatchError::callee_inline_unsupported(..)). The error is not latched, so the walk
driver rolls back and replays the loop from entry — re-running a rich-compare dunder whose
body already applied its effects.

This is on main; it is not introduced by any open branch. It was found while adjudicating
the same defect in a sibling route (try_walker_inline_user_contains, PR #1405), which was
withdrawn for this reason.

Reproduction

class R:
    def __gt__(self, other): return True

class L:
    def __init__(self): self.n = 0
    def __lt__(self, other):
        self.n = self.n + 1        # observable effect
        return NotImplemented      # forces the route's Err path

l = L(); r = R()
N = 3000
hits = 0
i = 0
while i < N:
    if l < r:
        hits += 1
    i += 1
print("iterations", N, "body_runs", l.n, "hits", hits)

	body_runs
CPython	3000
pyre, JIT off	3000
pyre dynasm	3002

The answers stay correct — only the effect count is wrong.

The caller's loop shape matters. Under a for loop the FOR_ITER admission gate
(inline_call.rs, the CalleeReplaySafety::Dirty decline) rejects such bodies before the
route is reached, so a for-driven repro reads 3000 and looks like a refutation. Only
while reproduces.

Root cause

latch_abort_call_resume is the mechanism that turns a discarded sub-walk into a forward
resume point, and its own doc comment states the rule:

Every caller that discards a sub-walk result must go through this — returning the error
without latching leaves the driver replaying the loop, which repeats the callee's effects.

Two things make it unavailable here:

1. It delegates to reconstructed_all_ref_call_stack, which returns None unless
   pyre_helper is CallFn | CallFunctionEx. A rich-compare route arrives with a
   CompareOp descriptor, so even an explicit latch call is a no-op.
2. It refuses to latch when fbw_executed_effect_count() != executed_effects_before — i.e.
   precisely when the body already applied something, which is the case that needs it.

So the route has no sound way to abort after the body has run.

Why NotImplemented is the trigger

NotImplemented from __lt__ means "try the reflected operand", which is ordinary working
code, not a failure. A dunder that returns it on a hot path is normal, so the abort is
reachable in a loop rather than once.

Notes for whoever picks this up

• Gating admission on body_facts.exc_override_sample_safe was measured on the sibling
  route and admits only LoadConst/BoxInt bodies — self.a, x + 1 and
  len(self.items) all decline. It removes the route's value rather than fixing it.
• The disposition that is actually sound is to emit the object-level truth/reflected
  dispatch as a residual instead of aborting, so the already-run body's result is
  converted rather than discarded. No walker-side helper mints that descr today.
• MAJIT_STATS=1 is the cheap discriminator: the repro reads loops_aborted=2 with the
  route active.

— reported by Claude

[youknowone]

Correction to the root cause above, and the fix

The "Root cause" section is wrong about why the latch is unavailable. I wrote that
latch_abort_call_resume "refuses when the body already applied something — i.e. precisely
when it needs it". The refusal is real, but the reasoning is backwards: the latch resumes
at the call, so the interpreter re-executes it. Refusing once
fbw_executed_effect_count() has moved is therefore correct behaviour, not a gap. There
is no latch-based fix for an effectful body, and extending
reconstructed_all_ref_call_stack past CallFn | CallFunctionEx would not have helped.

The actual root cause is already documented in this repo, in the FOR_ITER admission
gate's CalleeReplaySafety::DeferredCall arm:

A BINARY_OP or COMPARE_OP entry is not [a call boundary]: the flush resumes one
operand short and the whole iteration's contribution is dropped, silently

That knowledge sits inside if fbw_foriter_inflight_active(), so it only applies under a
for loop. That is exactly why the repro needs while — outside a FOR_ITER the gate never
runs, and a Dirty body walks straight into a route whose abort cannot rewind.

It is a pair, not a single route

try_walker_inline_user_binop has the identical shape — same bare
Err(callee_inline_unsupported) on a sampled NotImplemented, same GuardFalse(PtrEq(..))
for the non-constant case. Confirmed by measurement with an effectful __add__ returning
NotImplemented:

	body_runs (N=3000)
CPython	3000
pyre dynasm, __lt__	3002
pyre dynasm, __add__	3002

Fix

Admission carries the decision: a route whose entry opcode is not a call boundary accepts
only a CalleeReplaySafety::Clean callee body. DeferredCall is not enough — its promise
rests on the same rewind the comment above says does not reach these entries.

After the gate, on both backends:

	before	after
__lt__ repro	3002	3000
__add__ repro	3002	3000
clean __lt__ still inlines	loops_compiled=1	loops_compiled=1

Clean excludes only bodies that commit a live-heap effect, so ordinary comparison dunders
(return self.x < other.x) keep inlining; the repro's self.n = self.n + 1 is what becomes
Dirty.

Measured blast radius on check.py --synthetic-only, gated binary vs the same suite on a
pre-gate binary: exactly two fixtures move. unspecialized_long_math_zip_paths goes back to
matching its baseline, and inline_freevar_after_mayforce moves loops_compiled 6 → 7 (an
improvement, re-recorded). The other 17 failing fixtures are identical before and after the
gate — pre-existing baseline drift against baselines last recorded at 0ff42415cef (#1266),
unrelated to this change.

Fix rides on #1405.

— commented by Claude