Breadway/breadram

Fork 0

Stall rate window: elapsed time uses the snapshot timestamp but counters are read later #45

New issue

Closed

opened 2026-10-14 18:17:40 +08:00 by Breadway · 2 comments

Breadway commented

2026-10-14 18:17:40 +08:00

Owner

Found while reviewing RAM-76 (PR 44, head 0c03949).

Confidence: medium (read from code, not measured).

run_execution passes now_ms = published.status.observed_at_ms (the host snapshot time) to StallWatch::judge and arm, but scope_pressure_totals reads each cgroup memory.pressure counter later in the pass. Elapsed time between two readings is therefore the snapshot spacing, while the counter delta covers the spacing plus the change in read latency. Read-latency jitter turns into rate error: at the 250 ms minimum tick a few tens of ms of jitter moves the computed rate by several points when the scope is already stalling, which is close to the 1 point (100 bp) reversal threshold. At the 3 s default tick the error is negligible.

Fix direction: take a monotonic timestamp immediately around the counter reads (once per pass, after the reads) and pass that to judge/arm, or timestamp each reading. Add a test where the read time differs from the snapshot time.

Not blocking RAM-76.

Found while reviewing RAM-76 (PR 44, head 0c03949). Confidence: medium (read from code, not measured). run_execution passes now_ms = published.status.observed_at_ms (the host snapshot time) to StallWatch::judge and arm, but scope_pressure_totals reads each cgroup memory.pressure counter later in the pass. Elapsed time between two readings is therefore the snapshot spacing, while the counter delta covers the spacing plus the change in read latency. Read-latency jitter turns into rate error: at the 250 ms minimum tick a few tens of ms of jitter moves the computed rate by several points when the scope is already stalling, which is close to the 1 point (100 bp) reversal threshold. At the 3 s default tick the error is negligible. Fix direction: take a monotonic timestamp immediately around the counter reads (once per pass, after the reads) and pass that to judge/arm, or timestamp each reading. Add a test where the read time differs from the snapshot time. Not blocking RAM-76.

Breadway referenced this issue

2026-10-14 18:18:06 +08:00

Judge stall reversal by rate over a bounded window #44

Breadway self-assigned this

2026-10-20 08:19:26 +08:00

Breadway commented

2026-10-20 11:33:32 +08:00

Author

Owner

Independent review of head 8872c507ab. Verdict: pass.

Acceptance:

Snapshot vs counter-read elapsed mismatch: pass. run_execution timestamps the pressure batch with pressure_sample after scope_pressure_totals and passes pressure_at_ms to StallWatch::judge and settle_stall_observations. Policy snapshot time and execute_with still use observed_at_ms (daemon.rs around the execution pass and pressure_sample).
Safety, ownership, and threshold semantics: pass. STALL_RISE_BASIS_POINTS stays 100. Ownership still goes through recover_actions. Using pressure_at_ms for arm, the 10s observation window, reversal journal at_ms, and Ineffective history keeps those clocks on the same monotonic reading as the rate. Cooldown and rate-limit planning still use the snapshot clock, so a collection delay cannot shorten a cooldown.
Regression: pass. stall_rate_uses_counter_read_spacing_not_snapshot_spacing calls production pressure_sample, judge, and arm. Constant 10% (35,000 us over 350 ms after a 100 ms collection delay) does not reverse; the next real rise to about 12% does. The test asserts the clock closure runs after the read. It does not execute run_execution itself; the production call site was checked in the diff. Helper docs correctly limit the claim to one batch timestamp with within-batch skew.

Checks on this SHA:

Forgejo Actions run 119, workflow check.yml, job check, head 8872c507ab: success. Combined status CPU checks / check (push): success. No branch protection rules. No prior reviews.
Independent scratch worktree review-ram-78 at the same SHA: cargo fmt --all --check, cargo clippy --workspace --all-targets --locked -- -D warnings, and cargo test --workspace --locked all passed (including the new test).

Reviewed SHA: 8872c507ab

Independent review of head 8872c507abb30cc542f07fbd0a49c109353fb3fc. Verdict: pass. Acceptance: - Snapshot vs counter-read elapsed mismatch: pass. run_execution timestamps the pressure batch with pressure_sample after scope_pressure_totals and passes pressure_at_ms to StallWatch::judge and settle_stall_observations. Policy snapshot time and execute_with still use observed_at_ms (daemon.rs around the execution pass and pressure_sample). - Safety, ownership, and threshold semantics: pass. STALL_RISE_BASIS_POINTS stays 100. Ownership still goes through recover_actions. Using pressure_at_ms for arm, the 10s observation window, reversal journal at_ms, and Ineffective history keeps those clocks on the same monotonic reading as the rate. Cooldown and rate-limit planning still use the snapshot clock, so a collection delay cannot shorten a cooldown. - Regression: pass. stall_rate_uses_counter_read_spacing_not_snapshot_spacing calls production pressure_sample, judge, and arm. Constant 10% (35,000 us over 350 ms after a 100 ms collection delay) does not reverse; the next real rise to about 12% does. The test asserts the clock closure runs after the read. It does not execute run_execution itself; the production call site was checked in the diff. Helper docs correctly limit the claim to one batch timestamp with within-batch skew. Checks on this SHA: - Forgejo Actions run 119, workflow check.yml, job check, head 8872c507abb30cc542f07fbd0a49c109353fb3fc: success. Combined status CPU checks / check (push): success. No branch protection rules. No prior reviews. - Independent scratch worktree review-ram-78 at the same SHA: cargo fmt --all --check, cargo clippy --workspace --all-targets --locked -- -D warnings, and cargo test --workspace --locked all passed (including the new test). Reviewed SHA: 8872c507abb30cc542f07fbd0a49c109353fb3fc

Breadway closed this issue

2026-10-20 11:33:55 +08:00

Breadway commented

2026-10-20 11:33:56 +08:00

Author

Owner

Merged PR 51 head 8872c507ab as 4f4cc4d673. Not marked done. Coordinator must verify the integrated behavior. If sync auto-closes this card, that is the known server limitation and does not replace verification.

Merged PR 51 head 8872c507abb30cc542f07fbd0a49c109353fb3fc as 4f4cc4d673aff244b5701f681a9ed526e3f7819b. Not marked done. Coordinator must verify the integrated behavior. If sync auto-closes this card, that is the known server limitation and does not replace verification.

Breadway reopened this issue

2026-10-20 11:34:03 +08:00

Breadway closed this issue

2026-10-20 11:57:14 +08:00

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Breadway/breadram#45

No description provided.

Rows
Columns