Skip to content

Commit 1fbba98

Browse files
pmaxhoganclaude
andauthored
fix: let a slow add-source wizard finish instead of failing with a phantom disk error (#291)
## The bug (diagnosed from a user diagnostic bundle) Creating a backup source failed every time with **"Driven hit a disk error reading a file"** — with zero real disk errors anywhere and **zero trace of the six failed `add_source` calls in the logs**. Root cause chain: 1. `add_source` resolved the local folder from a single-use dialog token with a **5-minute TTL**. A real wizard session (big-tree exclusion preview + Drive destination browsing + thinking) outlives that, so Finish always failed. 2. `take_dialog_token` consumed the token **before** any validation, so the first failed Finish destroyed it and every retry in the same session failed too. 3. Both failure modes were mapped to `local.io_error`, whose copy tells the user their **disk** is failing. 4. Command rejections were never logged, so the diagnostic bundle contained nothing to diagnose with. ## The fix - **`add_source` peeks up front and spends the token only at commit** (the same R3-P2-1 pattern the restore command already uses), so failed validation leaves the token intact. - **`DIALOG_TOKEN_TTL` 5 min → 60 min**; the binding now stores its expiry instant (testable without `Instant` underflow). - **New stable code `internal.stale_dialog_token`** (SPEC §24 + en-US copy "pick the folder again") replaces the `local.io_error` mapping for unknown/spent/expired tokens across `add_source`, exclusion previews, restore, and the diagnostic export. The dialog-*cancel* sentinel is untouched. - **Every `CommandError` is now logged (WARN) as it serialises across the IPC boundary** — the single seam every rejection passes through — so failures land in the rolling log and diagnostic bundles. Known trade-off: a UI polling loop against a persistently failing command writes one WARN per poll; accepted for diagnosability. - **Wizard error UI**: the add-source and setup wizards render a muted, monospace technical-detail line (stable code + redacted backend message) under the localized error. ## ⚠️ Deliberate amendment to the R8-P2-1 contract R8-P2-1 said backend English must never render. This PR **narrows** that: the localized `t(errors.${code}.long)` string remains the only primary error, but a dedicated muted detail element may carry the stable code + backend `message`. Rationale: two failures sharing one code (expired token vs. genuinely unreadable folder) were indistinguishable on screen, which is how a token expiry masqueraded as a disk error. The `recovery-reveal-error-i18n` test is updated to enforce the new shape (primary line exactly localized; backend text only inside `*-error-detail`). ## Tests - `cargo check --workspace --all-targets` clean; `cargo fmt --check` clean (dockerized); driven-core 555 + driven-app 417 lib tests + ipc_path_validation pass - New: token-expiry rejection test (`expired_dialog_token_is_rejected_by_peek_and_take`), stale-token i18n copy test, detail-line rendering assertions - UI: eslint 0 errors, prettier clean, vue-tsc clean, 778 vitest pass README checked, no changes needed (no stale claims; behavior-level fix below the feature list). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LRMdu3VkuhL6Ny6hRcnFpU --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent f37ff68 commit 1fbba98

17 files changed

Lines changed: 354 additions & 69 deletions

File tree

crates/driven-core/src/types.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1178,6 +1178,16 @@ pub enum ErrorCode {
11781178
/// not an internal programming error, so the UI shows a "check your input"
11791179
/// message rather than "please report a bug".
11801180
InvalidInput,
1181+
/// `internal.stale_dialog_token` - a path-bearing command presented a
1182+
/// backend-minted dialog token that is unknown, already spent, or past its
1183+
/// TTL (SPEC s11.6.1 C1). The remedy is always the same: re-open the native
1184+
/// picker so a fresh token is minted, then retry - nothing else about the
1185+
/// request needs to change. Deliberately DISTINCT from
1186+
/// [`Self::LocalIoError`]: for months an expired add-source token surfaced
1187+
/// as "Driven hit a disk error reading a file", sending users off to check
1188+
/// a perfectly healthy drive when the wizard had simply outlived the
1189+
/// token's TTL.
1190+
StaleDialogToken,
11811191
/// `sftp.root_missing` - the SSH (SFTP) destination's configured root path
11821192
/// does not exist on the server. Surfaced only at account-creation time
11831193
/// (`create_sftp_account`'s probe never creates the root - a typo must not
@@ -1268,6 +1278,7 @@ impl ErrorCode {
12681278
ErrorCode::HarnessTimeout => "harness.timeout",
12691279
ErrorCode::InternalBug => "internal.bug",
12701280
ErrorCode::InvalidInput => "internal.invalid_input",
1281+
ErrorCode::StaleDialogToken => "internal.stale_dialog_token",
12711282
ErrorCode::SftpRootMissing => "sftp.root_missing",
12721283
ErrorCode::SftpRootNotADirectory => "sftp.root_not_a_directory",
12731284
ErrorCode::SftpDestMarkerMismatch => "sftp.dest_marker_mismatch",
@@ -1329,6 +1340,7 @@ impl ErrorCode {
13291340
"harness.timeout" => ErrorCode::HarnessTimeout,
13301341
"internal.bug" => ErrorCode::InternalBug,
13311342
"internal.invalid_input" => ErrorCode::InvalidInput,
1343+
"internal.stale_dialog_token" => ErrorCode::StaleDialogToken,
13321344
"sftp.root_missing" => ErrorCode::SftpRootMissing,
13331345
"sftp.root_not_a_directory" => ErrorCode::SftpRootNotADirectory,
13341346
"sftp.dest_marker_mismatch" => ErrorCode::SftpDestMarkerMismatch,

design/SPEC.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1821,6 +1821,7 @@ Stable codes (V1):
18211821
| `drive.dest_folder_permission_denied` | Destination folder's sharing changed to read-only for this account |
18221822
| `harness.timeout` | A stress-harness scenario exceeded its budget (chaos crate only) |
18231823
| `internal.bug` | Programming error — please report |
1824+
| `internal.stale_dialog_token` | A backend-minted dialog token (§11.6.1 C1) was unknown, already spent, or past its TTL — re-open the native picker and retry. Distinct from `local.io_error`: the disk is fine |
18241825

18251826
Frontend maps these to user-friendly messages via
18261827
`t('errors.${code}.short')` and `t('errors.${code}.long')` per DESIGN

src-tauri/src/app_state.rs

Lines changed: 64 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -263,9 +263,9 @@ pub struct AppState {
263263
/// bound to the path the USER actually chose. A path-bearing write command
264264
/// (`add_source`, `export_diagnostic_bundle`) must present a token that maps
265265
/// to exactly that path - so the (untrusted) webview can never inject an
266-
/// arbitrary path. Single-use (taken on validation) with a short TTL so a
267-
/// leaked token cannot be replayed later. Behind a sync `Mutex` (only ever
268-
/// held for a quick insert / take, never across an await).
266+
/// arbitrary path. Single-use (spent when the write commits) with a bounded
267+
/// TTL so a leaked token cannot be replayed later. Behind a sync `Mutex`
268+
/// (only ever held for a quick insert / take, never across an await).
269269
dialog_tokens: std::sync::Mutex<HashMap<String, DialogTokenBinding>>,
270270
/// R2-P1-1: per-account ASYNC lock serialising the FIRST-encrypted-source
271271
/// critical section (ensure-master-key -> stamp -> insert source). Without
@@ -606,20 +606,30 @@ fn prune_terminal_jobs(map: &mut HashMap<String, RestoreJobEntry>) {
606606
}
607607

608608
/// C1: one backend-minted dialog-token binding - the path the user chose via a
609-
/// native dialog plus the instant it was minted (for the short single-use TTL).
609+
/// native dialog plus the instant the binding expires (single-use TTL).
610610
struct DialogTokenBinding {
611611
/// The path the native dialog returned (a folder for the folder dialog, a
612612
/// concrete file path for the save dialog).
613613
path: std::path::PathBuf,
614-
/// When the token was minted (for TTL expiry).
615-
minted_at: std::time::Instant,
614+
/// When the token stops being valid (mint time + [`DIALOG_TOKEN_TTL`]).
615+
/// Stored as the deadline rather than the mint instant so tests can force
616+
/// expiry without `Instant` subtraction (which can underflow soon after
617+
/// boot in CI).
618+
expires_at: std::time::Instant,
616619
}
617620

618-
/// C1: how long a backend-minted dialog token stays valid. The webview calls the
619-
/// dialog command then immediately calls the write command with the token, so a
620-
/// few minutes is generous; a token older than this is rejected so a leaked one
621-
/// cannot be replayed much later.
622-
const DIALOG_TOKEN_TTL: Duration = Duration::from_secs(300);
621+
/// C1: how long a backend-minted dialog token stays valid.
622+
///
623+
/// This must cover the LONGEST honest gap between the native dialog and the
624+
/// write command that spends the token - which is NOT "immediately": the
625+
/// add-source wizard picks the local folder first and then walks the whole
626+
/// tree for the exclusion preview, browses the destination, and waits on the
627+
/// user to think, which on a big source is well over the 5 minutes this
628+
/// originally allowed. An expired token used to surface as `local.io_error`
629+
/// ("disk error"), sending users to check a healthy drive. An hour keeps the
630+
/// replay window bounded (the token is also single-use and process-local)
631+
/// without a live wizard session ever outrunning it.
632+
const DIALOG_TOKEN_TTL: Duration = Duration::from_secs(3600);
623633

624634
impl AppState {
625635
/// Build the managed state from the state repo, the per-account handles,
@@ -1208,12 +1218,12 @@ impl AppState {
12081218
let token = uuid::Uuid::new_v4().to_string();
12091219
let mut map = self.lock_dialog_tokens();
12101220
let now = std::time::Instant::now();
1211-
map.retain(|_, b| now.duration_since(b.minted_at) < DIALOG_TOKEN_TTL);
1221+
map.retain(|_, b| now < b.expires_at);
12121222
map.insert(
12131223
token.clone(),
12141224
DialogTokenBinding {
12151225
path,
1216-
minted_at: now,
1226+
expires_at: now + DIALOG_TOKEN_TTL,
12171227
},
12181228
);
12191229
token
@@ -1226,7 +1236,7 @@ impl AppState {
12261236
pub fn take_dialog_token(&self, token: &str) -> Option<std::path::PathBuf> {
12271237
let mut map = self.lock_dialog_tokens();
12281238
let binding = map.remove(token)?;
1229-
if std::time::Instant::now().duration_since(binding.minted_at) >= DIALOG_TOKEN_TTL {
1239+
if std::time::Instant::now() >= binding.expires_at {
12301240
return None;
12311241
}
12321242
Some(binding.path)
@@ -1242,12 +1252,23 @@ impl AppState {
12421252
pub fn peek_dialog_token(&self, token: &str) -> Option<std::path::PathBuf> {
12431253
let map = self.lock_dialog_tokens();
12441254
let binding = map.get(token)?;
1245-
if std::time::Instant::now().duration_since(binding.minted_at) >= DIALOG_TOKEN_TTL {
1255+
if std::time::Instant::now() >= binding.expires_at {
12461256
return None;
12471257
}
12481258
Some(binding.path.clone())
12491259
}
12501260

1261+
/// Test-only: force `token`'s binding past its TTL so expiry behaviour can
1262+
/// be exercised without sleeping or `Instant` arithmetic that underflows
1263+
/// soon after boot.
1264+
#[cfg(test)]
1265+
pub fn expire_dialog_token_for_test(&self, token: &str) {
1266+
let mut map = self.lock_dialog_tokens();
1267+
if let Some(b) = map.get_mut(token) {
1268+
b.expires_at = std::time::Instant::now();
1269+
}
1270+
}
1271+
12511272
/// The streaming-exclusion-preview registry (see the
12521273
/// [`Self::exclusion_previews`] field docs). Returns an owned `Arc` so the
12531274
/// spawned blocking walk can deregister itself after the command that
@@ -2068,6 +2089,34 @@ pub(crate) mod tests {
20682089
let _ = std::fs::remove_dir_all(dir);
20692090
}
20702091

2092+
#[tokio::test]
2093+
async fn expired_dialog_token_is_rejected_by_peek_and_take() {
2094+
// C1: a token past its TTL must resolve for NEITHER the non-consuming
2095+
// peek nor the consuming take - the wizard gets the stable
2096+
// `internal.stale_dialog_token` rejection (never a phantom disk error)
2097+
// and re-picking mints a fresh, working token.
2098+
let (state, dir) = temp_state().await;
2099+
let app_state = AppState::new(
2100+
state,
2101+
HashMap::new(),
2102+
RemoteMode::Fake,
2103+
default_fake_registry(),
2104+
);
2105+
let path = std::path::PathBuf::from("/home/u/slow-wizard-root");
2106+
let token = app_state.mint_dialog_token(path.clone());
2107+
assert_eq!(app_state.peek_dialog_token(&token), Some(path.clone()));
2108+
2109+
app_state.expire_dialog_token_for_test(&token);
2110+
assert_eq!(app_state.peek_dialog_token(&token), None);
2111+
assert_eq!(app_state.take_dialog_token(&token), None);
2112+
2113+
// Re-picking (a fresh mint of the same path) works immediately.
2114+
let fresh = app_state.mint_dialog_token(path.clone());
2115+
assert_eq!(app_state.take_dialog_token(&fresh), Some(path));
2116+
2117+
let _ = std::fs::remove_dir_all(dir);
2118+
}
2119+
20712120
#[tokio::test]
20722121
async fn ensure_master_key_lock_is_shared_per_account_and_serialises() {
20732122
// R2-P1-1: two `add_source` calls for the SAME account must get the SAME

src-tauri/src/commands/dialogs.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,12 @@ fn file_path_to_pathbuf(fp: tauri_plugin_dialog::FilePath) -> Option<std::path::
118118
}
119119

120120
/// The shared "user cancelled the dialog" error (a benign no-advance signal).
121+
/// The message is the [`crate::commands::DIALOG_CANCELLED_MSG`] sentinel the
122+
/// IPC-boundary logging recognises, so a cancelled picker is not logged as a
123+
/// command failure.
121124
fn cancelled() -> CommandError {
122-
CommandError::with_code(ErrorCode::LocalIoError, "dialog cancelled")
125+
CommandError::with_code(
126+
ErrorCode::LocalIoError,
127+
crate::commands::DIALOG_CANCELLED_MSG,
128+
)
123129
}

src-tauri/src/commands/mod.rs

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ use driven_core::types::ErrorCode;
4444
/// (not the SPEC s24 example's literal `retry_after_ms`). `code` + `message` are
4545
/// identical in both casings and `details` is single-word; only the retry-after
4646
/// hint differs, and the frontend reads only `.code`.
47-
#[derive(Debug, Clone, Serialize, Deserialize)]
47+
#[derive(Debug, Clone, Deserialize)]
4848
#[serde(rename_all = "camelCase")]
4949
pub struct CommandError {
5050
/// The stable dotted SPEC s24 error code (i18n key), e.g.
@@ -94,6 +94,46 @@ impl CommandError {
9494
}
9595
}
9696

97+
/// The benign "user closed the native dialog" sentinel message. It rides the
98+
/// normal error channel purely as a no-advance signal to the webview (see
99+
/// [`dialogs`]), so the boundary logging below must not turn every cancelled
100+
/// picker into a scary WARN line.
101+
pub(crate) const DIALOG_CANCELLED_MSG: &str = "dialog cancelled";
102+
103+
impl Serialize for CommandError {
104+
/// Serialise the stable SPEC s24 wire shape - and, because a command's
105+
/// `Err` is serialised exactly once, HERE, as it crosses the IPC boundary
106+
/// to the webview, this is the single seam where every command rejection
107+
/// can be logged into the rolling log file (and thus the SPEC s18
108+
/// diagnostic bundle). Before this, a rejected command left no backend
109+
/// trace at all: six straight failed `add_source` calls produced a
110+
/// diagnostic bundle with zero ERROR/WARN lines about them, and the bug
111+
/// had to be reconstructed from what was ABSENT from the log.
112+
fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
113+
if self.message != DIALOG_CANCELLED_MSG {
114+
tracing::warn!(
115+
target: "driven::app::ipc",
116+
code = %self.code,
117+
message = %self.message,
118+
"IPC command rejected"
119+
);
120+
}
121+
use serde::ser::SerializeStruct as _;
122+
let fields =
123+
2 + usize::from(self.retry_after_ms.is_some()) + usize::from(self.details.is_some());
124+
let mut s = serializer.serialize_struct("CommandError", fields)?;
125+
s.serialize_field("code", &self.code)?;
126+
s.serialize_field("message", &self.message)?;
127+
if let Some(retry_after_ms) = self.retry_after_ms {
128+
s.serialize_field("retryAfterMs", &retry_after_ms)?;
129+
}
130+
if let Some(details) = &self.details {
131+
s.serialize_field("details", details)?;
132+
}
133+
s.end()
134+
}
135+
}
136+
97137
impl std::fmt::Display for CommandError {
98138
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
99139
write!(f, "{}: {}", self.code, self.message)
@@ -543,6 +583,22 @@ pub fn atomic_write(dest: &Path, bytes: &[u8]) -> CommandResult<()> {
543583
/// component, (2) canonicalise via `dunce` (rejecting a non-existent path), and
544584
/// (3) require the canonical target to be a directory. Returns the canonical
545585
/// root the scanner then walks.
586+
/// The stable rejection for a backend-minted dialog token that is unknown,
587+
/// already spent, or past its TTL (SPEC s11.6.1 C1) - shared by every
588+
/// path-bearing command that resolves one (`add_source`, the exclusion
589+
/// previews, restore, the diagnostic-bundle export).
590+
///
591+
/// `internal.stale_dialog_token`, deliberately NOT `local.io_error`: the disk
592+
/// is fine, the folder pick just needs redoing. The io_error mapping made an
593+
/// expired add-source token render as "Driven hit a disk error reading a
594+
/// file", pointing users at a healthy drive.
595+
pub(crate) fn stale_dialog_token_error() -> CommandError {
596+
CommandError::with_code(
597+
ErrorCode::StaleDialogToken,
598+
"the folder selection expired or was already used; open the folder picker again",
599+
)
600+
}
601+
546602
pub fn validate_readable_dir(path: &Path) -> CommandResult<PathBuf> {
547603
if path.components().any(|c| matches!(c, Component::ParentDir)) {
548604
return Err(CommandError::with_code(

src-tauri/src/commands/restore.rs

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -298,12 +298,9 @@ pub async fn restore_files(
298298
// token is CONSUMED only immediately before the job is actually accepted (just
299299
// before the atomic seed+spawn), so the single use is spent only on a real
300300
// restore. A missing / replayed / expired token is still rejected here.
301-
let dest_dir = state.peek_dialog_token(&dest_token).ok_or_else(|| {
302-
CommandError::with_code(
303-
ErrorCode::LocalIoError,
304-
"no matching destination folder; pick a restore folder first",
305-
)
306-
})?;
301+
let dest_dir = state
302+
.peek_dialog_token(&dest_token)
303+
.ok_or_else(crate::commands::stale_dialog_token_error)?;
307304
// The destination directory must exist + be a directory (the user picked it).
308305
let dest_meta = std::fs::metadata(&dest_dir).map_err(|e| {
309306
CommandError::with_code(
@@ -346,10 +343,7 @@ pub async fn restore_files(
346343
// spend the single use so the token cannot be replayed. A concurrent take that
347344
// already consumed it (None) is rejected without spawning a job.
348345
if state.take_dialog_token(&dest_token).is_none() {
349-
return Err(CommandError::with_code(
350-
ErrorCode::LocalIoError,
351-
"no matching destination folder; pick a restore folder first",
352-
));
346+
return Err(crate::commands::stale_dialog_token_error());
353347
}
354348
// R5-P1-3 (DATA-SAFETY): BIND the approved root to a STABLE identity (canonical
355349
// path + on-disk dev/inode or volume file-id) right now, at consume time. The

src-tauri/src/commands/settings.rs

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1767,12 +1767,9 @@ pub async fn export_diagnostic_bundle(
17671767
// C1: resolve the save path from the backend-minted dialog token (single-use).
17681768
// Reject any request without a matching token - the webview never supplies a
17691769
// raw path here.
1770-
let dest = state.take_dialog_token(&token).ok_or_else(|| {
1771-
CommandError::with_code(
1772-
ErrorCode::LocalIoError,
1773-
"no matching dialog token for the export destination; pick a save location first",
1774-
)
1775-
})?;
1770+
let dest = state
1771+
.take_dialog_token(&token)
1772+
.ok_or_else(crate::commands::stale_dialog_token_error)?;
17761773

17771774
// C2: the token's path is a concrete FILE. Confine the write to its parent
17781775
// directory (the dialog-approved root) and re-validate the leaf.

0 commit comments

Comments
 (0)