Skip to content

Bind the handler-dispatch handle to its dispatch window, add GC traversal, fail fast on observer errors, and reclaim arena slots (#492, #497, #500, #506)#511

Merged
proboscis merged 4 commits into
mainfrom
fix-vm-core-audit
Jul 10, 2026
Merged

Conversation

@proboscis

Copy link
Copy Markdown
Owner

Closes #492, #497, #500, #506.

Part of the 2026-07-10 execution-substrate audit (#507 is the index). Independent of the other audit PRs (Rust VM crates + their tests; verified by integration merge). Rust changed → reviewers running locally need make sync.

What changed

  • vm-core: stale pending_handler_k_handle delivers an unrelated exception into an abandoned continuation and substitutes its return value #492 — stale pending_handler_k_handle eliminated structurally (a565673f). The VM-global slot is removed entirely; the handler-dispatch PyK handle is a local in eval_perform/eval_perform_with_k, threaded through the new eval_expand into an EvalReturnContinuation::ExpandReturn frame variant and consumed on both exits — value path (Program frame) and raise path (step_raise pops ExpandReturn and discontinues the perform-site chain via recover_from_k_handle). A wrong-arity @do handler's late TypeError can no longer deliver an unrelated exception into an abandoned continuation or substitute another dispatch's result. Uses the VM's own frame system (per the repo Change Protocol — no bolted-on external mechanism); the invariant checker scans ExpandReturn frames in live and detached fibers.
  • vm: Py-holding pyclasses lack __traverse__/__clear__ — reference cycles through doeff_vm objects are permanently uncollectable #500 — PyO3 GC protocol (3eebe836). __traverse__ on all Py-holding pyclasses (15 DoExpr classes, PythonCallable, PyIRStream, Ok/Err), __clear__ on the non-frozen ones; EffectBase's pyo3 dict slot removed so subclass __dict__ is CPython-managed (traversable). Cycles through VM objects are now collectable. PyK is deliberately excluded with a documented rationale (sole-owner chain + mid-dispatch recovery would break under tp_clear).
  • vm-core: WithObserve observer exceptions are silently swallowed #506 — observer exceptions fail fast (eda61eb2). call_all_observers returns a Result; the first failing observer aborts the walk and the exception is raised at the perform site via continue_raise (catchable like a handler error); the effect is not delivered. The DoCtrl::WithObserve contract doc now states this.
  • vm-core: ~0.5KB native memory leaked per abandoned continuation; arena slots stranded within a run (segment-leak family) #497 — arena slot stranding fixed; the "native leak" was this (c4fefe61). SlotReclaimQueue armed on detach_chain; DetachedFiberChain::Drop reports bare slot indices (fiber ownership rules preserved — pinned by a new arch test); alloc() drains the queue; clear() swaps it so late drops can't poison the next session. Measured: 20k-abort loops went from +9.5–10MB/run unbounded to a plateau at +0KB/run (head fiber index 2 vs 400 pre-fix).

Adversarial review (no blocking findings)

  • All four commits survived refutation. Test-theater check: the branch's new regression tests were run against the merge-base build directly — 8 red pre-fix, green post-fix.
  • vm-core: stale pending_handler_k_handle delivers an unrelated exception into an abandoned continuation and substitutes its return value #492 hardened with 7 adversarial variants (Pass-path wrong arity, consecutive bad dispatches, non-contamination of subsequent dispatches, pre-consumption handler raise, raise-after-Resume, victim catching the TypeError with a correct result, sibling-task error propagation under Spawn/Gather) — all pass.
  • Two informational notes: (a) DetachedFiberChain::append keeps self's reclaim queue and drops other's — only reachable via already-unsupported cross-VM continuation sharing; a debug assertion (Arc::ptr_eq) is suggested as cheap hardening. (b) A pre-existing residual class (measured identical at the merge base): Race-loser cancellation leaves per-Race segments/continuations/IR-streams uncollectable through PyK interiors — the documented PyK exclusion; recommend a separate issue for long-lived processes with heavy Race/timeout use.

Semantic changes

  • Observer exceptions now abort the dispatch and propagate (previously silently swallowed) — intended fail-fast, but a visible behavior change for any consumer with a raising observer.
  • Direct EffectBase() instances (not subclasses) no longer accept attribute assignment / pickle (base dict slot removed); zero in-tree or downstream (proboscis-ema) usage.
  • Residual GC gap documented: cycles routed exclusively through a DoExpr instance __dict__ or through PyK interiors remain uncollectable (pyo3 limitation / deliberate exclusion).

Verification

🤖 Generated with Claude Code

proboscis and others added 4 commits July 10, 2026 19:02
…492)

A wrong-arity @Do handler defers its failure: the @Do wrapper returns
Expand(Apply(Pure(thunk), [])) at call_handler time, and the arity
TypeError only fires later inside eval_apply — outside the dispatch's
synchronous recovery arm. The VM-global pending_handler_k_handle slot,
restored for Expand outcomes, was never cleared on that raise path, so
the stale handle was adopted by the NEXT Expand's Program frame: a later
unrelated uncaught exception was thrown INTO the long-abandoned
perform-site continuation and that continuation's return value was
silently substituted as the unrelated program's result (the 2026-07-07
exit-0 incident class, inside the VM itself).

Fix: remove the VM-global slot entirely and make the handle a
dispatch-local. eval_perform / eval_perform_with_k keep the Py<PyK>
handle in a local; for Expand outcomes it is threaded through the new
eval_expand helper into the EvalReturnContinuation::ExpandReturn frame
(struct variant), whose two exits both consume it:

- value path: step_eval_return moves it onto the resulting Program
  frame (unchanged recovery semantics for handler-body errors);
- raise path: step_raise, on popping an ExpandReturn frame that still
  owns a handle, recovers the perform-site chain and discontinues it
  with the exception (OCaml 5: discontinue k exn) — the same routing as
  the synchronous recovery arm for non-deferred handler failures. If
  the handler already consumed k, the PyK is empty and the exception
  propagates to the outer scope.

The handle is deliberately NOT carried inside DoCtrl::Expand: do_ctrl.rs
documents the instruction set as language-agnostic ("no Python types
here"), and the Expand node is user-visible AST constructed all over the
Python bridge; the ExpandReturn frame is the VM's own in-flight state
for exactly that Expand evaluation, so the Py<PyK> rides there. As a
side effect the dispatch is now re-entrancy-safe (no global slot to
clobber) and non-Expand outcomes drop the handle immediately, preserving
the stale-backup-leak fix.

Invariant checker: the VM-slot root scan is replaced by scanning
ExpandReturn frames (live arena fibers and detached chains alike) for
owned chains; checker tests root their PyK in an ExpandReturn frame.

Regression tests:
- Rust: vm_tests test 14 mirrors #492 at the DoCtrl level (deferred
  Expand(Apply) raising; a later unrelated error must propagate, not be
  swallowed by the abandoned victim with a substituted Int(666)); the
  existing stale-backup test now asserts no frame anywhere carries a
  handle. Verified red pre-fix / green post-fix.
- Python: tests/test_handler_exception_catchable.py ports the issue
  repro — after catching the arity TypeError, an unrelated RuntimeError
  must escape run() and the victim must not catch it. Verified red on
  the pre-fix build (DID NOT RAISE) and green post-fix.

Fixes #492.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ugh VM objects are collectable (#500)

No doeff_vm pyclass participated in CPython's cycle GC, so any reference
cycle through a program node, effect, callable wrapper, stream, or result
was permanently uncollectable — every accidental cycle pinned its whole
reachable subgraph for the life of the process.

Mechanism:

- __traverse__ visiting every Py<PyAny>/Py<PyK> field is added to Pure,
  Perform, Resume, Transfer, Apply, Expand, Pass, WithHandler,
  ResumeThrow, TransferThrow, WithObserve, GetTraceback, GetHandlers,
  GetBoundaries, TailEval (do_expr.rs), Callable/PythonCallable and
  IRStream/PyIRStream (python_generator_stream.rs), and Ok/Err
  (result.rs). Defining __traverse__ sets Py_TPFLAGS_HAVE_GC, so the
  collector can now see through these objects.
- __clear__ is implemented only on the non-frozen classes
  (PythonCallable, PyIRStream), replacing the held reference with None.
  The DoExpr and Ok/Err classes are `frozen` (no &mut self in pyo3), so
  their references cannot be dropped in-place; traverse-only is sound
  because field cycles cannot be constructed among frozen nodes alone —
  every real cycle routes through at least one mutable Python object
  whose tp_clear breaks it once traversal has made the cycle visible.
- EffectBase drops its pyo3 `dict` slot. pyo3 0.28 cannot visit a
  pyclass dict slot from __traverse__ (the method receives only &self),
  so a base-owned __dict__ was invisible to the GC. Without the base
  slot, Python subclasses (every real effect) get a CPython-managed
  __dict__ that subtype_traverse/subtype_clear handle natively, making
  cycles through effect attributes collectable. Direct EffectBase()
  instances no longer accept attribute assignment; nothing in the repo
  does that (effects are always subclasses).
- PyK is deliberately EXCLUDED (documented in continuation.rs): a live
  PyK is the sole owner of a detached fiber chain (SPEC-VM-021), and a
  GC-driven __clear__ could drop the chain while a dispatch-owned
  Py<PyK> handle may still recover it for exception propagation (#492),
  breaking the one-shot ownership invariant mid-dispatch. A
  traverse-only impl is not currently implementable either: the chain's
  Python refs live behind PyShared in fibers/frames/dyn IRStream values
  with no GC-visit API. PyK stays an opaque GC leaf.

Known residual limitation (documented in do_expr.rs): the pyo3 `dict`
slot kept on DoExpr classes for defp metadata (__doeff_body__ etc.) is
still not traversable in pyo3 0.28, so a cycle routed exclusively
through a program node's instance __dict__ remains invisible.

Regression tests (TestGcCycleCollection in test_pyvm.py) port the issue
repro: cycles through Pure(o), through EffectBase-subclass attributes,
between two EffectBase instances, and through WithHandler/Apply must be
collected (weakref dies); pure-Python control included. Verified red on
the unfixed build, green after.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…506)

call_all_observers discarded observer errors
(`let _ = observer.call(...)`), so an observer that raised was silently
dropped: the effect proceeded and the exception never surfaced. Observers
are exactly where tracing/slog/OTel layers hang — a broken audit layer in
a long-lived process died silently and stayed dead, against the fail-fast
doctrine downstream repos rely on.

Mechanism: call_all_observers now returns Result<(), VMError>; the first
failing observer aborts the walk. eval_perform propagates the error
before the handler is looked up — an UncaughtException is raised at the
perform site via continue_raise (identical to a synchronous handler
exception, so try/except around the yield sees it and an uncaught one
propagates out of run()); other VMErrors go through error_result. The
effect is NOT delivered to any handler. The DoCtrl::WithObserve doc
comment now states the fail-fast contract explicitly (it previously
promised only "return value ignored").

Existing tests relying on lenient observers: none found — a repo-wide
survey of observer tests (tests/test_intercept.py,
tests/test_with_observe_visibility.py,
tests/test_spawn_observer_inheritance.py, packages/doeff-vm*/tests) shows
no test raises from an observer, and the full suite (964 passed) is
unchanged, so no deliberate updates were needed.

Regression tests port the issue repro: a raising observer must make
run() raise that exception instead of returning the handled value, and
the exception must be catchable at the perform site. Verified red on the
unfixed build, green after.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…497)

Dropping a continuation without resuming it (abort-style handler,
scheduler cancellation, abandoned parked K) left its arena slots
vacant-reserved forever within the run: detach_chain empties the slots,
attach_chain is the only path that refills them, and free() is never
called for dropped chains. Every abandoned dispatch stranded ~2 slots,
growing the fiber slot vector unboundedly (measured: head fiber index
40000 after 20k aborts) and dragging ~528 B/abort of RSS that persisted
across runs via heap fragmentation of the regrown Vec<Option<Fiber>>.

Mechanism: the arena owns a SlotReclaimQueue (Arc<Mutex<Vec<usize>>>);
detach_chain arms each DetachedFiberChain with a handle to it. The
chain's new Drop impl reports the slot indices it still owns — its
fibers are destroyed normally by the same Drop; only bare indices flow
back, never fibers or continuations, so SPEC-VM-021 single-owner
semantics and the G1 fiber-ownership guards are preserved (this is the
arena analogue of OCaml 5's "k dropped => fibers freed by GC").
into_fibers/append drain the fiber vec first and therefore report
nothing. alloc() drains the queue into the free list before allocating;
clear() swaps in a fresh queue so chains outliving a run session cannot
poison the next session's free list. Under invariant-checks, reclaim
panics if a reported slot is not vacant-reserved.

Measured after the fix: abort-loop head fiber index stays at 2 (was
2*N), and RSS plateaus at 0 KB/run growth by run 5 (was +9.5-10 MB per
20k-abort run, no plateau) — the "native residue" from the issue was
this stranding plus its allocator fragmentation, not a separate PyO3
leak. Control (Transfer-resume) loop unchanged at 0 growth.

Regression coverage: arena unit tests for drop-reclaim, bounded
slot_count over 100 abandon cycles, stale-drop-after-clear isolation,
and no-reclaim-on-attach; a Python abort-loop test asserting bounded
head fiber indices via k.to_dict() (red: max index 400 for 200 aborts;
green: 2); a new G1 guard pinning SlotReclaimQueue to bare usize
indices.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vm-core: stale pending_handler_k_handle delivers an unrelated exception into an abandoned continuation and substitutes its return value

1 participant