mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(fuse,core): renames across folders, replaced files, and writes after a create
Found by the seeded simulation runs (docs/BUGS.md). - B122: a rename across folders goes to Drive as one move-multiple request, so a sibling holding the new name in the old folder no longer fails it with EIO. The drain uses the same request. - B123: a second write open on a file just written no longer fails with EIO. An empty file counts as complete, our own renames, moves and online creates record their echo, a duplicated event is skipped, a rename keeps the file's mtime while its revision stays, and a landed upload is cached before open handles are rebased onto it. - B124: a node put back after a failed replacing rename leaves `hidden` and its folder is listed again, so it shows in the mount. - B131: a landed create is read back with the same short retry as an online create, and its bytes are cached under the real uid before an open handle is repointed at it. - B135: an online rename settles the tree through the state guard, so the database has both folders unlisted before the reply. A lookup right after it no longer rebuilds the destination without the file. The simulation runs no longer tolerate B122 or B131.
This commit is contained in:
1 parent
1c262eb601
commit
cd90c566cd
10 files changed
+679
-190
No files matched your search
@@ -365,6 +365,14 @@ pub struct StagedWrite {
|
||||
pub based_on: Option<Baseline>,
|
||||
}
|
||||
|
||||
impl StagedWrite {
|
||||
/// Whether `authored` ranges cover a file of `len` bytes, so the staged
|
||||
/// bytes are its complete new content. An empty file has nothing to cover.
|
||||
pub fn covers(authored: &[(u64, u64)], len: u64) -> bool {
|
||||
len == 0 || authored == [(0, len)]
|
||||
}
|
||||
}
|
||||
|
||||
/// The identity of a remote revision, as far as we can observe it.
|
||||
///
|
||||
/// `revision_id` is the authoritative identity when present: the server assigns a
|
||||
@@ -2056,6 +2064,17 @@ mod tests {
|
||||
NodeUid::new(VolumeId::from("vol"), LinkId::from(link))
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_empty_write_covers_the_file_and_a_gap_does_not() {
|
||||
assert!(
|
||||
StagedWrite::covers(&[], 0),
|
||||
"an empty file has nothing to cover"
|
||||
);
|
||||
assert!(StagedWrite::covers(&[(0, 7)], 7));
|
||||
assert!(!StagedWrite::covers(&[(0, 3), (4, 7)], 7));
|
||||
assert!(!StagedWrite::covers(&[], 7));
|
||||
}
|
||||
|
||||
/// A unique temp directory removed on drop; avoids a dev-dependency.
|
||||
struct TempDir(PathBuf);
|
||||
impl TempDir {
|
||||
|
||||
@@ -233,6 +233,48 @@ fn acknowledge_applied_event(
|
||||
Ok(id)
|
||||
}
|
||||
|
||||
/// How many applied node-update ids [`RecentEvents`] remembers. Far more than a
|
||||
/// replay can reach back: one comes from a batch whose last event was not its
|
||||
/// newest, so it repeats at most what that batch held.
|
||||
const RECENT_EVENTS: usize = 4096;
|
||||
|
||||
/// The node updates applied lately, by event id, so one that comes round again
|
||||
/// is skipped rather than applied twice.
|
||||
///
|
||||
/// The feed can deliver the same event twice in a batch, and a batch whose
|
||||
/// events come out of order leaves the cursor short of its newest event, so the
|
||||
/// next poll replays what lay past it. An update applied twice is not harmless:
|
||||
/// the first copy of our own change claims the echo [`Core::note_self_change`]
|
||||
/// recorded, and the second, unclaimed, reads as someone else's edit and evicts
|
||||
/// the bytes a partial write is about to gap-fill from (docs/BUGS.md B123).
|
||||
///
|
||||
/// Only node updates: a delete is idempotent, and a continuity loss names the
|
||||
/// cursor it lost, which is an id already applied.
|
||||
#[derive(Default)]
|
||||
struct RecentEvents {
|
||||
ids: HashSet<DriveEventId>,
|
||||
order: VecDeque<DriveEventId>,
|
||||
}
|
||||
|
||||
impl RecentEvents {
|
||||
/// Record `event` as applied, reporting whether it was already.
|
||||
fn repeat(&mut self, event: &DriveEvent) -> bool {
|
||||
let DriveEvent::NodeUpdated { id, .. } = event else {
|
||||
return false;
|
||||
};
|
||||
if !self.ids.insert(id.clone()) {
|
||||
return true;
|
||||
}
|
||||
self.order.push_back(id.clone());
|
||||
if self.order.len() > RECENT_EVENTS
|
||||
&& let Some(old) = self.order.pop_front()
|
||||
{
|
||||
self.ids.remove(&old);
|
||||
}
|
||||
false
|
||||
}
|
||||
}
|
||||
|
||||
/// The folders one batch of events changed, collected so the listings are
|
||||
/// dropped once at the end of the batch rather than once per event.
|
||||
///
|
||||
@@ -377,6 +419,7 @@ pub(super) async fn run_event_sync(
|
||||
};
|
||||
info!(?cursor, "event sync started");
|
||||
|
||||
let mut recent = RecentEvents::default();
|
||||
loop {
|
||||
core.next_poll(POLL_INTERVAL).await;
|
||||
let events = match link::bounded(client.enumerate_events(&scope, cursor.as_ref())).await {
|
||||
@@ -397,6 +440,11 @@ pub(super) async fn run_event_sync(
|
||||
// did apply.
|
||||
let mut dirty = DirtyParents::default();
|
||||
for event in &events {
|
||||
if recent.repeat(event) {
|
||||
debug!(id = %event.id().as_str(), "skipping an event already applied");
|
||||
cursor = Some(event.id().clone());
|
||||
continue;
|
||||
}
|
||||
// Converge the SDK's own caches (folder keys, entity cache) on the
|
||||
// server before applying the event to our tree. Without this, a node
|
||||
// re-keyed/moved by another client keeps a stale key in the SDK for
|
||||
@@ -717,6 +765,43 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_node_update_seen_again_is_skipped_but_other_events_are_not() {
|
||||
let update = |id: &str| DriveEvent::NodeUpdated {
|
||||
id: DriveEventId::from(id),
|
||||
node_uid: uid("file"),
|
||||
parent_node_uid: None,
|
||||
is_trashed: false,
|
||||
is_shared: false,
|
||||
};
|
||||
let mut recent = RecentEvents::default();
|
||||
assert!(!recent.repeat(&update("e1")));
|
||||
assert!(!recent.repeat(&update("e2")));
|
||||
assert!(recent.repeat(&update("e1")), "a duplicate or replay");
|
||||
let lost = DriveEvent::ContinuityLost {
|
||||
id: DriveEventId::from("e2"),
|
||||
};
|
||||
assert!(!recent.repeat(&lost), "names the cursor, which was applied");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recent_events_forget_the_oldest_past_their_bound() {
|
||||
let update = |n: usize| DriveEvent::NodeUpdated {
|
||||
id: DriveEventId::from(format!("e{n}")),
|
||||
node_uid: uid("file"),
|
||||
parent_node_uid: None,
|
||||
is_trashed: false,
|
||||
is_shared: false,
|
||||
};
|
||||
let mut recent = RecentEvents::default();
|
||||
for n in 0..=RECENT_EVENTS {
|
||||
assert!(!recent.repeat(&update(n)));
|
||||
}
|
||||
assert_eq!(recent.ids.len(), RECENT_EVENTS);
|
||||
assert!(!recent.repeat(&update(0)), "the oldest was forgotten");
|
||||
assert!(recent.repeat(&update(RECENT_EVENTS)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn foreign_delete_invalidates_an_earlier_refresh_generation() {
|
||||
let own_volume = VolumeId::from("own");
|
||||
|
||||
@@ -29,16 +29,14 @@ use std::time::{Duration, Instant};
|
||||
|
||||
use parking_lot::Mutex;
|
||||
|
||||
use futures::StreamExt;
|
||||
use pdfs_core::batch;
|
||||
use pdfs_core::cache::{Baseline, StagedWrite};
|
||||
use pdfs_core::control::{ActivityKind, TransferDirection};
|
||||
use pdfs_core::db::{
|
||||
OP_CREATE, OP_MKDIR, OP_RENAME, OP_REVISION, OP_TRASH, PARK_EXPIRY_MS, PendingOp, RenameMeta,
|
||||
};
|
||||
use proton_drive_rs::proton_sdk::error::ProtonError;
|
||||
use proton_drive_rs::proton_sdk::ids::NodeUid;
|
||||
use proton_drive_rs::{Node, NodeKind, NodeMoveItem};
|
||||
use proton_drive_rs::{Node, NodeKind};
|
||||
use tracing::{debug, error, info, warn};
|
||||
|
||||
use super::state::{Intervals, PendingRevision};
|
||||
@@ -693,7 +691,7 @@ impl Core {
|
||||
// no window for the requirements to go stale between two calls —
|
||||
// the race B46 queued this op to get away from.
|
||||
let target = (node.name != name).then_some(name.as_str());
|
||||
match self.move_renaming(&uid, &parent, target) {
|
||||
match self.move_rename_remote(&uid, &parent, target) {
|
||||
Ok(()) => name.clone(),
|
||||
// The destination holds that name already. Landing under a
|
||||
// *different* name is the non-destructive resolution: it neither
|
||||
@@ -704,7 +702,7 @@ impl Core {
|
||||
}
|
||||
let alt = conflict_name(&name, now_secs());
|
||||
warn!(%uid, name, alt, "destination already holds that name; using a conflict name");
|
||||
self.move_renaming(&uid, &parent, Some(&alt))?;
|
||||
self.move_rename_remote(&uid, &parent, Some(&alt))?;
|
||||
self.adopt_drained_name(&uid, &alt);
|
||||
self.log_activity(
|
||||
ActivityKind::Rename,
|
||||
@@ -741,7 +739,6 @@ impl Core {
|
||||
}
|
||||
};
|
||||
self.db.delete_op(op.id)?;
|
||||
self.note_self_change(&uid);
|
||||
info!(%uid, name = %landed, "pending rename landed");
|
||||
Ok(())
|
||||
}
|
||||
@@ -780,33 +777,6 @@ impl Core {
|
||||
}
|
||||
}
|
||||
|
||||
/// Move `uid` under `parent` in one request, renaming it to `target_name` on
|
||||
/// the way when one is given.
|
||||
fn move_renaming(
|
||||
&self,
|
||||
uid: &NodeUid,
|
||||
parent: &NodeUid,
|
||||
target_name: Option<&str>,
|
||||
) -> Result<(), ProtonError> {
|
||||
let item = NodeMoveItem {
|
||||
uid: uid.clone(),
|
||||
target_name: target_name.map(str::to_string),
|
||||
};
|
||||
self.rt
|
||||
.block_on(async {
|
||||
let mut outcomes =
|
||||
std::pin::pin!(self.drive.move_nodes_streaming(vec![item], parent.clone()));
|
||||
match outcomes.next().await {
|
||||
Some(Ok((_, outcome))) => outcome,
|
||||
Some(Err(e)) => Err(e),
|
||||
None => Err(ProtonError::invalid_operation(format!(
|
||||
"move of {uid} reported no outcome"
|
||||
))),
|
||||
}
|
||||
})
|
||||
.inspect(|()| self.forget_sdk_node(uid))
|
||||
}
|
||||
|
||||
/// Apply a queued trash to the remote.
|
||||
///
|
||||
/// A node that is already gone is a success, not a failure: the outcome the
|
||||
@@ -941,7 +911,23 @@ impl Core {
|
||||
// uploaded: it goes on as a revision of the node instead.
|
||||
let uploaded = (!adopted).then_some(op.blob_path.as_deref()).flatten();
|
||||
let newer = self.retire_create(op, uploaded, &local, &real)?;
|
||||
self.adopt_real_uid(&local, &real)?;
|
||||
// The uploaded blob is the new file's content, so it becomes the cached
|
||||
// content, as for a revision. A partial write fills its gaps from it;
|
||||
// without it the next write open was refused (`docs/BUGS.md` B123).
|
||||
let landed: Option<StagedWrite> = uploaded
|
||||
.filter(|_| !newer)
|
||||
.and(op.meta_json.as_deref())
|
||||
.and_then(|json| serde_json::from_str(json).ok());
|
||||
self.adopt_real_uid(&local, &real, |node| {
|
||||
if let (Some(blob), Some(meta)) = (uploaded, &landed)
|
||||
&& node_size(node) == meta.len
|
||||
&& keeps_landed_upload(self.cache.is_pinned(&real), meta.len, self.cache.budget())
|
||||
{
|
||||
let _ =
|
||||
self.cache
|
||||
.store_file(&real, node.modification_time, meta.len, Path::new(blob));
|
||||
}
|
||||
})?;
|
||||
// The feed will report this create back to us; the tree already has it
|
||||
// under its real uid, so that event is ours to ignore (`Core::self_changes`).
|
||||
self.note_self_change(&real);
|
||||
@@ -1096,6 +1082,9 @@ impl Core {
|
||||
}
|
||||
newer.is_some()
|
||||
};
|
||||
if !newer && let Some(blob) = uploaded {
|
||||
self.cache_created(op, local, real, blob);
|
||||
}
|
||||
self.for_each_state(|st| {
|
||||
if let Some(ino) = st.by_uid.remove(local) {
|
||||
st.by_uid.insert(real.clone(), ino);
|
||||
@@ -1112,6 +1101,38 @@ impl Core {
|
||||
Ok(newer)
|
||||
}
|
||||
|
||||
/// Cache a landed create's uploaded `blob` under `real`, keyed as the tree
|
||||
/// has the file.
|
||||
///
|
||||
/// A write handle open on the file is repointed at `real` right after this,
|
||||
/// and a release fills its gaps from the cache by the size and mtime it
|
||||
/// opened over. Drive lists the new node a moment after the create, so
|
||||
/// `adopt_real_uid` caches the bytes only once it can read the node back.
|
||||
/// A release in between found nothing to fill from, and the next write open
|
||||
/// was refused (docs/BUGS.md B131).
|
||||
fn cache_created(&self, op: &PendingOp, local: &NodeUid, real: &NodeUid, blob: &str) {
|
||||
let Some(meta) = op
|
||||
.meta_json
|
||||
.as_deref()
|
||||
.and_then(|json| serde_json::from_str::<StagedWrite>(json).ok())
|
||||
else {
|
||||
return;
|
||||
};
|
||||
let mut mtime = None;
|
||||
self.for_each_state(|st| {
|
||||
if let Some(entry) = st.by_uid.get(local).and_then(|ino| st.entries.get(ino)) {
|
||||
mtime = Some(entry.node.modification_time);
|
||||
}
|
||||
});
|
||||
if let Some(mtime) = mtime
|
||||
&& keeps_landed_upload(self.cache.is_pinned(real), meta.len, self.cache.budget())
|
||||
{
|
||||
let _ = self
|
||||
.cache
|
||||
.store_file(real, mtime, meta.len, Path::new(blob));
|
||||
}
|
||||
}
|
||||
|
||||
/// Swap a placeholder uid for the real one across everything that keyed off
|
||||
/// it: queued children, the DB, the in-memory tree, and the caches.
|
||||
///
|
||||
@@ -1125,14 +1146,22 @@ impl Core {
|
||||
/// its `local~` uid, which reads as an empty file for as long as the daemon
|
||||
/// runs (`docs/BUGS.md` B74). A uid is unique across mounts, so at most one
|
||||
/// of them matches and the others are no-ops.
|
||||
///
|
||||
/// `adopt` sees the node as Drive made it, before anything is rebased onto
|
||||
/// it.
|
||||
pub(crate) fn adopt_real_uid(
|
||||
&self,
|
||||
local: &NodeUid,
|
||||
real: &NodeUid,
|
||||
adopt: impl FnOnce(&Node),
|
||||
) -> Result<(), Box<dyn std::error::Error>> {
|
||||
// Drive answers a create before it lists the new node.
|
||||
let node = self
|
||||
.fetch_node(real)
|
||||
.read_back(real)
|
||||
.map_err(|e| self.errno_error(e, "fetch node"))?;
|
||||
// Before open handles are rebased onto the node, so one released in
|
||||
// between finds what `adopt` keeps (`docs/BUGS.md` B123).
|
||||
adopt(&node);
|
||||
// Repoints queued children and node rows, and readdresses the placeholder row.
|
||||
// A no-op after `retire_create`, which has already done both.
|
||||
self.db
|
||||
@@ -1667,10 +1696,7 @@ impl Core {
|
||||
// an orphaned file (harmless), whereas the reverse would leave a queued
|
||||
// op pointing at nothing.
|
||||
self.db.delete_op(op.id)?;
|
||||
self.release_pending(&uid, &blob);
|
||||
self.evict_reader(&uid);
|
||||
let landed = self.refresh_after_upload(&uid);
|
||||
|
||||
// The staged blob now matches the sealed revision, so it becomes the
|
||||
// file's cached content, stamped like the revision just read back.
|
||||
// Reading the file again then needs no network, which an editor or a
|
||||
@@ -1678,14 +1704,15 @@ impl Core {
|
||||
// B119). Adopted by link, not by value: the blob is the whole file, and
|
||||
// a video read into a `Vec` here is an OOM. `discard_staged` below drops
|
||||
// the staging name only — the cache keeps the inode.
|
||||
if let Some(node) = landed
|
||||
&& node_size(&node) == meta.len
|
||||
&& keeps_landed_upload(self.cache.is_pinned(&uid), meta.len, self.cache.budget())
|
||||
{
|
||||
let _ = self
|
||||
.cache
|
||||
.store_file(&uid, node.modification_time, meta.len, &blob);
|
||||
}
|
||||
self.refresh_after_upload(&uid, &blob, |node| {
|
||||
if node_size(node) == meta.len
|
||||
&& keeps_landed_upload(self.cache.is_pinned(&uid), meta.len, self.cache.budget())
|
||||
{
|
||||
let _ = self
|
||||
.cache
|
||||
.store_file(&uid, node.modification_time, meta.len, &blob);
|
||||
}
|
||||
});
|
||||
self.cache.discard_staged(&blob);
|
||||
self.log_activity(ActivityKind::Upload, &name, "uploaded", true);
|
||||
// Both times, so a slow drain shows whether the upload itself or the
|
||||
@@ -1720,11 +1747,27 @@ impl Core {
|
||||
/// we just sealed. Skipping both, as this used to, is what produced a file
|
||||
/// that conflicted with itself and left a full-size duplicate behind.
|
||||
///
|
||||
/// `blob` is the revision that landed, which stays the pending one until
|
||||
/// open write handles are rebased off it, and `adopt` gets the node read
|
||||
/// back before that, so the uploaded bytes can be cached under its
|
||||
/// revision first. A handle released in between fills its gaps from one
|
||||
/// or the other; with the blob let go first and the cache filled last, it
|
||||
/// found neither, and the next write open was refused (docs/BUGS.md B123).
|
||||
///
|
||||
/// Best effort: a failure here costs a spurious conflict copy on the next
|
||||
/// write, not this upload, which has already landed. Returns the node as
|
||||
/// the server now has it, or `None` when it could not be read back.
|
||||
pub(crate) fn refresh_after_upload(&self, uid: &NodeUid) -> Option<Node> {
|
||||
let node = match self.fetch_node_remote(uid) {
|
||||
pub(crate) fn refresh_after_upload(
|
||||
&self,
|
||||
uid: &NodeUid,
|
||||
blob: &Path,
|
||||
adopt: impl FnOnce(&Node),
|
||||
) -> Option<Node> {
|
||||
let fetched = self.fetch_node_remote(uid);
|
||||
if !matches!(fetched, Ok(Some(_))) {
|
||||
self.release_pending(uid, blob);
|
||||
}
|
||||
let node = match fetched {
|
||||
Ok(Some(node)) => node,
|
||||
// Trashed or deleted under us: the tree will hear it from the event
|
||||
// sync, which is better placed to unhook the inode than we are.
|
||||
@@ -1736,6 +1779,7 @@ impl Core {
|
||||
return None;
|
||||
}
|
||||
};
|
||||
adopt(&node);
|
||||
// This function's whole job is to bring the tree level with the revision
|
||||
// we just sealed, so the feed's report of that revision has nothing left
|
||||
// to tell us. Claimed here rather than at the upload call so it is only
|
||||
@@ -1768,6 +1812,7 @@ impl Core {
|
||||
}
|
||||
self.rebase_open_writes(uid, &node);
|
||||
}
|
||||
self.release_pending(uid, blob);
|
||||
// Ordered so that a write queued *during* the fetch above is still
|
||||
// caught: it took its baseline from the node's optimistic stamp, and
|
||||
// this overwrites it with the revision the server actually holds.
|
||||
|
||||
@@ -1435,7 +1435,7 @@ impl ProtonFs {
|
||||
len,
|
||||
base_size,
|
||||
base_mtime,
|
||||
complete: authored == [(0, len)],
|
||||
complete: StagedWrite::covers(&authored, len),
|
||||
authored,
|
||||
based_on: self
|
||||
.core
|
||||
@@ -1944,12 +1944,14 @@ impl ProtonFs {
|
||||
// now and this handle's is stale. A retry resolves it.
|
||||
Ok(false) => {
|
||||
warn!(%uid, name, newname, "queued create vanished under a rename");
|
||||
self.core.restore_replaced(victim.as_ref(), newname);
|
||||
self.core
|
||||
.restore_replaced(victim.as_ref(), &new_parent_uid, newname);
|
||||
reply.error(Errno::EBUSY);
|
||||
}
|
||||
Err(e) => {
|
||||
error!(%uid, error = %e, "rewriting a queued create's target failed");
|
||||
self.core.restore_replaced(victim.as_ref(), newname);
|
||||
self.core
|
||||
.restore_replaced(victim.as_ref(), &new_parent_uid, newname);
|
||||
reply.error(Errno::EIO);
|
||||
}
|
||||
}
|
||||
@@ -2001,7 +2003,8 @@ impl ProtonFs {
|
||||
}
|
||||
}
|
||||
Err(e) => {
|
||||
self.core.restore_replaced(victim.as_ref(), newname);
|
||||
self.core
|
||||
.restore_replaced(victim.as_ref(), &new_parent_uid, newname);
|
||||
reply.error(e);
|
||||
}
|
||||
}
|
||||
@@ -2015,8 +2018,6 @@ impl ProtonFs {
|
||||
queue_it(reply);
|
||||
return;
|
||||
}
|
||||
// Rename first if both halves change; a move that then fails leaves the
|
||||
// node renamed in its source folder, which the tree is told below.
|
||||
// A failure past this point has already trashed any node the rename was
|
||||
// replacing, so put it back rather than leaving the caller with neither
|
||||
// the source moved nor the destination intact.
|
||||
@@ -2026,48 +2027,37 @@ impl ProtonFs {
|
||||
&old_parent_uid,
|
||||
&new_parent_uid,
|
||||
) {
|
||||
self.core.restore_replaced(victim.as_ref(), newname);
|
||||
self.core
|
||||
.restore_replaced(victim.as_ref(), &new_parent_uid, newname);
|
||||
reply.error(error);
|
||||
return;
|
||||
}
|
||||
if newname != name
|
||||
&& let Err(e) = self.core.rename_remote(&uid, newname)
|
||||
{
|
||||
// A move that also renames is one request, so the name only has to be
|
||||
// free in the destination, and the reply never stands for half of it.
|
||||
let renamed = if newparent != parent {
|
||||
let target = (newname != name).then_some(newname);
|
||||
self.core.move_rename_remote(&uid, &new_parent_uid, target)
|
||||
} else if newname != name {
|
||||
self.core.rename_remote(&uid, newname)
|
||||
} else {
|
||||
Ok(())
|
||||
};
|
||||
if let Err(e) = renamed {
|
||||
if self.core.lost_link(&e, "rename") {
|
||||
queue_it(reply);
|
||||
return;
|
||||
}
|
||||
error!(%uid, error = %e, "rename failed");
|
||||
self.core.restore_replaced(victim.as_ref(), newname);
|
||||
reply.error(Errno::EIO);
|
||||
return;
|
||||
}
|
||||
// A successful reply must mean both remote halves landed, not merely
|
||||
// that the second half was queued.
|
||||
if newparent != parent
|
||||
&& let Err(e) = self.core.move_remote(&uid, &new_parent_uid)
|
||||
{
|
||||
if self.core.lost_link(&e, "move") {
|
||||
queue_it(reply);
|
||||
return;
|
||||
}
|
||||
error!(%uid, error = %e, "move after rename failed");
|
||||
// The rename half landed in the source directory.
|
||||
let mut state = self.core.state();
|
||||
let old_parent_uid = state
|
||||
.entries
|
||||
.get(&parent)
|
||||
.map(|entry| entry.uid.clone())
|
||||
.unwrap_or_else(|| uid.clone());
|
||||
state.rename_in_place(ino, parent, &old_parent_uid, newname);
|
||||
drop(state);
|
||||
self.core.restore_replaced(victim.as_ref(), newname);
|
||||
self.core
|
||||
.restore_replaced(victim.as_ref(), &new_parent_uid, newname);
|
||||
reply.error(Errno::EIO);
|
||||
return;
|
||||
}
|
||||
// Through the guard, so the database has both folders unlisted before
|
||||
// the reply. The next lookup rebuilds a folder still marked listed from
|
||||
// the database, which did not have the move yet (docs/BUGS.md B135).
|
||||
self.core
|
||||
.state
|
||||
.lock()
|
||||
.state()
|
||||
.relocate(ino, parent, newparent, &new_parent_uid, newname);
|
||||
reply.ok();
|
||||
}
|
||||
|
||||
+125
-23
@@ -74,7 +74,7 @@ use proton_drive_rs::proton_sdk::api::ResponseCode;
|
||||
use proton_drive_rs::proton_sdk::error::ProtonError;
|
||||
use proton_drive_rs::proton_sdk::ids::{DriveEventId, LinkId, NodeUid, ShareId, VolumeId};
|
||||
use proton_drive_rs::{
|
||||
DriveEvent, DriveEventScopeId, MemberRole, Node, NodeKind, ProtonDriveClient,
|
||||
DriveEvent, DriveEventScopeId, MemberRole, Node, NodeKind, NodeMoveItem, ProtonDriveClient,
|
||||
ProtonPhotosClient, SharedWithMeItem, ThumbnailType,
|
||||
};
|
||||
|
||||
@@ -1477,7 +1477,7 @@ fn merge_over_pending(
|
||||
.filter(|&(_, _, authored)| authored)
|
||||
.map(|(s, e, _)| (s, e))
|
||||
.collect();
|
||||
meta.complete = meta.authored == [(0, meta.len)];
|
||||
meta.complete = StagedWrite::covers(&meta.authored, meta.len);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1627,6 +1627,38 @@ impl Core {
|
||||
})
|
||||
}
|
||||
|
||||
/// Move `uid` into the folder `parent` on Drive, renaming it to
|
||||
/// `target_name` on the way when one is given, in one request.
|
||||
///
|
||||
/// Done as a rename and then a move, a sibling in the source folder that
|
||||
/// held the new name made Drive refuse the first half, although the
|
||||
/// destination was free (docs/BUGS.md B122). One request also leaves no
|
||||
/// half-applied state between the two.
|
||||
pub(crate) fn move_rename_remote(
|
||||
&self,
|
||||
uid: &NodeUid,
|
||||
parent: &NodeUid,
|
||||
target_name: Option<&str>,
|
||||
) -> Result<(), ProtonError> {
|
||||
self.change_remote(uid, || {
|
||||
let item = NodeMoveItem {
|
||||
uid: uid.clone(),
|
||||
target_name: target_name.map(str::to_string),
|
||||
};
|
||||
self.block_on_bounded(async {
|
||||
let mut outcomes =
|
||||
std::pin::pin!(self.drive.move_nodes_streaming(vec![item], parent.clone()));
|
||||
match outcomes.next().await {
|
||||
Some(Ok((_, outcome))) => outcome,
|
||||
Some(Err(e)) => Err(e),
|
||||
None => Err(ProtonError::invalid_operation(format!(
|
||||
"move of {uid} reported no outcome"
|
||||
))),
|
||||
}
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
/// Run a rename or move of `uid`, then drop the SDK's now stale copy of it.
|
||||
fn change_remote(
|
||||
&self,
|
||||
@@ -1637,7 +1669,16 @@ impl Core {
|
||||
self.forget_sdk_node(uid);
|
||||
std::thread::sleep(OUT_OF_DATE_RETRY_DELAY);
|
||||
})
|
||||
.inspect(|()| self.forget_sdk_node(uid))
|
||||
.inspect(|()| self.changed_remote(uid))
|
||||
}
|
||||
|
||||
/// Bookkeeping after a rename or move of `uid` landed: the SDK's copy of it
|
||||
/// is stale, and the feed will echo the change back. Without the second,
|
||||
/// that echo reads as someone else's edit and evicts the cached bytes a
|
||||
/// partial write is about to gap-fill from (docs/BUGS.md B123).
|
||||
pub(crate) fn changed_remote(&self, uid: &NodeUid) {
|
||||
self.forget_sdk_node(uid);
|
||||
self.note_self_change(uid);
|
||||
}
|
||||
|
||||
/// Drop the SDK's cached copy of `uid` after this daemon renamed or moved it.
|
||||
@@ -3047,19 +3088,32 @@ impl Core {
|
||||
/// drain finds the node by name and adopts it: the application gets its
|
||||
/// file or folder either way, never an `ENOENT` for something that exists.
|
||||
fn fetch_minted_node(&self, uid: &NodeUid, name: &str) -> Option<Node> {
|
||||
// The feed echoes the create. Unclaimed, it would take the echo the
|
||||
// upload of the file's bytes records, and that one would then evict
|
||||
// the bytes just uploaded (docs/BUGS.md B123).
|
||||
self.note_self_change(uid);
|
||||
let node = self.read_back(uid).ok();
|
||||
if node.is_none() {
|
||||
warn!(%uid, name, "Drive made the node but did not hand it back");
|
||||
}
|
||||
node
|
||||
}
|
||||
|
||||
/// [`Core::fetch_node`] for a node Drive has just made, asked again for a
|
||||
/// moment while Drive does not list it yet.
|
||||
pub(crate) fn read_back(&self, uid: &NodeUid) -> Result<Node, Errno> {
|
||||
let mut delays = MINTED_READ_BACK_DELAYS.iter();
|
||||
loop {
|
||||
match self.fetch_node(uid) {
|
||||
Ok(node) => return Some(node),
|
||||
Err(e) if e == Errno::ENOENT => match delays.next() {
|
||||
Some(delay) => std::thread::sleep(*delay),
|
||||
None => break,
|
||||
},
|
||||
Err(_) => break,
|
||||
Err(e)
|
||||
if e == Errno::ENOENT
|
||||
&& let Some(delay) = delays.next() =>
|
||||
{
|
||||
std::thread::sleep(*delay);
|
||||
}
|
||||
fetched => return fetched,
|
||||
}
|
||||
}
|
||||
warn!(%uid, name, "Drive made the node but did not hand it back");
|
||||
None
|
||||
}
|
||||
|
||||
/// [`Core::fetch_node`] without the collapse to an `Errno`, for the drain:
|
||||
@@ -3160,7 +3214,7 @@ impl Core {
|
||||
len: h.len,
|
||||
base_size: h.base_size,
|
||||
base_mtime: h.base_mtime,
|
||||
complete: authored == [(0, h.len)],
|
||||
complete: StagedWrite::covers(&authored, h.len),
|
||||
authored,
|
||||
based_on: self.remote_baseline(
|
||||
&h.uid,
|
||||
@@ -3193,7 +3247,7 @@ impl Core {
|
||||
len: h.len,
|
||||
base_size: h.base_size,
|
||||
base_mtime: h.base_mtime,
|
||||
complete: authored == [(0, h.len)],
|
||||
complete: StagedWrite::covers(&authored, h.len),
|
||||
authored,
|
||||
based_on: self.remote_baseline(
|
||||
&h.uid,
|
||||
@@ -4803,7 +4857,11 @@ impl Core {
|
||||
/// further to try, and the node is still in the trash where `pdfs restore`
|
||||
/// can reach it. Says so loudly in that case, because the alternative is a
|
||||
/// file the user believes was only renamed quietly sitting in the trash.
|
||||
fn restore_replaced(&self, victim: Option<&(u64, NodeUid)>, name: &str) {
|
||||
///
|
||||
/// A restored node comes back to the mount as the ordinary restore brings
|
||||
/// it: out of `hidden`, and through a fresh listing of `parent` in every
|
||||
/// mount that shows it (`docs/BUGS.md` B124).
|
||||
fn restore_replaced(&self, victim: Option<&(u64, NodeUid)>, parent: &NodeUid, name: &str) {
|
||||
let Some((_, uid)) = victim else { return };
|
||||
if is_local_uid(uid) {
|
||||
// Its queued create was discarded and cannot be reconstructed from
|
||||
@@ -4817,6 +4875,13 @@ impl Core {
|
||||
.and_then(batch::into_unit)
|
||||
{
|
||||
Ok(()) => {
|
||||
self.hidden.lock().remove(uid);
|
||||
self.for_each_mount(|st, notify| {
|
||||
if let Some(&ino) = st.by_uid.get(parent) {
|
||||
st.invalidate_listing(ino);
|
||||
notify.inval_entry(ino, name.to_string());
|
||||
}
|
||||
});
|
||||
self.invalidate_trash();
|
||||
debug!(%uid, name, "restored the node a failed rename had replaced");
|
||||
}
|
||||
@@ -6301,7 +6366,7 @@ mod merge_over_pending_tests {
|
||||
len,
|
||||
base_size,
|
||||
base_mtime: 100,
|
||||
complete: authored == [(0, len)],
|
||||
complete: StagedWrite::covers(authored, len),
|
||||
authored: authored.to_vec(),
|
||||
based_on: Some(Baseline {
|
||||
mtime: 100,
|
||||
@@ -8458,6 +8523,20 @@ mod tests {
|
||||
assert_eq!(replaced.matches("self.withdraw_queued_ops(uid)").count(), 2);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_replaced_node_put_back_shows_in_the_mount_again() {
|
||||
// The restore after a failed rename left the node hidden and out of
|
||||
// the listing, so a create by its name failed with ENOENT (B124).
|
||||
let restore = function_source(include_str!("lib.rs"), "fn restore_replaced(");
|
||||
assert_before(
|
||||
restore,
|
||||
"self.drive.restore_nodes(",
|
||||
"self.hidden.lock().remove(uid)",
|
||||
);
|
||||
assert!(restore.contains("st.invalidate_listing(ino)"));
|
||||
assert!(restore.contains("notify.inval_entry(ino, name.to_string())"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_size_upgrade_leaves_the_page_cache_alone() {
|
||||
// Reads resolve provisional sizes inline, while the kernel holds the
|
||||
@@ -8540,21 +8619,32 @@ mod tests {
|
||||
usize::from(source.contains("fn move_remote(")),
|
||||
"call Core::move_remote instead of the SDK move directly"
|
||||
);
|
||||
assert_eq!(
|
||||
source
|
||||
.matches(concat!("drive.move_nodes_streaming", "("))
|
||||
.count(),
|
||||
usize::from(source.contains("fn move_rename_remote(")),
|
||||
"call Core::move_rename_remote instead of the SDK move directly"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_rename_or_move_drops_the_sdk_copy_of_the_node() {
|
||||
fn a_rename_or_move_drops_the_sdk_copy_and_expects_its_echo() {
|
||||
// The SDK would send the old name hash with the node's next rename or
|
||||
// move until the event poll caught up (B121).
|
||||
// move until the event poll caught up (B121), and the echo of the
|
||||
// change, taken for someone else's, evicted the cached bytes (B123).
|
||||
let change = function_source(include_str!("lib.rs"), "fn change_remote(");
|
||||
assert_eq!(
|
||||
change.matches("self.forget_sdk_node(uid)").count(),
|
||||
2,
|
||||
"forget the node before each retry and once the change landed"
|
||||
assert!(
|
||||
change.contains("self.forget_sdk_node(uid)"),
|
||||
"forget the node before each retry"
|
||||
);
|
||||
let drained = function_source(include_str!("drain.rs"), "fn move_renaming(");
|
||||
assert!(drained.contains("self.forget_sdk_node(uid)"));
|
||||
assert!(change.contains("self.changed_remote(uid)"));
|
||||
let moved = function_source(include_str!("lib.rs"), "fn move_rename_remote(");
|
||||
assert!(moved.contains("self.change_remote(uid"));
|
||||
let changed = function_source(include_str!("lib.rs"), "fn changed_remote(");
|
||||
assert!(changed.contains("self.forget_sdk_node(uid)"));
|
||||
assert!(changed.contains("self.note_self_change(uid)"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -8632,6 +8722,18 @@ mod tests {
|
||||
assert_before(upload, "self.db.op_exists(op.id)", "File::open(&blob)");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_online_rename_writes_the_move_through_before_it_answers() {
|
||||
// Through the raw lock, the move stayed in the state's outbox, and the
|
||||
// next lookup rebuilt the destination from a database without it
|
||||
// (B135).
|
||||
let rename: String = function_source(include_str!("filesystem.rs"), "fn serve_rename(")
|
||||
.split_whitespace()
|
||||
.collect();
|
||||
assert!(rename.contains("self.core.state().relocate("));
|
||||
assert!(!rename.contains(".lock().relocate("));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_landed_upload_stays_pending_until_its_bytes_are_cached() {
|
||||
let drain = function_source(include_str!("drain.rs"), "pub(crate) fn drain_revision(");
|
||||
|
||||
@@ -309,4 +309,115 @@ mod tests {
|
||||
assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown)));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
/// Write `bytes` at `offset` into an existing file, without truncating it.
|
||||
fn write_at(path: &Path, offset: u64, bytes: &[u8]) -> io::Result<()> {
|
||||
use std::os::unix::fs::FileExt;
|
||||
std::fs::OpenOptions::new()
|
||||
.write(true)
|
||||
.open(path)?
|
||||
.write_all_at(bytes, offset)
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
||||
fn a_file_closed_empty_takes_a_second_write_at_once() {
|
||||
// The empty revision never counted as complete, so the next write
|
||||
// open before it drained was refused with EIO (B123).
|
||||
let drive = FakeDrive::new();
|
||||
let dir = scratch("empty-rewrite");
|
||||
let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap();
|
||||
|
||||
let path = daemon.mountpoint.join("empty.txt");
|
||||
std::fs::write(&path, b"old").unwrap();
|
||||
assert!(wait_until(Duration::from_secs(30), || {
|
||||
drive.tree().contains_key("empty.txt")
|
||||
}));
|
||||
std::fs::write(&path, b"").unwrap();
|
||||
write_at(&path, 0, b"filled").unwrap();
|
||||
assert!(
|
||||
wait_until(Duration::from_secs(30), || {
|
||||
drive.tree().get("empty.txt") == Some(&Entry::File(Arc::new(b"filled".to_vec())))
|
||||
}),
|
||||
"the second write landed: {:?}",
|
||||
drive.tree()
|
||||
);
|
||||
|
||||
assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown)));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
||||
fn the_echo_of_our_own_rename_keeps_the_cached_bytes() {
|
||||
// The echo read as someone else's change and evicted the bytes, so a
|
||||
// partial write could not gap-fill, and the next write open on the
|
||||
// file was refused with EIO (B123).
|
||||
let drive = FakeDrive::new();
|
||||
let dir = scratch("rename-echo");
|
||||
let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap();
|
||||
|
||||
let before = daemon.mountpoint.join("a.txt");
|
||||
std::fs::write(&before, b"0123456789").unwrap();
|
||||
assert!(wait_until(Duration::from_secs(30), || {
|
||||
drive.tree().contains_key("a.txt")
|
||||
}));
|
||||
assert_eq!(std::fs::read(&before).unwrap(), b"0123456789");
|
||||
let after = daemon.mountpoint.join("b.txt");
|
||||
std::fs::rename(&before, &after).unwrap();
|
||||
// Long enough for the next event poll to bring the echo.
|
||||
std::thread::sleep(crate::POLL_INTERVAL + Duration::from_secs(3));
|
||||
|
||||
write_at(&after, 3, b"x").unwrap();
|
||||
write_at(&after, 5, b"y").unwrap();
|
||||
assert!(
|
||||
wait_until(Duration::from_secs(30), || {
|
||||
drive.tree().get("b.txt") == Some(&Entry::File(Arc::new(b"012x4y6789".to_vec())))
|
||||
}),
|
||||
"both writes landed: {:?}",
|
||||
drive.tree()
|
||||
);
|
||||
|
||||
assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown)));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
||||
fn a_rename_across_folders_ignores_the_new_name_in_the_old_folder() {
|
||||
// The rename went to Drive as a rename in the old folder and then a
|
||||
// move, so a sibling there with the new name failed it with EIO
|
||||
// (B122).
|
||||
let drive = FakeDrive::new();
|
||||
let dir = scratch("rename-across");
|
||||
let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap();
|
||||
|
||||
let mount = &daemon.mountpoint;
|
||||
std::fs::create_dir(mount.join("x")).unwrap();
|
||||
std::fs::create_dir(mount.join("z")).unwrap();
|
||||
std::fs::write(mount.join("x/f.txt"), b"moved").unwrap();
|
||||
std::fs::write(mount.join("x/d"), b"sibling").unwrap();
|
||||
std::fs::write(mount.join("z/d"), b"replaced").unwrap();
|
||||
let file = |bytes: &[u8]| Some(Entry::File(Arc::new(bytes.to_vec())));
|
||||
assert!(wait_until(Duration::from_secs(30), || {
|
||||
let tree = drive.tree();
|
||||
[
|
||||
("x/f.txt", b"moved".as_slice()),
|
||||
("x/d", b"sibling"),
|
||||
("z/d", b"replaced"),
|
||||
]
|
||||
.iter()
|
||||
.all(|(path, bytes)| tree.get(*path).cloned() == file(bytes))
|
||||
}));
|
||||
|
||||
std::fs::rename(mount.join("x/f.txt"), mount.join("z/d")).unwrap();
|
||||
assert_eq!(std::fs::read(mount.join("z/d")).unwrap(), b"moved");
|
||||
let tree = drive.tree();
|
||||
assert_eq!(tree.get("z/d").cloned(), file(b"moved"));
|
||||
assert_eq!(tree.get("x/d").cloned(), file(b"sibling"));
|
||||
assert!(!tree.contains_key("x/f.txt"));
|
||||
|
||||
assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown)));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
}
|
||||
@@ -537,6 +537,17 @@ impl Faults {
|
||||
}
|
||||
}
|
||||
|
||||
/// A good link whose event feed is slow and untidy: every echo of our own
|
||||
/// changes comes late, often twice, often out of order.
|
||||
pub(crate) fn echoes() -> Self {
|
||||
Self {
|
||||
event_delay: Duration::from_secs(1),
|
||||
duplicate_events: 0.3,
|
||||
reorder_events: 0.3,
|
||||
..Self::lan()
|
||||
}
|
||||
}
|
||||
|
||||
/// The Wi-Fi of the 2026-10 account runs: slow and lossy, with listings
|
||||
/// and events well behind.
|
||||
pub(crate) fn wifi() -> Self {
|
||||
|
||||
@@ -65,25 +65,6 @@ struct Known {
|
||||
}
|
||||
|
||||
const KNOWN: &[Known] = &[
|
||||
// A rename across folders renames in the source folder first, so a
|
||||
// sibling there with the new name refuses it.
|
||||
Known {
|
||||
bug: "B122",
|
||||
is: |model, op, errno, _, _| {
|
||||
let FsOp::Rename { from, to } = op else {
|
||||
return false;
|
||||
};
|
||||
errno == libc::EIO
|
||||
&& parent_of(from) != parent_of(to)
|
||||
&& name_of(from) != name_of(to)
|
||||
&& model
|
||||
.tree()
|
||||
.contains_key(&join(parent_of(from), name_of(to)))
|
||||
},
|
||||
// The destination it was replacing is restored on Drive but stays
|
||||
// gone from the mount (B124).
|
||||
ends_seed: |model, op| matches!(op, FsOp::Rename { to, .. } if model.tree().contains_key(to)),
|
||||
},
|
||||
// A replacing rename trashes what it replaces first, so Drive refuses the
|
||||
// name only when it holds a node the mount has forgotten: the create of
|
||||
// the replaced file landed after its op was dropped.
|
||||
@@ -99,20 +80,10 @@ const KNOWN: &[Known] = &[
|
||||
.iter()
|
||||
.any(|line| line.starts_with("rename failed") && line.contains("AlreadyExists"))
|
||||
},
|
||||
// The file it was replacing is gone from the mount (B124).
|
||||
// The model made the rename, and Drive holds a node the mount does
|
||||
// not know.
|
||||
ends_seed: |_, _| true,
|
||||
},
|
||||
// A write over a queued revision whose gaps are not filled yet is refused.
|
||||
Known {
|
||||
bug: "B123",
|
||||
is: |_, _, errno, logged, _| {
|
||||
errno == libc::EIO
|
||||
&& logged
|
||||
.iter()
|
||||
.any(|line| line.starts_with("refusing write over incomplete queued revision"))
|
||||
},
|
||||
ends_seed: |_, _| false,
|
||||
},
|
||||
// A folder made offline is listed on Drive once its listing has been
|
||||
// invalidated, and Drive does not know it, or does not list it yet. Only
|
||||
// the client writes its folder, so a folder of its own is never really
|
||||
@@ -649,13 +620,6 @@ impl Run {
|
||||
/// - B128: the file's queued create ran into the name of the file it
|
||||
/// replaced and took that over, trash and all.
|
||||
///
|
||||
/// A conflict copy of an earlier version, which the model no longer
|
||||
/// holds, takes the drain saying it kept a queued write as that copy
|
||||
/// after a landed create could not be read back:
|
||||
///
|
||||
/// - B131: the tree kept the create's placeholder, so a write based on it
|
||||
/// found Drive's copy changed.
|
||||
///
|
||||
/// A conflict copy of a file the client deleted takes the drain saying it
|
||||
/// kept that file's write as the copy, because Drive had the file in the
|
||||
/// trash while the tree still placed it:
|
||||
@@ -693,16 +657,6 @@ impl Run {
|
||||
})
|
||||
})
|
||||
};
|
||||
let unread_create = self.logged.iter().any(|line| {
|
||||
line.starts_with("pending upload failed; will retry uid=local~")
|
||||
&& line.ends_with("error=fetch node: no such file or folder")
|
||||
});
|
||||
let kept_write = |copy: &str| {
|
||||
let alt = format!("alt={:?}", name_of(copy));
|
||||
self.logged.iter().any(|line| {
|
||||
line.starts_with("queued write landed as a conflict copy") && line.ends_with(&alt)
|
||||
})
|
||||
};
|
||||
let kept_deleted = |copy: &str| {
|
||||
let alt = format!(" alt={:?}", name_of(copy));
|
||||
self.logged.iter().any(|line| {
|
||||
@@ -724,15 +678,11 @@ impl Run {
|
||||
if !is_conflict_copy(copy) || model.contains_key(copy) {
|
||||
continue;
|
||||
}
|
||||
let bug = if unread_create && kept_write(copy) {
|
||||
"B131"
|
||||
} else if kept_deleted(copy) {
|
||||
"B134"
|
||||
} else {
|
||||
if !kept_deleted(copy) {
|
||||
continue;
|
||||
};
|
||||
}
|
||||
damage.push(KnownDamage {
|
||||
bug,
|
||||
bug: "B134",
|
||||
path: copy.clone(),
|
||||
copy: Some(copy.clone()),
|
||||
change: FsOp::Create {
|
||||
@@ -1068,6 +1018,19 @@ mod tests {
|
||||
drain_budget: Duration::from_secs(20),
|
||||
};
|
||||
|
||||
static ONE_CLIENT_ECHOES: Profile = Profile {
|
||||
name: "one-client-echoes",
|
||||
clients: 1,
|
||||
steps: 60,
|
||||
faults: Faults::echoes,
|
||||
link_flaps: false,
|
||||
restarts: false,
|
||||
deletes_mid_upload: false,
|
||||
settle_budget: Some(Duration::from_secs(6)),
|
||||
syscall_budget: Duration::from_secs(2),
|
||||
drain_budget: Duration::from_secs(20),
|
||||
};
|
||||
|
||||
static ONE_CLIENT_FLAKY: Profile = Profile {
|
||||
name: "one-client-flaky",
|
||||
clients: 1,
|
||||
@@ -1106,6 +1069,12 @@ mod tests {
|
||||
run_seeds(&ONE_CLIENT_DELETES, 3);
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse --lib sim::run -- --ignored`"]
|
||||
fn one_client_with_stale_hashes_and_reordered_echoes() {
|
||||
run_seeds(&ONE_CLIENT_ECHOES, 6);
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse --lib sim::run -- --ignored`"]
|
||||
fn one_client_on_a_flaky_link() {
|
||||
|
||||
@@ -825,11 +825,54 @@ impl State {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn intern(&mut self, parent: u64, node: Node) -> u64 {
|
||||
pub(crate) fn intern(&mut self, parent: u64, mut node: Node) -> u64 {
|
||||
self.keep_revision_mtime(&mut node);
|
||||
self.outbox.push(DbWrite::Upsert(vec![node.clone()]));
|
||||
self.intern_mem(parent, node)
|
||||
}
|
||||
|
||||
/// Keep what is already known of a file whose content has not changed.
|
||||
///
|
||||
/// Drive stamps a link's `ModifyTime` on a rename or a move as well, while
|
||||
/// POSIX leaves a file's mtime alone there. The content cache is keyed by
|
||||
/// it, too: taking the new stamp made the cached bytes unreachable, so the
|
||||
/// next partial write could not fill its gaps (docs/BUGS.md B123). The
|
||||
/// active revision id says whether the content is still the same, and so
|
||||
/// whether the size and digest a listing left out are still the ones known.
|
||||
fn keep_revision_mtime(&self, node: &mut Node) {
|
||||
let Some(entry) = self
|
||||
.by_uid
|
||||
.get(&node.uid)
|
||||
.and_then(|ino| self.entries.get(ino))
|
||||
else {
|
||||
return;
|
||||
};
|
||||
if let (
|
||||
NodeKind::File {
|
||||
active_revision_id: Some(now),
|
||||
claimed_size,
|
||||
content_sha1,
|
||||
..
|
||||
},
|
||||
NodeKind::File {
|
||||
active_revision_id: Some(was),
|
||||
claimed_size: size_was,
|
||||
content_sha1: sha1_was,
|
||||
..
|
||||
},
|
||||
) = (&mut node.kind, &entry.node.kind)
|
||||
&& now == was
|
||||
{
|
||||
node.modification_time = entry.node.modification_time;
|
||||
if claimed_size.is_none() {
|
||||
*claimed_size = *size_was;
|
||||
}
|
||||
if content_sha1.is_none() {
|
||||
content_sha1.clone_from(sha1_was);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Allocate (or reuse) a stable inode for a node, updating the hot-cache maps
|
||||
/// only. Every caller owes the DB a write-through — see the callers below;
|
||||
/// the split exists so a batch can pay for one transaction instead of `n`.
|
||||
@@ -915,7 +958,10 @@ impl State {
|
||||
///
|
||||
/// This is what keeps `ls` on a large folder quick: one commit for the
|
||||
/// listing, rather than one autocommit — and one fsync — per child.
|
||||
pub(crate) fn intern_batch(&mut self, parent: u64, nodes: Vec<Node>) -> Vec<u64> {
|
||||
pub(crate) fn intern_batch(&mut self, parent: u64, mut nodes: Vec<Node>) -> Vec<u64> {
|
||||
for node in &mut nodes {
|
||||
self.keep_revision_mtime(node);
|
||||
}
|
||||
self.outbox.push(DbWrite::Upsert(nodes.clone()));
|
||||
nodes
|
||||
.into_iter()
|
||||
@@ -1937,6 +1983,51 @@ mod tests {
|
||||
assert_eq!(st.entries[&shared].access, Access::Viewer);
|
||||
}
|
||||
|
||||
fn at_revision(mut node: Node, revision: &str, mtime: i64) -> Node {
|
||||
node.modification_time = mtime;
|
||||
if let NodeKind::File {
|
||||
active_revision_id, ..
|
||||
} = &mut node.kind
|
||||
{
|
||||
*active_revision_id = Some(revision.into());
|
||||
}
|
||||
node
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_renamed_file_keeps_its_mtime_until_its_content_changes() {
|
||||
let (mut st, _dir) = state();
|
||||
let root = st.intern(0, node("root", "none", "My Files", true));
|
||||
let mut file = node("file", "root", "a.txt", false);
|
||||
if let NodeKind::File {
|
||||
claimed_size,
|
||||
total_size_on_storage,
|
||||
..
|
||||
} = &mut file.kind
|
||||
{
|
||||
*claimed_size = Some(7);
|
||||
*total_size_on_storage = 519;
|
||||
}
|
||||
let ino = st.intern(root, at_revision(file.clone(), "r1", 100));
|
||||
// Drive stamps the link on a rename; the content is still r1.
|
||||
let mut renamed = at_revision(file.clone(), "r1", 160);
|
||||
renamed.name = "b.txt".into();
|
||||
st.intern_batch(root, vec![renamed]);
|
||||
assert_eq!(st.entries[&ino].node.modification_time, 100);
|
||||
assert_eq!(st.entries[&ino].node.name, "b.txt");
|
||||
// A listing that leaves the size out still describes revision r1.
|
||||
let mut bare = at_revision(file.clone(), "r1", 170);
|
||||
assert_eq!(crate::node_size(&st.entries[&ino].node), 7);
|
||||
if let NodeKind::File { claimed_size, .. } = &mut bare.kind {
|
||||
*claimed_size = None;
|
||||
}
|
||||
st.intern(root, bare);
|
||||
assert_eq!(st.entries[&ino].node.modification_time, 100);
|
||||
assert_eq!(crate::node_size(&st.entries[&ino].node), 7);
|
||||
st.intern(root, at_revision(file, "r2", 200));
|
||||
assert_eq!(st.entries[&ino].node.modification_time, 200);
|
||||
}
|
||||
|
||||
fn handle(len: u64, base_size: u64) -> WriteHandle {
|
||||
let path = std::env::temp_dir().join(format!("pdfs-handle-test-{}", std::process::id()));
|
||||
let file = File::create(&path).unwrap();
|
||||
|
||||
+92
-26
@@ -12,6 +12,30 @@ Conventions:
|
||||
|
||||
---
|
||||
|
||||
## B135 — A file just moved is sometimes not found by its new path
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-03, by the simulation runs (`sim::run`, profile three-clients, seed 1), once
|
||||
B131 was fixed. `rename a.txt -> y/f.txt` succeeded, and the `rename y/f.txt -> y/a.txt` two
|
||||
milliseconds later failed with `ENOENT`. Once in about four replays of the seed.
|
||||
|
||||
**Where:** the online path of `serve_rename` in `crates/pdfs-fuse/src/filesystem.rs`.
|
||||
|
||||
**Cause.** After the move lands on Drive, `serve_rename` settles the tree with `State::relocate`.
|
||||
It took the inode lock directly (`state.lock()`) instead of through `Core::state`, whose guard
|
||||
writes what a mutation owes the database once the lock is released (DB1). So the writes stayed in
|
||||
the state's outbox: both folders no longer listed, and the node's new parent. Whichever thread
|
||||
next released a guard applied them, after it had let go of the lock. The lookup of `y/f.txt`
|
||||
found `y` unlisted in memory and asked the database, which still marked `y` as listed and placed
|
||||
the file in the root, so it rebuilt `y` without the file. It took another thread taking the lock
|
||||
in between, so it happened only now and then.
|
||||
|
||||
**Fix.** `serve_rename` relocates through `Core::state`, so the database has the move before the
|
||||
reply.
|
||||
|
||||
**Test:** the unit test `an_online_rename_writes_the_move_through_before_it_answers`. With the
|
||||
fix, seed 1 of three-clients passes repeatedly with `PDFS_SIM_KNOWN=fail`.
|
||||
|
||||
## B134 — A file deleted online while the drain checks its write is kept as a conflict copy
|
||||
|
||||
**Status:** Open.
|
||||
@@ -87,7 +111,7 @@ one-client-flaky fails on it when the mkdir is still queued, which depends on ti
|
||||
|
||||
## B131 — A write to a file whose create could not be read back lands as a conflict copy
|
||||
|
||||
**Status:** Open.
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-03, by the simulation runs (`sim::run`, profile three-clients, seed 1).
|
||||
`a.txt` was created while the link was down, and the link came back right after. The create
|
||||
landed, and the drain logged "pending upload failed; will retry" for its placeholder with
|
||||
@@ -106,9 +130,21 @@ write opened afterwards takes that node as its base. Drive stamped the file when
|
||||
landed, and if that was in a later second than the placeholder's mtime, the drain sees the
|
||||
remote revision changed and keeps the client's own write as a conflict copy.
|
||||
|
||||
**Test:** the simulation runs count a conflict copy that the drain kept for a queued write, when
|
||||
a landed create could not be read back. With `PDFS_SIM_KNOWN=fail`, seed 1 of three-clients
|
||||
fails on it when the create lands in a later second than it was made, which depends on timing.
|
||||
Once the read-back succeeded, the same seed found a second gap. A write handle open on the file
|
||||
is repointed at the real uid when the op retires, and its release fills its gaps from the content
|
||||
cache. `adopt_real_uid` cached the create's bytes only after the read-back, which waits for Drive
|
||||
to list the node. A release in between found nothing to fill from and queued an incomplete
|
||||
revision, and the next write open was refused with `EIO` (as in B123).
|
||||
|
||||
**Fix.** `adopt_real_uid` reads the node back with `Core::read_back`, which asks again for a moment
|
||||
while Drive does not list it yet, as the online create does (B118). Before the op retires,
|
||||
`Core::cache_created` caches the uploaded bytes under the real uid, keyed by the size and mtime
|
||||
the tree has for the file, which is what an open handle has as its base. `adopt_real_uid` caches
|
||||
them again, keyed as Drive has the node, before it rebases anything onto it.
|
||||
|
||||
**Test:** seed 1 of three-clients passes with `PDFS_SIM_KNOWN=fail`. Before the fix it failed on
|
||||
the conflict copy when the create landed in a later second than it was made, which depends on
|
||||
timing, and then on the refused write.
|
||||
|
||||
## B130 — A read of a file that a remote change invalidates can hang the daemon
|
||||
|
||||
@@ -250,7 +286,7 @@ fails on it.
|
||||
|
||||
## B124 — After a replacing rename fails, the file it was replacing is gone from the mount
|
||||
|
||||
**Status:** Open.
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-lan, seed 1). A
|
||||
rename `x/f.txt -> z/d` over an existing `z/d` failed with `EIO` (B122). Then `create z/d`
|
||||
failed with `ENOENT`, although `z/d` was back on Drive.
|
||||
@@ -263,40 +299,66 @@ every mount's state and adds it to `hidden`, which filters listings. When the re
|
||||
in the state, as the ordinary restore does. Until the daemon restarts, the file is on Drive but
|
||||
missing from the mount, and a create by its name fails.
|
||||
|
||||
**Test:** the simulation runs end a seed on it. With `PDFS_SIM_KNOWN=fail`, seed 1 of
|
||||
`cargo test -p pdfs-fuse --lib sim::run::tests::one_client_on_a_good_link -- --ignored
|
||||
--test-threads=1` fails on it.
|
||||
**Fix.** `restore_replaced` takes the node out of `hidden`, and drops the listing of the folder
|
||||
it is restored to, and the kernel's entry for its name, in every mount that shows the folder. The
|
||||
next lookup lists the folder again and finds the node.
|
||||
|
||||
**Test:** the unit test `a_replaced_node_put_back_shows_in_the_mount_again`. Since B122 is fixed,
|
||||
the simulation runs no longer reach it through a rename across folders.
|
||||
|
||||
## B123 — A second write open on a file just written fails with `EIO`
|
||||
|
||||
**Status:** Open.
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-lan, seeds 5 and
|
||||
9). A write at an offset (seed 5) or a create over an existing file (seed 9) failed with `EIO`.
|
||||
The daemon logged "refusing write over incomplete queued revision".
|
||||
The daemon logged "refusing write over incomplete queued revision". The one-client-echoes profile,
|
||||
whose events come late, twice and out of order, found it in four more seeds.
|
||||
|
||||
**Where:** the write open in `crates/pdfs-fuse/src/filesystem.rs`, and the `complete` flag that
|
||||
`queue_revision` in `crates/pdfs-fuse/src/lib.rs` stamps on a queued revision.
|
||||
**Where:** the write open in `crates/pdfs-fuse/src/filesystem.rs`, the `complete` flag that
|
||||
`queue_revision` in `crates/pdfs-fuse/src/lib.rs` stamps on a queued revision, and everything that
|
||||
leaves the content cache without the bytes of the file's base.
|
||||
|
||||
**Cause.** A write open over a queued revision that is not complete is refused by design, because
|
||||
a second partial edit on top of one with gaps cannot be represented. Two ordinary cases leave a
|
||||
a second partial edit on top of one with gaps cannot be represented. Ordinary cases left a
|
||||
revision incomplete, though:
|
||||
|
||||
- A file closed empty never counts as complete. `queue_revision` compares the authored ranges with
|
||||
`[(0, len)]`, which for a length of 0 is `[(0, 0)]`, while the authored ranges are empty. The
|
||||
truncate path already treats a size of 0 as complete.
|
||||
- The gaps of a partial edit are filled from the content cache, and the cache can be empty
|
||||
then. One way is our own rename: `rename_remote` and `move_remote` do not call
|
||||
`note_self_change`, so the echo of the rename evicts the cached bytes.
|
||||
- A file closed empty never counted as complete. `queue_revision` compared the authored ranges
|
||||
with `[(0, len)]`, which for a length of 0 is `[(0, 0)]`, while the authored ranges are empty.
|
||||
The truncate path already treated a size of 0 as complete.
|
||||
- The gaps of a partial edit are filled from the content cache, and the cache could be empty or
|
||||
out of reach then:
|
||||
- The echo of our own change evicted it, taken for someone else's. `rename_remote`,
|
||||
`move_remote` and the drain's `move_renaming` did not record an echo, and neither did a
|
||||
create made online, so the create's echo took the one the upload recorded. The feed can also
|
||||
deliver an event twice, or replay one after a batch out of order, and the second copy found
|
||||
the echo already taken.
|
||||
- Drive stamps a link's `ModifyTime` on a rename or a move too. The listing after it moved the
|
||||
file's mtime, and the cache, keyed by mtime and size, no longer matched.
|
||||
- When an upload landed, the drain let go of the staged blob, read the node back, rebased open
|
||||
write handles onto it, and only then cached the blob. A handle released in between found
|
||||
neither the blob nor the cached bytes.
|
||||
|
||||
Either way, the next write open on the file within the drain debounce (`DRAIN_REVISION_DEBOUNCE`,
|
||||
2 s) fails, until the revision has landed.
|
||||
2 s) failed, until the revision had landed.
|
||||
|
||||
**Test:** the simulation runs count it and go on. With `PDFS_SIM_KNOWN=fail`, seeds 5 and 9 of
|
||||
one-client-lan fail on it.
|
||||
**Fix.** `StagedWrite::covers` counts an empty file as complete, at every site that sets the flag.
|
||||
`Core::changed_remote` records the echo of every rename and move, and `fetch_minted_node` the echo
|
||||
of every create made online. The event sync skips a node update whose id it applied lately
|
||||
(`RecentEvents`). `State::intern` keeps a file's mtime, and the size a listing left out, while its
|
||||
active revision id stays the same. `refresh_after_upload` caches the uploaded bytes before it
|
||||
rebases open handles, and lets go of the staged blob only after.
|
||||
|
||||
**Test:** the unit tests `an_empty_write_covers_the_file_and_a_gap_does_not`,
|
||||
`a_node_update_seen_again_is_skipped_but_other_events_are_not`,
|
||||
`a_renamed_file_keeps_its_mtime_until_its_content_changes` and
|
||||
`a_rename_or_move_drops_the_sdk_copy_and_expects_its_echo`, and the FUSE tests
|
||||
`a_file_closed_empty_takes_a_second_write_at_once` and
|
||||
`the_echo_of_our_own_rename_keeps_the_cached_bytes` in `sim::daemon`. The simulation runs no
|
||||
longer tolerate it.
|
||||
|
||||
## B122 — A rename across folders fails with `EIO` when the source folder holds the new name
|
||||
|
||||
**Status:** Open.
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-lan, seeds 1, 7
|
||||
and 10). A `rename(2)` like `x/f.txt -> z/d` failed with `EIO` while `x/d` existed. The daemon
|
||||
logged "rename failed" with Drive's `AlreadyExists`.
|
||||
@@ -309,8 +371,12 @@ the source folder with the new name makes Drive refuse the first call, although
|
||||
is free. B46's follow-up already notes that this path was not converted to the single
|
||||
`move-multiple` request the drain uses. If the rename was replacing a file, B124 follows.
|
||||
|
||||
**Test:** the simulation runs count it and go on. With `PDFS_SIM_KNOWN=fail`, seed 1 of
|
||||
one-client-lan fails on it.
|
||||
**Fix.** `Core::move_rename_remote` moves and renames in one `move-multiple` request, so the name
|
||||
only has to be free in the destination. `serve_rename` uses it for every rename across folders,
|
||||
and the drain uses it in place of its own `move_renaming`.
|
||||
|
||||
**Test:** the FUSE test `a_rename_across_folders_ignores_the_new_name_in_the_old_folder` in
|
||||
`sim::daemon`. The simulation runs no longer tolerate it.
|
||||
|
||||
## B121 — A move right after a rename fails with `EIO`, and a rename across folders takes seconds
|
||||
|
||||
@@ -340,7 +406,7 @@ the cached node. Every rename, and every move but the drain's, goes through `Cor
|
||||
or `Core::move_remote`. Both drop the entry once the change has landed, and before each retry of a
|
||||
request Drive called out of date. The drain's `move_renaming` drops it once its move lands.
|
||||
|
||||
**Test:** the unit tests `a_rename_or_move_drops_the_sdk_copy_of_the_node` and
|
||||
**Test:** the unit tests `a_rename_or_move_drops_the_sdk_copy_and_expects_its_echo` and
|
||||
`every_remote_rename_and_move_rides_out_a_stale_hash`. On the account run, "regression B111" and
|
||||
"regression B113" should show no `status=422` warnings.
|
||||
|
||||
|
||||
Reference in new issue
Block a user