Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
martintmk
left a comment
There was a problem hiding this comment.
[AI AGENT]: I reviewed all eight added public contracts (Driver, DriverContext, DriverInit, DriverProvider, Parker, Shutdown, SystemTask, and SystemTaskSpawner), their rustdoc, the example runtime, dependencies, naming, object safety, shutdown behavior, and the new public-surface tests. Two targeted probes reproduced lifecycle contradictions below. The remaining public surface is documented and intentionally narrow; I found no additional dependency, naming, performance, resilience, or test-weakening issue. CI was still running with no reported failures at the final check.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #737 +/- ##
=========================================
- Coverage 100.0% 98.5% -1.5%
=========================================
Files 583 659 +76
Lines 62930 89164 +26234
=========================================
+ Hits 62930 87857 +24927
- Misses 0 1307 +1307
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58677290-64b2-44fc-9e40-6c80b7e9e5c0
martintmk
left a comment
There was a problem hiding this comment.
[AI AGENT]: Pragmatic Rust public API review completed with no findings. Verified the all-features cargo public-api surface, generated rustdoc JSON, package tests, clippy, spelling, the two-driver example, and the 100% coverage gate.
Coverage: reviewed Driver, DriverContext, DriverProvider, DriverInit, Parker, SystemTasks, and SystemTask from the downstream consumer and evolution perspective. The audit cannot prove runtime behavior or soundness from API output alone; those were covered separately by tests and multi-model review.
The resulting surface removes speculative Clone from DriverInit, hides Arc<dyn ...> behind the cloneable SystemTasks handle, and documents the boxed shutdown future as the deliberate object-safe shape for Box<dyn Driver<Context = C>>.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 206cc953-3282-4edf-b114-ee2b7f518bdf
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 206cc953-3282-4edf-b114-ee2b7f518bdf
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 206cc953-3282-4edf-b114-ee2b7f518bdf
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6a475d84-6296-497e-975b-7442019e433b
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Reviewed the exact PR head across API design, correctness, tests, performance, naming, telemetry, resilience, consistency, public API, and public documentation. I found three blocking issues and two non-blocking issues. Four are anchored inline; the remaining non-blocking issue is that the PR description still documents the obsolete DriverContext/Parker/boxed-future API and should be refreshed to match the current IoContext, interruptor, and blocking shutdown(self) contract.
Focused validation: cargo check -p arty_io_core passed; specialist runs also passed the 13 public-surface tests and the two-thread example. The focused correctness probe reproduced shutdown returning while admitted I/O was still active.
Posted by an AI agent · Non-blocking
The PR summary describes an obsolete arty_io_core API
Problem
The PR body still says DriverContext selects the provider, Driver exposes only shared-reference callbacks, Parker defines waiting, and shutdown returns an object-safe boxed future. At the pinned head, IoContext::provider selects the provider, process_completions takes &mut self, interruptor returns Waker, and shutdown(self) blocks and returns Result<(), ShutdownError>.
Why this matters
Reviewers and future readers are given a materially different registration, completion, and shutdown contract from the code and the new requirements/design documents.
Suggested fix
Refresh the PR summary to name IoContext, process_completions/interruptor, and the consuming blocking shutdown result, removing the obsolete Parker, shared-reference-callback, and boxed-future claims.
| /// Panics when this worker's driver instance cannot be initialized. Driver registration is | ||
| /// runtime-fundamental: after one worker fails to initialize, the runtime cannot continue in a | ||
| /// coherent partially registered state. | ||
| fn create(self, context: DriverContext) -> Self::Driver; |
There was a problem hiding this comment.
Posted by an AI agent
DriverProvider::create makes environmental initialization failures fatal
Problem
DriverProvider::create returns Self::Driver and its contract requires implementations to panic whenever a worker instance cannot be initialized. Driver initialization can fail for runtime conditions such as exhausted OS resources or a permission change after the separately suggested capability check, so the public contract provides no typed way to report a normal initialization failure.
Why this matters
A runtime cannot roll back registration or report the affected driver reliably; with panic = abort the mandated path terminates the process, and even unwinding forces every runtime to treat ordinary setup failure as a panic across a foundational extension boundary.
Suggested fix
Make create return Result<Self::Driver, DriverInitializationError> (or the crate's canonical extensible error type), and let the runtime abort and roll back the partial registration explicitly after receiving the error.
|
|
||
| fn shutdown(self) -> Result<(), ShutdownError> { | ||
| // Release publishes admission closure before graceful cleanup starts. | ||
| self.state.shutdown_started.store(true, Ordering::Release); |
There was a problem hiding this comment.
Posted by an AI agent
Example shutdown returns before admitted I/O completes
Problem
EchoContext::perform_io checks shutdown_started and then performs the operation without acquiring an active-operation lease, while EchoDriver::shutdown only stores the flag and immediately returns; SampleContext uses the same check-then-act pattern at sample_driver.rs:38. A focused probe using the exact head modules started a 256 MiB uppercase operation, waited for it to enter perform_io, and then shut down the runtime; shutdown completed and printed shutdown returned while operation was still active, contradicting R6's requirement that shutdown close admission and wait for active operations to drain.
Why this matters
The canonical runtime example demonstrates that a successful graceful shutdown may return while previously admitted I/O is still running, so driver authors following it can release resources or report shutdown completion before their operations and callbacks are finished.
Suggested fix
Give each example state a synchronized closed/active-operation lifecycle: atomically acquire a lease while admission is open, release it when the operation finishes, and have shutdown close admission before waiting with a bounded deadline for the active count to reach zero. Add a focused concurrent example test that proves shutdown cannot return before an admitted operation completes.
|
|
||
| context.system_tasks().spawn(|| {}); | ||
|
|
||
| assert_eq!(context.thread(), &worker); |
There was a problem hiding this comment.
Posted by an AI agent
The public-surface test never exercises SystemTasks::Debug
Problem
The compile-time assertion proves that SystemTasks implements Debug, but this test formats only DriverContext and ProviderContext. The matching package coverage run reports system_tasks.rs:39-41—the complete Debug::fmt implementation—as uncovered, leaving arty_io_core at 94.23% line coverage and matching Codecov's three missing lines.
Why this matters
The new test suite does not preserve the observable debug representation it advertises, and the uncovered implementation currently contributes to the failing codecov/patch and codecov/project checks.
Suggested fix
Add an assertion that formats the reachable SystemTasks handle and checks its stable type name, for example assert!(format!("{:?}", context.system_tasks()).contains("SystemTasks"));.
| /// coalesced, but an interrupt is never dropped. The returned interruptor remains safe to | ||
| /// invoke after the driver is dropped. | ||
| #[must_use] | ||
| fn interruptor(&self) -> Waker; |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
Driver::interruptor obscures that the handle is a standard Waker
Problem
This method returns std::task::Waker, while the workspace names the same returned concept waker (for example, arty_executor::wake::Wake::waker at crates/arty_executor/src/wake.rs:152). Renaming it to the newly coined interruptor makes this trait the only sibling to hide the foundational type behind a different noun.
Why this matters
Driver implementers and runtime authors must learn a crate-specific synonym, and the divergence propagates through the public trait, tests, examples, requirements, design document, generated README, and spelling dictionary.
Suggested fix
Rename the method back to waker and update those references; the current PR title does not mention either term and needs no change.
| fn interruptor(&self) -> Waker; | |
| fn waker(&self) -> Waker; |
Add a separate proposal for coordinating independently registered I/O drivers across shared IOCP and multiple io_uring sources. Document participation modes, notification routing, service and parking, safe draining, trade-offs, and public references without changing the current contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9fcc1db7-eda6-4c33-bbbe-77ff8b5e5ae6
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Reviewed the current PR head across API design, correctness, tests, performance, naming, telemetry, resilience, consistency, public API, and public documentation. Three blocking issues and two non-blocking issues remain applicable.
Four are already represented by unresolved current threads: provider creation mandates panic, graceful shutdown can return before admitted I/O drains, SystemTasks::Debug remains untested, and Driver::interruptor obscures its Waker contract. One new non-blocking naming issue is posted inline: SystemTasks names submitted work rather than its spawning capability.
Focused validation passed all 13 arty_io_core public-surface tests and the two-thread runtime example. Paired cargo public-api and rustdoc JSON extraction completed for the exact base/head snapshot.
| /// The facility remains available until every driver that received it has completed shutdown, so | ||
| /// cleanup work submitted during shutdown can still run. | ||
| #[derive(Clone)] | ||
| pub struct SystemTasks { |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
SystemTasks names the submitted work instead of the spawning capability
Problem
The type is documented as a cloneable handle and its only operation is spawn, but SystemTasks reads as a collection of tasks. The workspace names this role anyspawn::Spawner (crates/anyspawn/src/spawner.rs:99) and the boxed work BoxedBlockingTask (crates/anyspawn/src/custom.rs:51).
Why this matters
This is a new stable public contract, so consumers will otherwise have to learn an unexplained synonym and a later correction would require a breaking rename.
Suggested fix
Rename the handle to SystemTaskSpawner (and update DriverContext::system_tasks, docs, tests, examples, and generated README references); retain the narrower wrapper rather than exposing the broader anyspawn::Spawner capability.
Summary
Introduces the initial stable
arty_io_corecontract for injecting I/O drivers after runtime startup.DriverContextselects its associated provider, soget_context::<MyContext>()needs no registration value.Driverremains thread-local, boxable asBox<dyn Driver<Context = C>>, and exposes only shared-reference callbacks.Parkerdefines the latched wait/wake contract.SystemTasksis a cloneable crate-owned handle that hides runtime shared-ownership plumbing.is_inertcontract.The crate includes
REQUIREMENTS.md,DESIGN.md, public-surface tests, and a modular two-worker example. The example registers two independent driver types after worker startup, performs in-memory I/O through both contexts, caches repeated lookups, drains all four driver instances, and rejects operations through retained contexts after shutdown.Review
ClonefromDriverInitand replacing exposedArc<dyn ...>plumbing withSystemTasks.Validation
just package=arty_io_core clippyjust package=arty_io_core formatjust package=arty_io_core readmejust package=arty_io_core spellcheckcargo test -p arty_io_corecargo run -p arty_io_core --example two_thread_runtimecargo coverage-gate --package arty_io_corecargo public-api --all-features --manifest-path crates/arty_io_core/Cargo.tomlCI infrastructure blocker
The Windows coverage job fails after all 6,867 tests pass.
cargo llvm-covgenerates a 33,956-characterllvm-cov.exe exportcommand for 445 objects, exceeding the Windows 32,767-character process limit (os error 206). The failed job was retried once and reproduced identically. Linux and Linux ARM coverage, Codecov, mutation tests, static analysis, MSRV, semver, external-type exposure, spelling, and other completed checks pass.