Skip to content

Commit af8f048

Browse files
pmaxhoganclaude
andauthored
feat(core): shape bundle-build file reads with the io_priority setting (#179)
Closes the asymmetry I flagged in #176. ## The gap After #176, `io_priority` shaped large-file upload reads through the per-handle hint. Bundled small-file reads were still not shaped: they go through `build_bundle`'s own opens, covered only by the *thread* guard from #170 - and on Windows `low` maps to `THREAD_PRIORITY_BELOW_NORMAL`, which is CPU-only. So a `low` backup of a folder full of small files still read at normal I/O priority, which is exactly the workload someone is most likely to reach for when testing the setting. (At `idle` this was already covered: `THREAD_MODE_BACKGROUND_BEGIN` lowers I/O and memory priority alongside CPU. The gap was `low`-only.) ## The change `build_bundle` takes a `WorkPriority` and hints each member's file handle as it opens it. The executor passes the same value it already reads for the thread guard, so both levers run at one level and a settings change still takes effect per bundle with no restart. The bundle path is the one place that visibly needs **both** levers, which is what makes the two-lever split concrete: the thread guard covers the gzip CPU, the handle hint covers the reads. ## Why `std::fs::read` had to go `std::fs::read` opens and reads in one call and never exposes the `File`, so there is no handle to hint - and the hint has to land on the handle *before* the reads it is meant to shape. It is now split into a small `read_member` helper that opens, hints, then reads. Behaviour is deliberately identical: - **Same share mode.** `std::fs::read` uses `File::open` internally, so the open's sharing/locking semantics are unchanged - which matters because the locked-file/VSS logic elsewhere depends on those semantics. - **Same allocation.** The buffer is still sized from a stat, reusing the `pre` stat the loop already took instead of re-statting. - **The size argument is a capacity hint, never a read bound.** It deliberately does not short-circuit the read. Treating it as a bound would hide a grew-mid-read member from the caller's post-read coherency stat, which remains the sole judge of whether the bytes are a usable snapshot. Nothing else in the build loop moved: the pre-stat size re-validation, the accumulated-bytes ceiling, the post-read coherency check, and the skip bookkeeping are untouched. ## Tests - `priority_does_not_change_the_archive_or_the_members` - builds the same inputs (including one member that must be skipped) at all three levels and asserts the `.tar.gz` bytes, the packed members, and the skip list are identical. Byte-equality is a meaningful assertion here because the gzip layer is written with a zeroed mtime for reproducibility, so two builds over the same inputs are bit-identical. This is the guard that `io_priority` stays a pure scheduling hint. - `read_member_matches_fs_read_and_ignores_a_wrong_size_hint` - all bytes come back at size hints of 0, exact, and oversized, and a missing file errors rather than returning short, matching `std::fs::read` exactly. ## Gates - `cargo fmt --all -- --check` clean - `cargo clippy --workspace --all-targets -- -D warnings` clean - `cargo test -p driven-core`: 461 passed, 0 failed - `cargo test -p driven-app`: 296 passed; `driven-chaos`: 44 passed - LF endings, ASCII dashes only. No `ui/` changes. Unlike #176, this diff contains **no `cfg`-gated code** - `read_member` is plain `std`, and the per-OS branching all lives inside the already-cross-checked `apply_to_file_handle`. So there is no platform-divergent surface for the Windows-only local run to have missed; CI's ubuntu and macos legs are the confirmation. ## Platform reality, unchanged from #176 This is a Windows-only win. Linux and macOS have no per-descriptor I/O priority - both scope it to the thread - so `apply_to_file_handle` is a no-op there and bundle reads keep whatever the thread guard gives them. ## Docs `design/DESIGN.md` s11.2 previously named `open_shared` as the single hint site. Updated: both `open_shared` and `build_bundle` hint handles now, with a note on why the bundle path needs both levers and why the hint is redundant-but-harmless at `idle`. 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 8f49570 commit af8f048

4 files changed

Lines changed: 114 additions & 12 deletions

File tree

crates/driven-core/src/bundle.rs

Lines changed: 94 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,17 @@ fn mtime_ns(meta: &std::fs::Metadata) -> i64 {
129129
/// could not be read, or its `(size, mtime)` changed between the two stats or
130130
/// disagreed with the bytes read, so only a coherent snapshot is ever packed. The
131131
/// gzip layer is written with a zeroed mtime for reproducibility.
132+
///
133+
/// `priority` (SPEC s22 `io_priority`) is hinted onto each member's file handle
134+
/// as it is opened, so these reads are serviced below normal instead of
135+
/// competing with whatever the user has in the foreground. It is purely a
136+
/// scheduling hint: it changes nothing about which members are packed, what
137+
/// bytes they contain, or which are skipped. See
138+
/// [`crate::priority::apply_to_file_handle`] for the per-OS reality.
132139
pub fn build_bundle(
133140
inputs: &[(RelativePath, PathBuf, u64)],
134141
max_total_bytes: u64,
142+
priority: crate::priority::WorkPriority,
135143
) -> Result<BuildOutput> {
136144
use flate2::{Compression, GzBuilder};
137145

@@ -167,7 +175,7 @@ pub fn build_bundle(
167175
continue;
168176
}
169177

170-
let bytes = match std::fs::read(path) {
178+
let bytes = match read_member(path, pre.len(), priority) {
171179
Ok(b) => b,
172180
Err(_) => {
173181
skipped.push(rel.clone());
@@ -225,6 +233,33 @@ pub fn build_bundle(
225233
})
226234
}
227235

236+
/// Read one bundle member's bytes with the SPEC s22 `io_priority` hint attached
237+
/// to its handle.
238+
///
239+
/// This is `std::fs::read` split open so there is a handle to hint: that call
240+
/// opens and reads in one step and never exposes the `File`, and the hint has to
241+
/// land on the handle BEFORE the reads it is meant to shape. Behaviour is
242+
/// otherwise identical - same default share mode (`File::open` is what
243+
/// `std::fs::read` uses internally, so the open's sharing/locking semantics are
244+
/// unchanged), and the same "size the buffer from the stat we already took"
245+
/// allocation, using the caller's `pre` stat rather than re-statting.
246+
///
247+
/// `expected_size` is only a capacity hint. It is deliberately NOT trusted as a
248+
/// read bound: the caller's post-read coherency stat is what decides whether the
249+
/// bytes are a usable snapshot, and short-circuiting here would hide a
250+
/// grew-mid-read member from that check.
251+
fn read_member(
252+
path: &std::path::Path,
253+
expected_size: u64,
254+
priority: crate::priority::WorkPriority,
255+
) -> std::io::Result<Vec<u8>> {
256+
let mut file = std::fs::File::open(path)?;
257+
crate::priority::apply_to_file_handle(&file, priority);
258+
let mut bytes = Vec::with_capacity(usize::try_from(expected_size).unwrap_or(0));
259+
file.read_to_end(&mut bytes)?;
260+
Ok(bytes)
261+
}
262+
228263
/// Extract one member's plaintext bytes from a decompressed-in-memory `.tar.gz`
229264
/// bundle by its [`member_entry_name`]. Returns `Ok(None)` if no such entry
230265
/// exists. `max_decompressed` bounds the TOTAL bytes read from the gzip stream (a
@@ -275,6 +310,7 @@ pub fn extract_member(
275310
#[cfg(test)]
276311
mod tests {
277312
use super::*;
313+
use crate::priority::WorkPriority;
278314

279315
fn rel(s: &str) -> RelativePath {
280316
RelativePath::try_from(s.to_string()).expect("valid relative path")
@@ -295,6 +331,58 @@ mod tests {
295331
/// any fixture's total).
296332
const TEST_MAX_TOTAL: u64 = 8 * 1024 * 1024;
297333

334+
/// SPEC s22 `io_priority` is a SCHEDULING hint and nothing more: the archive
335+
/// bytes, the packed members, and the skip list must be identical at every
336+
/// level. Byte-equality is a meaningful assertion here because the gzip
337+
/// layer is written with a zeroed mtime for reproducibility, so two builds
338+
/// over the same inputs are bit-identical.
339+
#[test]
340+
fn priority_does_not_change_the_archive_or_the_members() {
341+
let dir = tempfile::tempdir().expect("tempdir");
342+
let mut inputs = Vec::new();
343+
for i in 0..6u8 {
344+
let name = format!("f{i}.txt");
345+
let body = format!("member {i} bytes {i}{i}{i}").into_bytes();
346+
std::fs::write(dir.path().join(&name), &body).expect("write");
347+
inputs.push((rel(&name), dir.path().join(&name), body.len() as u64));
348+
}
349+
// A member that will be skipped, so the skip path is compared too.
350+
inputs.push((rel("gone.txt"), dir.path().join("gone.txt"), 10));
351+
352+
let baseline = build_bundle(&inputs, TEST_MAX_TOTAL, WorkPriority::Normal).expect("build");
353+
assert_eq!(baseline.members.len(), 6);
354+
assert_eq!(baseline.skipped, vec![rel("gone.txt")]);
355+
356+
for level in [WorkPriority::Low, WorkPriority::Idle] {
357+
let out = build_bundle(&inputs, TEST_MAX_TOTAL, level).expect("build");
358+
assert_eq!(out.tar_gz, baseline.tar_gz, "{level:?} changed the archive");
359+
assert_eq!(out.members, baseline.members, "{level:?} changed members");
360+
assert_eq!(out.skipped, baseline.skipped, "{level:?} changed skips");
361+
}
362+
}
363+
364+
/// `read_member` replaced a `std::fs::read` call, so it has to reproduce that
365+
/// call's behaviour exactly: all the bytes on success, and an error (never a
366+
/// truncated read) for a missing file. The `expected_size` argument is a
367+
/// capacity hint only - passing a wrong one must not truncate or pad, because
368+
/// the caller's coherency stat is what decides whether the bytes are usable.
369+
#[test]
370+
fn read_member_matches_fs_read_and_ignores_a_wrong_size_hint() {
371+
let dir = tempfile::tempdir().expect("tempdir");
372+
let path = dir.path().join("payload.bin");
373+
let body = b"the exact bytes that must come back".to_vec();
374+
std::fs::write(&path, &body).expect("write");
375+
376+
for hint in [0, body.len() as u64, 1024] {
377+
let got = read_member(&path, hint, WorkPriority::Idle).expect("read");
378+
assert_eq!(got, body, "size hint {hint} must not change the bytes");
379+
}
380+
assert!(
381+
read_member(&dir.path().join("missing.bin"), 0, WorkPriority::Low).is_err(),
382+
"a missing file must error, exactly as std::fs::read does"
383+
);
384+
}
385+
298386
#[test]
299387
fn build_then_extract_roundtrips_each_member() {
300388
let dir = tempfile::tempdir().expect("tempdir");
@@ -308,7 +396,7 @@ mod tests {
308396
contents.push((rel(&name), body));
309397
}
310398

311-
let out = build_bundle(&inputs, TEST_MAX_TOTAL).expect("build");
399+
let out = build_bundle(&inputs, TEST_MAX_TOTAL, WorkPriority::Normal).expect("build");
312400
assert_eq!(out.members.len(), 12);
313401
assert!(out.skipped.is_empty());
314402

@@ -342,7 +430,7 @@ mod tests {
342430
dir.path().join("big.bin"),
343431
body.len() as u64,
344432
)];
345-
let out = build_bundle(&inputs, TEST_MAX_TOTAL).expect("build");
433+
let out = build_bundle(&inputs, TEST_MAX_TOTAL, WorkPriority::Normal).expect("build");
346434
// A cap below the member size must fail rather than return truncated bytes.
347435
let res = extract_member(&out.tar_gz, &member_entry_name(&rel("big.bin")), 1024);
348436
assert!(res.is_err(), "expected decompressed-cap error");
@@ -356,7 +444,7 @@ mod tests {
356444
(rel("present.txt"), dir.path().join("present.txt"), 2),
357445
(rel("gone.txt"), dir.path().join("gone.txt"), 2),
358446
];
359-
let out = build_bundle(&inputs, TEST_MAX_TOTAL).expect("build");
447+
let out = build_bundle(&inputs, TEST_MAX_TOTAL, WorkPriority::Normal).expect("build");
360448
assert_eq!(out.members.len(), 1);
361449
assert_eq!(out.skipped, vec![rel("gone.txt")]);
362450
}
@@ -380,7 +468,7 @@ mod tests {
380468
),
381469
];
382470

383-
let out = build_bundle(&inputs, TEST_MAX_TOTAL).expect("build");
471+
let out = build_bundle(&inputs, TEST_MAX_TOTAL, WorkPriority::Normal).expect("build");
384472

385473
assert_eq!(
386474
out.skipped,
@@ -412,7 +500,7 @@ mod tests {
412500

413501
// 2500-byte cap: the first two (1000 + 1000) pack; the third (would reach
414502
// 3000) is skipped.
415-
let out = build_bundle(&inputs, 2500).expect("build");
503+
let out = build_bundle(&inputs, 2500, WorkPriority::Normal).expect("build");
416504
assert_eq!(out.members.len(), 2, "two members fit under the ceiling");
417505
assert_eq!(out.skipped.len(), 1, "the overflowing member is skipped");
418506
}

crates/driven-core/src/executor.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1806,9 +1806,15 @@ impl DefaultExecutor {
18061806
// (see `crate::priority` for why that distinction is load-bearing). The
18071807
// level is read HERE, per bundle, so a settings change applies to the
18081808
// next bundle without a restart.
1809+
//
1810+
// The SAME level also goes INTO `build_bundle`, which hints each
1811+
// member's file handle. Both levers are needed here: the thread guard
1812+
// covers the gzip CPU (and, at `idle` on Windows, I/O too via background
1813+
// mode), while the handle hint is what lowers READ priority at `low`,
1814+
// where the Windows thread guard is CPU-only.
18091815
let priority = self.priority.get();
18101816
let built = match crate::priority::spawn_blocking(priority, move || {
1811-
crate::bundle::build_bundle(&inputs, crate::planner::BUNDLE_MAX_BYTES_CEILING)
1817+
crate::bundle::build_bundle(&inputs, crate::planner::BUNDLE_MAX_BYTES_CEILING, priority)
18121818
})
18131819
.await
18141820
{

design/DESIGN.md

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1614,11 +1614,18 @@ the release and the GH Actions build pipeline takes over.
16141614
attaches the hint to the FILE HANDLE instead (`IoPriorityHintLow` for `low`,
16151615
`IoPriorityHintVeryLow` for `idle`), so every read is shaped regardless of
16161616
which thread performs it, and nothing needs restoring because the handle
1617-
closes with the upload. Applied inside `executor::open_shared`, the single
1618-
choke point for every source-file read (live open, VSS snapshot open, resume
1619-
identity check, reconcile re-hash). A read-only handle carries sufficient
1620-
access, so the open's access mask - and therefore its sharing/locking
1621-
behaviour - is unchanged.
1617+
closes with the upload. Applied at both places the executor reads local file
1618+
bytes: `executor::open_shared` (the choke point for every individual-file read
1619+
- live open, VSS snapshot open, resume identity check, reconcile re-hash) and
1620+
`bundle::build_bundle`, which opens each member of a small-file bundle. A
1621+
read-only handle carries sufficient access, so the open's access mask - and
1622+
therefore its sharing/locking behaviour - is unchanged.
1623+
- The bundle path needs BOTH levers, which is what makes the split concrete.
1624+
Its thread guard covers the gzip CPU, but on Windows `low` maps to
1625+
`THREAD_PRIORITY_BELOW_NORMAL`, which is CPU-only - so without the handle
1626+
hint a `low` backup of many small files would still read at normal I/O
1627+
priority. At `idle` the thread's background mode already covers I/O, so the
1628+
hint is redundant there but harmless.
16221629
- Linux and macOS have no per-descriptor equivalent (both scope I/O priority
16231630
to the thread), so upload read I/O is unshaped there. Closing that gap means
16241631
owning the reader thread outright, a streaming-pipeline restructure that

src-tauri/src/commands/restore.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3516,6 +3516,7 @@ mod tests {
35163516
let built = driven_core::bundle::build_bundle(
35173517
&inputs,
35183518
driven_core::planner::BUNDLE_MAX_BYTES_CEILING,
3519+
driven_core::priority::WorkPriority::Normal,
35193520
)
35203521
.unwrap();
35213522
// Plaintext source stores the archive as-is; an encrypted source stores the

0 commit comments

Comments
 (0)