mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-04 08:56:59 +01:00
fixed race conditions + extendet test suite
This commit is contained in:
1 parent
a091c2f34f
commit
8470662c54
12 files changed
+2287
-243
No files matched your search
+172
-1
@@ -2148,7 +2148,10 @@ default (B71) precisely so that stays true until there is field evidence.
|
||||
|
||||
## B70 — Browser in-flight temp files uploaded and conflict-forked on the on-demand mount
|
||||
|
||||
**Status:** Layers A + B fixed in-tree 2026-07-25 — offline-tested; **live validation pending**.
|
||||
**Status:** Layers A + B fixed in-tree 2026-07-25 — **live-validated 2026-07-26**.
|
||||
Validation was blocked until B74 was fixed (its scenario ends in a rename, which
|
||||
B74 made read back as empty); with that out of the way the acceptance case
|
||||
`regression B70: transient download name is not sealed` passes live.
|
||||
**Found:** triaging the divergent conflict copies B69 left behind. In `~/Downloads`
|
||||
(an on-demand mount) the complete files had been forked into `(sync-conflict)`
|
||||
copies while their truncated partials kept the canonical name — e.g.
|
||||
@@ -2362,3 +2365,171 @@ or the CLI's response read in `crates/pdfs-cli/src/main.rs`.
|
||||
**Impact:** the trash listing is the user's recovery path after any destructive
|
||||
operation, including the B71 sweep. It needs to work before the sweep is allowed
|
||||
to trash anything.
|
||||
|
||||
## B73 — Unimplemented FUSE handlers log a WARN per probe
|
||||
|
||||
**Status:** Fixed in-tree 2026-07-26 — live-validated. Found 2026-07-26 by the
|
||||
expanded acceptance suite.
|
||||
**Found:** `scripts/fuse-acceptance.sh --live` step "unsupported operations refuse
|
||||
cleanly". Each probe produced a multi-line `fuser` warning in the daemon journal:
|
||||
|
||||
```
|
||||
WARN fuser: [Not Implemented] symlink(parent: INodeNo(0x2d), link_name: "symlink", target: "link-target")
|
||||
WARN fuser: [Not Implemented] link(ino: INodeNo(0x94), newparent: INodeNo(0x2d), newname: "hardlink")
|
||||
WARN fuser: [Not Implemented] mknod(parent: INodeNo(0x2d), name: "fifo", mode: 4516, umask: 0x12, rdev: 0)
|
||||
WARN fuser: [Not Implemented] setxattr(ino: INodeNo(0x91), name: "user.pdfs.probe", flags: 0x0, position: 0)
|
||||
WARN fuser: [Not Implemented] lseek(ino: INodeNo(0x93), fh: 0, offset: 0, whence: 3)
|
||||
```
|
||||
|
||||
**Cause:** `ProtonFs` does not implement these callbacks, so `fuser`'s default
|
||||
trait method answers `ENOSYS` *and logs at WARN*. The refusal is correct — the
|
||||
acceptance suite accepts every one of them as a clean refusal — but the logging
|
||||
is not conditional on anything the user did wrong.
|
||||
**Impact:** cosmetic, but these are not exotic calls: `git` probes symlinks,
|
||||
`cp -a` probes hard links, `tar` and `cp --sparse` probe `SEEK_HOLE`/`SEEK_DATA`
|
||||
(`whence: 3` above is `SEEK_DATA`), and any xattr-aware tool probes `setxattr`.
|
||||
Warnings that look like faults bury real ones and make both `--journal-check`
|
||||
and manual triage noisier.
|
||||
**Measured** across two full acceptance runs against one daemon lifetime (PID
|
||||
887441, mount never remounted):
|
||||
|
||||
| op | warnings | behaviour |
|
||||
|---|---|---|
|
||||
| `link` | 11 | repeats on every call |
|
||||
| `symlink` | 4 | repeats on every call |
|
||||
| `mknod` | 2 | repeats on every call |
|
||||
| `setxattr` | 1 | kernel cached the `ENOSYS`; never asked again |
|
||||
| `lseek` | 1 | cached |
|
||||
| `copy_file_range` | 1 | cached |
|
||||
| `fsyncdir` | 1 | cached |
|
||||
|
||||
So the FUSE kernel module remembers `ENOSYS` for the file-level operations and
|
||||
stops issuing them for the lifetime of the mount, but not for the namespace
|
||||
operations, which are re-issued forever.
|
||||
|
||||
`link` leads for a sharper reason than "tools probe it": git uses `link()` to
|
||||
place **every loose object**, falling back to `rename()` when it fails. In one
|
||||
workloads run, 9 of 10 `link` warnings carried a 38-hex-character name — the
|
||||
`.git/objects/ab/<38 hex>` layout — from just 3 commits over 3 files. The volume
|
||||
is therefore proportional to work done, not a fixed startup cost: a `git clone`
|
||||
of a real repository emits one multi-line warning per object. The lone `symlink`
|
||||
that is not the acceptance probe is git's `core.symlinks` detection at
|
||||
`git init` (a random name pointing at `testing`).
|
||||
|
||||
That moves this from cosmetic to a genuine operational problem: ordinary use of
|
||||
git inside a mount can bury every other log line.
|
||||
|
||||
**Fix (applied):** all seven callbacks are now implemented explicitly in
|
||||
`crates/pdfs-fuse/src/filesystem.rs`, each answering with **the same errno
|
||||
`fuser`'s default would** and nothing else — no WARN. Following `ioctl`
|
||||
(which already used this pattern and says why in its comment), the errno
|
||||
contract is unchanged, so the acceptance suite's clean-refusal assertions hold
|
||||
byte for byte:
|
||||
|
||||
| op | errno | why that one |
|
||||
|---|---|---|
|
||||
| `symlink`, `link` | `EPERM` | meaningful operation, unrepresentable on Drive; git falls back to `rename()` |
|
||||
| `mknod` | `ENOSYS` | no devices/FIFOs/sockets; regular files arrive via `create` |
|
||||
| `setxattr` | `ENOSYS` | surfaces as `EOPNOTSUPP`; reads stay implemented for the thumbnail interface |
|
||||
| `fsyncdir` | `ENOSYS` | kernel absorbs it and reports success; dir changes are already durable in SQLite |
|
||||
| `lseek`, `copy_file_range` | `ENOSYS` | **must stay** `ENOSYS` — it is what makes the kernel emulate `SEEK_HOLE`/`SEEK_DATA` and coreutils fall back to read/write instead of failing |
|
||||
|
||||
Only the three namespace ops were strictly required (the other four silence
|
||||
themselves once the kernel caches `ENOSYS`), but implementing all seven keeps the
|
||||
refusal explicit and documented at the call site rather than inherited.
|
||||
|
||||
**Verified:** full live acceptance run after the fix — 19 pass / 1 skip, the
|
||||
capability diff identical to the pre-fix run, and **0** `Not Implemented` lines
|
||||
in the journal across the whole run (was 17, 10 of them `link`).
|
||||
**Regression cover:** the acceptance suite's clean-refusal probes assert the
|
||||
errno contract; they pass unchanged.
|
||||
|
||||
## B74 — A drained create never reaches a sync-folder mount's inode space, so the file reads as empty
|
||||
|
||||
**Status:** Fixed in-tree 2026-07-26 — live-validated. Found 2026-07-26 by the
|
||||
expanded acceptance suite (`regression B70` case), then isolated to a broader
|
||||
trigger.
|
||||
**Impact:** on a *sync folder* mount (not `~/ProtonDrive`), a file whose create
|
||||
was still queued reads back as **0 bytes for as long as the daemon runs**, even
|
||||
though the bytes are on Drive intact. This is the write-to-temp-then-rename
|
||||
pattern used by every browser download, most editor saves, `rsync`, and `git`.
|
||||
|
||||
**Not data loss.** An earlier revision of this entry called it permanent data
|
||||
loss; that was wrong and is corrected here. The uploaded revision is complete and
|
||||
correct: for every "lost" file the node row carried the right `size` and
|
||||
`active_revision_state: "Active"`, and a daemon restart made all of them read
|
||||
back with the expected sha256. Nothing had to be recovered. The defect was
|
||||
entirely local and entirely in the read path.
|
||||
|
||||
**Reproduction** (on an on-demand mount, per file: write, close, rename, wait for
|
||||
`pdfs status` to report the queue drained, then read back):
|
||||
|
||||
| scenario | result |
|
||||
|---|---|
|
||||
| plain name, never renamed | content intact |
|
||||
| plain name, renamed after close | **0 bytes until restart** |
|
||||
| `.crdownload`, renamed after close | **0 bytes until restart** |
|
||||
| `.crdownload`, never renamed | content intact |
|
||||
| already-settled file, renamed | content intact |
|
||||
| any of the above, after a daemon restart | content intact |
|
||||
|
||||
The gap between `close()` and `rename()` does not matter — 0 s, 2 s and 20 s all
|
||||
behave the same. The boundary is not timing but state: the affected node is one
|
||||
whose **create op was still queued**, on a mount that is not the primary one.
|
||||
|
||||
**Root cause — one drain thread, many inode spaces.** The daemon mounts
|
||||
`~/ProtonDrive` plus one FUSE session per `ondemand` sync folder. Each of those
|
||||
is a `Core::fork_state` clone with its **own** `State` — its own `entries`,
|
||||
`by_uid`, `children`, `next_ino` — while sharing `db`, `cache`, `pending` and
|
||||
`client`. There is exactly **one** `pdfs-drain` thread in the process, owned by
|
||||
the primary mount, and it serves the whole shared `pending_op` table.
|
||||
|
||||
So when a create queued by a sync-folder mount lands, the drain calls
|
||||
`adopt_real_uid` — which looked only at `self.state`, the *primary* mount's inode
|
||||
space. The fork's entry keeps its `local~…` placeholder uid forever. And
|
||||
`Core::read_range` (`crates/pdfs-fuse/src/reads.rs:314`) short-circuits a local
|
||||
uid to an empty vec:
|
||||
|
||||
```rust
|
||||
if is_local_uid(uid) { return Ok(Vec::new()); }
|
||||
```
|
||||
|
||||
which is why the read is silent, instant, and zero-length. A restart rebuilds
|
||||
every state from the DB, where `remap_local_uid` had already written the real
|
||||
uid, so the file reads correctly from then on.
|
||||
|
||||
Confirmed live: `read handler uid=local~1785067300752-0 fsize=140000` three
|
||||
seconds after `pending create landed`, and at the same moment
|
||||
`adopt_real_uid … hit=None strays=[]` — the primary state had no entry for the
|
||||
uid at all. `/proc/<pid>/task/*/comm` showed 1 `pdfs-drain` against 8 fuser
|
||||
sessions.
|
||||
|
||||
**Fix:** `Core` gained a `states: Arc<Mutex<Vec<Weak<Mutex<State>>>>>` registry
|
||||
that every mount publishes itself into (`Core::register_state`, called from
|
||||
`mount.rs` for the primary and from `fork_state` for each fork), plus
|
||||
`Core::for_each_state`, which walks every live inode space one lock at a time.
|
||||
A uid is unique across mounts, so at most one state matches and the rest are
|
||||
no-ops. Five drain sites were converted: `adopt_real_uid`, `adopt_drained_name`,
|
||||
the conflict-copy `invalidate_listing`, the `active_writes` baseline rebase in
|
||||
`refresh_after_upload`, and that function's closing `intern`.
|
||||
|
||||
**It was completely silent.** No `ERROR`, no `WARN`, no failed op, no conflict
|
||||
copy, no retry — `pdfs status` reported the queue drained, because it had. Any
|
||||
diagnosis that relies on the journal or on queue state will not see this.
|
||||
|
||||
**Regression cover:** `scripts/fuse-acceptance.py`, case
|
||||
`regression B74: rename after close keeps its content` — the narrowest form
|
||||
(plain names, no overwrite, no transient suffix). It failed in two consecutive
|
||||
live runs before the fix and passes after it. The pre-existing B70 case failed on
|
||||
the same cause and now passes too.
|
||||
|
||||
**Remaining work.** `drain.rs` still has fork-blind *reads* (`node_place`,
|
||||
`root_uid`, `parent_is_gone`) and `lib.rs` has ~31 direct `state.lock()` sites;
|
||||
any of them reached from a background thread has the same shape of problem.
|
||||
Worth an audit, and worth a unit test over `for_each_state` so a future fork
|
||||
cannot silently drop out of the registry.
|
||||
|
||||
**Note on B70:** B70's own fix was never implicated. The `.crdownload` park/un-park
|
||||
behaves correctly in isolation; the empty read arrived with the rename, on the
|
||||
same path a plain temp file takes. B70's live validation was blocked on this and
|
||||
is now unblocked — its acceptance case passes.
|
||||
+95
-3
@@ -14,9 +14,12 @@ cargo clippy --workspace --all-targets -- -D warnings
|
||||
The account-free suite exercises the filesystem syscall contract against a
|
||||
temporary local directory. This validates the runner and covers creation/open
|
||||
flags, positioned and vectored I/O, truncation and sparse files, `mmap`,
|
||||
`sendfile`, durability barriers, namespace/error semantics, open-file lifetime,
|
||||
names and enumeration, and concurrent I/O. It never reads application state or
|
||||
contacts Proton:
|
||||
`sendfile`, `copy_file_range`, durability barriers, namespace/error semantics,
|
||||
`renameat2` flags, open-file lifetime, names and enumeration, readdir stability
|
||||
under concurrent mutation, metadata updates, extended attributes, allocation and
|
||||
punched holes, refusal semantics for unimplemented operations, concurrent I/O,
|
||||
and real application workloads. It never reads application state or contacts
|
||||
Proton:
|
||||
|
||||
```console
|
||||
scripts/fuse-acceptance.sh --offline-only
|
||||
@@ -34,6 +37,85 @@ Use a dedicated test account without irreplaceable data:
|
||||
scripts/fuse-acceptance.sh --live /mnt/on-demand-testmount
|
||||
```
|
||||
|
||||
### The mount is diffed against an ordinary filesystem
|
||||
|
||||
The local reference run is not only a self-test of the runner. Every case
|
||||
records the facts it established, and a live run is compared against those
|
||||
recordings, so a case fails when the mount *differs* from an ordinary
|
||||
filesystem — not only when it raises. The report names the exact key:
|
||||
|
||||
```
|
||||
[DIVERGENCE] live FUSE /mnt/testmount does not match the reference filesystem:
|
||||
! creation and open flags.truncated.size: live=6 reference=0
|
||||
```
|
||||
|
||||
Facts are recorded in one of two ways, chosen in the test body:
|
||||
|
||||
- **compared** — a claim about filesystem semantics. The mount must agree.
|
||||
- **noted** — context that legitimately differs: inode numbers, block counts,
|
||||
reported mode bits, and which optional syscalls exist at all. Reported under
|
||||
`[capabilities]`, never a failure.
|
||||
|
||||
Because the choice is made at each call site, the set of accepted divergences
|
||||
is visible in the test that accepts it. There is no separate allowlist to drift
|
||||
out of date. When adding a case, prefer `record`; reach for `note` only when an
|
||||
ordinary filesystem can do something a network filesystem genuinely cannot.
|
||||
|
||||
### Operations the filesystem does not implement
|
||||
|
||||
`git`, `rsync`, `cp -a`, `tar`, `df` and most installers probe for symlinks,
|
||||
hard links, FIFOs, device nodes, `statfs`, file locks, `copy_file_range`,
|
||||
`O_DIRECT` and `SEEK_HOLE` whether or not a filesystem offers them. The suite
|
||||
asserts that each either works or is *refused cleanly* — a recognised errno,
|
||||
never `EIO` and never a hang. A refusal that is merely unimplemented is fine;
|
||||
one that is dirty breaks software that never asked for the feature.
|
||||
|
||||
### Regressions from the bug ledger
|
||||
|
||||
Cases named `regression B<n>` reproduce a bug from `docs/BUGS.md` so it stays
|
||||
fixed. B69 (identical rewrite must not fork a `(sync-conflict …)` copy), B70 (an
|
||||
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.
|
||||
|
||||
B74 is currently **failing** — it is a live data-loss defect, not a regression
|
||||
guard, and it was found by exactly this mechanism: the B70 case failed, and
|
||||
narrowing it produced a smaller reproduction that has its own case now. Expect a
|
||||
red run against a real mount until it is fixed.
|
||||
|
||||
A regression case asserts against the *remote* outcome, so it must wait for the
|
||||
queue to drain before reading. Reading straight back through the mount proves
|
||||
nothing: the local attribute and content caches will happily return the bytes
|
||||
that were just written even when nothing reached Drive, which is precisely how
|
||||
B74 stayed invisible.
|
||||
|
||||
### Timeouts, reports, and hung mounts
|
||||
|
||||
A wedged FUSE operation cannot be interrupted from Python, so each case runs
|
||||
under a soft alarm (`--timeout`, default 180s) that reports the case and lets
|
||||
cleanup run, backed by a hard stop that dumps every thread's stack and exits.
|
||||
That backstop is the only way to learn *where* a mount hung. A timeout ends the
|
||||
run: later cases against a wedged mount produce noise, not information.
|
||||
|
||||
```console
|
||||
scripts/fuse-acceptance.sh --live /mnt/testmount \
|
||||
--timeout 300 --report-junit results.xml --budget 30
|
||||
```
|
||||
|
||||
`--report-json` also writes every recorded observation, which is what to attach
|
||||
to a bug report. `--budget SECONDS` flags cases that pass but got slower —
|
||||
B5 was a 186 ms-per-call regression that a pass/fail suite could not see.
|
||||
`--list` prints every case and which targets it runs against, and `--fail-fast`
|
||||
stops at the first failure.
|
||||
|
||||
`--journal-check` fails the run if `proton-drive.service` logged any error while
|
||||
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.
|
||||
|
||||
### Automated mode matrix
|
||||
|
||||
The managed live runner accepts two existing, empty, unmounted directories. It
|
||||
@@ -53,6 +135,16 @@ in `pdfs sync list`. Use `PDFS_ACCEPTANCE_SYNC_TIMEOUT` to extend transition
|
||||
timeouts and `PDFS_ACCEPTANCE_PDFS` to select a non-installed CLI binary, for
|
||||
example `target/debug/pdfs`.
|
||||
|
||||
### Durability drill
|
||||
|
||||
`--durability` restarts `proton-drive.service` in the middle of the suite and
|
||||
re-verifies the bytes written before it. `staging/` and `recovery/` can hold the
|
||||
only copy of a file whose upload has not finished, so this is the case that
|
||||
proves the queue is persistent rather than in-memory. It is opt-in because it
|
||||
interrupts a running daemon; `PDFS_ACCEPTANCE_UNIT` selects a different unit.
|
||||
|
||||
### Selecting cases
|
||||
|
||||
During development, `PDFS_ACCEPTANCE_ONLY` runs tests whose descriptive name
|
||||
contains the supplied text (for example `PDFS_ACCEPTANCE_ONLY=namespace`). A
|
||||
release acceptance run must leave it unset so the complete contract executes.
|
||||
|
||||
Reference in new issue
Block a user