(MOT-4217) fix(harness): let reaction sessions unregister their run's subscriptions - #586
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skill-check — worker0 verified, 48 skipped (no docs/).
Four for four. Nicely done. |
f48b36a to
b613c30
Compare
|
Restacked onto |
|
Third commit: level-triggered join predecessors (+ E2E-009 Live rctest5 iteration exposed the registration-vs-completion race: a rejected divergent sibling (MOT-4210's check, working as designed) forced a re-registration round, the watched writer sessions finished inside that window, and the re-armed edge-triggered
Live verification: with this stack deployed (this PR + #583 + #585), the rctest5 orchestration prompt now completes fully green end to end — self-verifying pass report, totals == GROUP BY (5/5/5), 15/15 events, cited trigger-spawned reactor session, zero leaked subscriptions, autonomous cleanup. All 9 direct e2e scenarios pass. |
b685ce3 to
4e5284a
Compare
4e5284a to
07cfbb5
Compare
… subscriptions rctest5-K7mQ ended deadlocked: the repair reactor hit `subscription belongs to a different session` trying to clean up the run, because unregistration is owner-session-scoped and the owner — the orchestrator — was parked waiting on a report notification its children could no longer satisfy. Three armed subscriptions leaked. Reaction spawns now record child → registrant lineage in the ephemeral subscription registry, and the unregister ownership check accepts any session whose lineage chain reaches the owner (transitively, hop-bounded against re-targeted-session cycles). Lineage entries are purged on session::deleted along with the session's subscriptions.
…e captures; armed-wake advisory Three pieces from the rctest5 live-run iteration: * Router serve-time captures: a generation can capture a runtime value (regex over its matched request) and later frames echo it via [[cap:name]] — the only way a static fixture can call engine::unregister_trigger with a runtime-generated sub_… id. Validation requires declared-before-referenced and a capture group. * E2E-008 reaction-unregisters-run: a STANDING binding's reaction, pinned to a separate session, unregisters the registrant's subscription. The gate is gen4's matcher demanding the unregister function_result with is_error:false AND removed:true (once:false is load-bearing — a one-shot binding retires itself first and the check never runs). The await phase now also drains the script after the awaited target call, so collection can't race an untracked session's turn tail. Gate check: reverting the ownership check to pre-lineage semantics times the scenario out. * armed_wake_advisory: a one-shot state-key wake registration now warns that nothing fires it automatically and the session sleeps forever unless a registered task explicitly sets that scope/key — the exact wiring gap that left the rctest5 orchestrator parked with its cleanup pending after every row landed correctly.
rctest5 attempt 4 exposed the registration-vs-completion race: a rejected sibling (the MOT-4210 divergent-spec check, working as designed) forced a re-registration round, the watched writer sessions finished inside that window, and the re-armed edge-triggered turn-completed join predecessors starved forever — finalizer never spawned, report never written, orchestrator parked. A completion BARRIER is only correct level-triggered: registering a join predecessor on a turn-completed binding whose filtered session is ALREADY terminally completed (durable status completed, no armed wake) now delivers a catch-up fire shaped like the real completion event, stamped __late_subscription_replay. Joins only — their per-key accumulator makes a rare double-delivery idempotent. Fail-open: a status lookup failure keeps edge semantics. The registration response carries a note naming the replay. E2E-009 late-join-predecessor-replay gates it end to end: a worker session completes (proved past via a call-mode completion witness — recorder call #1), then a probe-steered turn registers the late join; the joined downstream's recorder call #2 exists only through the replay. Gate check: disabling the replay times the scenario out. Runner: ProbeAction gains after_target_calls (an untracked session's only observable milestone is a controlled-function call). Verified live: rctest5 attempt 5 completed fully green — report row self-verifying pass, totals == GROUP BY (5/5/5), 15/15 events, cited trigger-spawned reactor, zero leaked subscriptions, no polling.
…alls Same authoring-mistake class CodeRabbit flagged on the await knob: a zero after_target_calls fails at runtime and a positive one without a controlled function waits until the deadline burns. Reject both in ScenarioFixture::validate.
07cfbb5 to
e01db52
Compare
1 participant
Why
rctest5-K7mQ ended in a cleanup deadlock. The repair reactor tried to tear down the run's subscriptions and hit
subscription belongs to a different session×3:engine::unregister_triggeris owner-session-scoped, and the owner — the orchestrator — was parked waiting on areportstate notification that its own children could no longer satisfy. Result: 3 leaked armed subscriptions and a permanently sleeping session.A reaction cleaning up its own run is the legitimate teardown path when the registrant is parked.
What
spawn_reactionrecords child session → registrant session lineage (from the trusted__owner_session_idstamp) in the ephemeralSubscriptionRegistry.session::deletedin both directions, keeping the map bounded by live sessions.Deliberately narrow: strangers and the reverse direction (registrant "cleaning up" a child's subs it doesn't own) are still refused.
Tests
forget_lineagepurges entries pointing at and from the deleted session267 harness tests pass; clippy clean.
Closes MOT-4217.