Skip to content

Commit adc81fa

Browse files
pmaxhoganclaude
andauthored
fix(core): stream the resumable-upload resume instead of buffering the whole file (#279)
## Incident (2026-08-14) Quitting Driven mid-upload of an 88.6 GB file (`dev-drives/dev.vhdx`) and relaunching made the app consume >10 GB of RAM in ~30 s (at disk read speed) until Windows killed it - and it repeated on every relaunch. Diagnostics zip + the laptop's `pending_ops` row confirmed the mechanism: - The startup **reconcile** (runs before any scan, so the UI still shows the indeterminate state) found the persisted resumable session (`size=88655003648`, `acked_offset=7436500992`), passed the resume-identity gate, and called `read_hash_encrypt` - which **buffers the entire upload body into one `Vec`**. The live upload path has streamed with bounded channels since DESIGN s11.4.3; only the restart-resume path (and the adopt re-hash) still used the legacy buffered read. - The OOM kill happens before `delete_pending_op`, so the op survives and every launch retries the same 88 GB read. - The whole fatal phase logged **nothing** at INFO - the diagnostics bundle was blind to it. ## Fix - **`resume_persisted` now streams** via a pass-based design: one sequential read that hashes every byte, discards the already-acked prefix, and pushes the tail in bounded wire chunks (~2 in flight, `ResumeAcc` RAII-guarded MemGauge accounting that stays balanced even on `?` error unwinds). The pacer is charged only for pushed wire bytes. - **Backend offset semantics** are interpreted against the streaming window (`offset` always equals the file position of the accumulator's first byte): driven-s3's one-shot post-hydration rewind-to-0 and sftp/localfs resyncs RESTART the pass from the store's stated offset (one-restart budget); in-window forward offsets drain exactly the newly covered bytes; an exact stall abandons. Without this, every S3 resume with prior progress degraded to a full re-upload. - **Legacy rows** (recorded hash, no identity) are verified by a hash-only prepass before any byte is pushed; identity-carrying rows get a cheap EOF re-fstat that aborts a mid-stream-modified file before the finalizing chunk (md5-vs-store remains the correctness backstop). - **`rehash_local_plaintext` streams** (hash-only): its old callee's plaintext arm buffered the whole body despite the comment claiming otherwise. - **Excluded paths**: reconcile skips only the *resume* for a path the user has since excluded and lets adopt-or-requeue run - a finalized object is adopted into a `file_state` row (never left as an invisible untracked orphan on the remote), an unfinalized one requeues into nothing (the next scan excludes the path; the abandoned session is GC'd). - **`push_chunks`** (fresh-session uploads) gets the same no-progress stall guard the resume path has. ## Instrumentation (make the next diagnostics zip self-diagnosing) - INFO breadcrumbs: cycle start with tick source, orchestrator state transitions (variant *name* only - never the Error variant's free-text details, which could leak partially-redacted paths), reconcile entry with pending-op count, resume start/abandon/restart with sizes + offsets + reasons. - RSS watchdog task (`driven::app::memlog`): samples every 15 s on the blocking pool, logs on >=128 MiB movement, keeps a trailing ~15 min window. - Diagnostic bundle: new `pending_ops.txt` (op shapes, sizes, offsets, ages, recovery-flag presence; session URLs are capability secrets and are never included) and `memory.txt` (current + peak RSS, a `sample=fresh|stale-or-unread` honesty flag, and the trailing sample window). Redaction policy text updated in the same change. ## Review An ultra multi-agent review ran against the first cut; its confirmed findings (S3 rewind misread, MemGauge leak on error unwind, excluded-op orphan gap, push-before-validate on legacy rows, state-transition Debug leak, plus several consistency nits) are fixed in the follow-up commit. Refuted findings (e.g. mid-stream prefix modification "undetected" - the full-stream md5 vs the store's stored md5 catches exactly that) are documented in code comments. ## Tests - `crash_mid_upload_resumes_persisted_session_byte_for_byte` asserts MemGauge `peak > 0` (proves the resume routes through the instrumented streaming loop) and `peak <= 3 * WIRE_CHUNK` (boundedness). - New: `resume_honours_a_one_shot_rewind_to_zero` (S3 contract; red on the stall-guard misread), `resume_error_midstream_keeps_the_mem_gauge_balanced` (red without the RAII guard), `reconcile_skips_resuming_a_now_excluded_paths_session` (+ control proving the gate), `reconcile_adopts_finalized_orphan_even_when_path_now_excluded` (red on a drop-the-op design), `pending_ops_summary_names_resumable_sessions_without_urls`, memlog sampler tests. - Full workspace suite green locally (macOS): driven-core 547 lib + 23 e2e + integration suites, driven-app 437. README checked - no changes needed (it does not enumerate bundle contents, and no feature claims changed). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01A7q3CvJzL4zZmDA9CbXyQQ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 4175050 commit adc81fa

8 files changed

Lines changed: 1945 additions & 108 deletions

File tree

Cargo.lock

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

crates/driven-core/src/executor.rs

Lines changed: 1357 additions & 104 deletions
Large diffs are not rendered by default.

crates/driven-core/src/orchestrator.rs

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,24 @@ const VSS_ORPHAN_SETTING_KEY: &str = "vss.orphans";
9999
/// `tokio::sync::Mutex` (held across the `.await`s of the DB read + write) makes
100100
/// each account's whole RMW atomic with respect to the others. `OnceLock` so
101101
/// every orchestrator in the process shares the same instance.
102+
/// The bare variant name of an [`OrchestratorState`], for the transition log
103+
/// (2026-08-14 incident). Deliberately NOT the Debug form: `Error` carries
104+
/// free-text details that can embed local paths, which must never enter a log
105+
/// line un-redacted.
106+
fn state_name(state: &OrchestratorState) -> &'static str {
107+
match state {
108+
OrchestratorState::Idle { .. } => "idle",
109+
OrchestratorState::PowerCheck => "power_check",
110+
OrchestratorState::Scanning { .. } => "scanning",
111+
OrchestratorState::Planning { .. } => "planning",
112+
OrchestratorState::Executing { .. } => "executing",
113+
OrchestratorState::Verifying { .. } => "verifying",
114+
OrchestratorState::Backoff { .. } => "backoff",
115+
OrchestratorState::Paused { .. } => "paused",
116+
OrchestratorState::Error { .. } => "error",
117+
}
118+
}
119+
102120
fn orphan_registry_lock() -> &'static tokio::sync::Mutex<()> {
103121
static LOCK: std::sync::OnceLock<tokio::sync::Mutex<()>> = std::sync::OnceLock::new();
104122
LOCK.get_or_init(|| tokio::sync::Mutex::new(()))
@@ -932,7 +950,28 @@ impl SyncOrchestrator {
932950
/// a tray-facing event - the next [`Orchestrator::state`] read still
933951
/// reflects the stored state.
934952
async fn transition(&self, next: OrchestratorState) {
935-
*self.state_machine.write().await = next.clone();
953+
let changed = {
954+
let mut guard = self.state_machine.write().await;
955+
let changed = *guard != next;
956+
*guard = next.clone();
957+
changed
958+
};
959+
// 2026-08-14 incident: log every REAL state change at INFO so a
960+
// diagnostics bundle shows which phase (reconcile / scan / plan /
961+
// execute) the app was in when something went sideways - the
962+
// incident's bundle had no phase breadcrumbs at all. A handful of
963+
// lines per cycle; same-state re-writes (e.g. Idle timestamp bumps on
964+
// consecutive quiet cycles carry differing `last_run_at`, so those
965+
// still log once per cycle) are the volume ceiling.
966+
//
967+
// Log the variant NAME only, never the Debug dump: the `Error`
968+
// variant carries free-text error details that routinely embed local
969+
// paths, and the bundle redactor's unquoted-run scanner truncates a
970+
// path at its first space - so a raw `?next` here would leak
971+
// partially-redacted paths into exported bundles.
972+
if changed {
973+
tracing::info!(target: TARGET, account_id = %self.account_id, state = state_name(&next), "state transition");
974+
}
936975
let _ = self
937976
.events
938977
.send(OrchestratorEvent::StateChanged { state: next });
@@ -2340,6 +2379,12 @@ impl SyncOrchestrator {
23402379
return Ok(());
23412380
}
23422381

2382+
// 2026-08-14 incident: one INFO breadcrumb per cycle naming what
2383+
// triggered it. The incident's diagnostics bundle could not even show
2384+
// whether the user's "Sync now" click had reached the orchestrator -
2385+
// the whole fatal phase (reconcile, pre-scan) logged nothing at INFO.
2386+
tracing::info!(target: TARGET, account_id = %self.account_id, ?tick, "cycle start");
2387+
23432388
// P1-6 (DESIGN s5.6, s5.7): the startup reconcile (DESIGN s5.6) is a
23442389
// REMOTE pass - it issues Drive find/metadata calls to adopt orphaned
23452390
// objects - so it MUST come AFTER the power / network / manual gates,

crates/driven-core/tests/e2e_fake.rs

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -947,6 +947,17 @@ async fn crash_mid_upload_resumes_persisted_session_byte_for_byte() {
947947

948948
// --- phase 2: a fresh executor reconciles -> resumes the session --------
949949
// The network drop was single-shot, so phase 2's requests all succeed.
950+
//
951+
// 2026-08-14 OOM regression guard: the resume re-read must STREAM, never
952+
// buffer the body (the original implementation read the ENTIRE file into
953+
// one `Vec` during reconcile - an interrupted 88 GB upload then OOM-killed
954+
// the app on every launch). The MemGauge is bumped by the resume loop as
955+
// bytes are buffered and released as Drive acks them; `peak > 0` proves
956+
// the resume actually routed through the instrumented streaming path (a
957+
// non-instrumented buffered path would read 0 and pass a bare `<=` bound
958+
// vacuously), and the ceiling proves boundedness: ~2 wire chunks in
959+
// flight, far below the ~16 MiB tail this file resumes.
960+
let resume_gauge = Arc::new(MemGauge::default());
950961
let exec2 = DefaultExecutor::with_clock(
951962
ExecutorDeps {
952963
remote: remote.clone(),
@@ -957,9 +968,21 @@ async fn crash_mid_upload_resumes_persisted_session_byte_for_byte() {
957968
network: None,
958969
},
959970
clock.clone(),
960-
);
971+
)
972+
.with_mem_gauge(resume_gauge.clone());
961973
exec2.reconcile(&src).await.unwrap();
962974

975+
let peak = resume_gauge.peak();
976+
assert!(
977+
peak > 0,
978+
"the resume must route through the gauge-instrumented streaming loop"
979+
);
980+
assert!(
981+
peak <= 3 * WIRE_CHUNK as u64,
982+
"resume memory must stay bounded at a few wire chunks regardless of \
983+
file size; peak was {peak} bytes for a {total_len}-byte file"
984+
);
985+
963986
// The upload completed via byte-level resume: exactly one object, and it
964987
// carries the full byte count (proving the resumed tail bytes landed, NOT a
965988
// truncated or from-zero re-do).

src-tauri/Cargo.toml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,11 @@ windows-sys = { version = "0.61", features = [
193193
# DwmSetWindowAttribute: force a dark titlebar/border so the native window
194194
# chrome matches Driven's dark theme instead of the user's Windows accent.
195195
"Win32_Graphics_Dwm",
196+
# memlog.rs (2026-08-14 incident): K32GetProcessMemoryInfo +
197+
# GetCurrentProcess for the RSS watchdog behind the diagnostic bundle's
198+
# memory samples.
199+
"Win32_System_ProcessStatus",
200+
"Win32_System_Threading",
196201
] }
197202

198203
[dev-dependencies]

0 commit comments

Comments
 (0)