Skip to content

Commit 8b1e25a

Browse files
pmaxhoganclaude
andcommitted
fix: eager VSS helper launch with attended UAC window and decline-only memoisation
The live smoke of the wired helper (#112) failed: the lazy at-first-locked-file launch gave the human only the ~5s pipe-connect budget to approve UAC, and one timed-out shot memoised failure until restart -> every locked file skipped as local.file_locked. This hardens the launch UX (Refs #25). - SEE_MASK_NOASYNC on ShellExecuteExW so the runas call WAITS for the user to approve/decline even on a no-message-loop worker thread (the likely root cause of the early-return spurious failure). - Eager launch on enable-toggle: flipping windows.vssHelper ON fires the attended UAC prompt immediately (the user is at Settings), on a background thread so the IPC returns at once; the UI polls status Pending -> Ready/Declined. Boot stays LAZY (no UAC at silent startup); the first locked file triggers it. - Manager now built at boot whenever un-elevated (setting-independent) with an enabled flag, shared into every provider, so a runtime toggle works without an app restart. - Decline-only memoisation: ERROR_CANCELLED (decline OR ignored prompt) memoises for the session; an approved-but-pipe-never-came-up launch is transient and retried on the next enable/start; an off->on re-toggle clears a prior decline. - Attended 90s window for the post-approval pipe handshake (only the one-shot launch; steady-state reconnects stay tight). - Transient classification: a locked file hit WHILE the helper is launching is skipped-and-requeued as the new local.vss_helper_pending (SnapshotOutcome:: Pending -> FallbackDecision::SkipRetryLater -> SkipReason::VssHelperPending), never misreported as a permanent local.file_locked. - Truthful status: launchPending / launchDeclined added; Rules tab shows a "waiting for elevation approval" hint (polled) and a "declined" hint. Refs #25 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LQMbCLUj5JT2e35qQMvsyA
1 parent 70ffbcb commit 8b1e25a

18 files changed

Lines changed: 1142 additions & 339 deletions

File tree

crates/driven-core/src/executor.rs

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,11 @@ pub enum SkipReason {
388388
/// up if Driven ran elevated" from "genuinely unreadable"
389389
/// (`local.vss_unavailable`, SPEC s24).
390390
VssUnavailable,
391+
/// The file is locked and the least-privilege VSS helper broker is still
392+
/// LAUNCHING / awaiting elevation approval (DESIGN s5.3.1). The file is
393+
/// skipped TRANSIENTLY this cycle and retried next cycle (once the broker is
394+
/// up) - NOT reported as a permanent lock (`local.vss_helper_pending`).
395+
VssHelperPending,
391396
}
392397

393398
impl SkipReason {
@@ -401,6 +406,7 @@ impl SkipReason {
401406
SkipReason::ChangedDuringUpload => ErrorCode::LocalFileChangedDuringUpload,
402407
SkipReason::Locked => ErrorCode::LocalFileLocked,
403408
SkipReason::VssUnavailable => ErrorCode::LocalVssUnavailable,
409+
SkipReason::VssHelperPending => ErrorCode::LocalVssHelperPending,
404410
}
405411
}
406412
}
@@ -1136,6 +1142,14 @@ impl DefaultExecutor {
11361142
};
11371143
EffectiveOpen::Skip(reason)
11381144
}
1145+
FallbackDecision::SkipRetryLater => {
1146+
// DESIGN s5.3.1: the least-privilege helper broker is launching /
1147+
// awaiting elevation approval. Skip this locked file TRANSIENTLY
1148+
// (it re-queues like any skip) and classify it as helper-pending -
1149+
// NOT a permanent lock - so the brief launch-in-progress window is
1150+
// not misreported. The next cycle (broker up) backs it up.
1151+
EffectiveOpen::Skip(SkipReason::VssHelperPending)
1152+
}
11391153
}
11401154
}
11411155

@@ -8635,6 +8649,67 @@ mod tests {
86358649
assert_eq!(children[0].size, Some(size));
86368650
}
86378651

8652+
/// Issue #25 (launch-UX): a LOCKED file while the least-privilege helper is
8653+
/// still LAUNCHING is skipped TRANSIENTLY (re-queued) and classified
8654+
/// `local.vss_helper_pending` - NOT the permanent `local.file_locked`. Windows
8655+
/// only (a real `ERROR_SHARING_VIOLATION` cannot be produced cross-OS) but
8656+
/// NON-elevated-safe: `FakeVssProvider::pending()` returns `Pending` without
8657+
/// any real COM, so this runs on the (non-elevated) Windows CI runner. The
8658+
/// pure decision is table-tested in `driven_vss::fallback_decision`.
8659+
#[cfg(windows)]
8660+
#[tokio::test]
8661+
async fn locked_file_while_helper_pending_skips_as_pending_not_locked() {
8662+
use std::os::windows::fs::OpenOptionsExt;
8663+
8664+
let h = harness().await;
8665+
let (rel, size) = h.write_file("locked-pending.dat", b"bytes-behind-a-launching-helper");
8666+
let live = h.tmp_src.path().join("locked-pending.dat");
8667+
8668+
const GENERIC_WRITE: u32 = 0x4000_0000;
8669+
let _exclusive = std::fs::OpenOptions::new()
8670+
.access_mode(GENERIC_WRITE)
8671+
.share_mode(0)
8672+
.write(true)
8673+
.open(&live)
8674+
.expect("open locked-pending.dat exclusively");
8675+
assert!(
8676+
matches!(
8677+
super::open_shared(&live).await,
8678+
Err(super::OpenError::Locked)
8679+
),
8680+
"test setup: file must be locked"
8681+
);
8682+
8683+
// The helper is available (capability) but its snapshot is Pending.
8684+
let vss: Arc<dyn driven_vss::VssProvider> = Arc::new(driven_vss::FakeVssProvider::pending(
8685+
driven_vss::VssMode::Auto,
8686+
));
8687+
let exec = h.executor_with_vss(vss);
8688+
let out = exec
8689+
.execute(
8690+
&h.source,
8691+
&h.upload_plan(&rel, size),
8692+
&noop_progress,
8693+
&noop_outcome,
8694+
)
8695+
.await
8696+
.unwrap();
8697+
assert!(
8698+
matches!(
8699+
out[0],
8700+
OpOutcome::Skipped {
8701+
reason: SkipReason::VssHelperPending,
8702+
..
8703+
}
8704+
),
8705+
"a pending helper must skip-as-pending (retry next cycle), not lock; got {:?}",
8706+
out[0]
8707+
);
8708+
// Not committed as Synced (it re-queues for the next cycle).
8709+
let row = h.state.get_file_state(h.source.id, &rel).await.unwrap();
8710+
assert!(row.is_none() || row.unwrap().status != FileStateStatus::Synced);
8711+
}
8712+
86388713
// --- resumable (large file) path ----------------------------------------
86398714

86408715
#[tokio::test]

crates/driven-core/src/types.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,6 +1031,12 @@ pub enum ErrorCode {
10311031
/// `local.vss_unavailable` - Driven needs elevation to use VSS but
10321032
/// isn't elevated.
10331033
LocalVssUnavailable,
1034+
/// `local.vss_helper_pending` - the least-privilege VSS helper broker is
1035+
/// still launching / awaiting elevation approval (DESIGN s5.3.1), so this
1036+
/// locked file is skipped TRANSIENTLY and retried on the next cycle. Not a
1037+
/// permanent lock - distinct from [`Self::LocalFileLocked`] so the brief
1038+
/// launch-in-progress window is not misreported as "file locked".
1039+
LocalVssHelperPending,
10341040
/// `local.file_changed_during_upload` - pre/post fstat showed file
10351041
/// mutated mid-upload; re-queued.
10361042
LocalFileChangedDuringUpload,
@@ -1136,6 +1142,7 @@ impl ErrorCode {
11361142
ErrorCode::DriveDestFolderPermissionDenied => "drive.dest_folder_permission_denied",
11371143
ErrorCode::LocalFileLocked => "local.file_locked",
11381144
ErrorCode::LocalVssUnavailable => "local.vss_unavailable",
1145+
ErrorCode::LocalVssHelperPending => "local.vss_helper_pending",
11391146
ErrorCode::LocalFileChangedDuringUpload => "local.file_changed_during_upload",
11401147
ErrorCode::LocalFileReplacedDuringUpload => "local.file_replaced_during_upload",
11411148
ErrorCode::LocalIoError => "local.io_error",
@@ -1189,6 +1196,7 @@ impl ErrorCode {
11891196
"drive.dest_folder_permission_denied" => ErrorCode::DriveDestFolderPermissionDenied,
11901197
"local.file_locked" => ErrorCode::LocalFileLocked,
11911198
"local.vss_unavailable" => ErrorCode::LocalVssUnavailable,
1199+
"local.vss_helper_pending" => ErrorCode::LocalVssHelperPending,
11921200
"local.file_changed_during_upload" => ErrorCode::LocalFileChangedDuringUpload,
11931201
"local.file_replaced_during_upload" => ErrorCode::LocalFileReplacedDuringUpload,
11941202
"local.io_error" => ErrorCode::LocalIoError,

crates/driven-vss-helper/src/launch.rs

Lines changed: 92 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -24,25 +24,50 @@ pub fn generate_pipe_name() -> String {
2424
format!(r"\\.\pipe\driven-vss-{}", uuid::Uuid::new_v4().simple())
2525
}
2626

27-
/// The on-demand launch seam the app-side [`BrokeredVssProvider`] consults the
28-
/// FIRST time a locked file needs the helper (DESIGN s5.3.1).
27+
/// The readiness of the elevated helper broker for one locked-file open
28+
/// (DESIGN s5.3.1). Returned by [`HelperLauncher::launch_status`].
29+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
30+
pub enum LaunchStatus {
31+
/// The broker is up and serving - proceed to read the locked file through it.
32+
Ready,
33+
/// A launch is in progress: the broker has been asked to start and is
34+
/// awaiting elevation approval / bringing its pipe up (within the attended
35+
/// window). The caller skips this file TRANSIENTLY and retries next cycle -
36+
/// it must NOT report the file as permanently locked, and must NOT block.
37+
Pending,
38+
/// The user declined (or ignored) the UAC prompt this session. Memoised: the
39+
/// caller degrades to skip-the-locked-file and no further prompt is raised
40+
/// until the app restarts (or the toggle is switched off then on again).
41+
Declined,
42+
/// The least-privilege helper is not in play (the `windows.vss_helper`
43+
/// setting is off, or off Windows). The caller behaves exactly like the
44+
/// historical un-elevated skip.
45+
Disabled,
46+
}
47+
48+
/// The on-demand launch seam the app-side [`BrokeredVssProvider`] consults on the
49+
/// locked-file path (DESIGN s5.3.1).
2950
///
3051
/// The provider does not launch the elevated broker itself: launch is an
3152
/// app-level, at-most-once concern (one UAC prompt, one helper process, one
3253
/// pipe name shared across every account's provider), so the app owns a single
33-
/// launcher and hands the SAME `Arc<dyn HelperLauncher>` to each provider.
34-
/// [`Self::ensure_launched`] is called lazily on the locked-file path and MUST
35-
/// be idempotent + memoised: a first call launches the broker; later calls (and
36-
/// calls from other accounts' providers) return the cached verdict without
37-
/// re-prompting - a user who declined the UAC prompt is not asked again for the
38-
/// rest of the session.
54+
/// launcher and hands the SAME `Arc<dyn HelperLauncher>` to each provider. The
55+
/// eager path (the user enabling the setting) launches ahead of any sync; the
56+
/// lazy path (the setting already on at boot) launches on the first locked file.
3957
pub trait HelperLauncher: Send + Sync {
40-
/// Ensure the elevated helper has been launched for this session, launching
41-
/// it at most once. Returns `true` when the helper is believed up (launch
42-
/// succeeded, or a prior call already launched it), `false` when it could
43-
/// not be brought up (UAC declined, helper exe missing) so the caller
44-
/// degrades to skip-the-locked-file.
45-
fn ensure_launched(&self) -> bool;
58+
/// Report the broker's readiness for a locked-file open, TRIGGERING an
59+
/// at-most-once lazy launch (non-blocking) when the helper is enabled but no
60+
/// launch has been attempted yet. Never blocks on the UAC prompt: a launch in
61+
/// progress reports [`LaunchStatus::Pending`] so the caller skips-and-retries
62+
/// rather than waiting.
63+
fn launch_status(&self) -> LaunchStatus;
64+
65+
/// Capability: is helper-brokered VSS in play at all this run (the setting is
66+
/// on AND the user has not declined)? The executor reads this as its
67+
/// `elevated` input to `fallback_decision`, so it must be `true` whenever the
68+
/// broker is up OR can still be brought up - and `false` once disabled or
69+
/// declined, so a disabled/declined provider behaves like the un-elevated skip.
70+
fn is_available(&self) -> bool;
4671
}
4772

4873
/// Build the helper's argv (excluding the program path itself):
@@ -87,17 +112,50 @@ pub fn parse_helper_args(args: &[String]) -> Result<(String, Vec<PathBuf>), Stri
87112
Ok((pipe_name, roots))
88113
}
89114

90-
/// Launch `helper_exe` elevated with `args` via the shell `runas` verb
91-
/// (raises one UAC prompt). Returns when the process has been STARTED, not when
92-
/// it exits (the helper runs for the session). On non-Windows this is
93-
/// unsupported.
115+
/// Why an elevated launch did not succeed (DESIGN s5.3.1). The distinction drives
116+
/// memoisation: [`Self::Declined`] is remembered for the session (never re-prompt);
117+
/// [`Self::Failed`] is transient and may be retried.
118+
#[derive(Debug, Clone, PartialEq, Eq)]
119+
pub enum LaunchError {
120+
/// The user DECLINED or ignored the UAC prompt (`ERROR_CANCELLED`, 1223).
121+
/// Because a cancel and a prompt-timeout both surface as `ERROR_CANCELLED`,
122+
/// this means "the user did not approve" - memoise it and do not re-prompt.
123+
Declined,
124+
/// Any OTHER launch failure (the exe is missing, a shell error, etc). This is
125+
/// transient - a later enable-toggle or app start may retry. Carries a
126+
/// secret-free detail for logs.
127+
Failed(String),
128+
}
129+
130+
impl std::fmt::Display for LaunchError {
131+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
132+
match self {
133+
LaunchError::Declined => write!(f, "elevation was declined at the UAC prompt"),
134+
LaunchError::Failed(detail) => write!(f, "{detail}"),
135+
}
136+
}
137+
}
138+
139+
/// Launch `helper_exe` elevated with `args` via the shell `runas` verb (raises
140+
/// one UAC prompt).
141+
///
142+
/// `SEE_MASK_NOASYNC` makes `ShellExecuteExW` WAIT for the elevation to resolve
143+
/// even on a thread with no message loop (e.g. a worker thread), so this returns
144+
/// only once the user has approved (the process is then starting) or the prompt
145+
/// was cancelled/ignored ([`LaunchError::Declined`]). Without that flag the call
146+
/// can return early while the prompt is still up and surface a spurious error -
147+
/// the root cause of the first-cut UAC race. It still returns as soon as the
148+
/// process is STARTED, not when it exits (the helper serves for the session). On
149+
/// non-Windows this is unsupported.
94150
#[cfg(windows)]
95-
pub fn launch_elevated(helper_exe: &std::path::Path, args: &[String]) -> Result<(), String> {
151+
pub fn launch_elevated(helper_exe: &std::path::Path, args: &[String]) -> Result<(), LaunchError> {
96152
use std::os::windows::ffi::OsStrExt;
97153

98154
use windows::core::PCWSTR;
99155
use windows::Win32::Foundation::{CloseHandle, GetLastError, ERROR_CANCELLED};
100-
use windows::Win32::UI::Shell::{ShellExecuteExW, SEE_MASK_NOCLOSEPROCESS, SHELLEXECUTEINFOW};
156+
use windows::Win32::UI::Shell::{
157+
ShellExecuteExW, SEE_MASK_NOASYNC, SEE_MASK_NOCLOSEPROCESS, SHELLEXECUTEINFOW,
158+
};
101159
use windows::Win32::UI::WindowsAndMessaging::SW_HIDE;
102160

103161
fn wide(s: &std::ffi::OsStr) -> Vec<u16> {
@@ -119,7 +177,10 @@ pub fn launch_elevated(helper_exe: &std::path::Path, args: &[String]) -> Result<
119177

120178
let mut info = SHELLEXECUTEINFOW {
121179
cbSize: std::mem::size_of::<SHELLEXECUTEINFOW>() as u32,
122-
fMask: SEE_MASK_NOCLOSEPROCESS,
180+
// NOASYNC: block until the elevation prompt resolves on a no-message-loop
181+
// thread. NOCLOSEPROCESS: keep the started process handle so we can close
182+
// it ourselves (we manage the helper over the pipe, not via this handle).
183+
fMask: SEE_MASK_NOCLOSEPROCESS | SEE_MASK_NOASYNC,
123184
lpVerb: PCWSTR(verb.as_ptr()),
124185
lpFile: PCWSTR(file.as_ptr()),
125186
lpParameters: PCWSTR(params_w.as_ptr()),
@@ -144,18 +205,24 @@ pub fn launch_elevated(helper_exe: &std::path::Path, args: &[String]) -> Result<
144205
// SAFETY: reading the thread's last-error code.
145206
let code = unsafe { GetLastError() };
146207
if code == ERROR_CANCELLED {
147-
Err("elevation was declined at the UAC prompt".to_string())
208+
// Cancel OR prompt-timeout - both are "the user did not approve".
209+
Err(LaunchError::Declined)
148210
} else {
149-
Err(format!("ShellExecuteEx(runas) failed (error {})", code.0))
211+
Err(LaunchError::Failed(format!(
212+
"ShellExecuteEx(runas) failed (error {})",
213+
code.0
214+
)))
150215
}
151216
}
152217
}
153218
}
154219

155220
/// Non-Windows: elevated launch is unsupported (VSS is Windows-only).
156221
#[cfg(not(windows))]
157-
pub fn launch_elevated(_helper_exe: &std::path::Path, _args: &[String]) -> Result<(), String> {
158-
Err("the VSS helper is only supported on Windows".to_string())
222+
pub fn launch_elevated(_helper_exe: &std::path::Path, _args: &[String]) -> Result<(), LaunchError> {
223+
Err(LaunchError::Failed(
224+
"the VSS helper is only supported on Windows".to_string(),
225+
))
159226
}
160227

161228
/// Quote a single argv element for a Windows command-line parameter string.

crates/driven-vss-helper/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ pub mod protocol;
3333
pub mod validate;
3434

3535
mod provider;
36-
pub use launch::HelperLauncher;
36+
pub use launch::{HelperLauncher, LaunchError, LaunchStatus};
3737
pub use provider::BrokeredVssProvider;
3838

3939
#[cfg(windows)]

0 commit comments

Comments
 (0)