fix(desktop): actually hold a sleep block on Windows - #6036
Conversation
themiguelamador
left a comment
There was a problem hiding this comment.
Two blocking changes are required before merge:
-
P1 — New production
unsafeviolates the repository safety policy. This patch adds direct Win32 FFI calls inprevent_sleep.rs(plus integer round-tripping of the rawHANDLE).AGENTS.mdpermits no new unsafe Rust outside the generated mobile FFI module; adding a local FFI implementation is explicitly outside that exception. This is avoidable without regressing the thread-lifetime requirement: the review fix uses the maintained safekeepawakewrapper, owns Windows' thread-local execution-state request on one dedicated worker for the full block lifetime, and signals/joins that worker on release. The same wrapper removes the legacy direct macOS FFI from this touched module, while Linux remains a no-op. -
Repository attribution policy. The PR commit contains an AI
Co-Authored-Bytrailer and the PR description contains a “Generated with” footer, both prohibited byAGENTS.md. The review branch rewrites the commit without that trailer. My GitHub account cannot edit another author's PR description, so the author still needs to remove the footer when adopting the branch.
The complete fix is available at Complear/buzz:review/pr-6036-fix (443faf9a7; its rewritten parent removes the prohibited commit trailer). It also adds the required Cargo.lock update for the safe wrapper.
Verification on that branch:
- Desktop Tauri tests: 2,545 passed, 16 ignored, 0 failed
- strict workspace/all-target clippy: passed
- sidecar-aware Desktop Tauri compile check: passed
- Rust formatting and
git diff --check: passed
The Windows-only implementation still needs the repository's Windows CI leg; local cross-checking reached a pre-existing native ring build requirement for the Windows SDK before compiling the app crate.
`prevent_sleep::acquire` only ever created a power assertion under `#[cfg(target_os = "macos")]`. On Windows it fell through, left `assertion_id` at `None`, skipped the `INACTIVITY_CAP_SECONDS` arming that is gated on that field, and returned `Ok(())`. The caller — and the Settings toggle above it — reported that sleep was prevented while nothing was held, so machines slept mid agent-turn. Windows now takes a Win32 power request (`PowerCreateRequest` + `PowerSetRequest(PowerRequestSystemRequired)`), the direct analogue of the macOS `PreventUserIdleSystemSleep` assertion. `SetThreadExecutionState` is deliberately not used: it is per-thread and dies with the calling thread, whereas `acquire`/`release` are reached from Tauri command workers and from the shutdown hook — different threads — so the pair would leak or evaporate nondeterministically. A power request is a process-owned kernel object, so any thread can create, set, clear and close it and the handle can live in shared state. The `Option<u32> assertion_id` field, which had no Windows analogue and was blindly cleared by two `#[cfg(not(target_os = "macos"))]` blocks, becomes an `Option<SleepBlock>` whose `Drop` performs the platform release. Explicit `release`, the inactivity-cap expiry, and process exit now all release through that one path, so no path can clear the bookkeeping while leaving the OS request outstanding. macOS behaviour is unchanged; Linux stays a documented no-op that holds nothing and arms no cap timer. `windows-sys` gains the `Win32_System_Power` feature; no new dependency. Sleep itself cannot be exercised on CI, so the release/expiry bookkeeping is tested cross-platform via a test-only inert block, and `ensure_block` — the value that gates `arm_cap_timer` — is asserted against the real OS API on macOS and Windows. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
97438a8 to
a99170b
Compare
Review follow-up on PR block#6036. AGENTS.md states "No `unsafe` code" without qualification, and the previous revision of this branch added Win32 FFI (PowerCreateRequest / PowerSetRequest / PowerClearRequest plus integer round-tripping of the raw HANDLE). That is the policy as written, so the local FFI goes. prevent_sleep.rs now reaches the platform mechanism through keepawake, which also retires the IOKit `extern "C"` block and the CoreFoundation string plumbing this module already carried on main. The file goes from 8 unsafe blocks to zero. The thread-lifetime constraint that ruled out SetThreadExecutionState still holds, so it is solved structurally rather than by choice of API: a dedicated thread creates the guard, parks on a channel, and drops the guard when the channel disconnects. SleepBlock::drop drops the sender and then JOINS that thread, so release is synchronous -- when drop returns, the OS request is provably gone, released on the same thread that took it. acquire and release can keep arriving on three different Tauri worker threads without the request evaporating or leaking. Dependency cost, measured rather than assumed: keepawake adds exactly ONE package to Cargo.lock. Every transitive dependency it names (windows, zbus, objc2-core-foundation, objc2-io-kit, derive_builder, thiserror, cfg-if) was already in the tree, and the lock diff contains no version changes. The Win32_System_Power feature added to windows-sys by the previous revision is removed, returning that line to its state on main. keepawake is target-gated to macOS and Windows. Its Linux path needs a live D-Bus session, and Linux here is a deliberate no-op, so gating keeps a Linux build free of a new runtime requirement. Verified: cargo check clean; cargo fmt clean; prevent_sleep tests 4/4 including ensure_block_holds_a_real_os_block_and_is_idempotent, which takes a real OS block through keepawake and asserts a second acquire reuses the same worker thread rather than stranding one; full desktop-tauri suite 2319 passed / 1 failed, that one being managed_agents::runtime::tests:: claude_spawn_uses_the_probed_cli_executable, which mutates process-global PATH and races other tests in the same process -- it passes in isolation and this branch does not touch that module. Not clean, and pre-existing: clippy -D warnings fails on Windows in buzz-terminal (unused std::io / Instant / portable_pty::Child imports that are only used behind #[cfg(unix)]). That crate is not in this diff. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
Review follow-up on PR block#6036. AGENTS.md states "No `unsafe` code" without qualification, and the previous revision of this branch added Win32 FFI (PowerCreateRequest / PowerSetRequest / PowerClearRequest plus integer round-tripping of the raw HANDLE). That is the policy as written, so the local FFI goes. prevent_sleep.rs now reaches the platform mechanism through keepawake, which also retires the IOKit `extern "C"` block and the CoreFoundation string plumbing this module already carried on main. The file goes from 8 unsafe blocks to zero. The thread-lifetime constraint that ruled out SetThreadExecutionState still holds, so it is solved structurally rather than by choice of API: a dedicated thread creates the guard, parks on a channel, and drops the guard when the channel disconnects. SleepBlock::drop drops the sender and then JOINS that thread, so release is synchronous -- when drop returns, the OS request is provably gone, released on the same thread that took it. acquire and release can keep arriving on three different Tauri worker threads without the request evaporating or leaking. Dependency cost, measured rather than assumed: keepawake adds exactly ONE package to Cargo.lock. Every transitive dependency it names (windows, zbus, objc2-core-foundation, objc2-io-kit, derive_builder, thiserror, cfg-if) was already in the tree, and the lock diff contains no version changes. The Win32_System_Power feature added to windows-sys by the previous revision is removed, returning that line to its state on main. keepawake is target-gated to macOS and Windows. Its Linux path needs a live D-Bus session, and Linux here is a deliberate no-op, so gating keeps a Linux build free of a new runtime requirement. Verified: cargo check clean; cargo fmt clean; prevent_sleep tests 4/4 including ensure_block_holds_a_real_os_block_and_is_idempotent, which takes a real OS block through keepawake and asserts a second acquire reuses the same worker thread rather than stranding one; full desktop-tauri suite 2319 passed / 1 failed, that one being managed_agents::runtime::tests:: claude_spawn_uses_the_probed_cli_executable, which mutates process-global PATH and races other tests in the same process -- it passes in isolation and this branch does not touch that module. Not clean, and pre-existing: clippy -D warnings fails on Windows in buzz-terminal (unused std::io / Instant / portable_pty::Child imports that are only used behind #[cfg(unix)]). That crate is not in this diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
themiguelamador
left a comment
There was a problem hiding this comment.
Re-reviewed updated head 0935472. The author correctly addressed the code blocker: the touched module now contains no unsafe Rust, keepawake is target-gated to macOS/Windows, the Windows thread-local request is owned by a dedicated worker, and SleepBlock drop disconnects and joins that worker so release is synchronous across explicit release, cap expiry, and process teardown. The four focused native tests pass, strict all-target Tauri Clippy passes, and formatting/diff checks pass. The first native attempt was blocked only until the repository-required sidecar stubs were staged with just _ensure-sidecar-stubs.
One repository-policy blocker remains: both source commits still contain AI Co-Authored-By trailers and the PR description still contains a Generated with Claude footer. I rewrote the effective tested tree into attribution-compliant signed commit 6f1715adf on Complear:review/pr-6036-latest-fix. Please adopt that commit and remove the footer from the PR description; the latter is author-only metadata I cannot edit.
0935472 to
d121b85
Compare
|
@themiguelamador — both asks from your review are satisfied at head 1. No new Your suggested direction is what shipped: the guard is owned by a dedicated Measured dependency cost: 2. Attribution trailers. These are removed — every commit now carries only One correction offered in good faith, since it shaped both of your reviews: I could not find the rule in this repo. Known remaining gap, not claimed as green: no repository CI has ever executed on this PR — all workflow runs sit at |
|
@themiguelamador — the attribution blocker looks resolved at the current head Checking both halves:
So I do not think the rewrite onto Worth flagging separately, since it is the actual source of this class of problem: those agent Nothing else outstanding from my side on this one — flagging only because the blocker appears |
The bug
Sleep prevention has never worked on Windows, and the UI says it does.
On
main, the entire assertion-creation block indesktop/src-tauri/src/prevent_sleep.rssits under#[cfg(target_os = "macos")](prevent_sleep.rs:112-158). On Windows,acquire()falls straight through to theif guard.assertion_id.is_some()check atprevent_sleep.rs:161, which isfalse, soarm_cap_timernever runs and the function returnsOk(()).set_prevent_sleep_active(desktop/src-tauri/src/commands/prevent_sleep.rs:9-15) propagates thatOk, so the Settings toggle reports success while nothing is held — the machine sleeps mid agent-turn.The fix
Reach the platform mechanism through the
keepawakecrate rather than local FFI.AGENTS.md:135says "Nounsafecode" andCONTRIBUTING.md:276says all crates enforce#![deny(unsafe_code)], so a safe wrapper is the policy-compliant route — and it also retires the IOKitextern "C"block and CoreFoundation string plumbing this module already carried onmain. The file goes from 3unsafe {}blocks and 2extern "C"blocks to zero.Thread lifetime is solved structurally, not by choice of API
keepawake's Windows backend is
SetThreadExecutionState(ES_CONTINUOUS | ES_SYSTEM_REQUIRED), which is per-thread: its effect dies with the thread that set it.acquire()andrelease()are reached from Tauri command workers and from the shutdown hook — potentially three different threads — so binding the request to any of them would either evaporate (the acquiring worker retires while an agent still runs) or leak (release lands on a thread that never set it).So the guard is owned by a dedicated
buzz-prevent-sleepthread for the block's whole lifetime: it creates the guard, parks on an mpsc channel, and drops the guard when the channel disconnects.SleepBlock::dropdrops the sender and then joins that thread, so release is synchronous — whendropreturns, the OS request is provably gone, released on the same thread that took it.One release path
assertion_id: Option<u32>becomesblock: Option<SleepBlock>, whoseDropperforms the platform release. That removes both#[cfg(not(target_os = "macos"))] { guard.assertion_id = None; }blocks rather than re-gating them (mainlines 91-94 and 187-190):guard.block = Nonenow is the release on every platform, so explicitrelease(), cap expiry and process exit share one path, and none can clear the bookkeeping while leaving an OS request outstanding.Platform behaviour
SetThreadExecutionState(ES_CONTINUOUS | ES_SYSTEM_REQUIRED), held for the block's lifetime by the dedicated thread.display(false).idle(true).sleep(false), keepawake creates exactly onePreventUserIdleSystemSleepassertion atkIOPMAssertionLevelOn— the same assertion type and levelmaincreated by hand. Two visible differences worth knowing: the reason string shown bypmset -g assertionschanges fromBuzz — agents are activetoAgents are active, and the failure message changes fromIOPMAssertionCreateWithName failed with IOReturn {ret}tokeepawake failed: {error}.INACTIVITY_CAP_SECONDSis reused unchanged, so Windows gets the same one-hour safety valve macOS has.Dependency cost, measured
desktop/src-tauri/Cargo.lockgrows by exactly one package — 1,228 → 1,229[[package]]entries:keepawake 0.6.0, MIT. Every transitive dependency it names (windows 0.62.2,zbus 5.17.0,objc2-core-foundation,objc2-io-kit,derive_builder,thiserror,cfg-if) was already locked. No package besides keepawake is added, removed, upgraded or downgraded.The lock diff is larger than that implies, and the extra churn is benign: re-resolution collapsed ~19 dependency edges onto versions already present in the tree (
windows-sys→ 0.61.2 forrustix,tempfile,errno,socket2,anstreamand friends;tempfile→getrandom 0.4.3;data-encoding-macro-internal→syn 2). Those crates declare wide ranges —tempfileasks forwindows-sys ">=0.52, <0.62"andgetrandom ">=0.3.0, <0.5";data-encoding-macro-internalasks forsyn ">= 1, < 3"— so cargo is free to unify them, and did. No pin moved.cargo-denyis unaffected: the root workspace excludesdesktop/src-tauri(Cargo.toml:34) and CI'srustpath filter excludes it too, so theSecurityjob never sees this crate.Known limitation, stated plainly
On Modern Standby (S0 low-power idle) systems on battery, Windows terminates system-required requests some minutes after the sleep timeout expires. No single Win32 mechanism closes that gap — the same rule applies to
PowerSetRequest, which is why the choice of API is not what would fix it. On that machine class this is strictly better than the previous no-op but does not fully achieve "stays awake for a long agent turn." On S3 systems (powercfg /a) the request is honoured for the full block.Behaviour change worth knowing for QA
acquire()now arms the inactivity cap on Windows for the first time, soprevent-sleep-expiredcan fire there andPreventSleepSettingsCardwill render the "Sleep prevention expired after 1 hour without agent activity" banner (desktop/src/features/settings/ui/PreventSleepSettingsCard.tsx:81-84). That is intended macOS parity, but it is a state Windows users have never seen.Testing
Four tests compile per platform in
prevent_sleep::tests:ensure_block_holds_a_real_os_block_and_is_idempotent— macOS/Windows only. Takes a real OS block through keepawake, asserts a block is held (that return value is exactly what gatesarm_cap_timer), and asserts a secondensure_blockreuses the same worker thread rather than stranding one holding a request nothing will ever release.release_drops_the_block_and_invalidates_the_timerexpire_ignores_a_stale_generation_and_keeps_the_blockexpire_releases_the_block_once_for_the_current_generation2–4 are platform-independent, using a
#[cfg(test)]-onlySleepBlock::Inert(Arc<AtomicUsize>)whoseDropbumps a counter — that is how release-exactly-once is asserted without an OS call. On Linux,unsupported_platforms_are_a_silent_successtakes the fourth slot and asserts the no-op reports "not held" without erroring.Honest note on coverage: only test 1 covers the bug, and it exercises
ensure_block(), which does not exist onmain— so it cannot be run red againstmaindirectly. The red was produced by cfg-ing out the newplatform_beginin the fixed file. The real regression guard is the Windows CI leg:cargo test --manifest-path desktop/src-tauri/Cargo.toml --target x86_64-pc-windows-msvcin theWindows Rust (x86_64-pc-windows-msvc)job (.github/workflows/ci.yml:1060), which this diff triggers through thedesktop-rustpath filter.Run locally (dev box;
just _ensure-sidecar-stubsfirst, otherwise the Tauri crate will not compile):cargo checkandcargo fmt --checkon the desktop Tauri manifest: cleanprevent_sleeptests: 4 passed / 0 failedmanaged_agents::runtime::tests::claude_spawn_uses_the_probed_cli_executable, which mutates process-globalPATHand races other tests in the same process. It passes in isolation and this branch does not touch that module.Not verified here: repository CI has not built this branch (workflow runs sit at
action_requiredpending maintainer approval), so no Windows or Linux CI leg has compiled it. Separately,clippy -D warningson Windows fails indesktop/src-tauri/crates/buzz-terminal(unusedstd::io/Instant/portable_pty::Childimports used only behind#[cfg(unix)]) — pre-existing onmain, that crate is not in this diff, and the Windows CI job runscargo check, not clippy, over the Tauri workspace.Out of scope
set_prevent_sleep_activecommand, the one-hour cap duration, or Linux behaviour.buzz-terminal's pre-existing Windows clippy warnings.