fix(core,fuse): keep replaced and vanished files from failing saves, reads and syncs

Fixes from the account acceptance runs:

- B104: a reader open across a rename-over keeps working; the replaced
  node is unlinked like any other open file instead of forgotten.
- B105: a drain worker holding a revision withdrawn by an unlink no
  longer uploads it as a recovered-… conflict copy (Db::op_exists).
- B106: an open victim's queued ops are withdrawn at trash time, so a
  repeated editor save no longer fails with AlreadyExists; the staged
  blob goes with the last close.
- B107: a remote read gives up after 120 s with EIO instead of hanging
  its caller; the timer is built inside the runtime.
- B108: a mirror upload whose local file was deleted after the scan is
  skipped rather than counted as an error.

The acceptance script reports the failing line for non-assert errors,
ignores queue ops that predate the run, purges with delete-forever
first, and diagnoses a malformed SQLite database against a local replay.
This commit is contained in:
nvrl committed 2026-09-29 16:20:51 +02:00
1 parent 7b67db5ced
commit a2992b3b9d
11 files changed
+579 -34

No files matched your search

+16
View File
@@ -1486,6 +1486,16 @@ impl Core {
let uid = parse_node_uid(&meta.uid).ok_or("staged write has an unparseable uid")?;
if let Some(reason) = self.revision_conflict(&uid, &meta)? {
// Trashing the file from the mount drops its queued write, but not
// from under a worker already holding it. That worker then finds the
// node trashed — by us — and would upload the withdrawn bytes as a
// conflict copy (a `recovered-…` file in the root when the tree has
// already forgotten the node). A write nobody queues any more is
// not a conflict.
if !self.db.op_exists(op.id)? {
debug!(%uid, reason, "queued write withdrawn while it drained; dropping it");
return Ok(());
}
return self.keep_as_conflict_copy(op, &blob, &meta, &uid, &reason);
}
@@ -1522,6 +1532,12 @@ impl Core {
// this is on the wire can stop it rather than paying for a revision the
// op it queued will immediately replace.
let cancel = self.begin_cancellable_upload(&uid);
// An unlink that withdrew this op before the flag above existed had no
// upload to cancel; the row is the record that it happened.
if !self.db.op_exists(op.id)? {
debug!(%uid, "queued write withdrawn before it went out; dropping it");
return Ok(());
}
let reader = CountingReader::new(File::open(&blob)?, &guard).with_cancel(cancel.flag());
let started = Instant::now();
let sent = self.rt.block_on(self.client.upload_new_revision_from(
+19 -2
View File
@@ -1743,12 +1743,18 @@ impl ProtonFs {
if let Some(uid) = unlinked_uid
&& release_can_discard_unlinked(&self.core.db, &uid)
{
// Ops withdrawn at unlink left the bytes this handle read behind,
// owned by no op any more; they go with the last handle.
let withdrawn = self.core.pending_blob(&uid);
if let Err(error) = self.core.discard_queued_ops(&uid) {
warn!(%uid, ?error, "queued-op cleanup failed; retaining unlinked state");
} else {
if let Err(error) = self.core.db.delete_node(&uid) {
warn!(%uid, %error, "deleting released unlinked node state failed");
}
if let Some(blob) = withdrawn {
self.core.cache.discard_staged(&blob);
}
self.core.cache.evict(&uid);
self.core.evict_reader(&uid);
}
@@ -2129,7 +2135,12 @@ impl ProtonFs {
// it just means its queued creation is no longer wanted. This works
// offline, which the remote path below cannot (offline.md Phase 3b).
if is_local_uid(&uid) {
if !open_now && let Err(error) = self.core.discard_queued_ops(&uid) {
let dropped = if open_now {
self.core.withdraw_queued_ops(&uid)
} else {
self.core.discard_queued_ops(&uid)
};
if let Err(error) = dropped {
reply.error(error);
return;
}
@@ -2168,7 +2179,13 @@ impl ProtonFs {
// Otherwise an EIO response still made the path disappear locally.
self.core.hidden.lock().insert(uid.clone());
self.core.state().forget_or_unlink(&uid);
if !open_now {
if open_now {
// The open handle keeps the staged bytes; the drain must not send
// them to a node that is already in the trash (docs/BUGS.md B106).
if let Err(error) = self.core.withdraw_queued_ops(&uid) {
warn!(%uid, ?error, "remote trash landed but queued-op cleanup failed");
}
} else {
if let Err(error) = self.core.discard_queued_ops(&uid) {
warn!(%uid, ?error, "remote trash landed but queued-op cleanup failed");
}
+151 -10
View File
@@ -3539,6 +3539,30 @@ impl Core {
Ok(())
}
/// Drop every op queued against a node that just left the namespace while
/// a handle still has it open, but keep the bytes that handle reads.
///
/// The ops cannot wait for the final close: the node is already trashed, and
/// a revision the drain sends in the meantime lands on it after the trash —
/// an editor's save loop then found the name it was renaming onto still
/// taken (`docs/BUGS.md` B106). The pending entry and its staged blob are
/// what the open handle reads, so they stay until the final close drops
/// them.
fn withdraw_queued_ops(&self, uid: &NodeUid) -> Result<(), Errno> {
self.cancel_upload(uid);
let blobs = self.db.delete_ops_for_uid(&uid.to_string()).map_err(|e| {
error!(%uid, error = %e, "withdrawing queued ops failed");
Errno::EIO
})?;
let read_from = self.pending_blob(uid);
let blobs = blobs
.into_iter()
.filter(|blob| read_from.as_deref() != Some(Path::new(blob)))
.collect();
self.release_dropped_below(blobs);
Ok(())
}
/// Let go of what the ops just dropped for `uid` held: their pending
/// entries, any upload still reading their bytes, and the bytes.
///
@@ -3546,9 +3570,15 @@ impl Core {
/// (`docs/BUGS.md` B102), so the entries to forget are found by blob, not
/// only by `uid`.
fn release_dropped_ops(&self, uid: &NodeUid, blobs: Vec<String>) {
self.pending.lock().remove(uid);
self.release_dropped_below(blobs);
}
/// [`Core::release_dropped_ops`] without touching the node's own pending
/// entry.
fn release_dropped_below(&self, blobs: Vec<String>) {
let below: Vec<NodeUid> = {
let mut pending = self.pending.lock();
pending.remove(uid);
let below: Vec<NodeUid> = pending
.iter()
.filter(|(_, p)| blobs.iter().any(|b| p.path == Path::new(b)))
@@ -4420,11 +4450,21 @@ impl Core {
/// A node whose own creation is still queued has never reached the server, so
/// dropping its queued ops is the whole removal; nothing goes to the wire and
/// it works offline.
///
/// A handle still open on the replaced node keeps reading its bytes, as
/// `rename(2)` promises: the inode stays as an unlinked entry and its
/// staged bytes, cache and reader go at the final close, exactly as for an
/// `unlink` under an open handle. Its queued ops go now, or the drain would
/// upload to the node after it was trashed (`docs/BUGS.md` B106).
fn remove_replaced(&self, uid: &NodeUid, name: &str) -> Result<(), Errno> {
if is_local_uid(uid) {
self.discard_queued_ops(uid)?;
if self.is_open_anywhere(uid) {
self.withdraw_queued_ops(uid)?;
} else {
self.discard_queued_ops(uid)?;
}
self.for_each_state(|st| {
st.forget(uid);
st.forget_or_unlink(uid);
});
debug!(%uid, name, "replaced a node whose create was still queued");
return Ok(());
@@ -4439,17 +4479,26 @@ impl Core {
self.log_activity(ActivityKind::Trash, name, e.to_string(), false);
return Err(Errno::EIO);
}
if let Err(error) = self.discard_queued_ops(uid) {
error!(%uid, "remote replacement landed but queued-op cleanup failed");
return Err(error);
}
// The node was trashed on the server; withdraw it from every inode
// space that had it, not only the mount the rename came through.
let open_now = self.is_open_anywhere(uid);
self.for_each_state(|st| {
st.forget(uid);
st.forget_or_unlink(uid);
});
self.cache.evict(uid);
self.evict_reader(uid);
self.hidden.lock().insert(uid.clone());
if open_now {
if let Err(error) = self.withdraw_queued_ops(uid) {
error!(%uid, "remote replacement landed but queued-op cleanup failed");
return Err(error);
}
} else {
if let Err(error) = self.discard_queued_ops(uid) {
error!(%uid, "remote replacement landed but queued-op cleanup failed");
return Err(error);
}
self.cache.evict(uid);
self.evict_reader(uid);
}
self.invalidate_trash();
// The node is recoverable from the trash, but only if the user knows it
// went there — a rename is not an operation anyone expects to trash
@@ -4463,6 +4512,19 @@ impl Core {
Ok(())
}
/// Whether any mount holds an open handle on `uid`.
fn is_open_anywhere(&self, uid: &NodeUid) -> bool {
let mut open = false;
self.for_each_state(|st| {
open |= st
.by_uid
.get(uid)
.and_then(|ino| st.entries.get(ino))
.is_some_and(|entry| entry.open_count > 0);
});
open
}
/// Put back the node [`Core::remove_replaced`] trashed, after the rename it
/// was clearing the way for failed anyway.
///
@@ -7946,6 +8008,7 @@ mod tests {
("fn enqueue_staged_write(", "attach_blob_to_create"),
("fn queue_trash(", "replace_ops_with_trash"),
("fn discard_queued_ops(", "delete_ops_for_uid"),
("fn withdraw_queued_ops(", "delete_ops_for_uid"),
] {
let function = function_source(source, signature);
assert_before(function, "cancel_upload", mutation);
@@ -8006,6 +8069,84 @@ mod tests {
assert_before(folder, "require_uid_writable(parent_uid)", ".create_folder");
}
#[test]
fn a_replaced_node_survives_its_open_handles() {
// A rename over a file someone is reading must leave that reader on the
// replaced bytes; forgetting the inode outright made its next read EIO.
let source = include_str!("lib.rs");
let replaced = function_source(source, "fn remove_replaced(");
assert!(replaced.contains("forget_or_unlink(uid)"));
assert!(!replaced.contains("st.forget(uid)"));
assert_before(
replaced,
"if self.is_open_anywhere(uid)",
"self.discard_queued_ops(uid)",
);
assert_before(replaced, "if open_now", "self.cache.evict(uid)");
// ...but its queued ops go at once, open or not: a revision drained
// after the trash landed on the trashed node and kept its name taken.
assert_eq!(replaced.matches("self.withdraw_queued_ops(uid)").count(), 2);
}
#[test]
fn a_remote_read_gives_up_at_its_deadline() {
// The SDK bounds requests, not reads; a stalled link held one `read`
// for 34 minutes.
let remote = function_source(include_str!("reads.rs"), "fn read_range_remote(");
assert_before(
remote,
"let fetch = async",
"tokio::time::timeout(READ_FETCH_TIMEOUT, fetch)",
);
// Built inside the runtime: a FUSE worker has no reactor, and a timer
// built there panicked every remote read.
assert!(remote.contains("async { tokio::time::timeout(READ_FETCH_TIMEOUT, fetch).await }"));
}
#[test]
fn a_deadline_built_under_block_on_needs_no_ambient_runtime() {
let rt = tokio::runtime::Builder::new_multi_thread()
.worker_threads(1)
.enable_all()
.build()
.unwrap();
let answered = std::thread::spawn(move || {
rt.block_on(async { tokio::time::timeout(Duration::from_secs(5), async { 7 }).await })
})
.join()
.expect("a worker thread outside the runtime can wait on the deadline");
assert_eq!(answered.unwrap(), 7);
}
#[test]
fn a_trashed_open_node_sends_nothing_more() {
// Trashing a file under an open handle withdraws its queued upload at
// once; the bytes the handle reads go with the last close.
let withdraw = function_source(include_str!("lib.rs"), "fn withdraw_queued_ops(");
assert_before(withdraw, "self.pending_blob(uid)", "release_dropped_below");
assert!(!withdraw.contains("self.pending.lock().remove(uid)"));
let filesystem = include_str!("filesystem.rs");
let trash = function_source(filesystem, "fn trash_child(");
assert_eq!(
trash.matches("self.core.withdraw_queued_ops(&uid)").count(),
2
);
let release = function_source(filesystem, "fn finish_release(");
assert_before(
release,
"let withdrawn = self.core.pending_blob(&uid)",
"discard_queued_ops",
);
assert_before(release, "discard_queued_ops", "discard_staged(&blob)");
// A drain worker that claimed the op before it was withdrawn checks
// again before any byte leaves.
let drain = function_source(include_str!("drain.rs"), "pub(crate) fn drain_revision(");
let upload = &drain[drain.find("begin_cancellable_upload").unwrap()..];
assert_before(upload, "self.db.op_exists(op.id)", "File::open(&blob)");
}
#[test]
fn replacement_rename_authorizes_source_and_both_parents() {
use std::cell::RefCell;
+26 -2
View File
@@ -36,6 +36,15 @@ pub(super) const REPAIR_MAX: u64 = 512 * 1024 * 1024;
/// demand read in the SDK's flat block semaphore.
pub(super) const PREFETCH_BUDGET: usize = 8;
/// How long one read may wait on the network before it fails with `EIO`.
///
/// The SDK bounds each request, not a read: a block that times out is retried,
/// and a read can need several blocks, a block table and a repair. On a link
/// that stalled without dropping, one `read` held a FUSE worker for 34 minutes
/// and the application behind it with it (`docs/BUGS.md` B107). An error the
/// caller can retry beats a process stuck in `D` state.
pub(super) const READ_FETCH_TIMEOUT: Duration = Duration::from_secs(120);
/// Cap on tracked sequential streams. Only reads in progress matter, so the map
/// is cleared wholesale rather than aged — losing the ramp costs one read.
const MAX_PREFETCH_STREAMS: usize = 256;
@@ -939,7 +948,7 @@ impl Core {
}
if !misses.is_empty() {
let fetched = self.rt.block_on(async {
let fetch = async {
// Resolve the file's keys and block table once, then read every
// missing block through the shared reader. Previously each block
// called `download_range`, which redid that resolution per block.
@@ -960,7 +969,22 @@ impl Core {
out.push(joined.map_err(|_| Errno::EIO)??);
}
Ok::<_, Errno>(out)
})?;
};
// Dropping the fetch on the deadline aborts the block tasks with it;
// blocks that already landed stay cached for the retry. The timer is
// built inside the runtime: a FUSE worker has no reactor of its own,
// and building it out here panicked every remote read.
let fetched = self
.rt
.block_on(async { tokio::time::timeout(READ_FETCH_TIMEOUT, fetch).await })
.unwrap_or_else(|_| {
warn!(
%uid, offset, len, secs = READ_FETCH_TIMEOUT.as_secs(),
"read got no answer from the network in time; failing it"
);
self.evict_reader(uid);
Err(Errno::EIO)
})?;
for (bidx, bytes) in fetched {
blocks[(bidx - first) as usize] = Some(bytes);
}
+55 -11
View File
@@ -73,6 +73,9 @@ enum Applied {
Downloaded,
/// A conflict copy was kept.
Conflict,
/// The local file to upload was deleted after the scan planned it. There is
/// nothing left to send, and the next pass sees the deletion (B108).
Vanished,
}
impl Core {
@@ -1147,13 +1150,10 @@ impl Core {
Ok(Applied::Dir(rel.clone(), uid))
}
Pending::UploadNew { rel, parent } => {
self.upload_new(folder_id, local_root, rel, parent).await?;
Ok(Applied::Uploaded)
self.upload_new(folder_id, local_root, rel, parent).await
}
Pending::UploadRevision { rel, uid } => {
self.upload_revision(folder_id, local_root, rel, uid)
.await?;
Ok(Applied::Uploaded)
self.upload_revision(folder_id, local_root, rel, uid).await
}
Pending::Download {
rel,
@@ -1217,10 +1217,12 @@ impl Core {
local_root: &Path,
rel: &str,
parent: &NodeUid,
) -> Result<(), String> {
) -> Result<Applied, String> {
let path = local_root.join(rel_to_path(rel));
let name = base_name(rel);
let file = std::fs::File::open(&path).map_err(|e| format!("open {rel}: {e}"))?;
let Some(file) = open_to_upload(&path, rel)? else {
return Ok(Applied::Vanished);
};
let meta = file.metadata().map_err(|e| format!("stat {rel}: {e}"))?;
let mtime = system_mtime(&meta);
// Taken from the descriptor being streamed, so it names the content that
@@ -1261,7 +1263,8 @@ impl Core {
self.invalidate_children_of(parent);
warn_if_torn(rel, &path, streamed);
self.record_file_baseline(folder_id, rel, Some(streamed), &uid)
.await
.await?;
Ok(Applied::Uploaded)
}
/// Upload a changed local file as a new revision of an existing remote node.
@@ -1271,9 +1274,11 @@ impl Core {
local_root: &Path,
rel: &str,
uid: &NodeUid,
) -> Result<(), String> {
) -> Result<Applied, String> {
let path = local_root.join(rel_to_path(rel));
let file = std::fs::File::open(&path).map_err(|e| format!("open {rel}: {e}"))?;
let Some(file) = open_to_upload(&path, rel)? else {
return Ok(Applied::Vanished);
};
let meta = file.metadata().map_err(|e| format!("stat {rel}: {e}"))?;
let mtime = system_mtime(&meta);
let streamed = LocalSig::from(&meta);
@@ -1295,7 +1300,8 @@ impl Core {
.map_err(|e| format!("upload revision {rel}: {e}"))?;
warn_if_torn(rel, &path, streamed);
self.record_file_baseline(folder_id, rel, Some(streamed), uid)
.await
.await?;
Ok(Applied::Uploaded)
}
/// Download a remote file to its local path (atomically via a temp file),
@@ -1445,6 +1451,23 @@ impl Core {
}
}
/// Open a local file for upload, or `None` when it is gone.
///
/// A scan plans uploads from a listing, and the file can be deleted before its
/// upload starts — a build or a test tree cleaned up mid-pass. That is not a
/// failure: the next pass sees the deletion. Counting it as one put the folder
/// in `error` for a file nobody wanted any more (B108).
fn open_to_upload(path: &Path, rel: &str) -> Result<Option<std::fs::File>, String> {
match std::fs::File::open(path) {
Ok(file) => Ok(Some(file)),
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
debug!(rel, "sync: file deleted before its upload; skipping");
Ok(None)
}
Err(e) => Err(format!("open {rel}: {e}")),
}
}
/// Say so when a file changed under an upload that was streaming it.
///
/// Purely diagnostic — the correction is already in place, because the baseline
@@ -2239,6 +2262,27 @@ mod tests {
assert_eq!(remote_sig(&e), None);
}
#[test]
fn a_file_deleted_before_its_upload_is_skipped_not_failed() {
let root = std::env::temp_dir().join(format!("pdfs-b108-{}", std::process::id()));
std::fs::create_dir_all(&root).unwrap();
let gone = root.join("gone.txt");
assert!(open_to_upload(&gone, "gone.txt").unwrap().is_none());
std::fs::write(&gone, b"x").unwrap();
assert!(open_to_upload(&gone, "gone.txt").unwrap().is_some());
// Other open failures still count as errors.
let err = open_to_upload(&gone.join("child"), "gone.txt/child");
assert!(err.unwrap_err().starts_with("open gone.txt/child:"));
let _ = std::fs::remove_dir_all(&root);
let mut o = Outcome::default();
o.record(&Applied::Vanished);
assert!(o.is_empty());
assert_eq!(o.errors, 0);
}
#[test]
fn outcome_summarises_only_what_it_moved() {
let mut o = Outcome::default();
+1
View File
@@ -108,6 +108,7 @@ impl Outcome {
Applied::Uploaded => self.uploaded += 1,
Applied::Downloaded => self.downloaded += 1,
Applied::Conflict => self.conflicts += 1,
Applied::Vanished => {}
}
}