Close two cross-file handoffs from the round-3 fixes
The terminal's drop target derived the tar entry name from the host path *after* symlink resolution, so dropping ~/Downloads/latest.log — where latest.log is a symlink — landed the file in the container under the target's name. Nothing errored; the user got a name they never typed. The Files pane had the identical bug and was fixed with `host_upload_name`; the terminal now calls the same helper, so the two drop targets cannot drift. It also drops the "dropped-file" fallback, which silently renamed anything the old `file_name()` could not parse. `migration_store::load` told the user a backup existed when it had deliberately not written one. `load` runs on every reconcile, survey and reaper pass, so a persistently corrupt record reaches MAX_CORRUPT_BACKUPS within seconds; from then on the copy was skipped while the log still read "(a copy was kept at <path>)". Three outcomes are now distinct, and the "enough already" case says so rather than naming a file that is not there — that being the message someone reads immediately before going to look for their data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc
This commit is contained in:
@@ -978,7 +978,7 @@ pub async fn resolve_host_read_path(path: &str) -> Result<String, String> {
|
|||||||
/// `Path::file_name` so a Windows path is split as one wherever this runs, and
|
/// `Path::file_name` so a Windows path is split as one wherever this runs, and
|
||||||
/// the answer goes through [`validate_entry_name`] because it becomes a tar
|
/// the answer goes through [`validate_entry_name`] because it becomes a tar
|
||||||
/// entry name, a container path and an argv element.
|
/// entry name, a container path and an argv element.
|
||||||
fn host_upload_name(path: &str) -> Result<String, String> {
|
pub(crate) fn host_upload_name(path: &str) -> Result<String, String> {
|
||||||
if normalize_host_path(path).ends_with('/') {
|
if normalize_host_path(path).ends_with('/') {
|
||||||
// A trailing separator names a directory, and `Downloads` is not the
|
// A trailing separator names a directory, and `Downloads` is not the
|
||||||
// name of a file to upload. The recursive-upload refusal further down
|
// name of a file to upload. The recursive-upload refusal further down
|
||||||
|
|||||||
@@ -203,6 +203,13 @@ pub async fn upload_host_file_to_terminal(
|
|||||||
// symlinks already resolved, so a visible directory that *leads* to `~/.ssh`
|
// symlinks already resolved, so a visible directory that *leads* to `~/.ssh`
|
||||||
// is refused too. What comes back is that resolved path, and it is what
|
// is refused too. What comes back is that resolved path, and it is what
|
||||||
// gets opened.
|
// gets opened.
|
||||||
|
// The name is taken from the path the user actually dropped, *before*
|
||||||
|
// resolution. Deriving it from the resolved path renames the file behind
|
||||||
|
// the user's back: dropping `~/Downloads/latest.log`, where `latest.log` is
|
||||||
|
// a symlink, would land it in the container as `2026-08-23.log`. The Files
|
||||||
|
// pane's upload had the same bug and fixes it the same way — one helper, so
|
||||||
|
// the two drop targets cannot drift.
|
||||||
|
let base = crate::commands::file_commands::host_upload_name(&host_path)?;
|
||||||
let host_path = crate::commands::file_commands::resolve_host_read_path(&host_path).await?;
|
let host_path = crate::commands::file_commands::resolve_host_read_path(&host_path).await?;
|
||||||
|
|
||||||
let container_id = state.exec_manager.get_container_id(&session_id).await?;
|
let container_id = state.exec_manager.get_container_id(&session_id).await?;
|
||||||
@@ -228,11 +235,7 @@ pub async fn upload_host_file_to_terminal(
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
let base = std::path::Path::new(&host_path)
|
|
||||||
.file_name()
|
|
||||||
.map(|s| s.to_string_lossy().to_string())
|
|
||||||
.filter(|s| !s.is_empty())
|
|
||||||
.unwrap_or_else(|| "dropped-file".to_string());
|
|
||||||
|
|
||||||
// Ensure the destination directory exists rather than relying on Docker's
|
// Ensure the destination directory exists rather than relying on Docker's
|
||||||
// archive extractor to create the parent for the uploaded tar entry.
|
// archive extractor to create the parent for the uploaded tar entry.
|
||||||
@@ -295,3 +298,38 @@ pub async fn stop_audio_bridge(
|
|||||||
state.exec_manager.close_session(&audio_session_id).await;
|
state.exec_manager.close_session(&audio_session_id).await;
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
/// Both drop targets must name a dropped file the way the *user* named it.
|
||||||
|
///
|
||||||
|
/// The bug this pins: `upload_host_file_to_terminal` derived the tar entry
|
||||||
|
/// name from the path *after* symlink resolution, so dropping
|
||||||
|
/// `~/Downloads/latest.log` — where `latest.log` is a symlink to
|
||||||
|
/// `2026-08-23.log` — silently landed the file in the container under the
|
||||||
|
/// target's name. Nothing errored; the user just got a name they never
|
||||||
|
/// typed. The Files pane had the identical bug.
|
||||||
|
///
|
||||||
|
/// What actually keeps the two from drifting is that they now call one
|
||||||
|
/// helper, so this asserts that helper's contract from the terminal side:
|
||||||
|
/// the answer comes from the spelling, and a path that does not name a file
|
||||||
|
/// is refused rather than silently substituted (it used to fall back to
|
||||||
|
/// `"dropped-file"`).
|
||||||
|
#[test]
|
||||||
|
fn a_dropped_file_keeps_the_name_the_user_dropped() {
|
||||||
|
use crate::commands::file_commands::host_upload_name;
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
host_upload_name("/home/u/Downloads/latest.log").unwrap(),
|
||||||
|
"latest.log"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
host_upload_name("/home/u/Downloads/").is_err(),
|
||||||
|
"a directory is not a file to drop"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
host_upload_name("/home/u/..").is_err(),
|
||||||
|
"the name becomes a tar entry, a container path and an argv element"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -82,22 +82,41 @@ pub fn load(project_id: &str) -> Result<Option<MigrationState>, String> {
|
|||||||
match serde_json::from_str::<MigrationState>(&data) {
|
match serde_json::from_str::<MigrationState>(&data) {
|
||||||
Ok(state) => Ok(Some(state)),
|
Ok(state) => Ok(Some(state)),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
|
// Three outcomes, and they must not be conflated: a copy was made,
|
||||||
|
// a copy was deliberately not made, or a copy failed. The previous
|
||||||
|
// version folded "already kept enough" into `Ok(())` and then told
|
||||||
|
// the user "a copy was kept at <path>" — naming a file that was
|
||||||
|
// never created. A message that invents a backup is worse than no
|
||||||
|
// message, because it is what someone reads before going to look
|
||||||
|
// for their data.
|
||||||
let backup = corrupt_backup_path(&path, &chrono::Utc::now());
|
let backup = corrupt_backup_path(&path, &chrono::Utc::now());
|
||||||
let copied = if backup.exists() || corrupt_backups_full(&path) {
|
let kept = if backup.exists() {
|
||||||
// Already kept a copy of this exact corruption this second, or
|
Kept::AlreadyThere
|
||||||
// kept as many as are worth keeping. Either way nothing to add.
|
} else if corrupt_backups_full(&path) {
|
||||||
Ok(())
|
Kept::EnoughAlready(MAX_CORRUPT_BACKUPS)
|
||||||
} else {
|
} else {
|
||||||
fs::copy(&path, &backup).map(|_| ())
|
match fs::copy(&path, &backup) {
|
||||||
|
Ok(_) => Kept::Copied,
|
||||||
|
Err(e) => Kept::Failed(e.to_string()),
|
||||||
|
}
|
||||||
};
|
};
|
||||||
log::error!(
|
log::error!(
|
||||||
"Failed to parse migration state for project {}: {} — treating as absent, but \
|
"Failed to parse migration state for project {}: {} — treating as absent, but \
|
||||||
the record is left in place so `has_record` still protects its rollback pin{}",
|
the record is left in place so `has_record` still protects its rollback pin{}",
|
||||||
project_id,
|
project_id,
|
||||||
e,
|
e,
|
||||||
match copied {
|
match kept {
|
||||||
Ok(()) => format!(" (a copy was kept at {})", backup.display()),
|
Kept::Copied | Kept::AlreadyThere =>
|
||||||
Err(ref e) => format!(" (could not keep a copy: {})", e),
|
format!(" (a copy is at {})", backup.display()),
|
||||||
|
// The earliest copies are the ones worth having, so the cap
|
||||||
|
// keeps those and drops this one. Say so, rather than
|
||||||
|
// implying a file exists.
|
||||||
|
Kept::EnoughAlready(n) => format!(
|
||||||
|
" (no copy kept — {} earlier copies of this record are already saved \
|
||||||
|
alongside it)",
|
||||||
|
n
|
||||||
|
),
|
||||||
|
Kept::Failed(ref e) => format!(" (could not keep a copy: {})", e),
|
||||||
}
|
}
|
||||||
);
|
);
|
||||||
Ok(None)
|
Ok(None)
|
||||||
@@ -105,6 +124,17 @@ pub fn load(project_id: &str) -> Result<Option<MigrationState>, String> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// What [`load`] did about a copy of an unparseable record, so the log line can
|
||||||
|
/// tell the truth about whether a file exists.
|
||||||
|
enum Kept {
|
||||||
|
Copied,
|
||||||
|
/// This exact second's copy was already on disk.
|
||||||
|
AlreadyThere,
|
||||||
|
/// The cap is reached; the earlier copies are kept and this one is not.
|
||||||
|
EnoughAlready(usize),
|
||||||
|
Failed(String),
|
||||||
|
}
|
||||||
|
|
||||||
/// Where a copy of an unparseable record is kept.
|
/// Where a copy of an unparseable record is kept.
|
||||||
///
|
///
|
||||||
/// Timestamped rather than a fixed `.bak`: a second corruption used to
|
/// Timestamped rather than a fixed `.bak`: a second corruption used to
|
||||||
@@ -440,4 +470,43 @@ mod tests {
|
|||||||
"ab62cd24-51aa-4645-8f5c-17a124062050"
|
"ab62cd24-51aa-4645-8f5c-17a124062050"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The log line must not name a backup that was never written.
|
||||||
|
///
|
||||||
|
/// `load` runs on every reconcile, every survey and every reaper pass, so a
|
||||||
|
/// persistently corrupt record hits the `MAX_CORRUPT_BACKUPS` cap within
|
||||||
|
/// seconds. The previous code folded "already kept enough" into `Ok(())`
|
||||||
|
/// and then reported " (a copy was kept at <path>)" — pointing at a file
|
||||||
|
/// that does not exist. That is the message someone reads immediately
|
||||||
|
/// before going to look for their data.
|
||||||
|
#[test]
|
||||||
|
fn the_corrupt_record_message_only_claims_a_copy_that_exists() {
|
||||||
|
let dir = std::env::temp_dir().join(format!(
|
||||||
|
"tc-mstore-{}",
|
||||||
|
uuid::Uuid::new_v4().simple()
|
||||||
|
));
|
||||||
|
std::fs::create_dir_all(&dir).unwrap();
|
||||||
|
let path = dir.join("p.json");
|
||||||
|
std::fs::write(&path, b"{ not json").unwrap();
|
||||||
|
|
||||||
|
// Fill the cap with copies that really are on disk.
|
||||||
|
for i in 0..MAX_CORRUPT_BACKUPS {
|
||||||
|
let b = path.with_extension(format!("json.corrupt-2026010{}-000000.bak", i));
|
||||||
|
std::fs::write(&b, b"{ not json").unwrap();
|
||||||
|
}
|
||||||
|
assert!(corrupt_backups_full(&path), "precondition: the cap is reached");
|
||||||
|
|
||||||
|
// With the cap reached, no new copy may be created — and that is the
|
||||||
|
// state in which the old message lied.
|
||||||
|
let before: Vec<_> = std::fs::read_dir(&dir).unwrap().flatten().collect();
|
||||||
|
let fresh = corrupt_backup_path(&path, &chrono::Utc::now());
|
||||||
|
assert!(
|
||||||
|
!fresh.exists(),
|
||||||
|
"the cap is reached, so this timestamped copy must not be written"
|
||||||
|
);
|
||||||
|
let after: Vec<_> = std::fs::read_dir(&dir).unwrap().flatten().collect();
|
||||||
|
assert_eq!(before.len(), after.len(), "nothing new appeared on disk");
|
||||||
|
|
||||||
|
std::fs::remove_dir_all(&dir).ok();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user