Skip to content

feat(seismograph): introduce runtime recording - #733

Open
Ralf Biedert (ralfbiedert) wants to merge 45 commits into
microsoft:mainfrom
ralfbiedert:u/ralfbiedert/seismograph-recording
Open

feat(seismograph): introduce runtime recording#733
Ralf Biedert (ralfbiedert) wants to merge 45 commits into
microsoft:mainfrom
ralfbiedert:u/ralfbiedert/seismograph-recording

Conversation

@ralfbiedert

Copy link
Copy Markdown
Collaborator

Summary

  • introduce Seismograph as the shared high-performance event recorder, thread-local ring-buffer, snapshot, and extensible source-data system
  • integrate allocator, runtime, I/O, synchronization, and event-only Cachet telemetry with independently gated recording policies
  • add monitor protocol and CLI controls while preserving legacy fixed-size message compatibility
  • consolidate allocation hints and allocator telemetry, add performables instrumentation, and migrate the rebased implementation to the current thread-aware APIs

Validation

  • just format
  • cargo clippy --workspace --no-deps --all-targets -- -D warnings
  • cargo test
  • cargo build
  • cargo build --release
  • just readme
  • just spellcheck

Copilot AI lite review requested due to automatic review settings September 4, 2026 17:23

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

There are a couple of concrete correctness/maintainability issues in changed tests (e.g., a Linux HAL test still references a removed stack-capture helper, and a parse-error test now asserts an overly brittle full error string).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces Seismograph as a shared runtime telemetry/event recording system (thread-local ring buffers + snapshots + extensible per-source payloads), and migrates allocator/runtime/I/O/synchronization telemetry to plug into that snapshot/monitoring model, including a new unified CLI.

Changes:

  • Add new Seismograph crates (core recorder, protocol, runtime + rallocator + I/O sources) and a new seismograph CLI for snapshot HTML + monitoring.
  • Migrate rallocator and related tooling from the old rallocator_* telemetry/wire/CLI crates to seismograph_* equivalents and update tests/benches/examples accordingly.
  • Introduce Performables telemetry integration (channels/sync/Arc events) and wire Cachet telemetry to Seismograph, plus update formatting/coverage scripts.
File summaries
File Description
README.md Updates top-level crate listing to Seismograph family and revised rallocator description.
justfiles/format.just Avoids cargo fmt --all Windows CLI limit by formatting per-package.
justfiles/coverage.just Narrows llvm-cov nextest target selection (--lib --bins --tests --examples).
crates/uniflight/README.md Regenerated README content.
crates/tick/src/fmt/iso_8601.rs Tightens parse error assertion in tests.
crates/tick/README.md Regenerated README content.
crates/thread_aware/README.md Regenerated README content.
crates/thread_aware_core/README.md Regenerated README content.
crates/testing_aids/src/lib.rs Switches test watchdog channels to Performables channels.
crates/testing_aids/Cargo.toml Adds Performables dependency for test utilities.
crates/templated_uri/README.md Regenerated README content and updated http link version.
crates/seismograph/src/recorder/thread.rs Adds thread identity + thread log model types for recorder snapshots.
crates/seismograph/src/recorder/runtime.rs Adds runtime identity types and RuntimeEvent fixed context payload.
crates/seismograph/src/recorder/alloc.rs Adds allocation event model types and heap classification.
crates/seismograph/README.md New crate README describing recorder + snapshot + monitor usage.
crates/seismograph/Cargo.toml New seismograph crate manifest with monitor feature.
crates/seismograph_runtime/src/task.rs Adds runtime task lifecycle instrumentation (wake/poll timing counters + events).
crates/seismograph_runtime/README.md New crate README for runtime snapshot source and compatibility contract.
crates/seismograph_runtime/Cargo.toml New seismograph_runtime crate manifest.
crates/seismograph_rallocator/src/wire/mod.rs Makes wire primitives private to crate and adds container tests module.
crates/seismograph_rallocator/src/wire/format.rs Moves/privatizes container framing types; uses snapshot Version.
crates/seismograph_rallocator/src/wire/container_tests.rs Updates tests to target private wire module types.
crates/seismograph_rallocator/src/topology.rs Updates module docs to reflect rallocator-specific topology model.
crates/seismograph_rallocator/src/snapshot.rs Introduces local Version type and adds optional runtime events to snapshot.
crates/seismograph_rallocator/README.md Renames/reframes crate as Seismograph rallocator source; updates links.
crates/seismograph_rallocator/Cargo.toml Renames crate/package and switches dependencies to seismograph + backtrace.
crates/seismograph_protocol/src/codec.rs Adds protocol codec helpers (little-endian primitives, hex, reader).
crates/seismograph_protocol/README.md New/updated README for seismograph protocol crate.
crates/seismograph_protocol/Cargo.toml Renames package to seismograph_protocol and adds unix libc dependency.
crates/seismograph_io/README.md New crate README for low-overhead I/O event instrumentation.
crates/seismograph_io/Cargo.toml New seismograph_io crate manifest.
crates/seismograph_cli/tests/support/mod.rs Migrates CLI test support from rallocator telemetry to seismograph rallocator.
crates/seismograph_cli/tests/render_migration.rs Updates render migration fixtures to new crate names.
crates/seismograph_cli/tests/render_html.rs Updates HTML rendering tests to new crate names.
crates/seismograph_cli/tests/cli.rs Renames CLI binary and adds a native seismograph capture read test.
crates/seismograph_cli/src/main.rs New seismograph CLI entrypoint with monitor + snapshot subcommands.
crates/seismograph_cli/src/commands/snapshot/mod.rs Adds snapshot command module scaffold.
crates/seismograph_cli/src/commands/snapshot/html.rs Adds decoding that supports both native seismograph captures and legacy allocator-only snapshots.
crates/seismograph_cli/src/commands/mod.rs Adds monitor command module and wires snapshot module.
crates/seismograph_cli/README.md New crate README for seismograph CLI.
crates/seismograph_cli/Cargo.toml Renames package/bin, adds monitor UI deps and seismograph-family deps.
crates/seatbelt/README.md Regenerated README content.
crates/seatbelt_http/README.md Regenerated README content.
crates/routerama/README.md Regenerated README content.
crates/routerama_build/README.md Regenerated README content.
crates/recoverable/README.md Regenerated README content.
crates/rallocator/tests/tls_teardown.rs Removes explicit allocator initialization call.
crates/rallocator/tests/support/mod.rs Adds snapshot reader to extract rallocator stats via seismograph snapshot source.
crates/rallocator/tests/seismograph.rs Adds integration test asserting seismograph snapshot contains rallocator source.
crates/rallocator/tests/runtime_snapshot.rs Adds regression test for runtime snapshot symbol-cache lifetime behavior.
crates/rallocator/tests/performables_telemetry.rs Adds test ensuring performables telemetry emits seismograph events.
crates/rallocator/tests/no_global_allocator.rs Adds test for passive allocation hints without rallocator as global allocator.
crates/rallocator/tests/multithreaded.rs Refactors to use new stats helper and unified allocator macro; removes explicit initialize.
crates/rallocator/tests/macro_configuration.rs Adds test for unified macro configuration and tunables.
crates/rallocator/tests/initialization.rs Updates tests to reflect hints working without explicit initialization.
crates/rallocator/src/tuning_telemetry.rs Updates import paths and switches to shared test lock.
crates/rallocator/src/telemetry/stats.rs Removes public re-export module (internalized).
crates/rallocator/src/telemetry/snapshot.rs Removes public re-export module (internalized).
crates/rallocator/src/telemetry/mod.rs Reframes telemetry module as internal snapshot-source support.
crates/rallocator/src/heap/general.rs Adds general-purpose heap options and invariants tests.
crates/rallocator/src/heap/bump/mod.rs Introduces bump heap API module and re-exports options.
crates/rallocator/src/heap/bump/api.rs Makes bump heap options internal and adds default/options tests.
crates/rallocator/src/hal/win64.rs Removes stack capture implementation and Debug API feature usage.
crates/rallocator/src/hal/native.rs Removes peek_free_requested shim.
crates/rallocator/src/hal/mod.rs Updates platform exports to remove capture_stack/peek_free_requested for non-miri.
crates/rallocator/src/hal/miri.rs Removes capture_stack and peek_free_requested for miri.
crates/rallocator/src/hal/linux.rs Removes stack capture implementation but retains a test referencing it (see comments).
crates/rallocator/src/domain/mod.rs Refactors domain handling; introduces test-only Domain wrapper and retains default domain state.
crates/rallocator/examples/scoped_bump_heap.rs Updates example to passive allocation_hints heap APIs and removes explicit initialize/usage print.
crates/rallocator/examples/allocation_tracking.rs Switches tracking from old telemetry to Seismograph recording + new snapshot filenames.
crates/rallocator/Cargo.toml Updates deps to seismograph family, adds dev deps, and renames benches.
crates/rallocator/benches/tracking_none.rs Removes old bench.
crates/rallocator/benches/tracking_callers_capable.rs Removes old bench.
crates/rallocator/benches/tracking_all.rs Removes old bench.
crates/rallocator/benches/tracking_all_callers.rs Updates bench to use seismograph recording for allocation backtraces.
crates/rallocator/benches/tracking_aggregates.rs Updates bench to new macro config + revised description.
crates/rallocator/benches/rallocator.rs Removes explicit initialize call.
crates/rallocator/benches/rallocator_tracking_events.rs Adds bench for event recording without backtraces.
crates/rallocator/benches/rallocator_tracking_events_1_in_20.rs Adds sampled event bench (1/20).
crates/rallocator/benches/rallocator_tracking_events_1_in_100.rs Adds sampled event bench (1/100).
crates/rallocator/benches/rallocator_threaded.rs Removes explicit initialize call.
crates/rallocator/benches/bump_allocation_hints.rs Updates bump workloads bench to passive hint heaps + renamed identifier.
crates/rallocator_wire/README.md Removes README for old wire crate (superseded by seismograph_protocol/private wire).
crates/rallocator_cli/src/main.rs Removes old CLI entrypoint (superseded by seismograph CLI).
crates/plurality/README.md Regenerated README content.
crates/performables/tests/support/waker.rs Adds RawWaker-based helper for tests.
crates/performables/tests/support/serializer.rs Adds minimal serializer for tests.
crates/performables/src/telemetry.rs Adds seismograph-backed telemetry recording mapping for sync/Arc events.
crates/performables/src/sync/mod.rs Adds sync module surface and poison error type.
crates/performables/src/lib.rs New crate root docs and module exports.
crates/performables/README.md New crate README.
crates/performables/Cargo.toml New performables crate manifest and benches.
crates/performables/benches/performables_uncontended.rs Adds uncontended ownership/lock benchmarks vs std.
crates/performables/benches/performables_telemetry_sampling.rs Adds telemetry sampling overhead benchmark for Arc deref.
crates/ohno/README.md Regenerated README content and updated macro link versions.
crates/observed/README.md Regenerated README content.
crates/observed_utils/README.md Regenerated README content.
crates/multitude/src/arena/mod.rs Adds let _ = self; to avoid unused self in cfg-gated stats block.
crates/msvc_spectre_libs_build/README.md Regenerated README content.
crates/layered/README.md Regenerated README content.
crates/internity/README.md Regenerated README content and updated serde link.
crates/http_path_template/README.md Regenerated README content.
crates/http_extensions/README.md Regenerated README content and updated http/bytes links.
crates/fetch/README.md Regenerated README content and updated bytes links.
crates/fetch_tls/README.md Regenerated README content.
crates/fetch_hyper/README.md Regenerated README content.
crates/fetch_azure/README.md Regenerated README content and updated azure_core links.
crates/data_privacy/README.md Regenerated README content.
crates/data_privacy_core/README.md Regenerated README content.
crates/cachet/src/telemetry/seismograph.rs Adds Seismograph-backed Cachet telemetry events + gating test.
crates/cachet/src/telemetry/mod.rs Wires seismograph telemetry module behind feature/test cfg.
crates/cachet/src/refresh.rs Records suppressed refresh events and distinguishes fallback errors.
crates/cachet/src/fallback.rs Adds promotion outcome telemetry on fallback-to-primary insert.
crates/cachet/src/cache.rs Adds compute success/failure/none telemetry in cache miss paths.
crates/cachet/README.md Regenerated README content.
crates/cachet/Cargo.toml Adds optional seismograph feature + dependency and test dep path.
crates/cachet_tier/README.md Regenerated README content.
crates/cachet_service/README.md Regenerated README content.
crates/cachet_memory/README.md Regenerated README content.
crates/bytesbuf/src/view.rs Adds lazy buffer identity tracking + seismograph I/O buffer trait implementation.
crates/bytesbuf/src/telemetry.rs Adds buffer identity implementation with seismograph/no-seismograph variants.
crates/bytesbuf/src/lib.rs Documents new seismograph feature and adds telemetry module.
crates/bytesbuf/src/buf.rs Adds lazy buffer identity tracking + seismograph I/O buffer trait implementation.
crates/bytesbuf/README.md Documents new seismograph feature.
crates/bytesbuf/Cargo.toml Adds optional seismograph_io dependency and seismograph feature.
crates/bytesbuf_io/README.md Regenerated README content.
crates/automation/src/process.rs Switches timeout wait channel from std mpsc to Performables channel with richer error checks.
crates/automation/Cargo.toml Adds Performables dependency.
crates/arty_executor/README.md Regenerated README content.
crates/anyspawn/src/handle.rs Switches join handle oneshot receiver to Performables oneshot receiver.
crates/anyspawn/src/custom.rs Switches custom spawner oneshot channel to Performables channel.
crates/anyspawn/README.md Regenerated README content.
crates/anyspawn/Cargo.toml Replaces futures-channel dependency with Performables.
crates/anyspawn_azure/README.md Regenerated README content and updated azure_core links.
crates/allocation_hints/src/domain.rs Removes domain backend API (passive hinting model no longer owns a backend).
crates/allocation_hints/README.md Updates docs to passive hint model and new heaps APIs/examples.
crates/allocation_hints/Cargo.toml Updates description/links and adds dependencies for passive TLS model.
crates/allocation_hints/build.rs Updates build script docs to passive hint TLS resolution.
Cargo.toml Adds workspace deps for new seismograph/performables crates and TUI/monitor deps; removes old rallocator_* crates.
.spelling Adds new words for spellcheck allowlist.
Review details
  • Files reviewed: 134/198 changed files
  • Comments generated: 1
  • Review effort level: Lite

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

Comment thread crates/rallocator/src/hal/linux.rs Outdated

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.

Static review of the seismograph recording change

Five passes — security, correctness, testing, performance, conformance — over the 198-file diff, scoped by subsystem. Nothing was executed: no build, tests, clippy, Miri, loom, coverage or benchmarks, so every claim below is from reading the code, and no number is cited that is not already in the tree.

Headline. Two findings stand out. capture_stack() on the allocation record path (recorder.rs:495) runs without a SuppressionGuard, while the sibling capture_backtrace() deliberately takes one — and nothing else on the alloc -> begin_allocation -> commit -> record chain establishes suppression, so glibc's allocating backtrace() can re-enter the recording allocator. Separately, unsafe impl<T: Sync, F: Send> Sync for LazyLock (once.rs:157) drops the T: Send that its OnceLock<T> field requires, which safe external code can reach.

On the description's claims. "Preserving legacy fixed-size message compatibility" holds at the size level — the 34/50/82-byte blocks are stable and asserted — but it is preserved by not encoding RecordingConfiguration::cache, which decode_recording then silently reconstructs as the default. So the size is compatible and the data is lost. The validation list also reports cargo test and cargo build passing, which cannot hold on Linux: hal/linux.rs:127 calls capture_stack, which linux.rs does not define and hal/mod.rs:20 re-exports only under cfg(miri). That is the point Copilot already raised on that line, and it appears to be a genuine build break rather than a stale comment.

What I attacked and could not break. The custom Arc does not reimplement reference counting — it delegates to std::sync::Arc (from_raw/increment_strong_count/decrement_strong_count), so those orderings are std's; there are no manual Send/Sync impls in arc/. The recorder's retain-snapshot path is mutually excluded by the per-slot spinlock, and the SeqCst writer_active/ACTIVE_SESSION handshake correctly prevents a destructive snapshot freeing a ring under a live writer. Mutex/RwLock acquire/release orderings are correct and their unsafe impl bounds match std. Seismograph's own recording path genuinely avoids the global allocator (System.alloc, hal::map, fixed stack arrays) — the reentrancy above comes from libc, not from this code. The report.rs move out of rallocator_cli changed no reporting behavior: the numeric machinery is byte-identical and only import paths and labels differ. The monitor's token comparison is constant-time, and I/O recording captures buffer metadata only, never contents.

Below the budget, not raised inline. The per-runtime Mutex<Vec<Arc<TaskControl>>> is taken on every task spawn and retire and is also held across snapshot cloning (seismograph_runtime/src/lib.rs:274, :631, :561) — worth a look, but I could not bound the hold time statically. crates/rallocator/tests/seismograph.rs:22 asserts only allocations != 0, which a badly miscounting allocator would survive. crates/seismograph/Cargo.toml declares getrandom and seismograph_protocol optional with no [dev-dependencies], against the rule in AGENTS.md; it is currently latent because the consumers sit behind cfg(feature = "monitor") alone, but recorder.rs already uses cfg(any(test, feature = "monitor")) nearby.

Open question. record_allocation and record_deallocation_stats both early-return under telemetry_suppressed() (telemetry/core.rs:1077, :1084). If a non-arena allocation made before a capture is freed by the capturing thread inside the suppressed region, live_bytes drifts upward permanently. I could not construct a trigger — every free I traced inside with_telemetry_suppressed is an arena allocation short-circuited earlier in dealloc. Auditing the Drops reachable from try_snapshot_with_runtime_events would settle it.

Comment thread crates/seismograph/src/recorder.rs
Comment thread crates/performables/src/sync/once.rs Outdated
Comment thread crates/seismograph_protocol/src/message.rs
Comment thread crates/performables/src/sync/condition.rs Outdated
Comment thread crates/performables/tests/sync.rs
Comment thread crates/seismograph/src/snapshot.rs
Comment thread crates/seismograph_protocol/src/message.rs
Comment thread crates/rallocator/src/telemetry/core.rs
Comment thread crates/seismograph_cli/src/commands/monitor/app.rs Outdated
Comment thread crates/rallocator/src/allocator.rs
Copilot AI review requested due to automatic review settings September 4, 2026 18:38
@ralfbiedert

Copy link
Copy Markdown
Collaborator Author

🤖 Addressed the initial review and CI findings in 510c97c. Please re-review the updated changes.

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

There is a confirmed buffer-identity handling bug in BytesView::append that can break Seismograph I/O correlation (and a small CLI docs mismatch).

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/bytesbuf/src/view.rs:817

  • BytesView::append clears the telemetry identity unconditionally. This changes the buffer identity even when other is empty, and it also discards other's identity when appending into an empty self (even though the resulting view is exactly other). This can break seismograph_io correlation for logically unchanged / moved data.
  • Files reviewed: 139/202 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@martintmk
martintmk self-requested a review September 7, 2026 06:17
Comment thread crates/performables/src/arc/mod.rs
Comment thread crates/performables/src/arc/mod.rs
Copilot AI review requested due to automatic review settings September 7, 2026 10:44

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 CLI snapshot decoding path can misreport native .seismograph decode failures as legacy allocator decode errors, and a new test helper manually parses snapshot bytes via brittle fixed offsets instead of using typed decode APIs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 139/202 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread crates/rallocator/tests/support/mod.rs
Comment thread crates/seismograph_cli/src/commands/snapshot/html.rs

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@martintmk martintmk 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.

[AI AGENT]: I reviewed the new public surfaces in performables, the Seismograph crate family and protocol, the allocator migration, feature/dependency wiring, and the removed rallocator_wire surface. I also traced the synchronization, decoding, recording/session, task/worker, monitor, I/O, allocator, telemetry, and changed test paths, including the 1,419-line update through current head 2b064318. Focused debug/release, downstream, concurrency-order, and allocation-growth probes reproduced the findings below. Current-head CI has not yet posted its normal test matrix, so I did not treat the earlier head's results as validation of this commit.

Beyond the two current unresolved threads, this review found eleven blocking issues and three non-blocking diagnostics/telemetry issues. The highest-impact results are affinity-lock poisoning after user panics, malformed-input decoder panics/wraparound, 90-second growth for a legitimate 64 MiB frame, cross-thread I/O state reversal, a self-incompatible runtime encoder, and unbound/drop-unsafe task poll tokens.

Verdict: changes requested.

Comment thread crates/performables/src/arc/mod.rs Outdated
Comment thread crates/seismograph/src/snapshot.rs Outdated
Comment thread crates/seismograph_protocol/src/lib.rs
Comment thread crates/seismograph_rallocator/src/lib.rs
Comment thread crates/seismograph_cli/src/commands/monitor/data.rs
Comment thread crates/rallocator/src/telemetry/core.rs
Comment thread crates/rallocator/src/heap/bump/state.rs
Comment thread crates/seismograph_io/src/lib.rs Outdated
Comment thread crates/seismograph/src/monitor.rs Outdated
Comment thread crates/cachet/src/telemetry/cache.rs Outdated
Add class-gated event recording, thread-local buffers, snapshots, and extensible source data for allocator, runtime, I/O, and cache telemetry.

Integrate monitoring and CLI controls, passive allocation hints, performables telemetry, and the current thread-aware APIs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove stale Linux and Miri HAL references, restore an acyclic test utility graph, stabilize telemetry tests, and satisfy cross-platform static checks.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 7, 2026 12:44
@ralfbiedert
Ralf Biedert (ralfbiedert) force-pushed the u/ralfbiedert/seismograph-recording branch from 2b06431 to a0ecbcf Compare September 7, 2026 12:44
@ralfbiedert

Copy link
Copy Markdown
Collaborator Author

🤖 Completed the Arc consolidation and rebased this PR onto the latest origin/main.

  • Removed thread_aware::Arc, its strategies/storage implementation, obsolete tests, and design docs.
  • Migrated all workspace consumers and relocation benchmarks to performables::arc::Arc (PerCore, PerNuma, or PerProcess).
  • Made every Performables Arc strategy implement ThreadAware; PerProcess relocation is a no-op.
  • Updated Uniflight's public strategy API and regenerated affected crate READMEs/API docs.
  • Coverage gates report 100% for performables, thread_aware, and uniflight in both all-features and no-default-features configurations.
  • The clean stable-toolchain format, Clippy, test, debug-build, release-build, and spellcheck sequence passes.

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

Copy link
Copy Markdown
Collaborator Author

🤖 Pushed b743270c for the remaining ARM Miri failures. The substantive fix initializes each snapshot arena's parent before publishing its raw TLS pointer, preserving pointer provenance under Stacked Borrows. The repeated Rallocator/Seismograph UB failures now pass under the pinned Miri nightly.

Tests that inherently require filesystem, sockets, real cross-thread identity, or scheduler timing are now Miri-ignored while remaining enabled in native CI. Targeted Ubuntu WSL Miri reproductions for all ten failed artifacts pass or report the intended ignore. Copilot review

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

It introduces and wires a new cross-cutting telemetry subsystem across many crates (allocator, runtime, sync primitives, CLI/tooling), making holistic correctness and compatibility verification beyond an automated review’s safe scope.

Review details
  • Files reviewed: 149/243 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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

Copy link
Copy Markdown
Collaborator Author

🤖 Pushed 9acbbc04 for the final two ARM Miri failures. The live monitor socket test and the UI test that formats local wall-clock time now remain covered on native targets but are skipped under isolated Miri, which does not support sockets or realtime clock_gettime. Both exact Miri reproductions pass with the intended ignores. Copilot review

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

It introduces and wires a new cross-cutting telemetry stack across many crates and workflows, so it warrants final human review despite only minor localized issues found.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/performables/tests/support/waker.rs:26

  • This test helper uses expect(...) when locking the mutex. In test/support code, prefer unwrap() so failures rely on backtraces rather than custom messages (and to keep panic messages consistent across the workspace).
  • Files reviewed: 149/243 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@ralfbiedert

Copy link
Copy Markdown
Collaborator Author

🤖 Reviewed the suppressed suggestion to replace the test-support mutex expect() with unwrap(). This repository enforces clippy::unwrap_used for integration-test targets, and the suggested change fails the required clippy gate. The existing invariant-specific expect() is therefore retained; no code change is needed.

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

Copy link
Copy Markdown
Collaborator Author

🤖 Pushed f7ce4f79 for the latest Miri failures. The Anyspawn cancellation regression now uses a Tokio current-thread runtime without enabling the Windows I/O driver, preserving Miri coverage while avoiding unsupported CreateIoCompletionPort. The Seismograph monitor directory-permission test and CLI panel edge-state render test remain covered natively but are skipped under isolated Miri because they require filesystem creation and realtime local-clock access.

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 PR spans multiple new crates and broad cross-cutting telemetry/runtime/allocator migrations, so it needs careful human validation of feature-gating and compatibility implications.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

crates/cachet_memory/Cargo.toml:35

  • performables has default = ["seismograph"] (crates/performables/Cargo.toml), so adding it without default-features = false implicitly pulls Seismograph into cachet_memory’s normal dependency graph. If cachet_memory only needs the thread-aware Arc strategies and not telemetry, disable default features here and opt in explicitly where required.
    crates/observed/Cargo.toml:42
  • performables enables seismograph by default (crates/performables/Cargo.toml). Depending on it here without default-features = false makes Seismograph an implicit transitive dependency of observed, even though this crate’s usage in the diff is just performables::arc::Arc. If that’s not intended, disable default features and only opt into telemetry where needed.
  • Files reviewed: 149/243 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@ralfbiedert

Copy link
Copy Markdown
Collaborator Author

🤖 Reviewed the two suppressed dependency suggestions for cachet_memory and observed. Their performables default features are intentional: this PR migrates these crates from thread_aware::Arc to performables::arc::Arc specifically so their Arc operations participate in Seismograph telemetry. Adding default-features = false would silently remove the recording behavior the migration is meant to introduce, so no change is appropriate.

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

Copy link
Copy Markdown
Collaborator Author

🤖 Pushed 617ac31f for the latest Linux ARM Miri failure. The contention test no longer erases the process-lifetime recorder pointer to an integer and reconstructs a provenance-free reference on another thread. It now shares the original reference through std::thread::scope, preserving provenance while exercising the same writer-quiescence spin path.

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 change set is large and cross-cutting (new telemetry stack + widespread dependency and behavior migrations), so it warrants final human review for system-level correctness and compatibility.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/performables/tests/support/waker.rs:25

  • This is test-only support code; per the repo guidelines, prefer unwrap() over expect() in tests (the backtrace is sufficient) to avoid carrying a custom message that can drift from actual failure context.
  • Files reviewed: 149/243 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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

Copy link
Copy Markdown
Collaborator Author

🤖 Pushed 83d316d8 for the latest Linux ARM Miri failure. The snapshot-arena test still exercises deterministic oversized-layout rejection under Miri, but leaves the deliberate 1 << 60 system-allocation failure assertion to native tests because Miri treats simulated resource exhaustion as a fatal interpreter error rather than returning null.

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

It introduces a couple of concrete API/documentation and dependency-surface issues (notably ThreadLog::name semantics and anyspawn enabling Seismograph in performables without usage) that should be resolved before approval.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/anyspawn/Cargo.toml:38

  • anyspawn enables performables with the seismograph feature, but this crate does not reference Seismograph anywhere. Pulling in Seismograph telemetry by default increases dependency surface and build cost for consumers that only need the spawner/oneshot functionality.
  • Files reviewed: 149/243 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@ralfbiedert

Copy link
Copy Markdown
Collaborator Author

🤖 Reviewed the suppressed anyspawn dependency suggestion. The Seismograph feature is intentional: Anyspawn was migrated from thread_aware::Arc and futures_channel::oneshot to Performables' Arc and oneshot channel so those primitive operations participate in Seismograph telemetry. Disabling the feature would remove the recording behavior this migration introduces. The review headline also mentions ThreadLog::name, but the review contains no corresponding comment or actionable detail, and no new inline comment was posted.

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

Copy link
Copy Markdown
Collaborator Author

🤖 Pushed cfc3cd98 for the latest Linux ARM Miri failure. The source/snapshot storage assertions remain active under Miri, while only the filesystem persistence portion is compiled for native tests because isolated Miri does not support open. This preserves useful Miri coverage instead of ignoring the combined test.

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 change set is extremely broad (new telemetry stack + migrations across many crates) and includes API-surface shifts that merit final human review for compatibility and operational impact.

Review details
  • Files reviewed: 149/243 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/rallocator/src/telemetry/mod.rs
Comment thread crates/anyspawn/Cargo.toml Outdated

[dependencies]
futures-channel = { workspace = true, features = ["alloc"] }
performables = { workspace = true, features = ["seismograph"] }

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.

should seismograph feature by enabled by default? Isn't this something that every crate should expose as optional feature and the cascade it across whole tree?

(I don't want to pay "seismograph" cost for default)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🤖 Addressed in 69eb1893. performables now has no default features; anyspawn, cachet_memory, observed, tick, fetch, and uniflight expose opt-in seismograph features, and Cachet cascades its existing feature across the relevant dependency tree. Default consumers no longer pay the Seismograph cost.

Comment thread justfiles/format.just

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.

Add changes under anvil folder should be made directly to anvil otherwise these will disappear with the next anvil upgrade.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🤖 Addressed upstream in microsoft/ox-tools#168 and integrated in 69eb1893. The generated package-selector fixes now have an Anvil source-of-truth, including full Cargo package-ID normalization and the short Windows LLVM coverage target path.

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

Copy link
Copy Markdown
Collaborator Author

🤖 Pushed 69eb1893 with the current CI and review fixes: Seismograph integrations are opt-in and cascade explicitly; nine Loom models cover mutex, RwLock, Condvar, reusable barrier, channel send/close registration, and once publication; recorder snapshot quiescence now closes the writer-entry race; Windows Miri environment/provenance and -D warnings failures are fixed; and the remaining Anvil package-ID coverage normalization is applied with upstream source in microsoft/ox-tools#168. Affected-package Clippy, tests, debug/release builds, exact rallocator Miri, native rallocator tests, API docs, spelling, README generation, and the final nine-model Loom suite pass.

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 change set is large and cross-cutting (new telemetry subsystem plus widespread migrations), making it difficult to confidently validate behavioral compatibility through diff review alone.

Review details
  • Files reviewed: 150/244 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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

Copy link
Copy Markdown
Collaborator Author

🤖 Pushed 30a1221f after integrating Martin's ox-tools review. Oxidizer's generated consumers now rely on Anvil's documented portable name@version impact-cache contract instead of parsing opaque Cargo package IDs. The Windows LLVM coverage workaround remains, using ignored target/.c; the matching source-of-truth is in microsoft/ox-tools#168.

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 changes span many crates and cross-cut core concurrency/telemetry surfaces, so a final human review is needed to validate the overall integration and compatibility impact.

Review details
  • Files reviewed: 150/240 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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.

4 participants