Skip to content

fix: strengthen Rust safety invariants - #757

Draft
martintmk wants to merge 4 commits into
mainfrom
user/martintomka/20260911-cargo-evaluate-all-crates
Draft

martintmk wants to merge 4 commits into
mainfrom
user/martintomka/20260911-cargo-evaluate-all-crates

Conversation

@martintmk

Copy link
Copy Markdown
Member

Summary

  • document and harden unsafe UTF-8, executor lifetime, and thread-safety invariants
  • replace an internal HTTP-body panic with the existing typed error channel
  • add regression coverage for multibyte formatting across the 128-byte stack-buffer boundary
  • preserve all public names, signatures, visibility, trait surfaces, and future output types

Cargo evaluate

Ran cargo evaluate across all 80 workspace crates. The final report contains 1,323 findings: 23 deny, 1,245 warn, and 55 suggestion. The remaining denials require prohibited public API changes, cover pre-existing test-fixture or unsafe-abstraction audits, or apply M365-only Tower/URI policy to this open-source compatibility surface. No cargo-evaluate report, JSON evidence, HTML output, or loop journal is committed.

Validation

The repository README, formatting, Clippy, unit tests, doctests with default and all features, build, rustdoc, Cargo sorting, spelling, licenses, cargo-deny, and SemVer checks completed successfully. Compile-based workspace checks excluded only the unchanged msvc_spectre_libs package because this machine does not have the Visual Studio Spectre-mitigated libraries installed.

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

Copilot-Session: c2cc5207-182e-48b4-ad85-48744b685e42
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c2cc5207-182e-48b4-ad85-48744b685e42
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c2cc5207-182e-48b4-ad85-48744b685e42
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c2cc5207-182e-48b4-ad85-48744b685e42
@martintmk martintmk added the agency-rocket Touched by a rocket skill label Sep 14, 2026
@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (704eba8) to head (516ca70).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff           @@
##            main     #757   +/-   ##
======================================
  Coverage   99.9%   100.0%           
======================================
  Files        634      634           
  Lines      84746    84758   +12     
======================================
+ Hits       84744    84758   +14     
+ Misses         2        0    -2     
Flag Coverage Δ
linux 88.2% <100.0%> (?)
linux-arm 88.3% <100.0%> (?)
windows 88.6% <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.

@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

The review found one unsafe-contract defect, two non-blocking contract inconsistencies, and one diagnostic-message nit. All ten Review Lens areas completed at the pinned head; paired public API captures for all seven affected crates were unchanged, and cargo check passed for every changed crate.

Because this is the author's own PR, this review is COMMENT-only and casts no vote.


Posted by an AI agent

WakeSignal::waker reverses the unsafe drop condition

Problem
is_inert() returns true only when no wakers remain and the signal is safe to drop, but this safety contract says the owner must keep the signal alive until it returns false. The actual owner correctly waits for true.

Why this matters
A caller following this unsafe API documentation could drop the storage while raw wakers still reference it, allowing use-after-free and undefined behavior.

Suggested fix
Change false to true so the documented precondition matches is_inert() and the existing call sites.

let body = result?;
body.into_bytes_no_buffering()
.unwrap_or_else(|| unreachable!("once body is buffered, it must be a view over a byte sequence"))
.ok_or_else(|| HttpError::validation("buffered body did not contain an in-memory byte sequence"))

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 · Non-blocking

HttpBody::into_bytes classifies an internal invariant failure as validation

Problem
The new fallback uses HttpError::validation, even though failure after a successful into_buffered() means the library violated its own representation invariant; the caller supplied no invalid input.

Why this matters
The standard validation label feeds metrics and logging, so an implementation regression would be attributed to caller input and routed to the wrong owner.

Suggested fix
Return an internal/other error with a stable dedicated label and RecoveryInfo::never() instead of the validation classification.

/// This is returned by [`Spawner::spawn`](crate::Spawner::spawn) and implements
/// [`Future`] to allow awaiting the task's completion.
///
/// A child panic, runtime cancellation, or early result-channel close is a hard failure.

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 · Non-blocking

Spawner methods under-document the expanded JoinHandle failure contract

Problem
This type now explicitly documents runtime cancellation and early result-channel closure as hard failures, but Spawner::spawn and spawn_blocking still mention only child panic, while spawn_anywhere has no matching panic section.

Why this matters
Consumers commonly learn the returned handle's behavior from its construction method and may incorrectly assume cancellation or channel closure is distinguishable or non-panicking.

Suggested fix
Synchronize all three spawner methods with this complete panic contract, preferably by linking back to JoinHandle to prevent future drift.

debug_assert!(self.is_inert());
debug_assert!(
self.is_inert(),
"WakeSignal dropped while wakers are still outstanding (waker_count > 0)"

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 · Nit

WakeSignal assertion message violates the lowercase convention

Problem
The new diagnostic begins with uppercase WakeSignal, while this repository requires panic and assertion messages to start lowercase.

Why this matters
Keeping diagnostics consistent makes panic output and message review predictable across the workspace.

Suggested fix
Use wake signal dropped while wakers are still outstanding (waker_count > 0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agency-rocket Touched by a rocket skill

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants