refactor(tick): align API with Rust guidelines by martintmk · Pull Request #632 · microsoft/oxidizer

martintmk · 2026-08-03T07:19:28Z

Overview

Aligns the tick public API with the Rust API Guidelines and Microsoft Rust Guidelines. The change makes time conversion failures explicit, replaces mutable test-clock configuration with a builder, strengthens error and future contracts, and fixes timer waker handling so callbacks run outside internal locks.

Workspace consumers are migrated to the revised API to keep behavior consistent. This is justified by clearer failure semantics, a smaller and more predictable public surface, feature-safe documentation, and correct async task wake behavior.

Validation

Every tick feature combination compiles
192 unit tests and 62 all-feature doctests pass
Strict Clippy, rustdoc warnings, formatting, generated README, and spelling checks pass
Nightly line coverage reaches 100% across all-feature and no-default-feature configurations
Mutation testing reports no surviving mutants or timeouts
Independent Opus and Sonnet reviews found no significant issues
fetch, fetch_hyper, http_extensions, seatbelt, and seatbelt_http compile with all features and targets

Make fallible conversions explicit, replace mutable clock configuration with a builder, strengthen error and timer contracts, and migrate workspace consumers to the revised API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d077867b-9f23-43df-86db-c3efd05948f4

Copilot

Pull request overview

This PR refactors the tick crate’s public API to better align with Rust/Microsoft API guidelines, making time conversion failures explicit, introducing a ClockControlBuilder, improving timeout/error classification, and adjusting timer wake behavior so wakers can be invoked outside internal locks. It also updates downstream crates/tests/examples to use the revised APIs.

Changes:

Replace infallible/panicking system_time_as conversions with fallible try_system_time_as on Clock/SimpleClock, and migrate usages.
Rework timer advancement/waker handling to collect ready wakers and wake outside locks; add “latest waker wins” behavior for Delay/PeriodicTimer.
Introduce ClockControlBuilder (plus ClockControl::new_auto_advancing) and extend tick::Error with timeout classification and backtrace capture.

Reviewed changes

Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/tick/src/timers.rs	Adds waker update support and changes timer advancement to collect ready wakers for out-of-lock wakeups.
crates/tick/src/timeout.rs	Simplifies `Timeout` to use `Delay` directly and returns a typed timeout error.
crates/tick/src/stopwatch.rs	Makes `Stopwatch` clonable and replaces `From` conversions with an explicit `start()` accessor.
crates/tick/src/state.rs	Wakes timer wakers after releasing synchronization guards.
crates/tick/src/simple_clock.rs	Adds docsrs feature annotations and introduces fallible `try_system_time_as`.
crates/tick/src/runtime/mod.rs	Inlines runtime re-exports in docs.
crates/tick/src/runtime/inactive_clock.rs	Strengthens marker traits/docs and adds docsrs feature annotation for `rt-shared`.
crates/tick/src/runtime/clock_gone.rs	Makes `ClockGone` trivially copyable/comparable for easier consumption.
crates/tick/src/periodic_timer.rs	Updates waker when polled by a different task; adds tests to ensure latest waker is notified.
crates/tick/src/lib.rs	Adjusts crate docs for feature-gated examples and inlines key public re-exports.
crates/tick/src/future_ext.rs	Updates `timeout` API to return `Timeout<Self>` and improves docs/example.
crates/tick/src/fmt/unix_seconds.rs	Updates doctests to avoid `unwrap_err` and simplifies parsing error conversion.
crates/tick/src/fmt/rfc_2822.rs	Updates doctest to avoid `unwrap_err`.
crates/tick/src/fmt/mod.rs	Updates docs to point to fallible conversion API and inlines formatter re-exports.
crates/tick/src/error.rs	Extends `Error` with timeout kind, classification helpers, and optional backtrace capture.
crates/tick/src/delay.rs	Updates waker when polled by a different task; adds tests to ensure latest waker is notified.
crates/tick/src/clock.rs	Introduces `try_system_time_as`, adds docsrs feature annotations, and supports timer waker updates.
crates/tick/src/clock_control.rs	Introduces `ClockControlBuilder`, `new_auto_advancing`, and wakes wakers after releasing the state lock.
crates/tick/README.md	Regenerates README to reflect the new API usage patterns.
crates/tick/examples/interop_time.rs	Migrates example away from infallible conversion API.
crates/tick/examples/interop_jiff.rs	Migrates example to `try_system_time_as`.
crates/tick/examples/interop_chrono.rs	Migrates example away from infallible conversion API.
crates/tick/examples/data.rs	Makes cached timestamp updates fallible and propagates errors.
crates/tick/examples/clock.rs	Makes example operation fallible to propagate conversion failures.
crates/tick/examples/clock_control.rs	Migrates to `new_auto_advancing` and avoids `unwrap` in example.
crates/tick/Cargo.toml	Updates crate metadata (category + documentation URL).
crates/seatbelt/tests/timeout.rs	Updates tests to use `ClockControlBuilder`/`new_auto_advancing`.
crates/seatbelt/tests/retry.rs	Updates tests to use `new_auto_advancing`.
crates/seatbelt/tests/hedging.rs	Updates tests to use `new_auto_advancing`.
crates/seatbelt/tests/chaos_latency.rs	Updates tests to use `new_auto_advancing`.
crates/seatbelt/src/timeout/service.rs	Updates internal tests to use `ClockControlBuilder`.
crates/seatbelt/src/retry/service.rs	Updates internal tests to use `new_auto_advancing`.
crates/seatbelt/src/hedging/service.rs	Updates internal tests to use `new_auto_advancing`.
crates/seatbelt/src/chaos/latency/service.rs	Updates internal tests to use `ClockControlBuilder`.
crates/seatbelt_http/src/timeout.rs	Updates tests to use `new_auto_advancing`.
crates/seatbelt_http/src/retry.rs	Updates tests to use `new_auto_advancing`.
crates/seatbelt_http/src/hedging.rs	Updates tests to use `new_auto_advancing`.
crates/seatbelt_http/src/breaker.rs	Updates tests to use `new_auto_advancing`.
crates/http_extensions/src/fake_handler.rs	Updates test clock creation to use `new_auto_advancing`.
crates/http_extensions/src/body/timeout_body.rs	Updates tests to use `new_auto_advancing`.
crates/fetch/tests/timeout.rs	Updates tests to use `new_auto_advancing`.
crates/fetch/tests/standard_pipeline.rs	Updates tests/helpers to use `new_auto_advancing`.
crates/fetch/tests/resilience.rs	Updates tests/helpers to use `new_auto_advancing`.
crates/fetch/src/fake.rs	Updates fake builder test to use `ClockControlBuilder`.
crates/fetch_hyper/tests/smoke.rs	Updates helper clock to use `new_auto_advancing`.
crates/fetch_hyper/src/tls/connector.rs	Updates tests to use `new_auto_advancing`.
crates/fetch_hyper/src/timer.rs	Updates tests to use `new_auto_advancing`.
crates/fetch_hyper/src/testing.rs	Updates tests to use `new_auto_advancing`.
crates/fetch_hyper/src/connection/hyper_handler.rs	Updates tests to use `new_auto_advancing`.
crates/fetch_hyper/src/connection/hyper_connector_adapter.rs	Updates tests to use `new_auto_advancing`.
crates/fetch_hyper/src/connection/client_connector.rs	Updates tests to use `new_auto_advancing`.
crates/fetch_hyper/src/builder.rs	Updates docs/tests to use `new_auto_advancing`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-08-03T07:38:01Z

Codecov Report

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

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #632    +/-   ##
========================================
  Coverage   100.0%   100.0%            
========================================
  Files         472      473     +1     
  Lines       45447    45553   +106     
========================================
+ Hits        45447    45553   +106

Flag	Coverage Δ
linux	`82.5% <100.0%> (-17.5%)`	⬇️
linux-arm	`83.0% <100.0%> (-17.0%)`	⬇️
scheduled	`?`
windows	`83.6% <100.0%> (-16.4%)`	⬇️

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.

Add coverage for error classification, backtrace storage, builder defaults, and maximum-instant timer advancement. Also use the preferred duration unit and guard auto-advance progress so mutation testing cannot hang. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d077867b-9f23-43df-86db-c3efd05948f4

Copilot

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/tick/src/runtime/inactive_clock.rs:44

The Shared mode bullet says it “Does not implement Clone”, but Shared now derives Clone/Copy. Even if the intent is to describe InactiveClock<Shared>, the current wording is ambiguous and can be read as referring to the marker type itself. Clarify that the inactive clock specialization is the non-Clone/non-ThreadAware type.

Remove redundant derives, rustdoc attributes, and the unused stopwatch start accessor while preserving the correctness-related timer changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d077867b-9f23-43df-86db-c3efd05948f4

Copilot

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.

Remove redundant rustdoc attributes and retain expired wakers in their existing map nodes until locks are released. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d077867b-9f23-43df-86db-c3efd05948f4

Keep builder defaults solely in ClockControl::builder rather than exposing an unnecessary Default implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d077867b-9f23-43df-86db-c3efd05948f4

Copilot

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.

github-actions · 2026-08-03T13:51:17Z

⚠️ Potential breaking changes detected

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

`tick`

     Cloning origin/main
    Building tick v0.4.0 (current)
       Built [   2.401s] (current)
     Parsing tick v0.4.0 (current)
      Parsed [   0.005s] (current)
    Building tick v0.4.0 (baseline)
       Built [   2.641s] (baseline)
     Parsing tick v0.4.0 (baseline)
      Parsed [   0.005s] (baseline)
    Checking tick v0.4.0 -> v0.4.0 (no change; assume minor)
     Checked [   0.017s] 196 checks: 193 pass, 3 fail, 0 warn, 49 skip

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/auto_trait_impl_removed.ron

Failed in:
  type Timeout is no longer UnwindSafe, in /home/runner/work/oxidizer/oxidizer/crates/tick/src/timeout.rs:11
  type Timeout is no longer RefUnwindSafe, in /home/runner/work/oxidizer/oxidizer/crates/tick/src/timeout.rs:11

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/inherent_method_missing.ron

Failed in:
  ClockControl::auto_advance, previously in file /home/runner/work/oxidizer/oxidizer/target/semver-checks/git-origin_main/93ee193b63a938953585d154c792fbb1f6cc1d24/crates/tick/src/clock_control.rs:221
  ClockControl::auto_advance_limit, previously in file /home/runner/work/oxidizer/oxidizer/target/semver-checks/git-origin_main/93ee193b63a938953585d154c792fbb1f6cc1d24/crates/tick/src/clock_control.rs:261
  ClockControl::auto_advance_timers, previously in file /home/runner/work/oxidizer/oxidizer/target/semver-checks/git-origin_main/93ee193b63a938953585d154c792fbb1f6cc1d24/crates/tick/src/clock_control.rs:314
  ClockControl::advance_millis, previously in file /home/runner/work/oxidizer/oxidizer/target/semver-checks/git-origin_main/93ee193b63a938953585d154c792fbb1f6cc1d24/crates/tick/src/clock_control.rs:343
  Clock::system_time_as, previously in file /home/runner/work/oxidizer/oxidizer/target/semver-checks/git-origin_main/93ee193b63a938953585d154c792fbb1f6cc1d24/crates/tick/src/clock.rs:370
  SimpleClock::system_time_as, previously in file /home/runner/work/oxidizer/oxidizer/target/semver-checks/git-origin_main/93ee193b63a938953585d154c792fbb1f6cc1d24/crates/tick/src/simple_clock.rs:164

--- failure type_allows_fewer_generic_type_params: type now allows fewer generic type parameters ---

Description:
A type now allows fewer generic type parameters than it used to. Uses of this type that supplied all previously-supported generic types will be broken.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-parameter-no-default
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/type_allows_fewer_generic_type_params.ron

Failed in:
  Struct Timeout allows 2 -> 1 generic types in /home/runner/work/oxidizer/oxidizer/crates/tick/src/timeout.rs:11

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [   5.543s] tick

Restore marker trait implementations to their pre-PR surface and avoid unnecessary public trait exposure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d077867b-9f23-43df-86db-c3efd05948f4

Copilot

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/tick/src/error.rs:187

Error captures and stores a Backtrace, but the std::error::Error impl does not expose it via the trait. As a result, when tick::Error is used behind dyn std::error::Error (or wrapped by other error types), callers will not be able to retrieve the backtrace through the standard Error::backtrace() API.

Consider overriding fn backtrace(&self) in the std::error::Error impl to return the captured backtrace.

## Problem Pull requests from forks can never merge. They sit forever on: > Code scanning is waiting for results from CodeQL for the commits `<sha>` or `<sha>`. Current example: #624 (from `sandersaares/oxidizer`). **Root cause:** this repo uses CodeQL **default setup**, and default setup does not run for pull requests from forks — [that exclusion is documented behaviour](https://docs.github.com/en/code-security/concepts/code-scanning/setup-types), not a bug. The code-scanning merge-protection rule still expects a `CodeQL` result, so the check stays pending indefinitely and the PR is permanently blocked. Evidence gathered on this repo: | PR | Head repo | Fork? | CodeQL run | CodeQL check | |----|-----------|-------|-----------|--------------| | #621, #632, #568, #570-#573 | `microsoft/oxidizer` | no | yes | yes | | #624 | `sandersaares/oxidizer` | yes | none | none, blocked | | #622 | `kate-shine/oxidizer` | yes | none | none, blocked | ## Why advanced setup fixes it An advanced-setup workflow triggered by `pull_request` **does** run for fork PRs, and GitHub accepts its SARIF upload on public repositories even though the fork's `GITHUB_TOKEN` is read-only. Verified empirically against `prettier/prettier`, a public repo on advanced setup. On fork PR head `bf2849cee9d467aedf3fbd42a95201ea4393f855` (from `splincode/prettier`): - the only workflow runs are `pull_request` ones, including `.github/workflows/codeql.yml`; there is no `dynamic` (default setup) run at all; - yet `github-advanced-security` posted check `CodeQL` with conclusion `success`. ## History `.github/workflows/codeql.yml` already exists and is correct, but it has been in the `disabled_manually` state since **2026-01-22** and has not run since. It was introduced by #219 ("ci: Switch to advanced CodeQL mode"), whose stated goal was, verbatim, *"Once this is checked in, then we can tweak to improve permissions on forks"* — exactly the problem above. It ran 33 times, all successful, and roughly 4.5 hours after that PR merged, default setup was switched back on in the repository settings, which automatically disabled this workflow. No PR, issue or comment records a reason. Since then #232, #237, #462, #470, #484, #543, #563 and #574 have all been maintaining a workflow that never runs. ## Changes - Document why advanced setup is preferred, so this does not get silently reverted to default setup a second time. - Add the `merge_group` trigger. `main.yml` and `anvil-pr.yml` both have it and `codeql.yml` did not; without it the merge queue stalls on this workflow once it is required. - Bump `github/codeql-action` to `v4.37.3` (confirmed `"immutable": true` via the releases API, consistent with the existing tag-pinning rationale in the file). ## Required admin action, this PR alone is not sufficient Merging this changes nothing by itself, because the workflow is still disabled and GitHub will keep it disabled while default setup is on. A repository admin must, **in this order**: 1. Settings, Advanced Security, **disable CodeQL default setup**. 2. Re-enable this workflow: `gh api -X PUT repos/microsoft/oxidizer/actions/workflows/codeql.yml/enable`. Doing step 2 first does not work. ## Trade-off Fork PRs from **first-time contributors** will show CodeQL as `action_required` until a maintainer clicks "Approve and run". That is standard GitHub Actions fork policy and it also applies to every other `pull_request` workflow in this repo today. It is a single click, versus the current situation where fork PRs cannot merge at all. ## Draft Left as a draft until an admin confirms they will make the settings change, since merging without it is a no-op. Co-authored-by: Kateřina Churanová <katerina.churanova@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings August 3, 2026 07:19

martintmk added the agency-rocket Touched by a rocket skill label Aug 3, 2026

Copilot started reviewing on behalf of martintmk August 3, 2026 07:20 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/timeout.rs

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/runtime/clock_gone.rs Outdated

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/runtime/mod.rs Outdated

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/clock.rs Outdated

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/periodic_timer.rs

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/periodic_timer.rs

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/stopwatch.rs Outdated

Copilot AI review requested due to automatic review settings August 3, 2026 08:07

Copilot started reviewing on behalf of martintmk August 3, 2026 08:08 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 3, 2026 08:12

Copilot started reviewing on behalf of martintmk August 3, 2026 08:13 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Kateřina Churanová (kate-shine) mentioned this pull request Aug 3, 2026

ci: restore advanced CodeQL setup so fork PRs are not blocked #633

Merged

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/fmt/mod.rs Outdated

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/simple_clock.rs Outdated

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/state.rs Outdated

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/clock_control.rs Outdated

fix(tick): avoid extra timer wake allocation

7f18893

Remove redundant rustdoc attributes and retain expired wakers in their existing map nodes until locks are released. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d077867b-9f23-43df-86db-c3efd05948f4

Copilot AI review requested due to automatic review settings August 3, 2026 13:24

Copilot started reviewing on behalf of martintmk August 3, 2026 13:25 View session

review(tick): remove builder default impl

1b1c673

Keep builder defaults solely in ClockControl::builder rather than exposing an unnecessary Default implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d077867b-9f23-43df-86db-c3efd05948f4

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 3, 2026 13:29

Copilot started reviewing on behalf of martintmk August 3, 2026 13:30 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

martintmk commented Aug 3, 2026

View reviewed changes

Comment thread crates/tick/src/runtime/inactive_clock.rs Outdated

review(tick): retain minimal marker derives

d0f2f0a

Restore marker trait implementations to their pre-PR surface and avoid unnecessary public trait exposure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d077867b-9f23-43df-86db-c3efd05948f4

Copilot AI review requested due to automatic review settings August 3, 2026 14:07

Copilot started reviewing on behalf of martintmk August 3, 2026 14:08 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Uh oh!

Conversation

martintmk commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

codecov Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Potential breaking changes detected

tick

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

martintmk commented Aug 3, 2026 •

edited

Loading

codecov Bot commented Aug 3, 2026 •

edited

Loading

github-actions Bot commented Aug 3, 2026 •

edited

Loading

`tick`