Skip to content

feat(arty): add thread-aware runtime - #785

Open
martintmk wants to merge 130 commits into
mainfrom
martintmk-arty-runtime-migration
Open

martintmk wants to merge 130 commits into
mainfrom
martintmk-arty-runtime-migration

Conversation

@martintmk

@martintmk martintmk commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Summary

Port the scheduling runtime into Arty as a thread-aware runtime where each async task remains on one worker for its lifetime. This is the arty 0.4.0 API line: the default feature set enables rt and macros, while --no-default-features remains available for a feature-minimal facade.

Arty provides worker-bound task scheduling, clocks, telemetry, and blocking-task pools. SDK I/O drivers, memory-pool services, task metadata, and consumers remain outside this change.

#[arty::main]
async fn main(cx: arty::task::Builtins) -> Result<(), arty::task::JoinError> {
    let answer = cx.scheduler().spawn(async |_| 42).await?;
    println!("{answer}");
    Ok(())
}

Public runtime API

  • Export Runtime, RuntimeBuilder, WorkersPolicy, BlockingPoolPolicy, RuntimeOperations, and Error under arty::runtime.
  • Configure explicit runtimes with .workers(...), .stack_size(...), .blocking_pool(...), .clock(...), and .sink(...).
  • WorkersPolicy supports at_most, exactly, and all. WorkersPolicy::default() is the automatic-selection policy; there is no separate auto() constructor.
  • Blocking pools are shared by default. Use BlockingPoolPolicy::shared().max(n) for one capped runtime-wide pool or BlockingPoolPolicy::per_worker().max(n) for a capped pool per async worker. max takes usize directly; omitting it uses the runtime default.
  • Zero worker counts and zero blocking-pool limits are retained by their policies and rejected by RuntimeBuilder::build, so a later setter can replace an invalid provisional value.
  • Runtime::scheduler() returns the non-cloneable runtime-wide RuntimeScheduler. Worker-bound Scheduler remains cloneable and preserves worker affinity.
  • RuntimeOperations only requests shutdown; the public processor-pinning operation has been removed.

Scheduling and joins

  • Export Builtins, Scheduler, RuntimeScheduler, JoinHandle, and JoinError under arty::task.
  • Scheduler::spawn creates the future on its worker, allowing non-Send state created inside the future while factory captures and results remain Send.
  • The separate LocalScheduler and LocalJoinHandle subsystem has been removed, including its local result transport, thread-local executor binding, tests, docs, and benchmark workload.
  • Worker-bound spawn_anywhere and spawn_everywhere explicitly relocate ThreadAware payloads. Runtime-wide spawn_anywhere accepts a ThreadAware payload and any Send result.
  • Scheduler::spawn_blocking and RuntimeScheduler::spawn_blocking move synchronous I/O or library calls to blocking pools.
  • Join handles are futures and are awaited. There is no public synchronous JoinHandle::wait operation.
  • Joining yields Result<T, JoinError>. Task or factory panics report is_panic(); shutdown cancellation or rejection reports is_shutdown().
  • Dropping a join does not cancel its task or resume a task panic.

Synchronous callers and blocking work

RuntimeScheduler::block_on is the narrow synchronous bridge into Arty. It can run a future that borrows caller-owned data and guarantees that the borrowing factory/future is destroyed before returning, including on cancellation or panic.

block_on rejects calls from async Arty workers and from an already-running futures executor. It may be called from a blocking callback, but the submitted async work must not depend on another callback queued to the same saturated blocking pool.

Arty deliberately does not expose synchronous joining from async workers. Blocking a worker stalls all tasks and timers assigned to it; synchronously driving a child join that requires the same worker can deadlock. Direct polling of a blocking join from its own pool is rejected, while transitive dependency cycles remain the application's responsibility.

Lifecycle and shutdown

  • Each runtime owns its workers and services; there is no process-global runtime singleton.
  • RuntimeOperations::request_stop requests shutdown without waiting.
  • Runtime::stop consumes the owner, waits when safe, and returns shutdown errors. Dropping the owner requests shutdown and normally waits but cannot report errors.
  • Self-waits are rejected when stop/drop occurs on an async worker or the runtime's own blocking callback.
  • Shutdown closes admission before queued commands are drained. New submissions are rejected without invoking factories, pending async tasks are cancelled, queued blocking callbacks are skipped, and already-running blocking calls finish normally.
  • Worker command processing is bounded per cycle so ready tasks and timers continue to progress under sustained submissions.
  • Queued remote tasks carry the shared shutdown signal into their task wrapper, preventing a deferred factory from running if shutdown begins after command admission but before first poll.
  • Scoped borrowed work cannot return before its storage has been destroyed.

Entry-point macros

  • Export arty::main and arty::test.
  • The only attribute configuration is workers = N, which caps async workers through WorkersPolicy::at_most(N).
  • The former builder = ... and runtime_path = ... options have been removed. Applications needing custom clocks, telemetry, stack sizes, blocking pools, or exact/all-worker policies construct Runtime explicitly.
  • Macro entry points take Builtins by value. Controlled tests may take a second owned ClockControl argument.
  • Generated entry points preserve the annotated function's visibility and return type, stop the runtime before returning, preserve application errors, resume the original root panic payload, and report construction, root cancellation, or shutdown failures as panics.

Thread awareness and placement

A running task never migrates between workers. ThreadAware values can update worker-bound state when explicitly relocated for newly spawned work. Moving or cloning a worker-bound value alone does not relocate it.

Schedulers, joins, Builtins, clocks, and runtime-operation handles do not keep the runtime alive. Relocation does not transfer worker-bound handles to another runtime, and ordinary spawn and block_on do not relocate captures or results.

Time facade

arty::time now re-exports only Clock and, with test-util, ClockControl. Applications needing delay/timeout future types, stopwatch types, periodic timers, or FutureExt import them directly from tick.

Documentation and implementation guidance

The public documentation is organized around scheduling, configuration, shutdown, thread awareness, time, telemetry, and blocking work. It explicitly documents that Arty has no async network or file I/O driver and explains why blocking work must stay off async workers.

A maintainer-facing runtime implementation map documents endpoint publication, dispatcher distribution, readiness, runtime ownership, worker command lifecycle, shutdown coordination, and scoped borrowed-task destruction without exposing those internals as public API.

Panic and safety hardening

  • Completion-result publication is separated from receiver notification. A panic from a join receiver waker does not turn successful work into task-panic telemetry.
  • Cancellation and blocking-result publication contain receiver-notification panics; worker panic payloads are disposed before shutdown completion is published.
  • Task and queued-factory destruction remains contained at proven boundaries, with fail-closed handling for repeated panic-payload disposal.
  • Independent waker metadata and task retirement prevent stale notifications from referencing reused task storage.
  • Runtime validation failures retain private typed identities while the public runtime::Error aggregate and its documented non-classification contract remain unchanged.
  • Validated stack and blocking-pool limits are carried internally as nonzero values.

performables and package versioning

Arty uses the current performables synchronization and channel APIs for runtime ownership, worker startup, dispatch, blocking-pool coordination, and shutdown. The approved breaking API changes are released as arty 0.4.0 with matching arty_macros and arty_macros_impl 0.4.0 packages. arty_executor is 0.1.3 for the independent-waker API.

Telemetry

Runtime events and metrics use the arty.rt namespace. Blocking-pool saturation is arty.rt.blocking_worker.pool_saturated with blocking_worker_pool.mode (shared or per_worker) and blocking_worker_pool.max_threads dimensions. Saturation warnings are transition-gated per episode and reset after deterministic pool recovery. Panic messages use a separate redaction class from routine system metadata. The blocking-pool thread name is arty-blocking; existing arty.thread.id and private arty/SystemMetadata classification are retained.

Benchmarks

The matched scheduling benchmarks and concurrent cache-contention benchmark align Arty and Tokio workload shapes by task count and result waiting. The removed local-scheduler workload was deleted with that API. The contention workload submits 10 outer tasks concurrently; each submits 10 worker-bound child tasks, for 100 cache operations per sample. A matched Linux Gungraun/Callgrind backend covers the same workload.

Latest Windows x86-64-v3 Criterion sample:

Case Arty Tokio Arty vs Tokio
concurrency_w1 81,150 ns 74,914 ns 1.083x slower
concurrency_w8 152,417 ns 95,946 ns 1.588x slower

Matched Gungraun instruction counts were lower for Arty in the same workload: 10,059 vs 11,070 at w1, and 9,981 vs 11,210 at w8. These results are workload- and host-specific optimization evidence, not a claim of universal superiority.

Validation

Local validation for the current head includes:

  • Targeted Clippy, formatting, Cargo manifest ordering, generated README, and spelling checks passing.
  • 284 Arty tests and 12 macro-expansion tests passing.
  • 45 ordinary Arty doctests and 4 feature-gated doctests passing.
  • All 10 Arty feature combinations passing with cargo hack.
  • cargo public-api confirming the removed local/pinning/auto surfaces are absent and arty::time exports only Clock and ClockControl.
  • cargo-udeps reporting all Arty dependencies used.
  • cargo-semver-checks reporting only the intentional breaking removals and blocking-pool signature change.

Comprehensive platform, coverage, runtime-analysis, mutation, and Miri validation remains provided by CI. OS thread-creation failures and cleanup after partial startup remain explicitly deferred as a separate lifecycle/error-design change.

martintmk and others added 4 commits September 29, 2026 18:48
Port the scheduling runtime and runtime-only entry-point macros without SDK I/O, memory pools, or task metadata. Keep runtime APIs under arty::rt, centralize submission on TaskScheduler, and add compatibility tests and metabench comparisons.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Record all 28 scheduling pairs and telemetry measurements, their confidence bounds, reproduction configuration, and architecture and measurement limitations.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Register the approved main/test macro reexports with the external-types gate and remove the unused clock strategy allowance.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@codecov

ghost commented Sep 29, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (a412386) to head (7133ff2).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##             main     #785     +/-   ##
=========================================
  Coverage   100.0%   100.0%             
=========================================
  Files         767      796     +29     
  Lines      116600   119357   +2757     
=========================================
+ Hits       116600   119357   +2757     
Flag Coverage Δ
linux 100.0% <100.0%> (ø)
linux-arm 100.0% <100.0%> (ø)
scheduled ?
windows 100.0% <100.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Martin Tomka and others added 13 commits September 29, 2026 22:32
Remote spawns no longer box the future before inserting it into the
pooled task set; the factory now receives the TaskSet and inserts the
concrete future, removing one allocation per remote spawn.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve the Review Lens 0.19.1 report for #785 at 45f3881.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add foreign-owner collision and macro consumer cases, improve wake and panic coverage, and apply the explicitly approved exclusions for test scaffolding and diagnostic/invariant-only code.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Expose flat runtime configuration and a single runtime::Error, preserve owned Builtins callbacks, close local admission before cancellation, avoid system-task self-join, retain outcome enrichment, and remove redundant internal adapters. Preserve full macro argument types, reuse telemetry benchmark storage, and expand Miri-compatible integration tests with simulated hardware. Keep startup thread-creation panics as explicitly deferred by the user.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Expose TaskScheduler::spawn_blocking and align runtime internals, tests, documentation, and current benchmark names. Preserve telemetry wire identifiers, the OS pool thread name, and historical benchmark data.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add at-most worker limits and a RuntimeBuilder expression escape hatch. Inject opt-in ClockControl into tests without overriding custom builder clocks, preserving runtime paths, return types, and owned callbacks.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
(cherry picked from commit a7edc4c55efa78f7b9104af1e018f2e6847289b0)
Move the canonical macro exports to arty::main and arty::test and update authored docs, examples, alias tests, and generated READMEs. Keep runtime types and generated runtime_path references under arty::runtime.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rename WorkerPoolPolicy and its builder method to BlockingPoolPolicy and blocking_pool_policy, including private pool types and current telemetry/thread identifiers. Preserve generic SystemMetadata/OS concepts and historical benchmark and review records as explicitly requested.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rename current event and metric identifiers from oxidizer.rt to arty.rt, including blocking-pool saturation. Preserve thread-id attributes, SystemMetadata classification, and historical benchmark/review records.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…lints

Diagnose signed LitInt values explicitly without restricting positive literals to the host pointer width. Keep the existing invalid-input oracle and add signed-zero/suffixed/hexadecimal cases. Make thread-local admission closure an associated operation while retaining the scope guard through teardown.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
(cherry picked from commit 5184a444478d440248634cf1e8e9706ada8a984a)
Comment thread crates/arty/docs/snippets/async_task.md Outdated
Comment thread crates/arty/examples/arty_main.rs Outdated
Comment thread crates/arty/examples/arty_basic.rs

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

It introduces a large draft concurrency runtime and unsafe scoped execution machinery whose final validation remains dependent on comprehensive CI and human review.

Review effort: Balanced
Findings: None

What changed in this PR

Adds an opt-in, thread-aware Arty runtime with worker-affine scheduling, blocking pools, telemetry, controlled time, and entry-point macros.

Changes:

  • Introduces runtime/task APIs, shutdown semantics, relocation, and observability.
  • Adds arty::main/arty::test through companion proc-macro crates.
  • Adds extensive tests, benchmarks, examples, and user guides.
File Description
crates/​arty/​tests/​time.rs Tests worker-driven timers.
crates/​arty/​tests/​test_macro.rs Tests runtime test macros.
crates/​arty/​tests/​stop.rs Tests shutdown behavior.
crates/​arty/​tests/​spawning.rs Tests task submission paths.
crates/​arty/​tests/​scheduler.rs Tests stored schedulers.
crates/​arty/​tests/​runtime_operations.rs Tests runtime operations and relocation.
crates/​arty/​tests/​relocation.rs Tests capability and pool relocation.
crates/​arty/​tests/​public_paths.rs Verifies public API paths and traits.
crates/​arty/​tests/​panic.rs Tests panic propagation through macros.
crates/​arty/​tests/​observability.rs Verifies runtime telemetry.
crates/​arty/​tests/​emit_enrichment_propagation.rs Tests enrichment propagation.
crates/​arty/​tests/​construction.rs Tests construction failures.
crates/​arty/​tests/​block_on_spawn.rs Tests scoped borrowing.
crates/​arty/​src/​task/​scheduler.rs Implements remote and blocking scheduling.
crates/​arty/​src/​task/​mod.rs Exposes task APIs.
crates/​arty/​src/​task/​local.rs Implements local task scheduling.
crates/​arty/​src/​task/​join/​remote.rs Implements remote joins.
crates/​arty/​src/​task/​join/​mod.rs Exposes join handles.
crates/​arty/​src/​task/​join/​local.rs Implements local joins.
crates/​arty/​src/​task/​execution/​result.rs Defines internal task outcomes.
crates/​arty/​src/​task/​execution/​remote.rs Wraps remote task execution.
crates/​arty/​src/​task/​execution/​preparation.rs Prepares task transports.
crates/​arty/​src/​task/​execution/​mod.rs Organizes execution helpers.
crates/​arty/​src/​task/​execution/​local.rs Wraps local task execution.
crates/​arty/​src/​runtime/​worker/​signal.rs Implements worker wakeups.
crates/​arty/​src/​runtime/​worker/​protocol.rs Defines worker commands.
crates/​arty/​src/​runtime/​worker/​mod.rs Organizes worker internals.
crates/​arty/​src/​runtime/​thread/​waiter.rs Coordinates concurrent shutdown waits.
crates/​arty/​src/​runtime/​thread/​mod.rs Organizes thread utilities.
crates/​arty/​src/​runtime/​thread/​lifecycle.rs Adds thread lifecycle telemetry.
crates/​arty/​src/​runtime/​thread/​blocking.rs Guards blocking APIs.
crates/​arty/​src/​runtime/​telemetry/​mod.rs Defines telemetry classification.
crates/​arty/​src/​runtime/​mod.rs Exposes runtime APIs.
crates/​arty/​src/​runtime/​error.rs Adds opaque runtime errors.
crates/​arty/​src/​runtime/​dispatch/​mod.rs Organizes dispatch internals.
crates/​arty/​src/​runtime/​dispatch/​dispatcher_client.rs Adds shared dispatcher access.
crates/​arty/​src/​runtime/​context/​operations.rs Implements runtime operations.
crates/​arty/​src/​runtime/​context/​init.rs Initializes worker capabilities.
crates/​arty/​src/​runtime/​config/​processors.rs Adds processor policies.
crates/​arty/​src/​runtime/​config/​mod.rs Exposes runtime configuration.
crates/​arty/​src/​runtime/​config/​blocking_pool_policy.rs Adds blocking-pool policies.
crates/​arty/​src/​runtime/​bootstrap/​pools.rs Builds shared or isolated pools.
crates/​arty/​src/​runtime/​bootstrap/​mod.rs Organizes runtime bootstrap.
crates/​arty/​src/​macros.rs Documents and re-exports macros.
crates/​arty/​src/​lib.rs Exposes feature-gated runtime surface.
crates/​arty/​src/​documentation/​time.rs Adds time guide.
crates/​arty/​src/​documentation/​thread_awareness.rs Adds relocation guide.
crates/​arty/​src/​documentation/​telemetry.rs Adds telemetry guide.
crates/​arty/​src/​documentation/​scheduling.rs Adds scheduling guide.
crates/​arty/​src/​documentation/​mod.rs Exposes advanced guides.
crates/​arty/​src/​documentation/​lifecycle.rs Adds lifecycle guide.
crates/​arty/​src/​documentation/​configuration.rs Adds configuration guide.
crates/​arty/​README.md Regenerates crate overview.
crates/​arty/​examples/​arty_main.rs Demonstrates macro entry points.
crates/​arty/​examples/​arty_basic.rs Demonstrates explicit runtime ownership.
crates/​arty/​docs/​snippets/​local_task.md Documents local tasks.
crates/​arty/​docs/​snippets/​fn_runtime_stop.md Documents shutdown requests.
crates/​arty/​docs/​snippets/​blocking_task.md Documents blocking tasks.
crates/​arty/​docs/​snippets/​async_task.md Documents asynchronous tasks.
crates/​arty/​docs/​PANICS.md Redirects panic guidance.
crates/​arty/​docs/​IO.md Updates the I/O support boundary.
crates/​arty/​docs/​DESIGN.md Points to canonical guides.
crates/​arty/​Cargo.toml Adds features, dependencies, and targets.
crates/​arty/​benches/​arty_telemetry.rs Benchmarks telemetry overhead.
crates/​arty_macros/​tests/​entrypoints.rs Tests proc-macro expansion behavior.
crates/​arty_macros/​src/​lib.rs Defines proc-macro shims.
crates/​arty_macros/​README.md Documents macro usage.
crates/​arty_macros/​Cargo.toml Defines the proc-macro crate.
crates/​arty_macros_impl/​README.md Documents implementation crate.
crates/​arty_macros_impl/​Cargo.toml Defines macro implementation dependencies.
Cargo.toml Registers crates and dependencies.
Cargo.lock Locks the expanded dependency graph.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@martintmk martintmk changed the title feat(arty): add feature-gated thread-aware runtime feat(arty): add thread-aware runtime Sep 30, 2026
Martin Tomka and others added 3 commits September 30, 2026 15:34
Introduce task::JoinError with panic/shutdown classification. Return Result from remote and local joins and from run/block_on; preserve annotated macro signatures and root panic payloads at the macro boundary. Reject new work immediately, cancel pending asynchronous work and queued blocking callbacks, and retain scoped-storage destruction guarantees. Update consumers, documentation and focused behavioral contracts.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the Git dependency example aligned with JoinError and mark older benchmark data as historical for the changed cancellation contract.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Apply the PR author's requests: remove shared rustdoc snippets, rename the explicit-owner example to arty_advanced, and make arty_basic a macro entry point with a 1ms clock delay and a simple message.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment thread docs/reviews/arty-runtime-pr-785.md Outdated

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early feedback on the draft: I found two lifecycle paths where a panicking configured telemetry processor can interrupt required shutdown work, and a command-drain loop that can starve task and timer progress under sustained submissions. The three inline comments describe the distinct failure paths.

The typed-join and rejected-submission paths I traced preserve their stated error behavior; local admission closes before cancellation, so the earlier destructor-reentrancy concern appears addressed. The macro return/panic boundary and opt-in feature wiring also matched the described contracts. The benchmark report correctly limits its measurements to the older revision.

This was a static review of the security, correctness, testing, performance, and conformance surfaces of the change; nothing was executed. Generated README content and the explicitly deferred OS thread-creation failure handling were not raised.

Comment thread crates/arty/src/runtime/bootstrap/startup.rs
Comment thread crates/arty/src/runtime/dispatch/dispatcher_core.rs
Comment thread crates/arty/src/runtime/worker/async_worker.rs Outdated
Martin Tomka and others added 2 commits October 7, 2026 10:51
Remove JoinHandle::wait and blocking-wait provenance tracking, simplify async-worker detection, and migrate tests and documentation to async joins or test-only helpers.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Public documentation and the advertised 0.4 API currently contain conflicting type, join, and telemetry-classification contracts.

Review effort: Balanced
Findings: 1 Medium severity · 3 Low severity

Open (4)

Comment thread crates/arty/docs/PANICS.md Outdated
Comment thread crates/arty/src/documentation/telemetry.rs Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The documented 0.4 API promises CpuPolicy and JoinHandle::wait, but neither API is exposed by the implementation.

Review effort: Balanced
Findings: 1 Medium severity · 3 Low severity

Open (4)

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Copilot speaking]

Published 17 findings. No finding follows up on an existing discussion thread.

See diagnostics
Diagnostic Value
Cache Miss

Comment thread crates/arty/src/runtime/worker/async_worker.rs
Comment thread crates/arty/src/runtime/thread/lifecycle.rs
Comment thread crates/arty/Cargo.toml
Comment thread crates/arty/src/runtime/blocking_worker/mod.rs Outdated
Comment thread crates/arty/src/runtime/worker/signal.rs
Comment thread crates/arty/src/documentation/scheduling.rs Outdated
Comment thread crates/arty/src/runtime/error.rs
Comment thread crates/arty/src/documentation/scheduling.rs Outdated
Comment thread crates/arty/src/runtime/blocking_worker/mod.rs Outdated
Comment thread crates/arty/src/task/join/error.rs Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Public documentation references a nonexistent join API, omits a telemetry redaction class, and disagrees with the advertised runtime policy type.

Review effort: Balanced
Findings: 1 Medium severity · 3 Low severity

Open (4)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The change introduces extensive unsafe executor-lifetime, panic-containment, concurrency, and shutdown behavior that warrants final human review.

Review effort: Balanced
Findings: None

Resolved since last review (4)

@github-actions

ghost commented Oct 7, 2026 •

Copy link
Copy Markdown

⚠️ SemVer check advisory

Potential breaking changes

cargo semver-checks flagged the following on this PR. This is informational -- breaking changes between commits are expected; the major-version bump happens at release time, not on every PR.

thread_aware

     Cloning a412386acdfa7caa06cb6db24fd5746bba918c6e
    Building thread_aware v0.12.0 (current)
       Built [   3.234s] (current)
     Parsing thread_aware v0.12.0 (current)
      Parsed [   0.002s] (current)
    Building thread_aware v0.12.0 (baseline)
       Built [   3.226s] (baseline)
     Parsing thread_aware v0.12.0 (baseline)
      Parsed [   0.002s] (baseline)
    Checking thread_aware v0.12.0 -> v0.12.0 (no change; assume minor)
     Checked [   0.017s] 196 checks: 195 pass, 1 fail, 0 warn, 58 skip

--- failure module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/module_missing.ron

Failed in:

     Summary semver requires new major version: 1 major and 0 minor checks failed
  mod thread_aware::_documentation, previously in file /home/runner/work/oxidizer/oxidizer/target/semver-checks/git-a412386acdfa7caa06cb6db24fd5746bba918c6e/61461d60151db1fd85dd8cff704f8205f069c882/crates/thread_aware/src/_documentation/mod.rs:4
    Finished [   6.773s] thread_aware

Inconclusive comparisons

cargo semver-checks could not complete the following comparisons. These failures are informational because an unbuildable baseline is not evidence of a breaking API change.

metabench (exit 101)

     Cloning a412386acdfa7caa06cb6db24fd5746bba918c6e
    Building metabench v0.1.1 (current)
error: running cargo-doc on crate 'metabench' failed with output:
-----
   Compiling proc-macro2 v1.0.107
   Compiling unicode-ident v1.0.26
   Compiling quote v1.0.47
   Compiling serde_core v1.0.229
   Compiling zmij v1.0.23
   Compiling serde v1.0.229
   Compiling serde_json v1.0.151
   Compiling libc v0.2.190
    Checking cfg-if v1.0.5
    Checking itoa v1.0.18
   Compiling syn v3.0.6
   Compiling syn v2.0.119
    Checking memchr v2.8.3
    Checking bitflags v2.13.2
   Compiling zerocopy v0.8.62
   Compiling find-msvc-tools v0.1.14
   Compiling shlex v2.0.1
   Compiling equivalent v1.0.2
   Compiling winnow v1.0.4
   Compiling hashbrown v0.17.1
   Compiling semver v1.0.28
   Compiling rustversion v1.0.23
   Compiling rustc_version v0.4.1
   Compiling toml_parser v1.1.4+spec-1.1.0
   Compiling indexmap v2.14.2
   Compiling serde_derive v1.0.229
   Compiling cc v1.6.0
   Compiling zerocopy-derive v0.8.62
   Compiling derive_more-impl v2.1.1
   Compiling toml_datetime v1.1.2+spec-1.1.0
   Compiling rustix v1.1.5
   Compiling autocfg v1.5.1
   Compiling toml_edit v0.25.16+spec-1.1.0
   Compiling num-traits v0.2.19
   Compiling alloca v0.4.0
    Checking either-or-both v0.3.1
   Compiling gungraun-macros v0.9.1
   Compiling proc-macro-error-attr3 v3.1.1
   Compiling thiserror v2.0.21
   Compiling cfg_aliases v0.2.2
    Checking regex-syntax v0.8.11
    Checking anstyle v1.0.14
    Checking clap_lex v1.1.1
   Compiling getrandom v0.4.3
    Checking linux-raw-sys v0.12.1
    Checking either v1.19.0
   Compiling bincode-next v3.1.1
    Checking ciborium-io v0.2.2
    Checking regex-automata v0.4.18
    Checking itertools v0.13.0
    Checking clap_builder v4.6.7
    Checking half v2.7.1
   Compiling nix v0.31.3
    Checking ciborium-ll v0.2.2
    Checking rapidhash v4.5.1
   Compiling proc-macro-error3 v3.1.1
   Compiling proc-macro-crate v3.5.0
   Compiling derive_more v2.1.1
   Compiling thiserror-impl v2.0.21
    Checking cast v0.3.0
   Compiling pastey v0.2.3
    Checking unty-next v0.1.2
   Compiling gungraun v0.19.4
    Checking same-file v1.0.6
    Checking walkdir v2.5.0
    Checking criterion-plot v0.8.2
    Checking clap v4.6.7
   Compiling metabench_macros_impl v0.1.1 (/home/runner/work/oxidizer/oxidizer/crates/metabench_macros_impl)
    Checking ciborium v0.2.2
    Checking gungraun-runner v0.20.0
    Checking regex v1.13.1
    Checking gungraun-runner v0.19.4
    Checking tinytemplate v1.2.1
    Checking folo_utils v0.1.14
    Checking nix v0.27.1
    Checking page_size v0.6.0
    Checking jiff-core v0.1.1
   Compiling metabench v0.1.1 (/home/runner/work/oxidizer/oxidizer/crates/metabench)
    Checking once_cell v1.21.4
    Checking fastrand v2.5.0
    Checking anes v0.1.6
    Checking oorandom v11.1.5
    Checking tempfile v3.27.0
    Checking criterion v0.8.2
    Checking command-group v5.0.1
    Checking jiff v0.2.38
    Checking gungraun-summary v6.0.0
error[E0432]: unresolved import `gungraun_runner::api::ValgrindTool`
  --> /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/gungraun-summary-6.0.0/src/v6.rs:10:59
   |
10 |     CachegrindMetric, DhatMetric, ErrorMetric, EventKind, ValgrindTool,
   |                                                           ^^^^^^^^^^^^ no `ValgrindTool` in `api`

error[E0432]: unresolved imports `gungraun_runner::metrics::model::MetricsDiff`, `gungraun_runner::metrics::model::MetricsSummary`
  --> /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/gungraun-summary-6.0.0/src/v6.rs:12:63
   |
12 | pub use gungraun_runner::metrics::model::{Metric, MetricKind, MetricsDiff, MetricsSummary};
   |                                                               ^^^^^^^^^^^  ^^^^^^^^^^^^^^ no `MetricsSummary` in `metrics::model`
   |                                                               |
   |                                                               no `MetricsDiff` in `metrics::model`

error[E0432]: unresolved imports `gungraun_runner::summary::model::Diffs`, `gungraun_runner::summary::model::FlamegraphSummary`, `gungraun_runner::summary::model::ProfileInfo`, `gungraun_runner::summary::model::SummaryFormat`, `gungraun_runner::summary::model::SummaryOutput`, `gungraun_runner::summary::model::ToolMetricSummary`
  --> /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/gungraun-summary-6.0.0/src/v6.rs:14:38
   |
14 |     BenchmarkKind, BenchmarkSummary, Diffs, FlamegraphSummary, Profile, ProfileData, ProfileInfo,
   |                                      ^^^^^  ^^^^^^^^^^^^^^^^^                        ^^^^^^^^^^^ no `ProfileInfo` in `summary::model`
   |                                      |      |
   |                                      |      no `FlamegraphSummary` in `summary::model`
   |                                      no `Diffs` in `summary::model`
15 |     ProfilePart, ProfileTotal, Profiles, SCHEMA_VERSION, SummaryFormat, SummaryOutput,
   |                                                          ^^^^^^^^^^^^^  ^^^^^^^^^^^^^ no `SummaryOutput` in `summary::model`
   |                                                          |
   |                                                          no `SummaryFormat` in `summary::model`
16 |     ToolMetricSummary, ToolRegression,
   |     ^^^^^^^^^^^^^^^^^ no `ToolMetricSummary` in `summary::model`

For more information about this error, try `rustc --explain E0432`.
error: could not compile `gungraun-summary` (lib) due to 3 previous errors
warning: build failed, waiting for other jobs to finish...

-----

error: failed to build rustdoc for crate metabench v0.1.1
note: this is usually due to a compilation error in the crate,
      and is unlikely to be a bug in cargo-semver-checks
note: the following command can be used to reproduce the error:
      cargo new --lib example &&
          cd example &&
          echo '[workspace]' >> Cargo.toml &&
          cargo add --path /home/runner/work/oxidizer/oxidizer/crates/metabench &&
          cargo check &&
          cargo doc

error: aborting due to failure to build rustdoc for crate metabench v0.1.1

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

A shutdown race can allow a queued task factory to run after shutdown has begun.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Previously missed (2)

In code that hasn't changed since last review

Low severity Correct workload description to reflect 100 operations

crates/​arty/​benches/​arty_contention.rs:8

This describes one outer task, but run creates CONCURRENCY (10) outer tasks and each creates 10 inner operations. The module-level workload description should match the measured 100-operation sample.

Low severity Remove incorrect claim that Callgrind is omitted

crates/​arty/​benches/​arty_contention.rs:21

This says Callgrind is omitted, but the file registers a Linux Gungraun/Callgrind backend below. Keeping this claim makes the benchmark documentation contradict both the implementation and the PR's reported instruction counts.

Comment thread crates/arty/src/runtime/worker/async_worker.rs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The extensive unsafe lifetime, concurrency, panic-containment, and shutdown changes require final human review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Public documentation still describes removed APIs and overstates the narrowed time facade.

4 open findings

🧠 Review effort: Balanced

Comment thread crates/arty/docs/PANICS.md Outdated
Comment thread crates/arty/src/documentation/time.rs Outdated
Comment thread crates/arty/src/runtime/error.rs Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Shutdown clears the worker guard before cancelling user futures, allowing cancellation destructors to perform prohibited blocking waits.

5 open findings
Previously missed (2)

In code that hasn't changed since last review

Low severity Callgrind omission contradicts registered benchmark backend

crates/​arty/​benches/​arty_contention.rs:21

This overview says Callgrind is omitted, but this same benchmark registers #[library_benchmark] cases and a #[library_benchmark_group]/main! backend below. That contradiction obscures that the PR does provide the matched Linux instruction-count workload described in its summary.

Low severity Zero blocking-pool validation is documented too narrowly

crates/​arty/​src/​runtime/​builder.rs:95

BlockingPoolPolicy::max(0) is retained and rejected for both shared() and per_worker() policies, but this text narrows the validation to shared pools; the build error list repeats the same qualifier at lines 181-182. Describe this as a zero blocking-pool limit so the documented validation matches both public policy modes.

🧠 Review effort: Balanced

Comment thread crates/arty/src/runtime/worker/async_worker.rs
Comment thread crates/arty_macros/tests/entrypoints.rs Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The manifest violates the workspace publication convention, and several new API and benchmark comments contradict implemented behavior.

0 open findings

5 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Low severity Correct the documented workload to 10 outer and 100 total tasks

crates/​arty/​benches/​arty_contention.rs:8

This documents only one outer task, but both case runners fill 10 outer-task handles and every outer task launches 10 children. The stated workload shape is therefore off by 10×; describe the 10 concurrent outer tasks and 100 total operations per sample.

This issue also appears on line 20 of the same file.

Low severity Describe max as a general pool limit

crates/​arty/​src/​runtime/​builder.rs:95

BlockingPoolPolicy::max applies to both shared and per-worker policies, and build rejects zero for either mode. Calling this specifically a “shared-pool” limit incorrectly implies that a zero per-worker limit is accepted; describe it as a pool limit generally.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Remove the arty_executor independent-waker mode while preserving unrelated executor safety fixes. Keep the Arty retained-waker integration contracts ignored pending a separate behavior decision.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 6bae42bd-ea55-4382-8d58-2d4f521750b0

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Cargo-renamed macro dependencies are unsupported, one workspace dev-dependency violates publishing policy, and public package metadata still describes a single-threaded runtime.

3 open findings

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread crates/arty_macros_impl/src/lib.rs
Comment thread crates/arty/Cargo.toml
Comment thread crates/arty/Cargo.toml
// We guarantee this via our own safety requirement (it happens in `run()`). We know that
// the caller has the chance to fulfill their safety guarantee because none of the code
// between this point and end of the current function can panic.
let executor = unsafe { Executor::builder().owner_waker(WorkerSignal::waker(&signal)).build() };

ghost Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖: Arty's safe runtime can now terminate the process when ordinary cloned task wakers are retained after task completion or cancellation.

Restore a supported retained-waker shutdown behavior before merging, and re-enable the four ignored lifecycle tests.

Arty now constructs the executor in its inline-waker mode here. A retained clone keeps that task's waker_count above zero, so completed storage cannot become inert and Runtime::stop() waits until the executor's 30-second deadline invokes TerminateProcess. Retaining a Waker is valid safe code—Waker is Send + Sync + 'static—and panic_waker_lifetimes.rs already demonstrates this after success, panic, and cancellation. panic_shutdown_timer.rs covers the related shutdown boundary. Ignoring those contracts as "deferred" leaves a known safe-code process-abort path in the release.

ghost Oct 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. Per the current scope decision, retained-waker shutdown behavior and the four ignored lifecycle contracts are deferred to consolidated follow-up work after this PR is merged and closed. I’m resolving this thread now and adding this risk and requested test re-enablement to that single follow-up ticket.

Auto-replied by the GitHub Copilot app

@@ -114,8 +117,7 @@ where
// We enforce this via an equivalent safety requirement on the `Task::initialize()`.

ghost Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖: A consuming task wake can unwind before releasing its counted reference, violating the raw-waker ownership contract and leaving task storage permanently live.

Make waker_wake decrement waker_count with an RAII guard even if signal.wake() panics, and add a regression with a panicking owner waker.

This task polls with the WakeSignal waker, but the reverted implementation in wake.rs:252 calls the arbitrary configured owner_waker before the fetch_sub at line 255. Safe code can construct a guarded executor with a Waker::from(Arc<impl Wake>) whose callback panics, clone the task waker, and consume that clone with wake(). The unwind skips the decrement even though RawWakerVTable::wake must release the consumed instance's resources; is_inert() then remains false and shutdown cannot release the task. The removed inline implementation's CountGuard provided the required unwind-safe decrement.

ghost Oct 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. The unwind-safe consuming-wake decrement and panicking-owner-waker regression are deferred to the same consolidated follow-up work after this PR is merged and closed. I’m resolving this thread now and adding the raw-waker ownership issue to that single follow-up ticket.

Auto-replied by the GitHub Copilot app

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.

9 participants