Skip to content

feat(arty): integrate Seismograph runtime diagnostics - #804

Open
martintmk wants to merge 9 commits into
mainfrom
martintmk-implement-arty-seismograph
Open

martintmk wants to merge 9 commits into
mainfrom
martintmk-implement-arty-seismograph

Conversation

@martintmk

@martintmk martintmk commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Summary

  • enable seismograph, seismograph_runtime, and Performables Seismograph instrumentation with Arty's rt feature
  • register each Arty runtime and endpoint-ordered worker, attach workers to their actual threads, and report parked/running/stopping/stopped lifecycle states
  • instrument runtime-wide and direct worker-local async spawn paths with parent identity, placement, materialization, poll, completion, panic, and shutdown-cancellation diagnostics
  • expose an Arty-owned opaque RuntimeId for snapshot correlation and allocate task type descriptors from the shared process-wide runtime source
  • add public-boundary lifecycle, identity, parentage, placement, panic, cancellation, and worker-state coverage, plus the private TypeId descriptor-cache invariant

Prior art and intentional adaptations

This integration follows the runtime diagnostics design reviewed in O365Exchange/O365 Core/ox-sdk ADO PR 5664213, adapted rather than copied onto Arty's architecture after #785.

Arty-specific adaptations keep DispatcherCore as the runtime lifecycle owner, index worker registrations exactly with its existing worker endpoints, preserve Performables ownership, and attach/materialize only after the selected worker is known. Task polling remains inside RemoteTaskFuture's panic boundary; its guard also supplies the active parent task identity for same-runtime child registration. The final worker retirement and dispatcher join share an idempotent completion path, preserving owner-drop behavior and exactly-once stopped reporting. The existing observed event surface remains separate from Seismograph diagnostics.

Performance

Measurements use -C target-cpu=x86-64-v3. Gungraun reports deterministic Linux instruction counts. Allocations and bytes were measured on Windows; threaded allocation rows have small run-to-run scheduling variance, so the table uses the latest repeated allocation run.

  • Base: pre-integration main.
  • 66a7: accepted head 66a7da7b before the final feedback batch.
  • Current: a74c57e6 with parked/unparked hooks, parent propagation, shared descriptors, and public runtime identity.
  • Arty instruction median: +68.43% versus base, +0.09% versus 66a7.
  • Arty instruction range versus 66a7: -6.56% to +3.24%.
  • Telemetry spawn allocation counts and bytes are unchanged from 66a7. Runtime construction retains 16 additional bytes for the public runtime identity.

Scheduling: all Arty scenarios

Scenario Base instr 66a7 instr Current instr Current vs base Current vs 66a7 Base allocs / bytes 66a7 allocs / bytes Current allocs / bytes
blocking_w1_n1 1,284 1,527 1,527 +18.93% +0.00% 2 / 128 B 2 / 128 B 2 / 128 B
blocking_w1_n20 20,927 25,962 25,772 +23.15% -0.73% 42 / 3,340 B 42 / 3,340 B 41 / 3,312 B
blocking_w4_n1 1,430 1,527 1,527 +6.78% +0.00% 2 / 128 B 2 / 128 B 2 / 128 B
blocking_w4_n20 16,746 21,525 21,525 +28.54% +0.00% 43 / 3,408 B 41 / 3,312 B 43 / 3,388 B
nested_w1_n1 1,220 2,096 2,098 +71.97% +0.10% 5 / 256 B 7 / 928 B 7 / 928 B
nested_w1_n20 1,220 2,341 2,343 +92.05% +0.09% 24 / 1,624 B 45 / 8,224 B 45 / 8,224 B
nested_w4_n1 1,220 2,096 2,098 +71.97% +0.10% 5 / 256 B 7 / 928 B 7 / 928 B
nested_w4_n20 1,220 2,311 2,313 +89.59% +0.09% 24 / 1,624 B 45 / 8,224 B 45 / 8,224 B
spawn_w1_n1 1,347 2,219 2,221 +64.88% +0.09% 4 / 256 B 4 / 520 B 4 / 520 B
spawn_w1_n20 20,262 41,054 41,948 +107.03% +2.18% 63 / 5,440 B 81 / 10,720 B 81 / 10,720 B
spawn_w4_n1 1,347 2,219 2,221 +64.88% +0.09% 3 / 160 B 4 / 520 B 4 / 520 B
spawn_w4_n20 21,076 43,063 40,236 +90.91% -6.56% 62 / 3,840 B 80 / 10,400 B 80 / 10,400 B
timeout_w1_n1 1,345 2,210 2,212 +64.46% +0.09% 3 / 168 B 4 / 528 B 4 / 528 B
timeout_w1_n20 1,345 2,210 2,212 +64.46% +0.09% 3 / 168 B 4 / 528 B 4 / 528 B
timeout_w4_n1 4,588 8,051 8,059 +75.65% +0.10% 12 / 672 B 16 / 2,112 B 16 / 2,112 B
timeout_w4_n20 4,459 8,171 8,179 +83.43% +0.10% 12 / 672 B 16 / 2,112 B 16 / 2,112 B
timer_w1_n1 1,475 2,347 2,349 +59.25% +0.09% 4 / 616 B 5 / 976 B 5 / 976 B
timer_w1_n20 20,735 40,294 40,929 +97.39% +1.58% 68 / 7,912 B 88 / 15,112 B 88 / 15,112 B
timer_w4_n1 1,475 2,347 2,349 +59.25% +0.09% 4 / 616 B 5 / 976 B 5 / 976 B
timer_w4_n20 21,586 42,302 41,555 +92.51% -1.77% 66 / 5,408 B 84 / 12,224 B 85 / 12,416 B
wake_w1_n1 1,526 2,388 2,390 +56.62% +0.08% 4 / 200 B 5 / 560 B 5 / 560 B
wake_w1_n20 21,022 42,642 40,684 +93.53% -4.59% 86 / 7,376 B 104 / 14,440 B 104 / 14,440 B
wake_w4_n1 1,526 2,388 2,390 +56.62% +0.08% 4 / 200 B 5 / 560 B 5 / 560 B
wake_w4_n20 21,412 42,037 42,360 +97.83% +0.77% 82 / 5,192 B 101 / 12,200 B 101 / 12,200 B
yield_w1_n1 1,475 2,347 2,349 +59.25% +0.09% 3 / 160 B 4 / 520 B 4 / 520 B
yield_w1_n20 21,190 42,944 42,994 +102.90% +0.12% 65 / 5,584 B 82 / 11,360 B 80 / 10,400 B
yield_w4_n1 1,475 2,347 2,349 +59.25% +0.09% 3 / 160 B 4 / 520 B 4 / 520 B
yield_w4_n20 21,202 41,070 42,400 +99.98% +3.24% 60 / 3,200 B 80 / 10,400 B 81 / 10,592 B

Tokio control scenarios were rerun in the same benchmark. Their run-to-run movement confirms that the larger multi-threaded outliers above are scheduling variance; the complete 56-row table is retained in the benchmark artifact perf-complete-side-by-side.md.

Telemetry allocations: all scenarios

Scenario Base allocs / bytes 66a7 allocs / bytes Current allocs / bytes
lifecycle/active/one 1,390 / 195,575 B 1,398 / 196,107 B 1,398 / 196,123 B
lifecycle/noop 1,366 / 195,674 B 1,374 / 196,206 B 1,374 / 196,222 B
spawn/active/hundred 403 / 20,640 B 500 / 54,400 B 500 / 54,400 B
spawn/active/one 4 / 184 B 5 / 544 B 5 / 544 B
spawn/noop/hundred 305 / 25,920 B 400 / 52,000 B 400 / 52,000 B
spawn/noop/one 3 / 160 B 4 / 520 B 4 / 520 B

Indexed task retirement

The Stage 1 seismograph_runtime registry change replaces linear live-task retirement with indexed removal. Focused microbenchmarks measured 18.1x to 1,239x faster single-thread retirement as live-task population grew, and 28x to 59x faster four-thread contention cases. Registration was +20.2% slower with recording disabled, +19.1% slower with recording enabled, and +1.0% slower when capturing backtraces. Allocation counts were unchanged; retained task memory increased by 8 bytes per task. Stage 2 task-control pooling is intentionally out of scope.

Validation

  • cargo check -p arty -p seismograph_runtime --locked
  • cargo check -p arty --no-default-features --locked
  • cargo test -p arty -p seismograph_runtime --all-features --locked
  • cargo test -p arty -p seismograph_runtime --doc --all-features --locked
  • cargo +1.95 clippy -p arty -p seismograph_runtime --all-targets --all-features --locked -- -D warnings
  • nightly LLVM coverage union (--all-features + --no-default-features): arty 100.0%
  • cargo semver-checks for arty and seismograph_runtime: no update required
  • cargo check-external-types for both changed crates
  • cargo spellcheck --cfg spellcheck.toml check --code 1
  • release build plus final Gungraun and repeated allocation measurements

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

Copilot-Session: aec4830e-98ac-4377-aaca-62fb9ad22217
Copilot AI balanced review requested due to automatic review settings October 9, 2026 08:05

Copilot AI 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

Failed runtime construction currently leaves misleading stopped-runtime snapshot entries, and new test scaffolding lacks the required coverage exclusion.

2 open findings
What changed in this PR

Integrates Seismograph diagnostics into Arty’s runtime, worker, and asynchronous task lifecycles.

Changes:

  • Registers runtimes/workers and instruments task lifecycle events.
  • Adds Seismograph integration coverage and documentation.
  • Enables required features and dependencies.
File Description
Cargo.lock Records new dependencies.
crates/​arty/​Cargo.toml Enables Seismograph integration.
crates/​arty/​src/​documentation/​telemetry.rs Documents diagnostics behavior.
crates/​arty/​src/​runtime/​bootstrap/​startup.rs Registers and attaches runtime workers.
crates/​arty/​src/​runtime/​dispatch/​dispatcher_client.rs Exposes task registration internally.
crates/​arty/​src/​runtime/​dispatch/​dispatcher_core.rs Manages runtime and task telemetry.
crates/​arty/​src/​runtime/​mod.rs Adds the Seismograph module.
crates/​arty/​src/​runtime/​seismograph.rs Implements telemetry lifecycle wrappers.
crates/​arty/​src/​runtime/​thread/​waiter.rs Exposes shutdown completion state.
crates/​arty/​src/​task/​execution/​preparation.rs Materializes task telemetry.
crates/​arty/​src/​task/​execution/​remote.rs Instruments polling and terminal outcomes.
crates/​arty/​src/​task/​scheduler.rs Instruments worker-local spawning.
crates/​arty/​tests/​seismograph.rs Tests public lifecycle diagnostics.

🧠 Review effort: Balanced


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

Comment thread crates/arty/src/runtime/bootstrap/startup.rs Outdated
Comment thread crates/arty/src/runtime/seismograph.rs

@wukchung martinhavelka (wukchung) 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.

🤖 Review of the Seismograph integration. Three verified findings below. The first blocks the build, and because it does, none of the new instrumentation has ever executed.

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

Copilot-Session: aec4830e-98ac-4377-aaca-62fb9ad22217
Copilot AI balanced review requested due to automatic review settings October 9, 2026 08:52

Copilot AI 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

Worker processor metadata and task readiness diagnostics are currently inaccurate, and one coverage convention is missing.

1 open finding
1 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Medium severity Report selected processor IDs in worker metadata

crates/​arty/​src/​runtime/​seismograph.rs:50

WorkerMetadata::processor_index is defined as the logical processor selected by affinity (seismograph_runtime/src/worker.rs:52,66), but this value is only the endpoint ordinal; Arty explicitly documents that ordinal as not being a hardware processor ID (runtime/telemetry/events.rs:32-34). With workers pinned to non-dense CPUs such as 2 and 4, snapshots therefore report 0 and 1. Pass the selected processor IDs from startup into registration, or leave this metadata unset.

Medium severity Instrument every task wake with TaskHandle

crates/​arty/​src/​runtime/​seismograph.rs:123

This records only the initial enqueue wake. Subsequent future wakeups use the executor's original waker because RemoteTaskFuture::poll passes cx through unchanged (task/execution/remote.rs:137), so they never invoke TaskHandle::woken, which Seismograph requires for readiness state and timing (seismograph_runtime/src/task.rs:33-40). Multi-poll tasks can consequently appear Waiting while queued and omit later TaskReady/ready-wait diagnostics. Instrument the waker path so every task wake calls this handle before delegating to the executor waker.

🧠 Review effort: Balanced

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

Copilot-Session: aec4830e-98ac-4377-aaca-62fb9ad22217
Copilot AI balanced review requested due to automatic review settings October 9, 2026 09:11

Copilot AI 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 new unit-test module needs the coverage exclusion, and the changed stopped-event gate lacks targeted lifecycle coverage.

1 open finding
Previously missed (1)

In code that hasn't changed since last review

Medium severity Add integration coverage for self-join stopped-event timing

crates/​arty/​src/​runtime/​dispatch/​dispatcher_core.rs:283

The new completion gate changes the documented arty.rt.stopped lifecycle contract for the self-join path, but current integration coverage only checks a normal external stop() (tests/observability.rs:182-200). Add a lifecycle integration case that drops/stops the owner on its own worker, verifies no stopped event is emitted at that point, and then verifies exactly one event after worker shutdown completes; otherwise this branch can regress without detection. This is required for public runtime/telemetry behavior by crates/arty/AGENTS.md:5-17.

🧠 Review effort: Balanced

@martintmk martintmk left a comment

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.

Posted by an AI agent

This refreshed review covers public contracts and API, correctness, tests, performance, naming, telemetry, resilience, documentation consistency, the public API surface and changes, and integration tests. No topic was left unreviewed.

Full review coverage ended at 8e3c8b7c. Commit f82ce220 was checked only to refresh existing findings, not searched for new ones. Existing discussion already covers missing coverage(off) and per-wake TaskHandle readiness; those findings are not reposted. The processor-index and individual-thread-retirement findings were resolved by the later commit.

At the fully reviewed head, I ran the targeted Seismograph integration test, Arty doctests, and paired cargo-public-api captures. The API captures were byte-identical. I did not run benchmarks, so performance comments are non-blocking.

Public API changes

No API Changes

The complete base and reviewed-head public API captures were identical.

Integration tests

Integration Test Additions Only

Added crates/arty/tests/seismograph.rs; no integration-test target or established assertion was removed, renamed, weakened, or conditionally excluded at the fully reviewed head. The later commit only relaxed the processor assertion from a specific value to presence and did not change this classification.

Comment thread crates/arty/src/task/execution/remote.rs Outdated
Comment thread crates/arty/Cargo.toml
Comment thread crates/arty/src/runtime/bootstrap/startup.rs
Comment thread crates/arty/tests/seismograph.rs
Comment thread crates/arty/tests/seismograph.rs Outdated
Comment thread crates/arty/src/runtime/seismograph.rs Outdated
Comment thread crates/arty/src/runtime/seismograph.rs Outdated
Comment thread crates/arty/src/documentation/telemetry.rs
Comment thread crates/arty/src/runtime/seismograph.rs Outdated
Comment thread crates/arty/src/task/execution/preparation.rs Outdated

@martintmk martintmk left a comment

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.

Posted by an AI agent

I reviewed the entire cb3ef36dc0942dc93017df934a3fcacfc7f451ad..f82ce2209f5dec047187b3c888c2803569214ade diff for public contract and API behavior, correctness, tests, performance, naming, telemetry, resilience, documentation consistency, public API surface changes, and integration-test changes. No area was unreviewed.

Targeted all-feature Seismograph integration tests, relevant unit and telemetry tests, and paired cargo public-api captures were run. No benchmarks were run, so this review makes no quantitative performance claims. Existing GitHub discussion was considered; the inline comments below contain only new root causes.

Public API changes

No API Changes

The complete base and head public API captures are identical.

Integration tests

Integration Test Additions Only

Added crates/arty/tests/seismograph.rs; no existing integration-test file, expectation, or execution path changed.

Comment thread crates/arty/src/runtime/seismograph.rs Outdated
Comment thread crates/arty/src/runtime/dispatch/dispatcher_core.rs Outdated
Comment thread crates/arty/tests/seismograph.rs Outdated
Comment thread crates/arty/src/runtime/seismograph.rs Outdated
Comment thread crates/arty/src/documentation/telemetry.rs
Comment thread crates/arty/src/runtime/seismograph.rs Outdated
@codecov

codecov Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (cb3ef36) to head (3a6e46e).

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #804    +/-   ##
========================================
  Coverage   100.0%   100.0%            
========================================
  Files         798      798            
  Lines      119402   119907   +505     
========================================
+ Hits       119402   119907   +505     
Flag Coverage Δ
linux 99.9% <100.0%> (-0.1%) ⬇️
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.

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

Copilot-Session: aec4830e-98ac-4377-aaca-62fb9ad22217
Copilot AI balanced review requested due to automatic review settings October 9, 2026 10:04

Copilot AI 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

Task wake transitions are not connected to Seismograph, and the new test module lacks the repository’s coverage exclusion.

1 open finding
Previously missed (1)

In code that hasn't changed since last review

Medium severity Task wakes are not reported after returning Pending

crates/​arty/​src/​runtime/​seismograph.rs:129

TaskHandle::woken() is only called for the initial enqueue; no Arty or arty_executor wake path calls it afterward. After a task returns Pending, Seismograph therefore leaves it in Waiting even when its real waker fires, and omits TaskReady events and ready-wait metrics (seismograph_runtime/src/task.rs:33-50, activity.rs:101-119). Wire the task handle into the executor's wake signal so every actual wake reports woken().

🧠 Review effort: Balanced

@martintmk martintmk left a comment

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.

Posted by an AI agent

I reviewed the public contract and API, correctness, tests, performance, naming, telemetry, resilience, documentation consistency, API surface and changes, and integration tests. All requested areas were covered.

Targeted Arty all-feature tests, lifecycle and telemetry tests, paired cargo-public-api captures, and Criterion base/head scheduling benchmarks ran. Allocation and instruction-count benchmarks were not run.

Public API changes

No API Changes

The complete base and head public API captures are identical.

Integration tests

Integration Test Additions Only

Added crates/arty/tests/seismograph.rs; no existing integration-test file, expectation, or execution path changed.

Comment thread crates/arty/src/runtime/dispatch/dispatcher_core.rs
Comment thread crates/arty/src/runtime/seismograph.rs
Comment thread crates/arty/tests/seismograph.rs
Comment thread crates/arty/src/runtime/seismograph.rs Outdated
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

⚠️ SemVer check advisory

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 cb3ef36dc0942dc93017df934a3fcacfc7f451ad
    Building metabench v0.1.1 (current)
error: running cargo-doc on crate 'metabench' failed with output:
-----
   Compiling proc-macro2 v1.0.107
   Compiling quote v1.0.47
   Compiling unicode-ident v1.0.26
   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 memchr v2.8.3
   Compiling syn v3.0.6
   Compiling syn v2.0.119
    Checking itoa v1.0.18
    Checking bitflags v2.13.2
   Compiling zerocopy v0.8.62
   Compiling semver v1.0.28
   Compiling shlex v2.0.1
   Compiling find-msvc-tools v0.1.14
   Compiling rustversion v1.0.23
   Compiling hashbrown v0.17.1
   Compiling winnow v1.0.4
   Compiling equivalent v1.0.2
   Compiling indexmap v2.14.2
   Compiling serde_derive v1.0.229
   Compiling cc v1.6.0
   Compiling toml_parser v1.1.5+spec-1.1.0
   Compiling rustc_version v0.4.1
   Compiling zerocopy-derive v0.8.62
   Compiling derive_more-impl v2.1.1
   Compiling toml_datetime v1.1.2+spec-1.1.0
   Compiling autocfg v1.5.1
   Compiling rustix v1.1.5
    Checking either-or-both v0.3.1
   Compiling toml_edit v0.25.17+spec-1.1.0
   Compiling num-traits v0.2.19
   Compiling alloca v0.4.0
   Compiling gungraun-macros v0.9.1
   Compiling proc-macro-error-attr3 v3.1.1
    Checking either v1.19.0
   Compiling thiserror v2.0.21
    Checking anstyle v1.0.14
    Checking ciborium-io v0.2.2
    Checking linux-raw-sys v0.12.1
   Compiling bincode-next v3.1.1
   Compiling cfg_aliases v0.2.2
    Checking clap_lex v1.1.1
   Compiling getrandom v0.4.3
    Checking regex-syntax v0.8.11
    Checking regex-automata v0.4.18
    Checking clap_builder v4.6.7
    Checking half v2.7.1
   Compiling nix v0.31.3
    Checking ciborium-ll v0.2.2
    Checking itertools v0.13.0
    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
   Compiling pastey v0.2.3
   Compiling gungraun v0.19.4
    Checking unty-next v0.1.2
    Checking same-file v1.0.6
    Checking cast v0.3.0
    Checking walkdir v2.5.0
    Checking criterion-plot v0.8.2
   Compiling metabench_macros_impl v0.1.1 (/home/runner/work/oxidizer/oxidizer/crates/metabench_macros_impl)
    Checking clap v4.6.7
    Checking ciborium v0.2.2
    Checking regex v1.13.1
    Checking gungraun-runner v0.20.0
    Checking gungraun-runner v0.19.4
    Checking folo_utils v0.1.14
    Checking tinytemplate v1.2.1
    Checking nix v0.27.1
    Checking page_size v0.6.0
   Compiling metabench v0.1.1 (/home/runner/work/oxidizer/oxidizer/crates/metabench)
    Checking jiff-core v0.1.1
    Checking once_cell v1.21.4
    Checking fastrand v2.5.0
    Checking anes v0.1.6
    Checking oorandom v11.1.5
    Checking criterion v0.8.2
    Checking command-group v5.0.1
    Checking jiff v0.2.38
    Checking tempfile v3.27.0
    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 App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: aec4830e-98ac-4377-aaca-62fb9ad22217
Copilot AI balanced review requested due to automatic review settings October 9, 2026 10:39

Copilot AI 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

Remote dispatch can record task lifecycle events out of order due to a send/worker race.

3 open findings

🧠 Review effort: Balanced

Comment thread crates/arty/src/runtime/dispatch/dispatcher_core.rs Outdated
Comment thread crates/arty/src/runtime/seismograph.rs Outdated

@martintmk martintmk left a comment

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.

Posted by an AI agent

I completed a fresh review of public API and contract behavior, correctness, tests, performance, naming, telemetry, resilience, documentation consistency, API reports, and integration-test changes. No review area was left unreviewed. Targeted unit, integration, and documentation tests and paired cargo public-api captures ran. Benchmarks were not run.

Public API changes

No API Changes

Base and head expose identical complete public surfaces.

Integration tests

Integration Test Additions Only

Added crates/arty/tests/seismograph.rs; no existing integration-test files or assertions were removed or weakened.

Comment thread crates/arty/src/task/execution/remote.rs
Comment thread crates/arty/src/runtime/seismograph.rs Outdated
Comment thread crates/arty/src/task/execution/preparation.rs
Comment thread crates/arty/tests/seismograph.rs
Comment thread crates/arty/src/documentation/telemetry.rs
Comment thread crates/arty/src/runtime/seismograph.rs Outdated
Comment thread crates/arty/src/runtime/seismograph.rs Outdated
notified,
release: Mutex::new(released),
}));
assert!(task.as_mut().poll(&mut Context::from_waker(&waker)).is_pending());

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.

🤖: This regression test can fail when the worker completes the immediately-ready task before the test's first poll. In that schedule, the join handle is already ready at line 134, so is_pending() is false and the required pipeline fails nondeterministically.

Please gate task completion until after this poll installs BlockingWake. For example, have the spawned future await an events_once::Event, poll the join handle, then release the event before waiting for wake_started.

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.

I did not change this test setup because this feedback came from a different human and no scoped code-change authorization was granted. The focused test has remained passing locally, but the suggested gating remains deferred; resolving with that disposition recorded.

Auto-replied by the GitHub Copilot app

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

Copilot-Session: aec4830e-98ac-4377-aaca-62fb9ad22217
Copilot AI balanced review requested due to automatic review settings October 9, 2026 11:51

Copilot AI 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

Remote lifecycle events can be reordered by a dispatch race, and scoped future descriptors can collide.

4 open findings

🧠 Review effort: Balanced

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

Copilot-Session: aec4830e-98ac-4377-aaca-62fb9ad22217
Copilot AI balanced review requested due to automatic review settings October 9, 2026 12:43

Copilot AI 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

The observed stopped event can be emitted before worker threads have exited or reported a panic.

1 open finding
4 resolved since last review

🧠 Review effort: Balanced

Comment on lines +63 to +64
if previous == 1 {
self.stopped();
let (future_factory, parent_task_enrichment, sink) = future_factory.into_parts();
let (future_factory, parent_task_enrichment, sink, task_telemetry) = future_factory.into_parts();
let task_telemetry = task_telemetry.expect("remote factories retain task telemetry");
task_telemetry.enqueued();

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.

🤖: Enqueue is now recorded only when the worker dequeues the factory, so Seismograph loses the scheduler queue-wait interval.

Record readiness when the task enters the worker queue, while preserving the guarantee that materialization cannot overtake it.

TaskTelemetryPlacement::enqueued() calls TaskHandle::woken(), which starts queued_since; calling it here immediately before materialized() leaves remotely spawned tasks in Waiting throughout any channel backlog, then reports near-zero Ready time. A backlogged runtime therefore reports no queued tasks and no wake-to-poll latency. Recording enqueue immediately before the send would preserve the ordering because the worker cannot receive the command first; a failed send can then retire the already-enqueued task as canceled.

@martintmk

Copy link
Copy Markdown
Member Author

The degradation of perf is a real concern, we need to double check whether we want to accept it. Do we really want to record tasks even if recording is not enabled?

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

Copilot-Session: aec4830e-98ac-4377-aaca-62fb9ad22217
Copilot AI balanced review requested due to automatic review settings October 9, 2026 14:59

Copilot AI 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 broad concurrent runtime-lifecycle instrumentation warrants final human review despite strong test coverage.

1 open finding

🧠 Review effort: Balanced

return descriptor.1;
}
let _suppression = SuppressionGuard::enter();
let descriptor = *TYPE_DESCRIPTORS

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.

🤖 Pato's Pull Request Agent: [perf · M-HOTPATH] The spawn path resolves a TypeDescriptor for every task via type_descriptor_id::<T>(), but the only fast path is the single-entry LAST_TYPE_DESCRIPTOR TLS cell. A worker that interleaves spawns of two or more distinct future types (e.g. a loop that spawns a timer future and a work future) misses the cell on every spawn and takes the process-global TYPE_DESCRIPTORS mutex each time, serializing the spawn hot path across all workers.

The same-type benchmarks in the PR description keep the TLS cell hot, so they would not surface this contention. Consider making the thread-local a small per-thread HashMap<TypeId, TypeDescriptorId> (or an N-entry cache) so steady-state heterogeneous spawning never reaches the shared lock; only the first sighting of a type per thread would.

return;
}
if let Some(telemetry) = &self.telemetry {
telemetry.parked();

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.

🤖: Idle workers now flood their bounded event logs with park/unpark events. AsyncWorker calls this wait with a 1 ms timeout, and every timeout records both WorkerParked and WorkerUnparked; with runtime-task recording enabled, an idle worker therefore emits about 2,000 events per second until useful task and poll history is overwritten.

Please report idleness without emitting a park/unpark pair for every polling timeout. Record transitions only when the worker genuinely enters or leaves an idle state, or otherwise coalesce consecutive idle cycles while keeping WorkerState accurate.

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

Copilot-Session: aec4830e-98ac-4377-aaca-62fb9ad22217
Copilot AI balanced review requested due to automatic review settings October 9, 2026 19:38

Copilot AI 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

Subsequent task wakeups bypass TaskHandle::woken, producing incorrect readiness state and wait metrics.

1 open finding
Previously missed (1)

In code that hasn't changed since last review

Medium severity Record telemetry for all task wakeups

crates/​arty/​src/​runtime/​seismograph.rs:227

TaskHandle::woken() is invoked only for the initial enqueue here; actual future wakeups go through arty_executor/src/wake.rs:188-212 and never reach this telemetry. After the first pending poll, seismograph_runtime/src/activity.rs:195-200 marks the task Waiting, so a later wake leaves snapshots in that state and omits both the subsequent TaskReady event and ready-wait metrics. Wire the task handle into the executor wake path (or wrap the poll waker) so every wake calls woken(), and assert the second ready transition in the existing self-waking integration scenario.

🧠 Review effort: Balanced

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.

6 participants