From b962e79f633dcc9e93bc0265e49602b8d138b6d4 Mon Sep 17 00:00:00 2001 From: Sree Narayanan Date: Wed, 21 Oct 2026 09:34:49 +0400 Subject: [PATCH] fix(native-sidecar): keep guest writes to API-seeded files Python and WASM/shell writes go only to the kernel VFS, so the VM root shadow kept the bytes from filesystem.writeFile. The next shadow-to-kernel sync saw the two sides differ and copied the stale shadow over the guest edit, and treated a guest rm/recreate as a host deletion. The shadow sync inventory now records a stamp for both sides at each reconciliation. A shadow change reaches the kernel only when the kernel node is unchanged since then, so every guest writer wins without a per-runtime mirror. Seeded bootstrap content still imports as before. Fixes #2022 and #1901. The Python test helper is adapted from #2023 by ankssjain. --- crates/native-sidecar/src/execution/launch.rs | 132 ++++++++++++++-- crates/native-sidecar/src/execution/mod.rs | 6 +- crates/native-sidecar/src/filesystem.rs | 72 ++++++--- crates/native-sidecar/src/state.rs | 67 ++++++++ crates/native-sidecar/tests/filesystem.rs | 147 ++++++++++++++++++ 5 files changed, 382 insertions(+), 42 deletions(-) diff --git a/crates/native-sidecar/src/execution/launch.rs b/crates/native-sidecar/src/execution/launch.rs index 109ffc9085..4ad277feda 100644 --- a/crates/native-sidecar/src/execution/launch.rs +++ b/crates/native-sidecar/src/execution/launch.rs @@ -813,7 +813,8 @@ fn sync_vm_shadow_root_to_kernel(vm: &mut VmState) -> Result<(), SidecarError> { } } - let failed_replacements = propagate_shadow_deletions_to_kernel(vm, &mut snapshot.entries); + let kernel_owned = kernel_owned_shadow_paths(vm, &snapshot); + let failed_replacements = propagate_shadow_deletions_to_kernel(vm, &mut snapshot); let mut synced_file_times = BTreeMap::new(); sync_host_directory_tree_to_kernel_inner( vm, @@ -821,16 +822,87 @@ fn sync_vm_shadow_root_to_kernel(vm: &mut VmState) -> Result<(), SidecarError> { &shadow_root, "/", &mut synced_file_times, - Some(&snapshot.entries), - Some(&failed_replacements), + Some(&ShadowSyncPlan { + inventory: &snapshot.entries, + failed_replacements: &failed_replacements, + kernel_owned: &kernel_owned, + }), )?; + for (path, entry) in &mut snapshot.entries { + if entry.deletion_pending { + continue; + } + if let Some(shadow) = snapshot.shadow_stamps.get(path) { + entry.baseline = Some(ShadowSyncBaseline { + shadow: Some(*shadow), + kernel: kernel_shadow_sync_stamp(vm, path), + }); + } + } vm.shadow_sync_inventory = snapshot.entries; Ok(()) } +/// Every guest runtime writes the kernel VFS directly, and most of those +/// writes never reach the shadow. A shadow change therefore applies to the +/// kernel only when the kernel node is unchanged since the last +/// reconciliation; otherwise the kernel holds the newer guest edit. +fn shadow_change_applies_to_kernel( + previous: Option<&ShadowSyncInventoryEntry>, + shadow: Option, + kernel: Option, +) -> bool { + match previous.map(|entry| entry.baseline) { + // A shadow node that was never inventoried may only fill a kernel gap. + None => kernel.is_none(), + // Seeded shadow content that was never reconciled is authoritative. + Some(None) => true, + Some(Some(baseline)) => baseline.shadow != shadow && baseline.kernel == kernel, + } +} + +/// Shadow paths whose kernel node must not be overwritten by this walk. +fn kernel_owned_shadow_paths(vm: &mut VmState, snapshot: &ShadowSyncSnapshot) -> BTreeSet { + let mut kernel_owned = BTreeSet::new(); + for path in snapshot.entries.keys() { + let kernel = kernel_shadow_sync_stamp(vm, path); + if !shadow_change_applies_to_kernel( + vm.shadow_sync_inventory.get(path), + snapshot.shadow_stamps.get(path).copied(), + kernel, + ) { + kernel_owned.insert(path.clone()); + } + } + kernel_owned +} + +/// Trusted bookkeeping read of a kernel node. It must not require the guest +/// `fs.stat` permission that a `KernelVm::lstat` would check. +fn kernel_shadow_sync_stamp(vm: &mut VmState, guest_path: &str) -> Option { + use agentos_kernel::vfs::VirtualFileSystem; + vm.kernel + .filesystem_mut() + .inner_mut() + .lstat(guest_path) + .ok() + .map(|stat| ShadowSyncStamp::kernel(&stat)) +} + +/// Paths the additive shadow walk must leave alone. +struct ShadowSyncPlan<'a> { + /// The entry appeared after the inventory pass when it is missing here. + inventory: &'a BTreeMap, + /// Stale kernel objects whose removal failed. + failed_replacements: &'a BTreeSet, + /// Kernel nodes that changed since the last reconciliation. + kernel_owned: &'a BTreeSet, +} + #[derive(Default)] struct ShadowSyncSnapshot { entries: BTreeMap, + shadow_stamps: BTreeMap, incomplete_subtrees: BTreeSet, } @@ -922,10 +994,23 @@ fn collect_shadow_sync_snapshot_inner( } else { continue; }; + let metadata = match entry.metadata() { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(error) => { + return Err(SidecarError::Io(format!( + "failed to inventory host shadow entry {}: {error}", + host_path.display() + ))); + } + }; snapshot.entries.insert( guest_path.clone(), ShadowSyncInventoryEntry::present(node_type), ); + snapshot + .shadow_stamps + .insert(guest_path.clone(), ShadowSyncStamp::host(&metadata)); if node_type == ShadowNodeType::Directory { collect_shadow_sync_snapshot_inner(shadow_root, &host_path, &guest_path, snapshot)?; } @@ -942,8 +1027,9 @@ fn collect_shadow_sync_snapshot_inner( /// leaves before the directories that contain them. fn propagate_shadow_deletions_to_kernel( vm: &mut VmState, - current: &mut BTreeMap, + snapshot: &mut ShadowSyncSnapshot, ) -> BTreeSet { + let current = &mut snapshot.entries; let stale = vm .shadow_sync_inventory .iter() @@ -962,6 +1048,16 @@ fn propagate_shadow_deletions_to_kernel( { continue; } + let kernel = kernel_shadow_sync_stamp(vm, &path); + if !shadow_change_applies_to_kernel( + Some(&previous), + snapshot.shadow_stamps.get(&path).copied(), + kernel, + ) { + // The kernel node was replaced or edited after the shadow copy + // went stale (for example `rm f && echo new > f` in a guest). + continue; + } let stat = match vm.kernel.lstat(&path) { Ok(stat) => stat, Err(error) if error.code() == "ENOENT" => continue, @@ -1003,10 +1099,13 @@ fn retain_shadow_deletion_tombstone( ) { current .entry(path.to_owned()) - .and_modify(|entry| entry.deletion_pending = true) + .and_modify(|entry| { + entry.deletion_pending = true; + entry.baseline = previous.baseline; + }) .or_insert(ShadowSyncInventoryEntry { - node_type: previous.node_type, deletion_pending: true, + ..previous }); } @@ -1115,7 +1214,6 @@ fn sync_host_directory_tree_to_kernel( &normalized_guest_root, &mut synced_file_times, None, - None, ) } @@ -1134,8 +1232,7 @@ fn sync_host_directory_tree_to_kernel_inner( current_host_dir: &Path, guest_root: &str, synced_file_times: &mut BTreeMap<(u64, u64), (u64, u64)>, - expected_inventory: Option<&BTreeMap>, - failed_replacements: Option<&BTreeSet>, + plan: Option<&ShadowSyncPlan<'_>>, ) -> Result<(), SidecarError> { let entries = match fs::read_dir(current_host_dir) { Ok(entries) => entries, @@ -1196,26 +1293,29 @@ fn sync_host_directory_tree_to_kernel_inner( if should_skip_shadow_sync_path(vm, &guest_path) { continue; } - if expected_inventory.is_some_and(|inventory| !inventory.contains_key(&guest_path)) { + if plan.is_some_and(|plan| !plan.inventory.contains_key(&guest_path)) { // The entry appeared after the inventory pass. Pick it up on the // next sync rather than mixing two different shadow snapshots. continue; } - if failed_replacements.is_some_and(|failed| failed.contains(&guest_path)) { + if plan.is_some_and(|plan| plan.failed_replacements.contains(&guest_path)) { // Never write through a stale object whose removal failed. The // retained tombstone will retry before a future additive write. continue; } + let kernel_owned = plan.is_some_and(|plan| plan.kernel_owned.contains(&guest_path)); if file_type.is_dir() { - ensure_kernel_shadow_node_type(vm, &guest_path, ShadowNodeType::Directory)?; + if !kernel_owned { + ensure_kernel_shadow_node_type(vm, &guest_path, ShadowNodeType::Directory)?; + } let metadata = entry.metadata().map_err(|error| { SidecarError::Io(format!( "failed to read host shadow metadata {}: {error}", host_path.display() )) })?; - if !is_shadow_bootstrap_dir(&guest_path) { + if !kernel_owned && !is_shadow_bootstrap_dir(&guest_path) { if !vm.kernel.exists(&guest_path).unwrap_or(false) { vm.kernel.mkdir(&guest_path, true).map_err(|error| { SidecarError::InvalidState(format!( @@ -1243,11 +1343,13 @@ fn sync_host_directory_tree_to_kernel_inner( &host_path, guest_root, synced_file_times, - expected_inventory, - failed_replacements, + plan, )?; continue; } + if kernel_owned { + continue; + } if file_type.is_file() { ensure_kernel_shadow_node_type(vm, &guest_path, ShadowNodeType::File)?; diff --git a/crates/native-sidecar/src/execution/mod.rs b/crates/native-sidecar/src/execution/mod.rs index 60495fb929..ee24f75724 100644 --- a/crates/native-sidecar/src/execution/mod.rs +++ b/crates/native-sidecar/src/execution/mod.rs @@ -153,9 +153,9 @@ use crate::state::{ PendingTcpSocket, PendingUnixConnectionGuard, PendingUnixSocket, PlainSocketWritePayload, ProcNetEntry, ProcessEventEnvelope, PythonHostSocket, PythonSocketConnectCompletion, PythonTcpReadBuffer, QueuedHttp2Command, QueuedHttp2Event, ReactorIoLimits, - ResolvedChildProcessExecution, ResolvedTcpConnectAddr, ShadowNodeType, - ShadowSyncInventoryEntry, SharedBridge, SharedSidecarRequestClient, SidecarKernel, - SocketDescriptionLease, SocketQueryKind, SocketReadinessRegistration, + ResolvedChildProcessExecution, ResolvedTcpConnectAddr, ShadowNodeType, ShadowSyncBaseline, + ShadowSyncInventoryEntry, ShadowSyncStamp, SharedBridge, SharedSidecarRequestClient, + SidecarKernel, SocketDescriptionLease, SocketQueryKind, SocketReadinessRegistration, SocketReadinessSubscribers, TlsWritePayload, VmDnsConfig, VmFetchBodyMode, VmFetchStreamState, VmHandle, VmListenPolicy, VmPendingByteBudget, VmState, DEFAULT_JAVASCRIPT_NET_BACKLOG, EXECUTION_DRIVER_NAME, EXECUTION_SANDBOX_ROOT_ENV, HOST_FUNCTION_DRIVER_NAME, diff --git a/crates/native-sidecar/src/filesystem.rs b/crates/native-sidecar/src/filesystem.rs index 42f24d3908..625b03a6aa 100644 --- a/crates/native-sidecar/src/filesystem.rs +++ b/crates/native-sidecar/src/filesystem.rs @@ -17,9 +17,9 @@ use crate::service::{ python_error, }; use crate::state::{ - ActiveExecutionEvent, ActiveProcess, BridgeError, ShadowNodeType, ShadowSyncInventoryEntry, - SharedBridge, SidecarKernel, VmHandle, VmState, EXECUTION_DRIVER_NAME, - PYTHON_VFS_RPC_GUEST_ROOT, + ActiveExecutionEvent, ActiveProcess, BridgeError, ShadowNodeType, ShadowSyncBaseline, + ShadowSyncInventoryEntry, ShadowSyncStamp, SharedBridge, SidecarKernel, VmHandle, VmState, + EXECUTION_DRIVER_NAME, PYTHON_VFS_RPC_GUEST_ROOT, }; use crate::{DispatchResult, NativeSidecar, NativeSidecarBridge, SidecarError}; @@ -736,23 +736,20 @@ fn refresh_shadow_inventory_path(vm: &mut VmState, guest_path: &str) -> Result<( } let guest_path = normalize_path(guest_path); let mut updates = collect_shadow_inventory_ancestors(vm, &guest_path)?; - let Some(node_type) = shadow_inventory_kernel_node_type(vm, &guest_path)? else { + let Some(entry) = shadow_inventory_entry(vm, &guest_path)? else { forget_shadow_inventory_path(vm, &guest_path); return Ok(()); }; - updates.insert( - guest_path.clone(), - ShadowSyncInventoryEntry::present(node_type), - ); - if node_type == ShadowNodeType::Directory { + updates.insert(guest_path.clone(), entry); + if entry.node_type == ShadowNodeType::Directory { let entries = vm .kernel .read_dir_recursive(&guest_path, None) .map_err(kernel_error)?; for entry in entries { let path = normalize_path(&entry.path); - if let Some(node_type) = shadow_inventory_kernel_node_type(vm, &path)? { - updates.insert(path, ShadowSyncInventoryEntry::present(node_type)); + if let Some(entry) = shadow_inventory_entry(vm, &path)? { + updates.insert(path, entry); } } } @@ -772,11 +769,8 @@ fn refresh_shadow_inventory_path(vm: &mut VmState, guest_path: &str) -> Result<( fn refresh_shadow_inventory_node(vm: &mut VmState, guest_path: &str) -> Result<(), SidecarError> { let guest_path = normalize_path(guest_path); let mut updates = collect_shadow_inventory_ancestors(vm, &guest_path)?; - if let Some(node_type) = shadow_inventory_kernel_node_type(vm, &guest_path)? { - updates.insert( - guest_path.clone(), - ShadowSyncInventoryEntry::present(node_type), - ); + if let Some(entry) = shadow_inventory_entry(vm, &guest_path)? { + updates.insert(guest_path.clone(), entry); } vm.shadow_sync_inventory.extend(updates); Ok(()) @@ -801,20 +795,21 @@ fn collect_shadow_inventory_ancestors( } let mut updates = BTreeMap::new(); for ancestor in ancestors.into_iter().rev() { - if shadow_inventory_kernel_node_type(vm, &ancestor)? == Some(ShadowNodeType::Directory) { - updates.insert( - ancestor, - ShadowSyncInventoryEntry::present(ShadowNodeType::Directory), - ); + if let Some(entry) = shadow_inventory_entry(vm, &ancestor)? { + if entry.node_type == ShadowNodeType::Directory { + updates.insert(ancestor, entry); + } } } Ok(updates) } -fn shadow_inventory_kernel_node_type( +/// Inventory a path whose kernel state the sidecar just mirrored into the +/// shadow, recording both sides as reconciled. +fn shadow_inventory_entry( vm: &mut VmState, guest_path: &str, -) -> Result, SidecarError> { +) -> Result, SidecarError> { // This inventory is trusted sidecar bookkeeping after the guest mutation has // already passed its operation-specific permission check. Re-entering through // `KernelVm::lstat` would incorrectly require a separate guest `fs.stat` @@ -831,7 +826,24 @@ fn shadow_inventory_kernel_node_type( } else { ShadowNodeType::File }; - Ok(Some(node_type)) + let shadow_path = shadow_host_path_for_guest(&vm.cwd, guest_path); + let shadow = match fs::symlink_metadata(&shadow_path) { + Ok(metadata) => Some(ShadowSyncStamp::host(&metadata)), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, + Err(error) => { + return Err(SidecarError::Io(format!( + "failed to inspect shadow path {}: {error}", + shadow_path.display() + ))); + } + }; + Ok(Some(ShadowSyncInventoryEntry::reconciled( + node_type, + ShadowSyncBaseline { + shadow, + kernel: Some(ShadowSyncStamp::kernel(&stat)), + }, + ))) } fn forget_shadow_inventory_path(vm: &mut VmState, guest_path: &str) { @@ -5377,8 +5389,20 @@ fn sync_active_shadow_path_to_kernel( if host_paths.is_empty() && !vm.kernel.exists(&guest_path).unwrap_or(false) { host_paths.push(shadow_host_path_for_guest(&vm.cwd, &guest_path)); } + // The root walk above already decided which side wins for a reconciled + // shadow path; importing it again here would undo a newer kernel edit. + let reconciled_in_root_shadow = vm + .shadow_sync_inventory + .get(&guest_path) + .is_some_and(|entry| entry.baseline.is_some()); + let vm_shadow_root = normalize_host_path(&vm.cwd); for host_path in host_paths { + if reconciled_in_root_shadow + && path_is_within_root(&normalize_host_path(&host_path), &vm_shadow_root) + { + continue; + } let metadata = match fs::symlink_metadata(&host_path) { Ok(metadata) => metadata, Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, diff --git a/crates/native-sidecar/src/state.rs b/crates/native-sidecar/src/state.rs index 008dc778e0..e56e5103ea 100644 --- a/crates/native-sidecar/src/state.rs +++ b/crates/native-sidecar/src/state.rs @@ -1771,6 +1771,10 @@ pub(crate) struct ShadowSyncInventoryEntry { /// makes the next walk retry instead of permanently forgetting a failed /// reconciliation. pub(crate) deletion_pending: bool, + /// Both sides of this path as of its last reconciliation. `None` for + /// shadow content that the sidecar seeded but never reconciled, such as + /// the bootstrap tree. + pub(crate) baseline: Option, } impl ShadowSyncInventoryEntry { @@ -1778,6 +1782,69 @@ impl ShadowSyncInventoryEntry { Self { node_type, deletion_pending: false, + baseline: None, + } + } + + pub(crate) fn reconciled(node_type: ShadowNodeType, baseline: ShadowSyncBaseline) -> Self { + Self { + baseline: Some(baseline), + ..Self::present(node_type) + } + } +} + +/// The shadow and kernel nodes at one path as the sidecar last reconciled them. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct ShadowSyncBaseline { + /// `None` when that side had no node at this path. + pub(crate) shadow: Option, + pub(crate) kernel: Option, +} + +/// Change detector for one filesystem node. Every write, truncate, chmod, +/// utimes, and replacement moves `ino`, `mode`, or the times, so an equal +/// stamp means the node was not modified. Directory times are left out: +/// they move with child changes, and children carry their own stamps. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct ShadowSyncStamp { + ino: u64, + mode: u32, + contents: Option, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct ShadowSyncContentStamp { + size: u64, + mtime: (i64, u32), + ctime: (i64, u32), +} + +impl ShadowSyncStamp { + pub(crate) fn host(metadata: &std::fs::Metadata) -> Self { + use std::os::unix::fs::MetadataExt; + Self { + ino: metadata.ino(), + mode: metadata.mode(), + contents: (!metadata.is_dir()).then(|| ShadowSyncContentStamp { + size: metadata.size(), + mtime: (metadata.mtime(), metadata.mtime_nsec() as u32), + ctime: (metadata.ctime(), metadata.ctime_nsec() as u32), + }), + } + } + + pub(crate) fn kernel(stat: &agentos_kernel::vfs::VirtualStat) -> Self { + Self { + ino: stat.ino, + mode: stat.mode, + contents: (!stat.is_directory || stat.is_symbolic_link).then_some( + ShadowSyncContentStamp { + size: stat.size, + mtime: (stat.mtime_ms as i64, stat.mtime_nsec), + ctime: (stat.ctime_ms as i64, stat.ctime_nsec), + }, + ), } } } diff --git a/crates/native-sidecar/tests/filesystem.rs b/crates/native-sidecar/tests/filesystem.rs index 4e1d2b8906..ccd07b8c69 100644 --- a/crates/native-sidecar/tests/filesystem.rs +++ b/crates/native-sidecar/tests/filesystem.rs @@ -2061,4 +2061,151 @@ try { dispose_vm_and_close_session(&mut sidecar, &connection_id, &session_id, &vm_id); } + + /// Regression for #2022 and #1901: Python and shell writes reach only the + /// kernel VFS, so the stale API-seeded shadow copy must not replace them + /// (or resurrect a deleted-then-recreated file) on the next host read. + #[test] + fn python_and_shell_edits_to_host_written_files_survive_shadow_reconciliation() { + if registry_command_root().is_none() { + return; + } + support::assert_node_available(); + + let mut sidecar = create_test_sidecar(); + let (connection_id, session_id) = authenticate_and_open_session(&mut sidecar); + let vm_id = create_vm_with_mounts(&mut sidecar, &connection_id, &session_id, Vec::new()); + + enum Editor { + Python, + Shell, + } + let cases = [ + ( + "python-overwrite", + Editor::Python, + "from pathlib import Path\nPath('/workspace/python-overwrite.txt').write_text('EDITED\\n')", + "EDITED\n", + ), + ( + "shell-overwrite", + Editor::Shell, + "echo EDITED > /workspace/shell-overwrite.txt", + "EDITED\n", + ), + ( + "shell-recreate", + Editor::Shell, + "rm /workspace/shell-recreate.txt && echo RECREATED > /workspace/shell-recreate.txt", + "RECREATED\n", + ), + ]; + + let mut request_id = 20; + for (name, editor, script, expected) in cases { + let target = format!("/workspace/{name}.txt"); + host_write_text( + &mut sidecar, + &connection_id, + &session_id, + &vm_id, + request_id, + &target, + "original\n", + ); + + let process_id = format!("proc-edit-{name}"); + let (_, stderr, exit_code) = match editor { + Editor::Python => execute_python_code( + &mut sidecar, + &connection_id, + &session_id, + &vm_id, + request_id + 1, + &process_id, + script, + ), + Editor::Shell => execute_command( + &mut sidecar, + &connection_id, + &session_id, + &vm_id, + request_id + 1, + &process_id, + "/bin/sh", + vec![String::from("-c"), String::from(script)], + ), + }; + assert_eq!(exit_code, Some(0), "{name}: stderr: {stderr}"); + + assert_eq!( + guest_read_text( + &mut sidecar, + &connection_id, + &session_id, + &vm_id, + request_id + 2, + &target, + ), + expected, + "{name}: host readFile lost the guest edit to a host-written file" + ); + + let (cat_stdout, cat_stderr, cat_exit_code) = execute_command( + &mut sidecar, + &connection_id, + &session_id, + &vm_id, + request_id + 3, + &format!("proc-cat-{name}"), + "/bin/cat", + vec![target.clone()], + ); + assert_eq!(cat_exit_code, Some(0), "{name}: stderr: {cat_stderr}"); + assert_eq!( + cat_stdout, expected, + "{name}: a later process saw stale host-written bytes" + ); + request_id += 10; + } + + dispose_vm_and_close_session(&mut sidecar, &connection_id, &session_id, &vm_id); + } + + fn execute_python_code( + sidecar: &mut agentos_native_sidecar::NativeSidecar, + connection_id: &str, + session_id: &str, + vm_id: &str, + request_id: i64, + process_id: &str, + code: &str, + ) -> (String, String, Option) { + let response = sidecar + .dispatch_wire_blocking(wire_request( + request_id, + wire_vm(connection_id, session_id, vm_id), + RequestPayload::ExecuteRequest(ExecuteRequest { + process_id: String::from(process_id), + command: None, + runtime: Some(GuestRuntimeKind::Python), + entrypoint: Some(String::from(code)), + args: Vec::new(), + env: HashMap::new(), + cwd: Some(String::from("/workspace")), + wasm_permission_tier: None, + retain_output: false, + }), + )) + .expect("dispatch Python execute"); + + match response.response.payload { + ResponsePayload::ProcessStartedResponse(started) => { + assert_eq!(started.process_id, process_id); + } + other => panic!("unexpected execute response: {other:?}"), + } + + drain_process_output(sidecar, connection_id, session_id, vm_id, process_id) + } }