Skip to content

Commit 51bb1f3

Browse files
pmaxhoganclaude
andauthored
feat(core): wire the ioPriority setting to real OS thread priorities (#170)
## What `global.io_priority` (SPEC s22, values `normal` | `low` | `idle`) has been in the settings UI and seeded to `"low"` by migration 0002 for a while, but it was wired to nothing: `settings.rs` validated and stored it, `dtos.rs` carried it, and the backend never read it. This makes it real. Motivation (from Max): "will heavy I/O apps fight with driven? ... i want it to be one notch below normal so it isn't fighting applications but not so low pri that nothing happens ... best-effort for best-cross-platform is good, as long as it's fail-working not fail-erroring." ## The new module: `crates/driven-core/src/priority.rs` - `WorkPriority` (`Normal` / `Low` / `Idle`) with `from_setting`, which degrades an unknown string to `Normal` rather than erroring. - `PriorityCell` - an `Arc<AtomicU8>` holding the live level. - `begin_background_work(p) -> PriorityGuard`, an RAII guard that restores the calling thread on drop, plus `apply_to_current_thread(p)` (one-shot, no restore) and `spawn_blocking(p, f)` for the common case. Everything is **best-effort**: no `Result`s, no panics, no propagation. A refused OS call logs at `debug` and the work runs at normal priority. The guard records **what it actually applied**, not what it intended, so a refused call never produces a bogus restore (`THREAD_MODE_BACKGROUND_END` without a matching successful begin fails with `ERROR_THREAD_MODE_NOT_BACKGROUND`). ## Per-OS mapping | | Windows | Linux | macOS | |---|---|---|---| | `Low` | `THREAD_PRIORITY_BELOW_NORMAL` (CPU only) | `ioprio_set` best-effort prio 6 (I/O only) | `IOPOL_UTILITY` disk policy (I/O only) | | `Idle` | `THREAD_MODE_BACKGROUND_BEGIN` (CPU + I/O + memory) | `ioprio_set` `IOPRIO_CLASS_IDLE` (I/O only) | `IOPOL_THROTTLE` + `PRIO_DARWIN_BG` (CPU + I/O) | Two gaps are deliberate, and documented in the module: - **Windows `Low` lowers CPU only.** The only documented per-thread I/O hint is background mode, which is all-or-nothing - it floors CPU, I/O *and* memory priority together. That is the `Idle` behaviour, so `Low` gets the CPU notch alone, which is exactly "one notch below normal". - **Linux does not touch the nice value inside a guard.** `setpriority` is a one-way ratchet for an unprivileged process: raising the nice value succeeds, lowering it back needs `CAP_SYS_NICE` or a raised `RLIMIT_NICE` (soft limit 0 on most distros). Inside a guard that would leave a pooled `spawn_blocking` thread deniced for the rest of the process's life. Only `ioprio_set` (which resets cleanly with `ioprio = 0`) runs in a guard; the nice bump is reserved for `apply_to_current_thread`, which makes no restore promise. All constants and signatures were verified against current docs ([SetThreadPriority](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-setthreadpriority), [ioprio_set(2)](https://man7.org/linux/man-pages/man2/ioprio_set.2.html), [setiopolicy_np(3)](https://keith.github.io/xcode-man-pages/setiopolicy_np.3.html), XNU `sys/resource.h`), not from memory. Windows uses local `extern "system"` declarations, matching the scanner's `FindFirstStreamW` ADS probe rather than pulling in the `windows` crate; unix uses `libc` (added to workspace deps under a `cfg(unix)` target section). ## Where the guard is applied, and why only there A `PriorityGuard` is only sound where the thread cannot yield - inside a `spawn_blocking` closure or a `fn` with no `.await`. Holding one across an `.await` would demote whatever thread the task resumed on and leak the demotion on the one it left. **`PriorityGuard` is deliberately `!Send`**, so that mistake is a compile error inside any spawned task rather than a runtime mystery. That narrows the apply surface to exactly one site on the backup path today: the executor's `build_bundle` `spawn_blocking` (reads every member off disk and gzips it). Deliberately **not** applied: - The executor's `cpu_stage` / `read_hash_encrypt` / `stream_upload` pipeline - these interleave `.await`s, and toggling per 64-KiB chunk would cost a syscall per chunk for a demotion the thread does not keep anyway. - `restore.rs`, `exclusion_stream.rs`, `sources.rs` blocking work - all user-initiated and in the foreground. A throttled restore or a sluggish exclusion preview is a regression, not a feature. - `scanner.rs` - untouched on purpose, since `feat/scan-parallel-pruning` is rewriting its walk internals into parallel workers. Those dedicated, Driven-owned worker threads are the natural consumer of `apply_to_current_thread` (including the Linux nice bump), and can adopt it once that PR lands. The API is in place for exactly that. ### Scope caveat - please read before judging the effect Because `build_bundle` is the only site, this PR shapes **bundled small-file uploads only**. A backup dominated by large files will see no measurable change from flipping the setting: that path's disk reads happen on `tokio::fs`'s internal blocking pool, which Driven has no handle on, and its hashing/encryption stages interleave `.await`s. Likewise the scanner's walk and deep-verify hashing run inline on the async task today. So the honest framing is: this PR lands the mechanism, the settings plumbing, and the one site where a guard is sound right now. The broad user-visible win lands when the scanner's dedicated worker threads (from `feat/scan-parallel-pruning`) call `apply_to_current_thread` at startup - a one-line adoption, which is why the API has that shape. ## Settings plumbing `OrchestratorConfig` gains `io_priority: WorkPriority`. `load_orchestrator_config` parses it from the persisted `global` blob, and the assembly creates one `PriorityCell` cloned into both the executor (the reader) and the orchestrator (the writer) - the same one-Arc-into-two-consumers wiring already used for the pacer, upload pool, and latency reservoir. `SyncOrchestrator::reconfigure` republishes the cell, so **a settings save applies to work that starts after it** without an app restart; work already in flight keeps the level it began with. `OrchestratorConfig` is never persisted or sent over IPC, so the new field needs no `serde(default)`. ## Behaviour change to call out Migration 0002 seeds `io_priority: "low"`, so **every existing install starts running its bundle builds one CPU notch below normal after this lands.** That is the point of the request, but it is a live default change, not opt-in. The Rust `OrchestratorConfig::default()` stays `Normal`, so tests, the chaos harness, and any settings-load failure behave exactly as before. ## Tests 11 unit tests in `priority.rs`, plus wiring assertions: - `from_setting` mapping, case/whitespace leniency, unknown-degrades-to-normal, round trip, and default-is-Normal. - `PriorityCell` sharing across clones. - Every level applies and restores without panicking, each on its own thread. - **`cfg(windows)`**: `Low` is observably `THREAD_PRIORITY_BELOW_NORMAL` via `GetThreadPriority`, and the guard hands the thread back at `THREAD_PRIORITY_NORMAL`. `GetThreadPriority` cannot report background mode, so the `Idle` test instead proves the guard issued its `THREAD_MODE_BACKGROUND_END` by asserting a fresh `BACKGROUND_BEGIN` succeeds - Windows fails that call while the thread is still in background mode. No test needs elevation. - `orchestrator.rs`: the shared cell is seeded at build time and republished on `reconfigure` (including back down to `Normal`). - `assembly.rs`: the cold-start config test now asserts a persisted `"low"` arrives as `WorkPriority::Low`. ## Gates - `cargo fmt --all -- --check` clean - `cargo clippy --workspace --all-targets -- -D warnings` clean - `cargo test -p driven-core`: 419 passed, 0 failed - `cargo test -p driven-app`: 295 passed, 0 failed - No `ui/` changes. LF endings, ASCII dashes only. Linux and macOS backends are `cfg`-gated and could not be executed on the Windows dev host; CI's `ubuntu-latest` / `macos-latest` legs are the check. ## Docs `design/DESIGN.md` s11.2 previously described an unimplemented plan that named `SetPriorityClass` (process-wide, which would drag the UI/IPC threads down too). Updated to describe what actually shipped. SPEC s22 only enumerates the setting values and is still accurate, so it is untouched. Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01JLB3E2Jm7knNJd37fVpH8X --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 8b983b3 commit 51bb1f3

10 files changed

Lines changed: 1010 additions & 8 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,11 @@ clap = { version = "4", features = ["derive", "env"] }
120120
# reproducible cross-platform build; matches the app's no-native-deps posture).
121121
tar = "0.4"
122122
flate2 = { version = "1", default-features = false, features = ["rust_backend"] }
123+
# Raw POSIX calls the std library does not expose: `ioprio_set` / `setpriority`
124+
# for the below-normal backup-thread priority (`driven_core::priority`), and the
125+
# chaos harness's fault injection. Pulled per-crate under a `cfg(unix)` target
126+
# section - the Windows backends call kernel32 directly.
127+
libc = "0.2"
123128

124129
[profile.dev]
125130
opt-level = 1

crates/driven-core/Cargo.toml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,13 @@ md5 = { workspace = true }
7676
tar = { workspace = true }
7777
flate2 = { workspace = true }
7878

79+
# Below-normal CPU + I/O priority for backup threads (SPEC s22 `io_priority`,
80+
# `driven_core::priority`). Unix only: Linux needs the `ioprio_set` syscall
81+
# number and `setpriority`, macOS needs `setpriority` for the Darwin background
82+
# band. The Windows backend calls kernel32 directly, so no dependency there.
83+
[target.'cfg(unix)'.dependencies]
84+
libc = { workspace = true }
85+
7986
[dev-dependencies]
8087
# `test-util` enables `#[tokio::test(start_paused = true)]` so the orchestrator
8188
# run-loop scheduled-tick test drives `tokio::time::interval` on virtual time

crates/driven-core/src/executor.rs

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -889,6 +889,14 @@ pub struct DefaultExecutor {
889889
/// `ensure_folder` per component instead of racing duplicate
890890
/// search+create calls (which manifest as duplicate folders on Drive).
891891
parent_walk: tokio::sync::Mutex<()>,
892+
/// The live backup-work priority (SPEC s22 `io_priority`), written by the
893+
/// orchestrator on every settings change when the app assembly wires the
894+
/// SAME cell into both (via [`Self::with_priority_cell`]). Read at each
895+
/// blocking-work site rather than captured once, so a settings save applies
896+
/// without a restart. Defaults to a private cell holding
897+
/// [`WorkPriority::Normal`], which is why every test and the chaos harness
898+
/// run at normal priority unless they opt in.
899+
priority: crate::priority::PriorityCell,
892900
/// Cooperative "stop dispatching new ops" flag (DESIGN s5.7 manual pause).
893901
/// Written by [`Executor::set_paused`], read by the `execute` dispatch loop
894902
/// before it starts each op. It is a dispatch gate, not a cancel: ops
@@ -975,6 +983,7 @@ impl DefaultExecutor {
975983
throughput: None,
976984
mem_gauge: None,
977985
latency: None,
986+
priority: crate::priority::PriorityCell::default(),
978987
parent_dirs: std::sync::Mutex::new(HashMap::new()),
979988
parent_walk: tokio::sync::Mutex::new(()),
980989
paused: AtomicBool::new(false),
@@ -1021,6 +1030,22 @@ impl DefaultExecutor {
10211030
self
10221031
}
10231032

1033+
/// Share the backup-work priority cell (SPEC s22 `io_priority`) with the
1034+
/// orchestrator.
1035+
///
1036+
/// Pass the SAME [`PriorityCell`](crate::priority::PriorityCell) given to
1037+
/// [`SyncOrchestrator::with_priority_cell`](crate::orchestrator::SyncOrchestrator::with_priority_cell),
1038+
/// the same one-cell-into-two-consumers wiring as
1039+
/// [`Self::with_upload_pool`]: the orchestrator writes it when settings
1040+
/// change, this executor reads it when it starts blocking work. Every other
1041+
/// construction path keeps a private cell pinned at
1042+
/// [`WorkPriority`](crate::priority::WorkPriority)`::Normal`.
1043+
#[must_use]
1044+
pub fn with_priority_cell(mut self, priority: crate::priority::PriorityCell) -> Self {
1045+
self.priority = priority;
1046+
self
1047+
}
1048+
10241049
/// Resolve the crypto decision for one source (M5 GA-blocking surface).
10251050
///
10261051
/// Consults the injected [`CryptoProvider`]; a `None` provider means every
@@ -1667,7 +1692,15 @@ impl DefaultExecutor {
16671692
)
16681693
})
16691694
.collect();
1670-
let built = match tokio::task::spawn_blocking(move || {
1695+
// SPEC s22 `io_priority`: this closure reads every member off disk and
1696+
// gzips it, so it is the executor's one genuinely blocking, genuinely
1697+
// heavy section - and being a `spawn_blocking` closure with no `.await`
1698+
// inside, it is somewhere a per-thread priority guard is actually SOUND
1699+
// (see `crate::priority` for why that distinction is load-bearing). The
1700+
// level is read HERE, per bundle, so a settings change applies to the
1701+
// next bundle without a restart.
1702+
let priority = self.priority.get();
1703+
let built = match crate::priority::spawn_blocking(priority, move || {
16711704
crate::bundle::build_bundle(&inputs, crate::planner::BUNDLE_MAX_BYTES_CEILING)
16721705
})
16731706
.await

crates/driven-core/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ pub mod network;
2929
pub mod orchestrator;
3030
pub mod pacer;
3131
pub mod planner;
32+
pub mod priority;
3233
pub mod scanner;
3334
pub mod state;
3435
pub mod telemetry;

crates/driven-core/src/orchestrator.rs

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ use crate::executor::{Executor, OpOutcome};
5858
use crate::hooks::{CommandRunner, HookKind, NoopCommandRunner};
5959
use crate::network::{NetworkProbe, NetworkState, ServiceHealth, ServiceName};
6060
use crate::pacer::{Pacer, PacerCeilings};
61+
use crate::priority::{PriorityCell, WorkPriority};
6162
use crate::state::{ActivityLevel, NewActivity, SourceRow, StateRepo};
6263
use crate::time::Clock;
6364
use crate::types::{
@@ -209,6 +210,15 @@ pub struct OrchestratorConfig {
209210
/// kill-switch - the pool stays FIXED at
210211
/// [`default_concurrent_uploads`](Self::default_concurrent_uploads).
211212
pub adaptive_parallelism_enabled: bool,
213+
/// How far below normal the threads doing backup work run (SPEC s22
214+
/// `global.io_priority`), so Driven yields to whatever the user has in the
215+
/// foreground.
216+
///
217+
/// [`Orchestrator::reconfigure`] publishes this into the
218+
/// [`PriorityCell`](crate::priority::PriorityCell) shared with the
219+
/// executor, so a settings save applies to work that STARTS after it; work
220+
/// already in flight keeps the level it began with.
221+
pub io_priority: WorkPriority,
212222
}
213223

214224
/// What Driven does on a metered network when
@@ -258,6 +268,11 @@ impl Default for OrchestratorConfig {
258268
metered_bandwidth_cap_mbps: None,
259269
default_concurrent_uploads: None,
260270
adaptive_parallelism_enabled: true,
271+
// Normal, not the settings seed of `low`: the code default is what
272+
// a test / the chaos harness / a settings-load failure gets, and
273+
// "behave exactly as before" is the right fallback there. The
274+
// persisted setting is what demotes a real install.
275+
io_priority: WorkPriority::Normal,
261276
}
262277
}
263278
}
@@ -428,6 +443,17 @@ pub struct SyncOrchestrator {
428443
/// network goes on / off metered. `None` (the default / tests) disables the
429444
/// runtime throttle; the cap then stays at its construction value.
430445
pacer: Option<Arc<dyn Pacer>>,
446+
/// The live backup-work priority (SPEC s22 `io_priority`), shared with the
447+
/// executor by the app assembly the same way [`Self::pacer`] is.
448+
///
449+
/// The orchestrator is the WRITER: it publishes
450+
/// [`OrchestratorConfig::io_priority`] here at construction and on every
451+
/// [`Orchestrator::reconfigure`], because it is the only component that
452+
/// sees a settings change. The executor is the reader, consulting it when
453+
/// it starts a piece of blocking work. When the assembly does not wire a
454+
/// shared cell (tests, the chaos harness) this is a private cell nobody
455+
/// reads, and the executor stays at [`WorkPriority::Normal`].
456+
priority: PriorityCell,
431457
/// Per-orchestrator record-at-create ledger (P1-A). The recorder hook wired
432458
/// into the provider by [`Self::with_vss`] pushes each freshly-created
433459
/// shadow GUID here synchronously; `record_vss_orphans` drains it into the
@@ -487,6 +513,7 @@ impl SyncOrchestrator {
487513
let (trigger_tx, trigger_rx) = mpsc::channel(1);
488514
let (watcher_tx, watcher_rx) = mpsc::channel(WATCHER_CHANNEL_CAPACITY);
489515
let (shutdown_tx, shutdown_rx) = watch::channel(false);
516+
let priority = PriorityCell::new(config.io_priority);
490517
Self {
491518
account_id,
492519
state,
@@ -508,6 +535,7 @@ impl SyncOrchestrator {
508535
vss: None,
509536
command_runner: Arc::new(NoopCommandRunner),
510537
pacer: None,
538+
priority,
511539
vss_create_ledger: Arc::new(std::sync::Mutex::new(std::collections::HashMap::new())),
512540
orphan_cleanup_done: Mutex::new(false),
513541
suspended: std::sync::atomic::AtomicBool::new(false),
@@ -533,6 +561,27 @@ impl SyncOrchestrator {
533561
self
534562
}
535563

564+
/// Share the backup-work priority cell (SPEC s22 `io_priority`) with the
565+
/// executor.
566+
///
567+
/// Pass the SAME [`PriorityCell`] given to
568+
/// [`DefaultExecutor::with_priority_cell`](crate::executor::DefaultExecutor::with_priority_cell),
569+
/// exactly like [`Self::with_pacer`]: this orchestrator is the only writer
570+
/// (on construction and on every [`Orchestrator::reconfigure`]) and the
571+
/// executor is the reader. Seeded immediately from the config this
572+
/// orchestrator was built with, so the very first cycle already runs at the
573+
/// user's configured level. Without this call the executor keeps its own
574+
/// cell and every thread runs at [`WorkPriority::Normal`].
575+
#[must_use]
576+
pub fn with_priority_cell(mut self, priority: PriorityCell) -> Self {
577+
// Carry over what `new` seeded from the config rather than reaching
578+
// through the config lock - this builder runs inside the assembly's
579+
// async fn, where a blocking lock acquisition would panic.
580+
priority.set(self.priority.get());
581+
self.priority = priority;
582+
self
583+
}
584+
536585
/// Attach the app-global latency reservoir (DESIGN s13 telemetry) so each
537586
/// scan records per-file processing latency. Pass the SAME `Arc` given to the
538587
/// executor via
@@ -2443,6 +2492,12 @@ impl Orchestrator for SyncOrchestrator {
24432492
if let Some(vss) = self.vss.as_ref() {
24442493
vss.set_mode(config.vss_mode);
24452494
}
2495+
// Publish the backup-work priority (SPEC s22 `io_priority`) into the
2496+
// cell the executor reads. Same reason as the VSS mode above: the
2497+
// setting would otherwise be frozen at whatever the orchestrator was
2498+
// built with. Work that has already started keeps its level; work that
2499+
// starts after this call picks the new one up.
2500+
self.priority.set(config.io_priority);
24462501
*self.config.write().await = config;
24472502
}
24482503

@@ -3456,6 +3511,66 @@ mod tests {
34563511
);
34573512
}
34583513

3514+
/// SPEC s22 `io_priority`: the app assembly hands ONE
3515+
/// [`PriorityCell`] to the orchestrator and the executor, and the
3516+
/// orchestrator is the writer. Construction must seed the shared cell from
3517+
/// the config, and `reconfigure` must republish it - otherwise a settings
3518+
/// save would be inert until the app restarted (exactly the bug that made
3519+
/// `vss_mode` inert before P1-5).
3520+
#[tokio::test]
3521+
async fn priority_cell_is_seeded_at_build_and_republished_on_reconfigure() {
3522+
let account = AccountId::new_v4();
3523+
let dir = tempfile::tempdir().unwrap();
3524+
let cell = PriorityCell::new(WorkPriority::Normal);
3525+
let (orch, _clock) = build(
3526+
account,
3527+
vec![source_in(account, dir.path())],
3528+
Arc::new(RecordingExecutor::default()),
3529+
power_on_ac(),
3530+
Arc::new(FakeNet::online()),
3531+
OrchestratorConfig {
3532+
io_priority: WorkPriority::Low,
3533+
..OrchestratorConfig::default()
3534+
},
3535+
);
3536+
// The executor's view of the cell before any settings edit.
3537+
let orch = orch.with_priority_cell(cell.clone());
3538+
assert_eq!(
3539+
cell.get(),
3540+
WorkPriority::Low,
3541+
"the first cycle must already run at the configured level, with no settings edit"
3542+
);
3543+
3544+
orch.reconfigure(OrchestratorConfig {
3545+
io_priority: WorkPriority::Idle,
3546+
..OrchestratorConfig::default()
3547+
})
3548+
.await;
3549+
assert_eq!(
3550+
cell.get(),
3551+
WorkPriority::Idle,
3552+
"a settings save must reach the executor's cell"
3553+
);
3554+
3555+
orch.reconfigure(OrchestratorConfig::default()).await;
3556+
assert_eq!(
3557+
cell.get(),
3558+
WorkPriority::Normal,
3559+
"turning the setting back to normal must un-demote future work"
3560+
);
3561+
}
3562+
3563+
/// The default config must not demote anything: a test, the chaos harness,
3564+
/// or a settings-load failure has to behave exactly as it did before this
3565+
/// feature existed.
3566+
#[test]
3567+
fn default_config_runs_backup_work_at_normal_priority() {
3568+
assert_eq!(
3569+
OrchestratorConfig::default().io_priority,
3570+
WorkPriority::Normal
3571+
);
3572+
}
3573+
34593574
#[tokio::test]
34603575
async fn ac_resumes_after_battery_pause() {
34613576
// Power gate: pause on battery, resume on AC (the two-cycle path).

0 commit comments

Comments
 (0)