mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(core,fuse,cli): harden writes, trash and cleanup found by account acceptance runs
The FUSE acceptance suite now runs on the signed-in account by default and cleans up after itself; its first live runs found three daemon bugs. - B101: a pwrite past EOF now authors the hole it leaves, so truncated bytes no longer come back from the base. A write attached to a create while that create uploads becomes a revision of the real uid instead of being dropped. - B102: removing a folder drops the queued revisions of files below it, found through the node tree, with their blobs and in-flight uploads. `pdfs rm` now drops queued ops too. - B103: each server call of a trash refresh times out after 120 s, so one hung call can no longer freeze the trash listing until restart. Acceptance suite: account mode with a manifest, lock and reaper; cleanup by uid; queued ops naming a test root count as leftovers; new edge-case, throughput and B100/B101 cases; the wide-directory case is split from the deep tree and gets a doubled budget. Adds `pdfs trash --json`.
This commit is contained in:
1 parent
84c9ade8e3
commit
7b67db5ced
13 files changed
+1785
-126
No files matched your search
+100
-1
@@ -12,6 +12,105 @@ Conventions:
|
||||
|
||||
---
|
||||
|
||||
## B103 — One stuck trash refresh freezes the trash listing until the daemon restarts
|
||||
|
||||
**Status:** Fixed (unverified). The hang itself has not been reproduced on purpose.
|
||||
**Found:** 2026-09-29, while timing the acceptance suite's wide-directory case. Its cleanup trashed
|
||||
the test folder, then waited for it to show up in the trash so it could delete it permanently. It
|
||||
never did. From 06:55 UTC on, every trash listing logged `trash refresh still running; answering
|
||||
with what has materialized` and returned the same 362 entries. No `trash refresh: enumerated` line
|
||||
followed, so the first server call of the refresh never returned.
|
||||
|
||||
**Where:** `crates/pdfs-fuse/src/lib.rs` (`Core::refresh_trash`, `within_trash_call`).
|
||||
|
||||
**Cause.** Trash refreshes are single-flight: a request joins the refresh already running instead
|
||||
of starting another. The refresh awaited its server calls with no deadline, so one call that
|
||||
never returned kept the flag set forever. Every later request joined it, waited out its 20 s
|
||||
budget, and answered with the stale listing.
|
||||
|
||||
**Fix.** Each server call of a refresh (`enumerate_trash_node_uids`, and each `enumerate_nodes`
|
||||
chunk) fails after `TRASH_CALL_TIMEOUT` (120 s). The refresh then ends with an error, clears the
|
||||
flag, and the next request starts a fresh one.
|
||||
|
||||
---
|
||||
|
||||
## B102 — Removing a folder leaves the queued uploads of the files inside it
|
||||
|
||||
**Status:** Fixed (unverified). No daemon carrying the change has run the acceptance suite yet.
|
||||
**Found:** 2026-09-29, after the account run that found B101. Its cleanup removed both test roots
|
||||
and reported "nothing left behind", but `pdfs sync queue` still listed two `revision` ops for
|
||||
`pdfs-acceptance-…/boundaries/reshaped`, each holding about 8 MiB of staged bytes. They retried
|
||||
against the trash forever ("the share has not been writable for 300s").
|
||||
|
||||
**Where:** `crates/pdfs-core/src/db/ops.rs` (`drop_doomed_ops`, used by
|
||||
`Db::replace_ops_with_trash` and `Db::delete_ops_for_uid`), `crates/pdfs-fuse/src/lib.rs`
|
||||
(`Core::delete`, `Core::release_dropped_ops`).
|
||||
|
||||
**Cause.** Two gaps.
|
||||
|
||||
- **The queue could not see which files a folder holds.** Removing a node dropped the ops for it
|
||||
and for anything *queued* beneath it, found through `pending_op.parent_uid`. Only `create` and
|
||||
`mkdir` ops carry a parent, so a `revision` of a file that already existed remotely was never
|
||||
found, however deep in the removed folder it sat.
|
||||
- **`pdfs rm` dropped nothing at all.** The control socket's delete trashed the node remotely but
|
||||
left every op queued for it, unlike an unlink through the mount.
|
||||
|
||||
**Fix.** Removal also follows the node tree (`nodes.parent_uid`) and drops the `revision` ops of
|
||||
every file below the node, with their blobs, pending entries and in-flight uploads. Queued renames
|
||||
below it are kept: one that moved a node into the folder still has to land for the node to go to
|
||||
the trash with it. `pdfs rm` now drops the queued ops once the remote trash succeeds. The
|
||||
acceptance janitor lists the daemon queue after cleanup and reports any op still naming one of its
|
||||
roots as a leftover.
|
||||
Regression test: `trashing_a_folder_drops_the_queued_revisions_of_files_below_it`.
|
||||
|
||||
---
|
||||
|
||||
## B101 — A shrunk file written past its end gets old bytes back, or fails with `EIO`; a write during a create's upload is lost
|
||||
|
||||
**Status:** Fixed (unverified). No daemon carrying the change has run the acceptance suite yet.
|
||||
**Found:** 2026-09-29, by the new acceptance case "block boundaries and overwrites that change
|
||||
the block count". It failed with `Input/output error` in My files and in an on-demand folder, every
|
||||
time. The journal had `the revision's block table disagrees with the node size ... fsize=8388611
|
||||
reader_size=0`, then `pending upload failed; will retry ... gap-fill from base failed: Errno(5)`
|
||||
on every retry.
|
||||
|
||||
**Where:** `crates/pdfs-fuse/src/state.rs` (`WriteHandle::record_write`),
|
||||
`crates/pdfs-fuse/src/drain.rs` (`Core::drain_local_node`, `Core::retire_create`),
|
||||
`crates/pdfs-fuse/src/lib.rs` (`Core::enqueue_staged_write`),
|
||||
`crates/pdfs-core/src/db/ops.rs` (`Db::finish_create`).
|
||||
|
||||
**Cause.** Two defects.
|
||||
|
||||
- **The hole of a write past the end was not authored.** A write handle tracks which ranges it
|
||||
wrote; everything else below the base size is filled from the base when it is committed. A
|
||||
grow through `setattr` marks its new tail as written, but a `pwrite` past the end did not mark
|
||||
the hole it leaves. After `ftruncate(fd, 4 MiB)` and `pwrite(fd, data, 4 MiB + 10)`, bytes
|
||||
4 MiB to 4 MiB + 10 were refilled from the old base instead of reading as zeros. That silently
|
||||
restored truncated bytes. When the base was itself a queued revision the remote did not have
|
||||
yet, the fill read a revision of the wrong size, and B84's check failed it: `EIO` on read, and
|
||||
a drain that retried forever.
|
||||
- **A write that closed while its file's create uploaded was lost.** The create of a new file
|
||||
drains on its own, often while the file is still open. A write that closes meanwhile attaches
|
||||
its blob to the create's queue row. When the create landed, the drain deleted that row by id,
|
||||
so the server kept the older bytes (often an empty file) and nothing was queued to replace
|
||||
them. A handle still open after the create landed carried the placeholder uid, found no create
|
||||
to attach to, and failed its `close` with `EIO`.
|
||||
|
||||
**Fix.** A write past the end authors the hole (`WriteHandle::record_write`).
|
||||
`Db::finish_create` retires a landed create in one transaction: if the row now holds a newer blob
|
||||
than the one uploaded, it becomes a revision of the real uid instead of being deleted. The drain
|
||||
does this under the `pending` lock, which `enqueue_staged_write` holds from attaching a blob until
|
||||
it records the pending entry, and then repoints open write handles to the real uid. A write that
|
||||
still finds the create gone waits for its inode's real uid (`Core::landed_uid`) and queues a
|
||||
revision against it.
|
||||
Regression tests: `a_write_past_the_end_authors_the_hole_it_leaves`,
|
||||
`a_write_attached_while_its_create_uploads_becomes_a_revision`,
|
||||
`a_create_whose_blob_landed_is_retired`, and the live acceptance cases "block boundaries and
|
||||
overwrites that change the block count" (now also read back after the queue drains) and
|
||||
`regression B101`.
|
||||
|
||||
---
|
||||
|
||||
## B100 — A freshly copied file reads as `EIO` or 53 bytes too big
|
||||
|
||||
**Status:** Fixed (unverified). No daemon carrying the change has read a file with a provisional
|
||||
@@ -48,7 +147,7 @@ network). A folder past the cap is now queued instead of dropped. When a running
|
||||
its thread takes over the oldest queued folder, so at most 8 threads still run.
|
||||
Regression tests: `a_folder_past_the_cap_is_queued_not_dropped`,
|
||||
`a_finished_batch_hands_its_thread_the_oldest_queued_one`. The read and open paths need a live
|
||||
mount and have no unit test.
|
||||
mount and have no unit test; the acceptance case `regression B100` covers them on a real mount.
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -11,7 +11,29 @@ scratch (user data in `staging/` and `recovery/` is never touched by this).
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Added
|
||||
- **`pdfs trash --json`** prints the trash listing as JSON.
|
||||
- **The FUSE acceptance suite tests the signed-in account by default.**
|
||||
`scripts/fuse-acceptance.sh` with no arguments creates test folders in My files and new
|
||||
on-demand and mirror sync folders, runs every case in each, and removes all of it afterwards,
|
||||
also on a failure or interrupt. A run that dies is cleaned up by the next one. New cases cover
|
||||
block boundaries, unusual names, a deep tree, a wide directory, rename patterns, handle coherency,
|
||||
throughput and B100. `--offline-only` runs the account-free suite as before.
|
||||
|
||||
### Fixed
|
||||
- **Truncating and then writing past the end no longer brings old bytes back.** The gap
|
||||
between the new end and the write now reads as zeros, as it should, instead of the file's
|
||||
earlier content. When that earlier content had not been uploaded yet, the file failed with
|
||||
`Input/output error` and its upload retried forever (B101).
|
||||
- **A write made while a new file is first uploading is no longer lost.** Drive could keep an
|
||||
empty or older copy of a file written right after it was created, and closing a file held open
|
||||
across its first upload could fail (B101).
|
||||
- **Removing a folder also cancels the pending uploads of the files inside it.** Before, an
|
||||
edit that had not reached Drive yet kept retrying against the trash forever and kept its
|
||||
staged copy on disk. `pdfs rm` now also drops what was queued for what it removes (B102).
|
||||
- **The trash no longer freezes after a server call hangs.** Until the daemon restarted, the
|
||||
trash kept showing the same entries, and a folder just deleted could not be found there to
|
||||
delete permanently (B103).
|
||||
- **Freshly listed files read correctly.** A file whose real size had not been fetched yet
|
||||
reported its encrypted size, about 53 bytes too big, and reading it failed with
|
||||
`Input/output error`. This happened most often right after copying a large folder. Reads and
|
||||
|
||||
+1
-1
@@ -177,7 +177,7 @@ commands are for scripts and for cases where the mount is not available.
|
||||
## Scripting with `--json`
|
||||
|
||||
The global `--json` flag makes query commands print JSON: `status`, `ls`, `pins`, `sync list`,
|
||||
`devices list`, `locations`, `transfers`, `activity` and `cache inspect`. Commands that change
|
||||
`devices list`, `locations`, `transfers`, `activity`, `trash` and `cache inspect`. Commands that change
|
||||
something keep their human-readable output; check their exit code.
|
||||
|
||||
```bash
|
||||
|
||||
+86
-10
@@ -25,18 +25,80 @@ Proton:
|
||||
scripts/fuse-acceptance.sh --offline-only
|
||||
```
|
||||
|
||||
Kernel/FUSE behavior and remote convergence need a real mount. Live testing is
|
||||
explicitly opt-in and always runs the account-free contract first. It runs
|
||||
destructive POSIX operations only below a fresh `pdfs-acceptance-*` directory.
|
||||
It refuses paths that are not FUSE mounts and removes its directory on success,
|
||||
failure, or interruption.
|
||||
Kernel/FUSE behavior and remote convergence need a real mount. With no
|
||||
arguments, the script tests the account the running service is logged in to:
|
||||
|
||||
Use a dedicated test account without irreplaceable data:
|
||||
```console
|
||||
scripts/fuse-acceptance.sh # every case, every mount kind
|
||||
scripts/fuse-acceptance.sh --quick # one on-demand/mirror pair instead of four
|
||||
```
|
||||
|
||||
An account run does this, in order:
|
||||
|
||||
1. Runs the account-free contract as the reference.
|
||||
2. Runs the contract in a fresh `pdfs-acceptance-<id>` folder in My files and
|
||||
diffs it against the reference.
|
||||
3. Registers two new sync folders under `~/.cache/pdfs-acceptance/run-<id>/`
|
||||
and runs the contract and the move cases in every on-demand/mirror pairing.
|
||||
4. Removes everything it created, locally and remotely, and reports a
|
||||
`cleanup` result. The run fails if anything is left behind.
|
||||
|
||||
Cleanup identifies remote folders by uid, never by name alone, so nothing of
|
||||
yours is touched. It removes each sync folder with `sync rm --delete-remote`,
|
||||
trashes the My files folder, and then deletes both permanently from the trash.
|
||||
It only counts a folder as gone once a fresh trash listing no longer shows it.
|
||||
It then reads `pdfs sync queue` and counts any queued op that still names one
|
||||
of its folders as left behind too, since such an op keeps its staged bytes and
|
||||
retries forever (B102).
|
||||
Cleanup also runs on a failure, Ctrl-C, `SIGTERM` or `SIGHUP`. While it runs,
|
||||
two more interrupts are ignored; a third abandons it.
|
||||
|
||||
Each run records what it is about to create in
|
||||
`~/.cache/pdfs-acceptance/run-<id>/manifest.json` before it creates it, and
|
||||
holds a lock on that directory. If a run dies without cleaning up (`kill -9`, a
|
||||
hard timeout, a power cut), the next run finds the unlocked manifest and
|
||||
finishes that cleanup first. It then removes stray `pdfs-acceptance-<32 hex>`
|
||||
folders that no live run claims: in My files (older than
|
||||
`PDFS_ACCEPTANCE_REAP_AGE`, default one hour), under the acceptance home, and
|
||||
in the trash. A run does not start while that reap leaves anything behind.
|
||||
|
||||
The run creates and destroys real remote folders, so a dedicated test account
|
||||
without irreplaceable data is still the safest target.
|
||||
|
||||
`--live PATH...` tests mounts you set up yourself and leaves the sync folder
|
||||
configuration alone. It runs destructive POSIX operations only below a fresh
|
||||
`pdfs-acceptance-*` directory, refuses paths that are not FUSE mounts, and
|
||||
removes its directory on success, failure, or interruption:
|
||||
|
||||
```console
|
||||
scripts/fuse-acceptance.sh --live /mnt/on-demand-testmount
|
||||
```
|
||||
|
||||
### Edge cases and performance
|
||||
|
||||
Besides the syscall contract, the suite covers:
|
||||
|
||||
- **Block boundaries.** Files of 0 bytes up to two 4 MiB blocks plus 3 bytes,
|
||||
including every size next to a boundary, then overwrites that grow, shrink
|
||||
and reshape them.
|
||||
- **Unusual names.** A 255-byte name, NFC and NFD forms of the same text,
|
||||
newline, tab, emoji, characters Windows forbids, leading and trailing
|
||||
spaces, and a case-only rename.
|
||||
- **A deep tree and a wide directory.** 24 levels of nesting renamed at the
|
||||
top; 128 entries created concurrently, concurrent unlinks, then `rm -rf`.
|
||||
Online, each create is a round trip to the server, and the kernel runs
|
||||
creates in one directory one at a time. So the wide case gets twice the
|
||||
per-case timeout and notes the time per create as `wide.create_ms`.
|
||||
- **Rename patterns.** Swaps, chains, log rotation, replacing a file under an
|
||||
open reader, the `EISDIR`/`ENOTDIR`/`ENOENT` refusals, and a folder moved out
|
||||
and back.
|
||||
- **Handle coherency.** Unsynced writes seen by a second handle, two `O_APPEND`
|
||||
writers, 30 rewrite generations, and a truncate under a reader.
|
||||
- **Throughput.** 64 MiB written and read sequentially, 1 MiB in 4 KiB writes,
|
||||
and metadata operations on 100 files. The rates are printed. The case fails
|
||||
below `PDFS_ACCEPTANCE_MIN_MIBPS` (default 10) or `PDFS_ACCEPTANCE_MIN_OPS`
|
||||
(default 20 operations per second).
|
||||
|
||||
### The mount is diffed against an ordinary filesystem
|
||||
|
||||
The local reference run is not only a self-test of the runner. Every case
|
||||
@@ -78,6 +140,16 @@ in-flight `.crdownload` must not be sealed as a revision) and B74 (a file
|
||||
renamed after close must keep its content) need a real mount and a reachable
|
||||
daemon; they are skipped, not failed, without one.
|
||||
|
||||
**B100** (a copied tree reads back at its exact sizes) copies 48 files of
|
||||
assorted sizes into the mount at once, as `cp -r` does. On a live mount it
|
||||
waits for the queue, checks every size and byte, runs `pdfs refresh` on each
|
||||
folder to drop the cached listing, and checks again. The refresh is what
|
||||
brought back the provisional sizes the bug reported.
|
||||
|
||||
**B101** (a write during its create's upload reaches Drive) holds new files open for
|
||||
different times before writing, so their empty creates land before, during and after the
|
||||
bytes arrive, then checks every file after the queue drains.
|
||||
|
||||
B74 was found by exactly this mechanism: the B70 case failed, and narrowing it
|
||||
produced a smaller reproduction that got its own case. The defect is fixed and
|
||||
live-validated (2026-07-26); the case now guards against its return.
|
||||
@@ -136,13 +208,17 @@ stops at the first failure.
|
||||
it ran. A suite can pass every assertion while the daemon logs failures behind
|
||||
it; that has happened, and it was caught only by reading the journal by hand.
|
||||
|
||||
Abandoned `pdfs-acceptance-*` roots from a crashed run are removed at startup,
|
||||
but only if they match the exact generated name and are over an hour old
|
||||
(`PDFS_ACCEPTANCE_REAP_AGE`), so a concurrent run is never disturbed.
|
||||
With `--live`, abandoned `pdfs-acceptance-*` roots from a crashed run are
|
||||
removed at startup, but only if they match the exact generated name and are
|
||||
over an hour old (`PDFS_ACCEPTANCE_REAP_AGE`), so a concurrent run is never
|
||||
disturbed. An account run instead uses the manifest and reaper described
|
||||
above.
|
||||
|
||||
### Automated mode matrix
|
||||
|
||||
The managed live runner accepts two existing, empty, unmounted directories. It
|
||||
An account run drives this matrix itself, in folders it creates. To use
|
||||
directories of your choice instead, the managed live runner accepts two
|
||||
existing, empty, unmounted directories. It
|
||||
registers both as new sync folders, waits for initial synchronization, then
|
||||
tests all four pairs in order: on-demand/on-demand, on-demand/mirror,
|
||||
mirror/on-demand, and mirror/mirror. Every transition must become idle, and an
|
||||
|
||||
Reference in new issue
Block a user