fix(stella-core): take retry jitter and hook stamps through ports, and guard the engine against I/O - #6482
Merged
Merged
Conversation
…d guard the engine against I/O The retry ladder drew its jitter from the OS entropy pool through `rand::rng()`, and the hook bus stamped every event from `SystemTime::now()`, while both files' headers said the crate reads nothing directly. `Sleeper` gains a required `jitter` draw, `HookBus::new` takes a `Clock` counting from the Unix epoch, and `rand` leaves stella-core's shipping dependencies for its dev-dependencies. `make core-no-io` (scripts/check-core-no-io.py) now reads the crate's shipping source and manifest on every gate run: a floor over every I/O, wall-clock, sleep and entropy surface, and a down-only count of the `Instant::now()` reads the deadline arithmetic still makes.
macanderson
added
the
closes-nothing
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 3 days and 19 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Contributor
Reviewer's GuideThis PR audits and hardens stella-core’s no-I/O boundary by injecting retry jitter and hook timestamps through ports, moving entropy and wall-clock implementations to host crates, and enforcing the contract with source/manifest checks plus a down-only Instant::now() ratchet wired into CI and repository gates. Sequence diagram for ported retry jitter and backoffsequenceDiagram
participant Engine as stella-core retry
participant Sleeper as Sleeper port
participant Host as Host TokioSleeper
participant Entropy as OS entropy
participant Timer as Tokio timer
Engine->>Sleeper: jitter(span)
Sleeper->>Host: jitter(upper)
Host->>Entropy: random draw
Entropy-->>Host: value
Host-->>Sleeper: value
Sleeper-->>Engine: clamped jitter
Engine->>Sleeper: sleep(delay_ms)
Sleeper->>Host: sleep(duration_ms)
Host->>Timer: tokio::time::sleep(duration_ms)
Timer-->>Engine: wait complete
Sequence diagram for clock-injected hook event stampssequenceDiagram
participant Host as Host runtime
participant Bus as HookBus
participant Clock as Clock port
participant Event as Hook event
participant Observer as Hook script or observatory
Host->>Bus: new(session_id, clock)
Host->>Bus: emit(event draft)
Bus->>Clock: now_ms()
Clock-->>Bus: Unix epoch milliseconds
Bus->>Bus: stamp()
Bus-->>Event: ISO 8601 timestamp
Event-->>Observer: event crosses process boundary
Flow diagram for the core-no-io gateflowchart TD
Start[Run core-no-io] --> Strip[Strip test code and comments]
Strip --> Floor[Check shipping source for I/O surfaces]
Floor --> Manifest[Check production dependencies and Tokio features]
Manifest --> Ratchet[Count Instant.now per file]
Ratchet --> Compare{At or below baseline?}
Compare -->|Yes| Pass[Gate passes]
Compare -->|No| Fail[Gate fails]
Floor -->|Hit found| Fail
Manifest -->|Denied dependency or feature| Fail
Update[--update] --> Compare
Compare -->|Lower counts only| Baseline[Write reduced baseline]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was referenced Sep 10, 2026
macanderson
added a commit
that referenced
this pull request
Sep 10, 2026
…eeper port (#6486) ## What this is The follow-up #6482 said it was leaving behind. That PR fixed the two ambient reads that were also I/O-adjacent (OS entropy, wall clock) and recorded 19 `Instant::now()` reads as ratchet debt. This one removes every ambient read of time from `stella-core`'s shipping code — the 19 explicit reads, the 9 hidden ones (`.elapsed()` is an `Instant::now()` in disguise), and the 4 `tokio::time::timeout`s, which are the runtime's timer rather than the port — and drops tokio's `time` feature from the crate. The ratchet baseline is empty. ## The port `retry::Sleeper` is now the engine's whole time port: `now() -> Instant` joins `sleep` and `jitter`. One port rather than a second `Clock` because a double cannot answer them apart — a sleeper that suspends virtually has moved its own `now`, and a timeout is a sleep racing a call. That is tokio's own shape: a paused runtime puts one virtual clock behind `sleep` and `Instant::now`. `ports::Clock` (`now_ms`) stays what it was — the millisecond clock hosts hand to things that stamp records (fleet ledger, runtime stamps, the hook bus) — and is untouched. `retry::bounded(sleeper, limit, call)` is `tokio::time::timeout` written against the port: `futures_util::future::select` over the call and `sleeper.sleep(limit)`, call polled first so a ready call never loses to a ready sleep, `Option` rather than `Result` because the limit passing is an answer, not an error. ## What moved, in shipping code - `Instant::now()` → `self.sleeper.now()` / `sleeper.now()`: driver (deadline notice, step timing, settlement, speculation pool), dispatch, completion, rate-limit park allowance, sub-agent deadline and tick, accounted call, retry attempt timing. - `.elapsed()` → `sleeper.now().duration_since(started)` at each of the nine sites; `CancelUsageGuard` carries `&dyn Sleeper` because a `Drop` has no caller to hand it `now`. - `tokio::time::timeout` → `retry::bounded`: the tool dispatch ceiling, the idle generation bound, the task-deadline bound, the accounted call's idle bound. `stella-core`'s tokio features are `["sync"]`. - `TurnState::new` / `TurnState::from_checkpoint` / `BorrowedTurn::adopt` take `now: Instant`; `bounded_generation` / `deadline_bounded_generation` take the sleeper. `from_checkpoint` is public, so `stella-cli`'s resume path and `stella-serve`'s checkpoint test pass `Instant::now()` — hosts are where that read belongs. - The hook bus times observer dispatch off the `Clock` it already holds (wall milliseconds); the quarantine rule needs three consecutive overruns, so one wall-clock step cannot quarantine anything. Its test advances a hand-stepped clock instead of `thread::sleep(100ms)`. ## The guard `check-core-no-io.py`'s floor gains `.elapsed()` and any `tokio::time` use; the tokio feature allowlist loses `time`; the baseline is empty and `--update` refuses to add to it, so any `Instant::now()` now fails. Harness: 27 cases (+3: `.elapsed()`, a tokio timer, the `time` feature). AGENTS.md rule 2, the crate README and the `ci.yml` comment say the count is zero. ## Test doubles — the part worth reading An instant-returning `Sleeper` double was tolerable while timeouts bypassed the port and is a lie once they go through it: `EngineConfig::default()` arms a 816 s model timeout and a 15 min tool timeout, and an instant sleep makes both fire the moment a provider future waits on another task. Twelve tests failed that way on the first run. The faithful double sleeps on tokio's clock and reads `now` from `tokio::time::Instant::now().into_std()`, and the files that use it run `#[tokio::test(start_paused = true)]`: virtual time is free while the runtime is idle and correct when it is not. `driver/tests.rs`'s and `subagent/tests.rs`'s shared doubles (now named `TokioSleeper`) and 17 + 7 test files changed that way; `hard_drop_write_back` and the streaming accounted-call test got the same double. Files with their own instant `NoSleep` that passed were left alone. A side effect worth stating: the `stella-core` lib suite went from 46 s to 2.5 s, because several tests had been paying real backoff sleeps through `tokio::time::timeout` that the paused clock now skips. ## Witness - `retry::tests::bounded_races_the_ports_own_sleep`: with a sleeper whose sleep returns at once, `bounded(1h, pending)` is `None` immediately and `bounded(1h, ready(5))` is `Some(5)`, and the port recorded exactly one 3 600 000 ms sleep. On `tokio::time::timeout` this waits the real hour. - `check-core-no-io.py` on `main` fails on 19 `Instant::now()` reads, 9 `.elapsed()` calls, 4 `tokio::time` uses and the `time` feature; here it passes with an empty baseline. No test was deleted. ## Verified - `cargo check -p stella-core -p stella-engine -p stella-tools -p stella-serve -p stella-cli --all-targets`: clean. - `cargo test -p stella-core`: 893 lib tests + every integration test pass (2.5 s lib). `cargo test -p stella-engine`: pass. - `make guards-fast`: green; `./scripts/test-core-no-io.sh`: 27 passed; shellcheck clean. Closes nothing by design (`closes-nothing`): this is the remaining half of the audit the maintainer asked for directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
An adversarial audit of
stella-coreagainst AGENTS.md rule 2, No I/O in the engine. The sweep covered every filesystem, process, network, environment, standard-stream, clock, sleep and entropy surface in the crate's shipping source, plus its manifest. It found no file, process or network I/O. It found two ambient reads the crate's own prose said were not there, and no guard that would have caught either.Two fixes
1. The retry ladder drew its jitter from the OS entropy pool.
retry.rscalledrand::rng()at four sites in the live path, while its header said "Nothing here reads a clock or a budget directly" and pointed at theClockport as the seam it follows. A retry ladder that rolls its own dice cannot be replayed, and the crate linkedrandfor one draw.The draw is now a port:
Sleepergainsfn jitter(&self, upper: u64) -> u64, a uniform draw in0..=upper.compute_backoff_delay_msandserver_hint_delay_mstake&dyn Sleeperin place of&mut impl Rng; the caller clamps the draw to the span, so a careless host widens nothing. The method has no default on purpose: a default of zero would let a host forget the draw and ship a fleet whose workers all wake on the same millisecond after a shared 429. Production impls instella-cli::runtime::TokioSleeperandstella-serve::remote::TokioSleeperdraw fromrand::rng(); every test double answers the floor.randmoves fromstella-core's[dependencies]to[dev-dependencies](the tests' seeded double still usesStdRng);stella-cligainsrandas a dependency, which is where the entropy now lives.Exemplar:
tower::retry::backoff::ExponentialBackoffMakertakes its RNG as a constructor parameter rather than seeding one inside the policy. Here the port is the existingSleeper, because the two questions it now answers — how long the wait is, and how it is waited out — are one decision about the backoff, and the trait's doc says so.2. The hook bus stamped every event from
SystemTime::now().bus.rs'ssealand both failure-log sites read the wall clock directly, whileports::Clock's doc said the crate "never carries a concrete time source of its own".HookBus::newnow takesimpl Clock + 'static, counting from the Unix epoch — the stamp crosses a process boundary to hook scripts and the observatory, so the host's wall clock is the right one and the host owns it.ports::FixedClock(u64)is the pinned clock a test or a replay hands in. Hosts:stella-cli::rulespassesWallClock;stella-serve::extensionspasses a newremote::WallClock(the same shape asstella-runtime'sHostClockand the CLI's, with the same doc). Every test site passesFixedClock(0).The guard:
core-no-ioscripts/check-core-no-io.py, a newmake gatestep in theguards-fastrung, wired intoci.yml,guard-self-tests.yml,check-gate-parity.sh, AGENTS.md's gate block and rule 2, CONTRIBUTING.md, and the crate README. Three questions per run:#[cfg(test)],tests/andtests.rsstripped) namesstd::fs/process/net/env, a standard stream, tokio's I/O drivers,SystemTime::now, a thread or tokio sleep, an entropy source, or a print macro. No baseline: the tree has none.[dependencies]names no I/O or entropy crate, andtokiotakes onlysync,time,macros,rt.Instant::now()reads per file, recorded inscripts/core-no-io-baseline.txt(19 reads in 10 files today), down-only.--updaterefuses to add a file or raise a count. The monotonic clock is not I/O, but a turn that reads it cannot be replayed from its record, which is whatports::Clockexists for.Run against
mainit fails on both fixes above (retry.rs: an entropy source,bus.rs: SystemTime::now); run here it passes.scripts/test-core-no-io.sh(make core-no-io-test) drives 24 fixture cases: each floor pattern, the three things that are not shipping code, both manifest directions, and every ratchet direction including--updaterefusing to grow and refusing to write over a red floor.Witness tests
crates/stella-core/tests/hook_bus_stamps_from_the_clock.rs— a bus givenFixedClock(1_700_000_000_123)stamps2023-11-14T22:13:20.123Z. Could not be written on the old code, where the stamp was whatever the wall clock said.retry::tests::the_jitter_is_the_ports_draw_and_a_wide_draw_is_clamped— a sleeper answering a fixed number lands the delay at floor + that number, and a draw past the span is clamped to the cap.No test was deleted.
What was not fixed, and why
The 19
Instant::now()reads in the driver/step/budget path are recorded, not removed. Routing them throughports::Clockmeans changingBudgetGuard'sInstant-typed deadline API and every host that arms it, anddriver.rsis a god file closed to growth. That is larger than this session; the ratchet holds the count where it is and each removal lowers it.Verified
cargo check -p stella-core --all-targets,-p stella-engine -p stella-tools -p stella-serve --all-targets,-p stella-cli --all-targets: clean.cargo test -p stella-core: all pass (retry 38, bus 55, and the full crate).make guards-fast: green,core-no-ioincluded../scripts/test-core-no-io.sh: 24 passed.shellcheckclean.Closes nothing by design (
closes-nothing): the audit was asked for directly, and the fix rides the PR rather than an issue.Summary by Sourcery
Enforce
stella-core's no-I/O architecture by injecting retry jitter and event clocks through ports and adding automated guards for ambient I/O and clock reads.New Features:
Sleeperport so production hosts provide entropy while tests and replays can control it.Clock, with fixed clocks available for deterministic tests and replay.Bug Fixes:
stella-corecode, preserving the engine's no-I/O boundary.Enhancements:
stella-coreto host crates.Instant::now()usage and enforce the core dependency feature policy.CI:
Documentation:
Tests: