Repository navigation
feat: exit signals, startup and runtime-drop semantics as in OTP (ranks 5e, 5f, 5g) - #240
unbalancedparentheses wants to merge 242 commits into
Conversation
…trait migration (WIP: tests not yet migrated)
…h, pipe_reply* and JobCtx emits (WIP)
…ith on owned native threads
…ith on owned native threads
…mits, delegation, bounds) and registry unit tests
…ocks, diverging bodies)
…Reply skips its drop The CI hot-path gate (benchmarks/hotpath.py, callgrind instructions per operation against main) measured async_ct_send_p1/p2 at +10.9%/+11.1%. Per operation, the plain-send path paid for request machinery: a `MessageEnvelope` with a detached `SlotHandle`, a `lent` field and their drops; `PendingReply::reply` on the detached obligation (the macro's send handler answers `()`), out of line, followed by the obligation's `Drop`. - `send` builds a `SendEnvelope` (`enqueue_send`): just the message; its dispatch builds a detached `PendingReply` and lends no caller. Requests keep `MessageEnvelope`. - `PendingReply::reply` is `#[inline]` and consumes the obligation through `ManuallyDrop`: once its slot is taken there is nothing left for `Drop` (fields are a reference and a marker). `Drop` itself is `#[inline]`. Semantics unchanged: a sent message still runs inside a turn (the turn phase classifies ReplyTo drops in it), and its `PendingReply` is detached as before. Gate (base 0791ace = main, Ir/op): async_ct_send_p1 855 -> 948 (+10.9%) now 841 (-1.6%) async_ct_send_p2 849 -> 944 (+11.1%) now 837 (-1.5%) async_ct_burst 3756 -> 4161 now 4119 (+9.7%) async_ct_request_seq 3479 -> 3709 now 3667 (+5.4%) async_ct_request_conc16 2589 -> 2923 now 2881 (+11.3%) threads_request_seq 3255 -> 2326 now 2269 (-30.3%) fmt, clippy (3 configurations), workspace tests, failpoints + death_boundary, sim 320/320, Loom reply_slot 25 / job_core 8 / link_fence 10 / registry_turn 3, traces 93/93 accepted with 130 negative-control counterexamples, model-sync: pass. No model change.
…nd a leaner in-turn reply Request-path trims measured with the CI hot-path gate (benchmarks/hotpath.py gate, callgrind Ir/op against main 0791ace): - `reply::clock` is the one place deadlines read time (`now`, `after`): today the precise `spawned_rt` clock, so a cheaper source can replace it without touching the reply protocol (that change awaits a decision and is not part of this commit). `request`/`request_with_timeout` fix their deadline as `clock::after(timeout)` in ticks (no `Instant::checked_add`, no `Deadline` round trip); `send_request_with` and `reply_slot` take ticks. The public `Deadline` API is unchanged. - `PendingReply`'s in-turn reply (`Slot::reply_in_turn`) takes a fast path: a slot answered in its own turn was never deferred (`defer()` consumes the obligation), so it holds no credit, job or side state: one CAS under the slot lock, then the receiver's waker. Any other word (closed first, or unexpectedly deferred) takes the general `publish`. - The first timer's millisecond count is computed in 64-bit arithmetic (no 128-bit division); `ReplyReceiver` and `SlotHandle` drops are inlined. Gate (base 0791ace, Ir/op; before = 155682f): async_ct_burst 4119 -> 4102 (+9.2%) async_ct_request_seq 3667 -> 3612 (+3.8%) async_ct_request_conc16 2881 -> 2826 (+9.2%) async_ct_send_p1/p2 unchanged (841/837, -1.6%/-1.5%) No semantics change: the deadline is the same instant (send time plus the timeout, in ticks), and the in-turn fast path is the same linearization (`OPEN -> REPLIED` under the slot lock) as `publish`. fmt, clippy (3 configurations), workspace tests, failpoints + death_boundary, sim 320/320, Loom reply_slot 25 / job_core 8 / link_fence 10 / registry_turn 3, traces 93/93 accepted with 130 negative-control counterexamples, model-sync: pass.
Keep the exact runtime deadline captured before enqueue and reuse its timer. Pack receiver bookkeeping into the slot and share the envelope slot across dispatch; avoid a redundant request future wrapper. Include production-slot Loom races, paused-clock/overflow tests and the ReplyTimer model with two negative controls. All six callgrind benchmarks pass the unchanged 3% gate against 33d4016 (three runs; worst +2.2%).
|
/claude Fresh release review requested for the final 0.8 candidate. Please cover the new changes since e8847a3 through ce4ea4d, particularly precise shared request timers, packed receiver bookkeeping and deadline-shortening races, envelope slot ownership/discharge, native-only feature builds, and the accompanying Loom/TLA+ updates. Earlier full lifecycle/deferred-reply reviews are recorded in this PR's history. Keep the performance threshold unchanged at 3%; review the semantics independently of green CI. If the diff supplied by the workflow is truncated, explicitly report that limitation rather than claiming the entire PR was reviewed. |
🤖 Claude Code Review
Review: PR #240 (exit signals, death point D, deferred replies, jobs)The diff is truncated partway through Overall the design is careful. The death point is a single boundary, notices are fenced on the receiver side by edge generation, no user code runs under the job and table locks, and payloads are held until every caller is answered. The Loom, TLA+ and trace coverage is strong. The items below are things I would fix or verify before merging. Correctness and concurrency1. { let mut inner = lock(&self.inner); if !inner.open() {..} if inner.scheduled > 0 {..held..} }
match self.notify(enqueue) { .. } // re-locksThe comment says "only the work schedules". If a
Fix options:
2.
This contradicts the
Please verify that nothing reachable from 3. Every 4. This is correct only if mailbox admission closes strictly after D on every path. The CHANGELOG promises that sends made while exiting are admitted. Please confirm that neither run loop closes or drains the mailbox before Also note the public-API side effect: any user converting a 5.
6.
Behaviour and API7. It logs
8.
9. The root re-exports 10.
Benches
Process
Automated review by Claude · claude-opus-5-5 · custom prompt |
Reproduce retained JobCtx admission during held-result release, join wakers under their own lock, and paused-clock premature slot expiry. Close emit admission atomically, latch work completion, wake blocked emitters outside locks, deduplicate join waiters, and keep runtime-relative waits separate from service-clock deadlines. Include three production-core Loom regressions, the EmitRace TLC configuration and negative control, and timer-lag refinement. Focused Codex review reports no actionable regressions. Corrected three-run instruction gate passes all six cases against 33d4016 (worst +2.8%); full CI remains required.
Reuse the message Option as the private dispatch marker, avoiding its duplicate boolean. The message is taken at the same dispatch boundary; refusal and death-point discharge are unchanged. Existing panic/discharge traces and focused regressions pass; independent Codex review found no actionable regressions. Three-run server gate passes every benchmark against 33d4016; maximum async growth +2.1%. Keep the 3% threshold, broaden gate scope to dependency/toolchain changes and release metadata, and avoid a pipefail/SIGPIPE false skip on large path lists.
|
/claude Please independently review the two new performance commits after 7f609bf (reply/turn bookkeeping and await-free generated dispatch). Keep this PR draft; no merge or release yet. Focus on the zero-external-token turn transition and last-token retirement, reply/defer races, macro source compatibility and name hygiene, parameter/destructor ordering, lazy inherent/Handler calls, and preservation of panic/death fences and causal actor context. Direct dispatch is restricted to messages with no drop glue; shared/explicit-lifetime receivers, suspending/deferred handlers and opaque macros/attributes keep their original adapter. Precise clock reads and the +3% instruction gate are unchanged. Local review found shared-receiver, ignored-parameter and shadowed Option regressions; these are fixed with regressions. The focused tests/Clippy, all 45 reply/turn/job Loom tests, 270 simulated tests and all 94 recorded traces pass. Negative controls/coverage and the final versioned quiet performance campaign are still running or pending. Please report any diff/context truncation explicitly rather than claiming a full review if the workflow cannot supply the whole change. |
🤖 Claude Code Review
Review: PR #240 (exit signals, death point, deferred replies and jobs)OverallThis is a very large PR. It mixes OTP exit-signal semantics, the death-point refactor, deferred replies and jobs, the deadline service, CI changes and a rewrite of every benchmark. It cannot be reviewed thoroughly as one unit. Consider splitting it into at least these PRs:
The core design is careful:
Specific findings follow. Correctness / concurrency1.
|
| Benchmark | New deadline | Old behaviour |
|---|---|---|
runtime_isolation.rs |
DEFAULT_REQUEST_TIMEOUT (5 s) |
request_raw, no deadline |
workload_research.rs |
DEFAULT_REQUEST_TIMEOUT (5 s) |
request_raw, no deadline |
designs.rs |
Duration::from_secs(10) |
request_raw, no deadline |
This has two effects:
- It adds per-request timer and deadline bookkeeping to the measured path, which skews any comparison with 0.7.
- Under overload experiments, it turns slow replies into
unwrap()panics.
Use Deadline::Never in these benchmarks for consistency.
10. baseline.rs threads reply can now hang forever
Self::Threads(rx) => rx.recv().expect("reply timeout/disconnected"),The old code used recv_timeout(10s). With Deadline::Never, a lost reply hangs the benchmark until the CI timeout. Keep a wall-clock guard here, or use a deadline only on this wait.
11. benchmarks/hotpath.py timeouts produce a raw traceback
timeout=180 and timeout=120 raise an uncaught subprocess.TimeoutExpired. Catch it and call sys.exit(f"... timed out") so the gate output stays readable. Also confirm that the sh() helper forwards the timeout keyword.
Smaller items
actor_cleanup.rsdischarged():log_stopped_skipped(self.id, "", "runtime_drop", ...)logs an emptyactor_type. Threadtype_name::<A>()through, or omit the field.reply/job_core.rsfinish: on theRefusedand early-return paths,laterandenqueueare dropped insideJobState::finish'strace::atomicsection. This only happens with thelifecycle-tracefeature, but it runs user payload destructors under the global trace mutex, which contradicts the module's stated rule.reply/deadline.rsDeadlineHeap::push→compact: compaction callspending_deadline()while the heap lock is held. That likely takes the slot lock, giving the order heap → slot. Make sure no path holds a slot lock while pushing.- Benchmarks: hand-written handlers now contain macro-expansion artifacts such as
let __reply_value = { ... }and an empty{ ... }; reply.reply(()). They read as pasted macro output. Simplify them, or use#[actor]/#[send_handler]where possible. verification.yml:MCLifecycleDeepandMCLifecycleLiveDeepare removed from scheduled CI and become manual pre-release checks. Make suredocs/TESTING.md's release checklist is actually enforced. Therelease.yamlcomment alone will not stop someone skipping it.- CHANGELOG:
- It still lists several "in progress" breaking entries ("Remaining: the heavy TLC configurations…"). Reconcile these before release.
- There is a stray blank line after the timer entry.
link_regression_tests.rslate_link_failure_relabels_...: this behaviour change is significant for users. A linked survivor of a cleanly stopping actor now cascades if a third actor crashes duringstopped(). Call it out prominently inMIGRATION.md, not only in the changelog.
Looks good
- Exit request model:
ExitRequestis a clean single place for reason precedence: kill > untrapped > request, with the hook-skip rules. The unit testreason_precedence_until_deathis clear. - Death-point ordering: in
ActorStartup::drop, the order D → discharge → wake-ups → per-item contained drops → payload → publish is correct. It also avoids double-unwind aborts by dropping each item individually. DropFailures::hold/release: this removes a whole class of "user destructor runs before the caller is answered" bugs.JobSetCore: admission and listing happen in one critical section withclosed, so no admitted job can escapecancel_all.- Error classification:
ActorErrorclassification helpers, plus an exhaustive in-crate test table for every variant, are a good pattern for#[non_exhaustive]enums.
Automated review by Claude · claude-opus-5-5 · custom prompt
Implements the accepted OTP exit-signal design (ROADMAP 5e/5f/5g), including the deferred-reply and pipe-job work from #239. This is a breaking 0.8 candidate; keep it draft until the final release gates pass.
The changes define signal versus administrative stop, trappable linked kills, startup rollback, runtime-drop death, late links/monitors and
NoProc, and reply discharge at the death point. The caller's expiry decides request timeouts, as documented insemantics.mdand the migration guide. No unsafe code was added to the core crates.Performance and review fixes
Plain sends carry no reply slot. Requests lend one envelope slot across a turn, allocate deferral state only when needed, and use the message's presence as the dispatch marker. Request construction is inlined. No coarse clock is used: the monotonic deadline and runtime-relative timer are captured before enqueue, preserving paused Tokio clock semantics.
The latest implementation passes the required GitHub perf-gate at
5e323ba3. An independent three-repetition callgrind run on fede (Rust 1.93.0, the same #270 driver, sizes 2,000/6,000, main33d40160) gives:The threshold remains +3%; it now also applies to dependency, toolchain and version changes. The final versioned candidate is being measured separately for release.
Independent Claude and focused Codex reviews found and reproduced two correctness issues: held job completion could reopen emit admission, and sharing an absolute Tokio timer deadline with the monotonic deadline broke paused-clock requests. Both are fixed, with reproducing regressions. Job join waiters are deduplicated and awakened outside the lock. Focused follow-up reviews found no actionable regressions in these fixes or the compact envelope.
Verification
ReplyTimer.tlamodel checks 725 bounded states, including delayed timer arms; both lost-update controls fail as expected. Paused Tokio clock behavior is covered by Rust regressions, rather than claimed as a property of this abstract model.5e323ba3passes; model-sync, simulator, sanitizers, API compatibility and native builds pass.7f609bf9adds a bounded startup acknowledgement before denying new threads; its reproduction passes on one CPU. Fresh full CI and both client ports are running.Release checks
A separate commit retains bounded weekly Lifecycle checks and moves the full
MCLifecycleDeepandMCLifecycleLiveDeepchecks to manual pre-release gates, alongside the soak. Their models, bounds and properties are unchanged. The full safety sizing run completed in 8h02m; deep liveness exhausted memory and remains incomplete. CI success does not certify those manual gates.Before release: final-candidate CI and performance gate, quiet performance comparison, release-commit soak/model checks and client validation. Earlier client/soak results predate the final optimizations. Versioning and curated release notes are kept separately in draft #271. No tag or publication has occurred.