mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(drain) failed attempts
This commit is contained in:
1 parent
481c3fe0c7
commit
4cd49168c4
4 files changed
+766
-56
No files matched your search
+190
-36
@@ -41,9 +41,9 @@ use super::state::Intervals;
|
|||||||
use super::transfers::CountingReader;
|
use super::transfers::CountingReader;
|
||||||
use super::{
|
use super::{
|
||||||
Core, DRAIN_BACKOFF_MAX, DRAIN_BACKOFF_MIN, DRAIN_IDLE_POLL, DRAIN_REVISION_DEBOUNCE,
|
Core, DRAIN_BACKOFF_MAX, DRAIN_BACKOFF_MIN, DRAIN_IDLE_POLL, DRAIN_REVISION_DEBOUNCE,
|
||||||
DRAIN_REVISION_DEBOUNCE_MAX, ROOT_INO, UPLOAD_TIME_MEMORY, conflict_name, is_already_exists,
|
DRAIN_REVISION_DEBOUNCE_MAX, ROOT_INO, UPLOAD_TIME_MEMORY, WriteAuthority, conflict_name,
|
||||||
is_gone, is_local_uid_str, media_type_for, node_revision_id, node_size, now_millis, now_secs,
|
is_already_exists, is_gone, is_local_uid_str, media_type_for, node_revision_id, node_size,
|
||||||
parse_node_uid,
|
now_millis, now_secs, parse_node_uid,
|
||||||
};
|
};
|
||||||
|
|
||||||
const DRAIN_ACCESS_RECHECK: Duration = Duration::from_secs(5);
|
const DRAIN_ACCESS_RECHECK: Duration = Duration::from_secs(5);
|
||||||
@@ -53,14 +53,18 @@ const DRAIN_ACCESS_RECHECK: Duration = Duration::from_secs(5);
|
|||||||
///
|
///
|
||||||
/// An access deferral costs no attempt and records no error, which is right for
|
/// An access deferral costs no attempt and records no error, which is right for
|
||||||
/// the case it was written for: a share downgraded while the queue was moving,
|
/// the case it was written for: a share downgraded while the queue was moving,
|
||||||
/// undone a moment later. It is wrong for anything that does not clear —
|
/// undone a moment later. It is wrong for anything that does not clear. Until
|
||||||
/// notably a node the local tree no longer has a row for, which
|
/// this window existed such an op was re-deferred every
|
||||||
/// [`Db::effective_node_access`] cannot distinguish from a revoked permission
|
/// [`DRAIN_ACCESS_RECHECK`] indefinitely with `attempts` at zero, `last_error`
|
||||||
/// and so reports as `EACCES` forever. Until this window existed such an op was
|
/// null and only a `debug!` line to show for it, so `pdfs status` counted it
|
||||||
/// re-deferred every [`DRAIN_ACCESS_RECHECK`] indefinitely with `attempts` at
|
/// among ordinary queued uploads and nothing ever said otherwise.
|
||||||
/// zero, `last_error` null and only a `debug!` line to show for it, so
|
///
|
||||||
/// `pdfs status` counted it among ordinary queued uploads and nothing ever said
|
/// The condition that produced that in practice — a node absent from the local
|
||||||
/// otherwise.
|
/// tree, which [`Db::effective_node_access`] used to report indistinguishably
|
||||||
|
/// from a revoked permission — is now answered directly by
|
||||||
|
/// [`Core::resolve_unknown_authority`]. This window remains the backstop for
|
||||||
|
/// every deferral that does not resolve, including a refetch that keeps not
|
||||||
|
/// helping.
|
||||||
///
|
///
|
||||||
/// Five minutes is far longer than a permission change takes to settle and far
|
/// Five minutes is far longer than a permission change takes to settle and far
|
||||||
/// shorter than a user's patience with bytes that are not moving. Past it each
|
/// shorter than a user's patience with bytes that are not moving. Past it each
|
||||||
@@ -116,10 +120,52 @@ fn adaptive_debounce(measured: Option<Duration>) -> Duration {
|
|||||||
.clamp(DRAIN_REVISION_DEBOUNCE, DRAIN_REVISION_DEBOUNCE_MAX)
|
.clamp(DRAIN_REVISION_DEBOUNCE, DRAIN_REVISION_DEBOUNCE_MAX)
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||||
enum DrainDisposition {
|
enum DrainDisposition {
|
||||||
Applied,
|
Applied,
|
||||||
AccessDeferred,
|
AccessDeferred,
|
||||||
|
/// An authority this op needs is missing from the local tree, so no access
|
||||||
|
/// answer exists for it yet. Carries the uid to ask the remote about.
|
||||||
|
AuthorityUnknown(NodeUid),
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Why an op went back on the clock without consuming an attempt. Only affects
|
||||||
|
/// what the user is told — every variant is deferred and reported identically.
|
||||||
|
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||||
|
enum AccessDeferral {
|
||||||
|
/// An authority the tree knows about refuses the write.
|
||||||
|
NotWritable,
|
||||||
|
/// An authority missing from the tree was just re-read from the remote and
|
||||||
|
/// re-interned; the access check gets another look on the next pass.
|
||||||
|
AuthorityRefetched,
|
||||||
|
/// An authority missing from the tree could not be asked about.
|
||||||
|
RemoteUnreachable,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl AccessDeferral {
|
||||||
|
fn log_reason(self) -> &'static str {
|
||||||
|
match self {
|
||||||
|
Self::NotWritable => "the node is not writable",
|
||||||
|
Self::AuthorityRefetched => "the node was missing locally and has been re-read",
|
||||||
|
Self::RemoteUnreachable => "the node is missing locally and the remote is unreachable",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Phrased to complete "<reason> for 300s." in a recorded `last_error`, so
|
||||||
|
/// the text the user sees names the actual condition rather than the
|
||||||
|
/// permission question it used to be flattened into.
|
||||||
|
fn failure_reason(self) -> &'static str {
|
||||||
|
match self {
|
||||||
|
Self::NotWritable => "the share has not been writable",
|
||||||
|
Self::AuthorityRefetched => {
|
||||||
|
"the node has been missing from the local tree, and re-reading it from the remote \
|
||||||
|
has not restored it"
|
||||||
|
}
|
||||||
|
Self::RemoteUnreachable => {
|
||||||
|
"the node has been missing from the local tree and the remote has been unreachable"
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn pending_op_authorities(op: &PendingOp) -> Result<Vec<NodeUid>, Box<dyn std::error::Error>> {
|
fn pending_op_authorities(op: &PendingOp) -> Result<Vec<NodeUid>, Box<dyn std::error::Error>> {
|
||||||
@@ -162,7 +208,7 @@ fn pending_op_authorities(op: &PendingOp) -> Result<Vec<NodeUid>, Box<dyn std::e
|
|||||||
|
|
||||||
fn run_authorized_drain(
|
fn run_authorized_drain(
|
||||||
op: &PendingOp,
|
op: &PendingOp,
|
||||||
mut require: impl FnMut(&NodeUid) -> Result<(), fuser::Errno>,
|
mut authority_of: impl FnMut(&NodeUid) -> WriteAuthority,
|
||||||
apply: impl FnOnce() -> Result<(), Box<dyn std::error::Error>>,
|
apply: impl FnOnce() -> Result<(), Box<dyn std::error::Error>>,
|
||||||
) -> Result<DrainDisposition, Box<dyn std::error::Error>> {
|
) -> Result<DrainDisposition, Box<dyn std::error::Error>> {
|
||||||
// This check is the admission/linearization point. A downgrade that lands
|
// This check is the admission/linearization point. A downgrade that lands
|
||||||
@@ -170,15 +216,14 @@ fn run_authorized_drain(
|
|||||||
// attempt. Every later queued operation rechecks. No permission lock is
|
// attempt. Every later queued operation rechecks. No permission lock is
|
||||||
// held across remote latency.
|
// held across remote latency.
|
||||||
for authority in pending_op_authorities(op)? {
|
for authority in pending_op_authorities(op)? {
|
||||||
match require(&authority) {
|
match authority_of(&authority) {
|
||||||
Ok(()) => {}
|
WriteAuthority::Writable => {}
|
||||||
Err(error) if error.code() == libc::EACCES => {
|
WriteAuthority::Denied => return Ok(DrainDisposition::AccessDeferred),
|
||||||
return Ok(DrainDisposition::AccessDeferred);
|
// Nothing in the tree can answer for this node. Deferring on it
|
||||||
}
|
// waits for a permission change that has no reason to come, so the
|
||||||
Err(error) => {
|
// caller asks the remote instead (B83).
|
||||||
return Err(
|
WriteAuthority::Unknown => {
|
||||||
format!("drain access check failed with errno {}", error.code()).into(),
|
return Ok(DrainDisposition::AuthorityUnknown(authority));
|
||||||
);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -254,11 +299,15 @@ impl Core {
|
|||||||
};
|
};
|
||||||
let outcome = run_authorized_drain(
|
let outcome = run_authorized_drain(
|
||||||
&op,
|
&op,
|
||||||
|uid| self.require_uid_writable(uid),
|
|uid| self.uid_write_authority(uid),
|
||||||
|| self.drain_op(&op),
|
|| self.drain_op(&op),
|
||||||
);
|
);
|
||||||
if matches!(outcome, Ok(DrainDisposition::AccessDeferred)) {
|
if matches!(outcome, Ok(DrainDisposition::AccessDeferred)) {
|
||||||
self.defer_for_access(&op);
|
self.defer_for_access(&op, AccessDeferral::NotWritable);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
if let Ok(DrainDisposition::AuthorityUnknown(ref authority)) = outcome {
|
||||||
|
self.resolve_unknown_authority(&op, authority);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
// The access check let this attempt through, so any earlier run of
|
// The access check let this attempt through, so any earlier run of
|
||||||
@@ -313,7 +362,7 @@ impl Core {
|
|||||||
///
|
///
|
||||||
/// [`Db::record_op_failure`]: pdfs_core::db::Db::record_op_failure
|
/// [`Db::record_op_failure`]: pdfs_core::db::Db::record_op_failure
|
||||||
/// [`FAILING_ATTEMPTS`]: pdfs_core::db::FAILING_ATTEMPTS
|
/// [`FAILING_ATTEMPTS`]: pdfs_core::db::FAILING_ATTEMPTS
|
||||||
fn defer_for_access(&self, op: &PendingOp) {
|
fn defer_for_access(&self, op: &PendingOp, reason: AccessDeferral) {
|
||||||
let now = now_millis();
|
let now = now_millis();
|
||||||
let next_attempt_at = now + DRAIN_ACCESS_RECHECK.as_millis() as i64;
|
let next_attempt_at = now + DRAIN_ACCESS_RECHECK.as_millis() as i64;
|
||||||
let since = match self.db.defer_op_for_access(op.id, now, next_attempt_at) {
|
let since = match self.db.defer_op_for_access(op.id, now, next_attempt_at) {
|
||||||
@@ -337,7 +386,8 @@ impl Core {
|
|||||||
warn!(
|
warn!(
|
||||||
uid = %op.uid,
|
uid = %op.uid,
|
||||||
kind = %op.kind,
|
kind = %op.kind,
|
||||||
"pending operation deferred: its node is not writable"
|
reason = reason.log_reason(),
|
||||||
|
"pending operation deferred"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
if blocked >= DRAIN_ACCESS_DEFER_LIMIT {
|
if blocked >= DRAIN_ACCESS_DEFER_LIMIT {
|
||||||
@@ -346,8 +396,8 @@ impl Core {
|
|||||||
.saturating_mul(1u32 << attempts.min(6))
|
.saturating_mul(1u32 << attempts.min(6))
|
||||||
.min(DRAIN_BACKOFF_MAX);
|
.min(DRAIN_BACKOFF_MAX);
|
||||||
let error = format!(
|
let error = format!(
|
||||||
"not writable locally for {}s; the node is missing from the local tree or the \
|
"{} for {}s. The staged bytes are kept.",
|
||||||
share is no longer writable. The staged bytes are kept.",
|
reason.failure_reason(),
|
||||||
blocked.as_secs()
|
blocked.as_secs()
|
||||||
);
|
);
|
||||||
warn!(uid = %op.uid, attempts, blocked_secs = blocked.as_secs(),
|
warn!(uid = %op.uid, attempts, blocked_secs = blocked.as_secs(),
|
||||||
@@ -363,12 +413,72 @@ impl Core {
|
|||||||
uid = %op.uid,
|
uid = %op.uid,
|
||||||
next_attempt_at,
|
next_attempt_at,
|
||||||
blocked_secs = blocked.as_secs(),
|
blocked_secs = blocked.as_secs(),
|
||||||
|
reason = reason.log_reason(),
|
||||||
"pending operation deferred until access is writable"
|
"pending operation deferred until access is writable"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
self.release_claim(op);
|
self.release_claim(op);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Answer an authority the local tree has no row for by asking the remote,
|
||||||
|
/// rather than deferring on a permission question nobody asked (B83).
|
||||||
|
///
|
||||||
|
/// Three outcomes, and the point of the whole exercise is that they are
|
||||||
|
/// three:
|
||||||
|
///
|
||||||
|
/// * the node is still there — the tree simply lost it (a pruned subtree, a
|
||||||
|
/// mount whose registration went away). Re-intern it and the next pass has
|
||||||
|
/// a real access answer to work with.
|
||||||
|
/// * the node is gone remotely. That is a permanent, *known* condition, so
|
||||||
|
/// it is recorded as a failure immediately instead of waiting out
|
||||||
|
/// [`DRAIN_ACCESS_DEFER_LIMIT`] to say so. The row and its staged blob are
|
||||||
|
/// kept, as on every other failure path — the bytes are still the user's,
|
||||||
|
/// and `pdfs recover` is how they get them back.
|
||||||
|
/// * we could not ask. Ordinary deferral; the network is the network.
|
||||||
|
fn resolve_unknown_authority(&self, op: &PendingOp, authority: &NodeUid) {
|
||||||
|
match self.fetch_node_remote(authority) {
|
||||||
|
Ok(Some(node)) => {
|
||||||
|
if let Err(error) = self.db.upsert_node(&node) {
|
||||||
|
error!(uid = %op.uid, %authority, %error,
|
||||||
|
"re-interning a queued operation's missing authority failed");
|
||||||
|
} else {
|
||||||
|
info!(uid = %op.uid, %authority,
|
||||||
|
"re-interned an authority missing from the local tree; retrying");
|
||||||
|
}
|
||||||
|
// Deferred, not applied: the re-read has to go through the same
|
||||||
|
// access check as everything else, on the next pass. The window
|
||||||
|
// keeps running, so a refetch that never actually helps still
|
||||||
|
// gets reported instead of looping in silence.
|
||||||
|
self.defer_for_access(op, AccessDeferral::AuthorityRefetched);
|
||||||
|
}
|
||||||
|
Ok(None) => {
|
||||||
|
let attempts = op.attempts + 1;
|
||||||
|
let backoff = DRAIN_BACKOFF_MIN
|
||||||
|
.saturating_mul(1u32 << attempts.min(6))
|
||||||
|
.min(DRAIN_BACKOFF_MAX);
|
||||||
|
let error = format!(
|
||||||
|
"{authority} no longer exists remotely, so this operation has nothing to \
|
||||||
|
apply to. The staged bytes are kept."
|
||||||
|
);
|
||||||
|
warn!(uid = %op.uid, %authority, attempts,
|
||||||
|
"a queued operation's authority is gone remotely; reporting it");
|
||||||
|
if let Err(error) = self.db.record_op_failure(
|
||||||
|
op.id,
|
||||||
|
&error,
|
||||||
|
now_millis() + backoff.as_millis() as i64,
|
||||||
|
) {
|
||||||
|
error!(uid = %op.uid, %error, "recording a missing-authority failure failed");
|
||||||
|
}
|
||||||
|
self.release_claim(op);
|
||||||
|
}
|
||||||
|
Err(error) => {
|
||||||
|
debug!(uid = %op.uid, %authority, %error,
|
||||||
|
"could not ask the remote about a missing authority");
|
||||||
|
self.defer_for_access(op, AccessDeferral::RemoteUnreachable);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Hand a claimed op back to the queue, logging rather than propagating —
|
/// Hand a claimed op back to the queue, logging rather than propagating —
|
||||||
/// there is no caller left to propagate to, and the drain must not stop.
|
/// there is no caller left to propagate to, and the drain must not stop.
|
||||||
fn release_claim(&self, op: &PendingOp) {
|
fn release_claim(&self, op: &PendingOp) {
|
||||||
@@ -1512,12 +1622,10 @@ mod tests {
|
|||||||
let applied = Cell::new(false);
|
let applied = Cell::new(false);
|
||||||
let disposition = run_authorized_drain(
|
let disposition = run_authorized_drain(
|
||||||
&op,
|
&op,
|
||||||
|uid| {
|
|uid| match db.effective_node_access(uid).unwrap() {
|
||||||
db.effective_node_access(uid)
|
Some(access) if access.writable() => WriteAuthority::Writable,
|
||||||
.unwrap()
|
Some(_) => WriteAuthority::Denied,
|
||||||
.filter(|access| access.writable())
|
None => WriteAuthority::Unknown,
|
||||||
.map(|_| ())
|
|
||||||
.ok_or(fuser::Errno::EACCES)
|
|
||||||
},
|
},
|
||||||
|| {
|
|| {
|
||||||
applied.set(true);
|
applied.set(true);
|
||||||
@@ -1557,7 +1665,7 @@ mod tests {
|
|||||||
|
|
||||||
let disposition = run_authorized_drain(
|
let disposition = run_authorized_drain(
|
||||||
&op,
|
&op,
|
||||||
|_| Err(fuser::Errno::EACCES),
|
|_| WriteAuthority::Denied,
|
||||||
|| {
|
|| {
|
||||||
applied.set(true);
|
applied.set(true);
|
||||||
Ok(())
|
Ok(())
|
||||||
@@ -1676,13 +1784,59 @@ mod tests {
|
|||||||
let _ = std::fs::remove_dir_all(dir);
|
let _ = std::fs::remove_dir_all(dir);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn an_authority_the_tree_does_not_have_is_reported_as_unknown_not_deferred() {
|
||||||
|
// The B83 distinction, at the point it is made: a uid with no row must
|
||||||
|
// not come back as `AccessDeferred`, because deferring waits on a
|
||||||
|
// permission change that nothing is going to make. The caller answers
|
||||||
|
// an `AuthorityUnknown` by asking the remote instead.
|
||||||
|
let op = pending(OP_REVISION);
|
||||||
|
let applied = Cell::new(false);
|
||||||
|
let disposition = run_authorized_drain(
|
||||||
|
&op,
|
||||||
|
|_| WriteAuthority::Unknown,
|
||||||
|
|| {
|
||||||
|
applied.set(true);
|
||||||
|
Ok(())
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(
|
||||||
|
disposition,
|
||||||
|
DrainDisposition::AuthorityUnknown(parse_node_uid(&op.uid).unwrap())
|
||||||
|
);
|
||||||
|
assert!(!applied.get(), "an unresolved authority never applies");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_missing_parent_authority_names_the_parent_not_the_node() {
|
||||||
|
// A create's authority is its parent, so that is the uid the remote has
|
||||||
|
// to be asked about — reporting the node instead would refetch the
|
||||||
|
// thing that does not exist yet.
|
||||||
|
let mut op = pending(OP_CREATE);
|
||||||
|
let parent = folder_node("vanished-parent", None);
|
||||||
|
op.parent_uid = Some(parent.uid.to_string());
|
||||||
|
let disposition =
|
||||||
|
run_authorized_drain(&op, |_| WriteAuthority::Unknown, || Ok(())).unwrap();
|
||||||
|
assert_eq!(
|
||||||
|
disposition,
|
||||||
|
DrainDisposition::AuthorityUnknown(parent.uid.clone())
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn owner_and_editor_drain_authorities_apply_normally() {
|
fn owner_and_editor_drain_authorities_apply_normally() {
|
||||||
for access in [Access::Owner, Access::Editor] {
|
for access in [Access::Owner, Access::Editor] {
|
||||||
let applied = Cell::new(false);
|
let applied = Cell::new(false);
|
||||||
let disposition = run_authorized_drain(
|
let disposition = run_authorized_drain(
|
||||||
&pending(OP_REVISION),
|
&pending(OP_REVISION),
|
||||||
|_| access.writable().then_some(()).ok_or(fuser::Errno::EACCES),
|
|_| {
|
||||||
|
if access.writable() {
|
||||||
|
WriteAuthority::Writable
|
||||||
|
} else {
|
||||||
|
WriteAuthority::Denied
|
||||||
|
}
|
||||||
|
},
|
||||||
|| {
|
|| {
|
||||||
applied.set(true);
|
applied.set(true);
|
||||||
Ok(())
|
Ok(())
|
||||||
|
|||||||
@@ -973,6 +973,27 @@ impl Core {
|
|||||||
/// downgrade cannot be hidden by stale live state, and a live downgrade
|
/// downgrade cannot be hidden by stale live state, and a live downgrade
|
||||||
/// cannot be hidden by a restored DB row. Missing UIDs fail closed.
|
/// cannot be hidden by a restored DB row. Missing UIDs fail closed.
|
||||||
fn require_uid_writable(&self, uid: &NodeUid) -> Result<(), Errno> {
|
fn require_uid_writable(&self, uid: &NodeUid) -> Result<(), Errno> {
|
||||||
|
match self.uid_write_authority(uid) {
|
||||||
|
WriteAuthority::Writable => Ok(()),
|
||||||
|
// A uid nobody can speak for is refused here just as a denied one
|
||||||
|
// is: a syscall is a live request against a live tree, and a stale
|
||||||
|
// handle must not be admitted because the tree forgot the node.
|
||||||
|
// The drain, which replays intent recorded long ago, is the one
|
||||||
|
// caller that has to tell the two apart — see
|
||||||
|
// [`Core::uid_write_authority`].
|
||||||
|
WriteAuthority::Denied | WriteAuthority::Unknown => Err(Errno::EACCES),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The full answer behind [`Core::require_uid_writable`]: whether every
|
||||||
|
/// authority agrees this node is writable, refuses it, or has never heard
|
||||||
|
/// of it.
|
||||||
|
///
|
||||||
|
/// The third case is the one worth naming. A node absent from the local
|
||||||
|
/// tree yields no access row, and collapsing that into "denied" told the
|
||||||
|
/// drain to wait for a permission change that was never coming, because
|
||||||
|
/// nothing about the node's permissions was ever the problem (B83).
|
||||||
|
pub(crate) fn uid_write_authority(&self, uid: &NodeUid) -> WriteAuthority {
|
||||||
let mut live_access = Vec::new();
|
let mut live_access = Vec::new();
|
||||||
{
|
{
|
||||||
let state = self.state();
|
let state = self.state();
|
||||||
@@ -988,22 +1009,38 @@ impl Core {
|
|||||||
live_access.push(access);
|
live_access.push(access);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
require_uid_access(&self.db, uid, &live_access)
|
uid_write_authority(&self.db, uid, &live_access)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn require_uid_access(db: &Db, uid: &NodeUid, live_access: &[Access]) -> Result<(), Errno> {
|
/// Whether a node may be written, as agreed by every authority that has an
|
||||||
|
/// opinion — or the fact that none of them does.
|
||||||
|
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||||
|
pub(crate) enum WriteAuthority {
|
||||||
|
/// The persisted tree and every resident mount agree: writable.
|
||||||
|
Writable,
|
||||||
|
/// At least one authority refuses it, or could not be consulted. Fails
|
||||||
|
/// closed, so a lookup error lands here.
|
||||||
|
Denied,
|
||||||
|
/// The node is not in the local tree, so there is no authority to consult.
|
||||||
|
/// Not the same as a refusal, and callers that can ask the remote should.
|
||||||
|
Unknown,
|
||||||
|
}
|
||||||
|
|
||||||
|
fn uid_write_authority(db: &Db, uid: &NodeUid, live_access: &[Access]) -> WriteAuthority {
|
||||||
let persisted = match db.effective_node_access(uid) {
|
let persisted = match db.effective_node_access(uid) {
|
||||||
Ok(Some(access)) => access,
|
Ok(Some(access)) => access,
|
||||||
Ok(None) => return Err(Errno::EACCES),
|
Ok(None) => return WriteAuthority::Unknown,
|
||||||
Err(error) => {
|
Err(error) => {
|
||||||
error!(%uid, %error, "db effective access lookup failed");
|
error!(%uid, %error, "db effective access lookup failed");
|
||||||
return Err(Errno::EACCES);
|
return WriteAuthority::Denied;
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
(persisted.writable() && live_access.iter().all(|access| access.writable()))
|
if persisted.writable() && live_access.iter().all(|access| access.writable()) {
|
||||||
.then_some(())
|
WriteAuthority::Writable
|
||||||
.ok_or(Errno::EACCES)
|
} else {
|
||||||
|
WriteAuthority::Denied
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// One kernel notification, recorded while the State lock is held so it can be
|
/// One kernel notification, recorded while the State lock is held so it can be
|
||||||
@@ -5754,9 +5791,19 @@ mod tests {
|
|||||||
preserve_on_access_denied, publish_virtual_root_in_listing,
|
preserve_on_access_denied, publish_virtual_root_in_listing,
|
||||||
reconcile_virtual_root_in_listing, release_can_discard_unlinked,
|
reconcile_virtual_root_in_listing, release_can_discard_unlinked,
|
||||||
release_must_retain_queued_trash, release_unlinked_entry, rename_needs_queue,
|
release_must_retain_queued_trash, release_unlinked_entry, rename_needs_queue,
|
||||||
require_node_parent_access, require_rename_access, require_uid_access,
|
require_node_parent_access, require_rename_access, resolve_anywhere_with,
|
||||||
resolve_anywhere_with, shared_with_me_uid, take_self_change, virtual_node,
|
shared_with_me_uid, take_self_change, uid_write_authority, virtual_node,
|
||||||
};
|
};
|
||||||
|
use super::{Db, WriteAuthority};
|
||||||
|
|
||||||
|
/// The collapse [`Core::require_uid_writable`] performs, without a `Core`:
|
||||||
|
/// every non-writable authority is one `EACCES` on the syscall path.
|
||||||
|
fn require_uid_access(db: &Db, uid: &NodeUid, live: &[Access]) -> Result<(), Errno> {
|
||||||
|
match uid_write_authority(db, uid, live) {
|
||||||
|
WriteAuthority::Writable => Ok(()),
|
||||||
|
WriteAuthority::Denied | WriteAuthority::Unknown => Err(Errno::EACCES),
|
||||||
|
}
|
||||||
|
}
|
||||||
use crate::filesystem::access_allowed;
|
use crate::filesystem::access_allowed;
|
||||||
use pdfs_core::cache::{Baseline, StagedWrite};
|
use pdfs_core::cache::{Baseline, StagedWrite};
|
||||||
use pdfs_core::db::{OP_REVISION, OP_TRASH, PendingOp};
|
use pdfs_core::db::{OP_REVISION, OP_TRASH, PendingOp};
|
||||||
@@ -6763,6 +6810,19 @@ mod tests {
|
|||||||
require_uid_access(&db, &stale, &[]).unwrap_err().code(),
|
require_uid_access(&db, &stale, &[]).unwrap_err().code(),
|
||||||
libc::EACCES
|
libc::EACCES
|
||||||
);
|
);
|
||||||
|
// Same refusal on the syscall path, but a distinguishable one
|
||||||
|
// underneath: the drain asks the remote about `Unknown` rather than
|
||||||
|
// waiting on a permission change (B83).
|
||||||
|
assert_eq!(
|
||||||
|
uid_write_authority(&db, &stale, &[]),
|
||||||
|
WriteAuthority::Unknown,
|
||||||
|
"a uid the tree has no row for is unknown, not denied"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
uid_write_authority(&db, &uid, &[]),
|
||||||
|
WriteAuthority::Denied,
|
||||||
|
"a uid the tree knows and refuses is denied"
|
||||||
|
);
|
||||||
assert_eq!(db.pending_ops().unwrap().len(), before);
|
assert_eq!(db.pending_ops().unwrap().len(), before);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+33
-11
@@ -14,9 +14,10 @@ Conventions:
|
|||||||
|
|
||||||
## B83 — An access-deferred op retries forever and is reported as an ordinary queued upload
|
## B83 — An access-deferred op retries forever and is reported as an ordinary queued upload
|
||||||
|
|
||||||
**Status:** Partly fixed (unverified) 2026-08-16 — the reporting half is in and
|
**Status:** Fixed (partly verified) 2026-08-16 — both halves are in and
|
||||||
unit-tested; the cause (an unknown node reading as `EACCES`) is still open, see
|
unit-tested. The reporting half was confirmed against the live 28-op backlog on
|
||||||
"Still open" below.
|
1.8.0 (schema migrated to v27, all 28 rows window-stamped, `WARN` lines per op);
|
||||||
|
the resolution half has not yet been observed draining that backlog.
|
||||||
**Found:** 2026-08-16, running `scripts/fuse-acceptance.sh --live` on a
|
**Found:** 2026-08-16, running `scripts/fuse-acceptance.sh --live` on a
|
||||||
throwaway CLI-made mount. Four regression cases (B69, B70, B74, B79) failed with
|
throwaway CLI-made mount. Four regression cases (B69, B70, B74, B79) failed with
|
||||||
`TimeoutError: daemon mutation queue did not drain` — the suite's
|
`TimeoutError: daemon mutation queue did not drain` — the suite's
|
||||||
@@ -25,7 +26,7 @@ been sitting at 28 for 30 days on the development machine. `pdfs status` called
|
|||||||
them 28 queued uploads holding 18.4 GiB; `pdfs transfers` showed nothing;
|
them 28 queued uploads holding 18.4 GiB; `pdfs transfers` showed nothing;
|
||||||
`failing_ops` and `parked_uploads` were both 0.
|
`failing_ops` and `parked_uploads` were both 0.
|
||||||
**Where:** `run_pending_drain` / `run_authorized_drain`
|
**Where:** `run_pending_drain` / `run_authorized_drain`
|
||||||
(`crates/pdfs-fuse/src/drain.rs`), `require_uid_access`
|
(`crates/pdfs-fuse/src/drain.rs`), `require_uid_writable`
|
||||||
(`crates/pdfs-fuse/src/lib.rs`), `Db::effective_node_access`
|
(`crates/pdfs-fuse/src/lib.rs`), `Db::effective_node_access`
|
||||||
(`crates/pdfs-core/src/db/share_access.rs`),
|
(`crates/pdfs-core/src/db/share_access.rs`),
|
||||||
`Db::defer_op_without_attempt` (`crates/pdfs-core/src/db/ops.rs`).
|
`Db::defer_op_without_attempt` (`crates/pdfs-core/src/db/ops.rs`).
|
||||||
@@ -48,7 +49,26 @@ select (select count(*) from pending_op p
|
|||||||
-- 0 | 28
|
-- 0 | 28
|
||||||
```
|
```
|
||||||
|
|
||||||
**Fix (this half).** Schema v27 adds `pending_op.access_deferred_since`.
|
**Fix, part 1 — stop guessing.** `WriteAuthority { Writable, Denied, Unknown }`
|
||||||
|
replaces the `Result<(), Errno>` the drain's access check used to return
|
||||||
|
(`Core::uid_write_authority`). `require_uid_writable` still collapses both
|
||||||
|
refusals to `EACCES`, which is right for a syscall: a stale handle must not be
|
||||||
|
admitted because the tree forgot the node. The drain does not collapse them —
|
||||||
|
`Unknown` becomes `DrainDisposition::AuthorityUnknown(uid)`, and
|
||||||
|
`Core::resolve_unknown_authority` asks the remote:
|
||||||
|
|
||||||
|
- node still there → `upsert_node` re-interns it and the op is deferred for one
|
||||||
|
recheck, so the next pass gets a real access answer;
|
||||||
|
- node gone (`Ok(None)` / `is_gone`) → `record_op_failure` **immediately**, with
|
||||||
|
a message naming the vanished uid, rather than waiting out the 5-minute window
|
||||||
|
to say something vaguer;
|
||||||
|
- could not ask → ordinary deferral.
|
||||||
|
|
||||||
|
The authority asked about is the op's own (`pending_op_authorities`), so a
|
||||||
|
`create`/`mkdir` refetches its *parent*, not the node it has not made yet. The
|
||||||
|
row and its staged blob survive every branch.
|
||||||
|
|
||||||
|
**Fix, part 2 — say something.** Schema v27 adds `pending_op.access_deferred_since`.
|
||||||
`Db::defer_op_for_access` stamps it on the first deferral of a run and returns
|
`Db::defer_op_for_access` stamps it on the first deferral of a run and returns
|
||||||
the stamp on every one; `Core::defer_for_access` warns on the first deferral and,
|
the stamp on every one; `Core::defer_for_access` warns on the first deferral and,
|
||||||
once the run passes `DRAIN_ACCESS_DEFER_LIMIT` (5 min), reports it through
|
once the run passes `DRAIN_ACCESS_DEFER_LIMIT` (5 min), reports it through
|
||||||
@@ -59,12 +79,14 @@ access check admits clears it too (`clear_op_access_deferral`). The row and its
|
|||||||
staged blob are untouched on every path — this changes only what the user can
|
staged blob are untouched on every path — this changes only what the user can
|
||||||
see.
|
see.
|
||||||
|
|
||||||
**Still open:** the conflation itself. A revision op whose node has left the
|
The deferral window stays as the backstop for everything that does not resolve,
|
||||||
local tree should refetch the node and either proceed or fail on a real remote
|
including a refetch that keeps not helping, and `AccessDeferral` now carries the
|
||||||
answer (404/trashed), not be deferred on a local guess. Same trap applies to
|
reason into both the log line and the recorded `last_error`.
|
||||||
`create`/`mkdir`, whose authority is the parent uid. Also unaddressed: the
|
|
||||||
acceptance runner waits on the *global* queue, so any unrelated stuck op fails
|
**Still open:** the acceptance runner waits on the *global* queue
|
||||||
those four cases on a machine that has one.
|
(`ManagedSyncPair.wait_for_queue`), so any unrelated stuck op fails those four
|
||||||
|
cases on a machine that has one — that is what turned this backlog into four
|
||||||
|
apparent FUSE regressions.
|
||||||
|
|
||||||
## B82 — A `parent_uid` cycle hangs the daemon on any short search
|
## B82 — A `parent_uid` cycle hangs the daemon on any short search
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,474 @@
|
|||||||
|
# Architecture audit — performance, locking, robustness (2026-08-15)
|
||||||
|
|
||||||
|
Scope: `pdfs-fuse`, `pdfs-core`, and `proton-sdk-rs` as consumed by the mount.
|
||||||
|
Method: five parallel deep-read passes (locks, read path, write path, DB/IPC, SDK
|
||||||
|
transport), each finding re-verified against the tree before landing here.
|
||||||
|
|
||||||
|
Line numbers are as of commit `b37ab78`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Implementation status (2026-08-15, uncommitted working tree)
|
||||||
|
|
||||||
|
| Landed | Item |
|
||||||
|
|---|---|
|
||||||
|
| ✅ | DB5 depth-capped `is_pinned`, with a cycle regression test |
|
||||||
|
| ✅ | DB6 daemon `set_write_timeout`; every client-supplied `limit` clamped |
|
||||||
|
| ✅ | T7 `"http2"` added to the SDK's reqwest features (`h2` now in its lock file) |
|
||||||
|
| ✅ | D1 sweep no longer discards queued ops after the trash round trip |
|
||||||
|
| ✅ | D2 `path == blob` guard shared by all three pending-entry drops (`Core::release_pending`) |
|
||||||
|
| ✅ | D3 `ENOSPC` propagated from `write`/`fsync`/staging, `emergency_evict` wired up, scratch marked durable before any staging error |
|
||||||
|
| ✅ | D4 `record_pending_write` returns its failure; `release` fails with `ENOSPC` |
|
||||||
|
| ✅ | D5 `rescue_scratch` moves a blob with an unreadable sidecar to `recovery/` unaccompanied instead of fabricating one |
|
||||||
|
| ✅ | F2 `open` write path on `Lane::Transfer`, staged blob duplicated by reflink (`ContentCache::clone_or_copy`) |
|
||||||
|
| ✅ | F3 `fsync` on `Lane::Transfer` |
|
||||||
|
| ✅ | F5 mount registry lock released before FUSE teardown + join |
|
||||||
|
| ✅ | F6 kernel notifications batched under the State lock, flushed after it (`NotifyBatch`) |
|
||||||
|
| 🟡 | T6 `Filesystem::init` negotiates `max_readahead`/`max_background`/`PARALLEL_DIROPS`/`CACHE_SYMLINKS`; `blksize` now 1 MiB; `statfs` implemented against the account quota. **`n_threads`/`clone_fd`, `ATOMIC_O_TRUNC`, `READDIRPLUS` and `FOPEN_KEEP_CACHE` deliberately deferred** — each needs a handler or ordering change first (see `spawn_session`) |
|
||||||
|
| ✅ | T4 LRU touches buffered in memory, flushed in one transaction (and before every eviction pass) |
|
||||||
|
| ✅ | T5 no fsync per cached block; `cached_block` validates by length instead |
|
||||||
|
| 🟡 | DB8 three of the four indexes. `idx_pending_op_due` is **not** wanted: `next_due_op` orders by rowid and already exits on the first row, and offering the planner a `next_attempt_at` index makes it sort — `next_due_op_does_not_scale_with_queue_length` measures the regression. `activity_time` dropped |
|
||||||
|
| ✅ | DB9 `cache_size`, `mmap_size`, `temp_store=MEMORY`, `wal_autocheckpoint` |
|
||||||
|
|
||||||
|
Second batch (uncommitted on top of `e396c6b`):
|
||||||
|
|
||||||
|
| Landed | Item |
|
||||||
|
|---|---|
|
||||||
|
| ✅ | DB10 `flock(LOCK_EX\|LOCK_NB)` single-writer lock in `Db::open`; a `SQLITE_CORRUPT`/`NOTADB` file is quarantined (`cache.db.corrupt-<ts>`, WAL sidecars too) and rebuilt |
|
||||||
|
| ✅ | DB7 `VACUUM` and the deep `integrity_check` ack-then-background (`Core::start_vacuum`, `Core::start_or_read_integrity_check`, visible as jobs); `Response::CacheReport` gained `integrity_running`, `pdfs doctor` polls for the verdict |
|
||||||
|
| ✅ | DB7 `local_finish_scan` no longer rebuilds the whole FTS index: `local_upsert_batch` maintains `local_fts` incrementally (valid because `path` is the key and `name` derives from it), migration V23 does one last rebuild |
|
||||||
|
| ✅ | §5 `recover_fsynced_writes` no longer `read_dir`s per idle pass (`ContentCache::recovery_pending`) |
|
||||||
|
| ✅ | §5 `own_sealed_revs` persisted (migration V24, `own_sealed_rev` table, 7-day TTL) — a restart no longer reopens the B70-layer-B fork window |
|
||||||
|
| ✅ | §5 staging reconcile at mount (`Core::reconcile_staging`): staged blobs no queued op refers to are re-queued, or named in the log when they cannot be addressed |
|
||||||
|
| ✅ | §5 queue/staging reporting — `Response::Status` gained `parked_uploads`, `failing_ops`, `failing_error`, `staged_bytes`, `staged_oldest_secs`; `pdfs status` prints them when non-zero |
|
||||||
|
|
||||||
|
Third batch (uncommitted on top of `7ad9231`):
|
||||||
|
|
||||||
|
| Landed | Item |
|
||||||
|
|---|---|
|
||||||
|
| ✅ | F1 `queue_revision` no longer gap-fills from the network. It stages an incomplete blob (`fill_gaps_cached` resolves only what the local caches already hold, capped at 64 MiB) and `drain_revision` finishes it off the dispatch loop |
|
||||||
|
| ✅ | F1 a partial write over an undrained one is merged instead of refused: `merge_over_pending` folds the superseded blob's authored ranges in and inherits its `base_mtime`/`base_size`/`based_on`. `release` keeps its inherited ordering, which is now affordable because only local I/O is left on it |
|
||||||
|
| ✅ | F4 size-upgrade waiters no longer occupy a pool thread: `getattr`/`lookup` park their `Reply` on `SizeWaitQueue`, served by one thread that exits when nothing is parked. `getattr` no longer needs a `Lane::Meta` job at all |
|
||||||
|
| ✅ | F4 upgrade threads bounded by `MAX_SIZE_UPGRADES` (8 folders in flight); past the cap a `stat` answers provisionally, the same fallback the deadline takes |
|
||||||
|
|
||||||
|
Corrected while implementing: the audit's claim that F1's `fill_gaps` bypasses
|
||||||
|
the block cache is wrong — it calls `read_range_remote` with `cache_blocks =
|
||||||
|
true`, which consults the whole-file blob and then the block cache before the
|
||||||
|
network. What made `release` slow was going to the network at all, not doing so
|
||||||
|
uncachedly.
|
||||||
|
|
||||||
|
Fourth batch (uncommitted, same working tree):
|
||||||
|
|
||||||
|
| Landed | Item |
|
||||||
|
|---|---|
|
||||||
|
| ✅ | T3 the in-memory ring (`StreamRing` → `BlockRing`, now a true LRU) sits in front of **both** block paths, not just the disk-bypassing stream. A block read off disk is promoted into it, so the other ~31 kernel reads of that block cost nothing |
|
||||||
|
| ✅ | T1 `BlockFlight`: `(uid, mtime, size, bidx)` single-flight over block fetches, so demand read, kernel read-ahead and prefetch share one download+decrypt. The leader's claim is a drop guard, so a cancelled `JoinSet` leaves no key nobody will publish; a follower whose leader vanished fetches for itself |
|
||||||
|
| ✅ | T2 `Core::prefetch_ahead` on both paths: per-file sequential detection (`Prefetch::window`), depth 2→8 doubling per sequential read, 0 after a seek, skipping blocks already in the ring or on disk (`ContentCache::has_block`), bounded by its own `PREFETCH_BUDGET` permits with `try_acquire_owned` so it never queues ahead of a demand read |
|
||||||
|
| ✅ | The block-cache disk write moved off the read's critical path (`spawn_blocking` inside `read_block`) |
|
||||||
|
|
||||||
|
Corrected while implementing: T3's premise is half stale — `ContentCache::read_range`
|
||||||
|
already `pread`s only the requested window, and T4/T5 removed the per-read SQLite
|
||||||
|
write and the fsync. What remained, and is what the ring fixes, is `cached_block`
|
||||||
|
reading and allocating the whole 4 MiB for each 128 KiB the kernel asks for.
|
||||||
|
T1's fix is client-side rather than in the SDK's `RevisionReader` as the audit
|
||||||
|
suggested: the SDK ships from a separate repo through crates.io, and the flight
|
||||||
|
map needs the client's `(mtime, size)` validity pair to be safe across revisions.
|
||||||
|
|
||||||
|
Fifth batch (uncommitted on top of `e48c4bb`):
|
||||||
|
|
||||||
|
| Landed | Item |
|
||||||
|
|---|---|
|
||||||
|
| ✅ | DB3 `nodes.path` is persisted (migration V25, backfilled by the same walk V21 used). `path_of` reads a column; `walk_path_of` stays as the fallback for a row written before it existed |
|
||||||
|
| ✅ | DB3 a folder upsert whose name, parent, path and trashed flag are all unchanged — i.e. every re-listing — no longer touches its subtree at all. `re_listing_a_folder_does_not_scale_with_its_subtree` measures it |
|
||||||
|
| ✅ | DB3 when the subtree *has* moved, one recursive walk carries both the new path and whether anything above is trashed, so a descendant costs two trivial statements instead of a `path_of` walk plus an ancestor walk of its own (`reindex_subtree_tx`) |
|
||||||
|
| ✅ | DB4 `sync_folder_list` and the pin lookup are hoisted out of the per-candidate loop (`Core::search_roots`, `Core::pinned_set`); `SearchRoots::resolve` maps a hit to its mountpoint by string arithmetic over the path the index already carries, with no query at all |
|
||||||
|
| ✅ | DB2 a pool of read-only connections (`ReadPool`, `Db::read`), and every `SELECT`-only method routed at it. `a_read_does_not_wait_for_the_write_connection` pins the property |
|
||||||
|
| ✅ | DB1 `State` no longer writes to SQLite. Mutations queue a `DbWrite` in an outbox that `StateGuard` applies **after** releasing the inode lock — so interning a 5000-child listing no longer commits a transaction with the whole mount frozen |
|
||||||
|
|
||||||
|
Corrected while implementing: DB4's "filter by score *before* decorating" was
|
||||||
|
already true — `relevance_score(…)?` short-circuits ahead of the `SearchHit`
|
||||||
|
construction. What actually cost the keystroke was `sync_folder_list` plus a
|
||||||
|
`path_relative_to` walk per (hit, sync folder) pair, and `is_pinned`'s ancestor
|
||||||
|
CTE per hit. DB3's suggested `path` column is here, but its companion suggestion
|
||||||
|
to skip the reindex on an unchanged name/parent needed the trashed flag in the
|
||||||
|
comparison too: a folder being trashed has to take its subtree out of the index,
|
||||||
|
and the old code only did that by accident, through reindexing unconditionally.
|
||||||
|
|
||||||
|
DB1 is done for writes, not reads: `State::has_children` still reads the DB
|
||||||
|
under the inode lock. That read now goes to the pool, so it no longer contends
|
||||||
|
with a writer, and moving it would mean changing what `rmdir` is allowed to
|
||||||
|
assume.
|
||||||
|
|
||||||
|
Sixth batch (on top of `433e206`):
|
||||||
|
|
||||||
|
| Landed | Item |
|
||||||
|
|---|---|
|
||||||
|
| ✅ | §5 parallel drain — `DRAIN_WORKERS = 3` threads over one queue, sharing it through `pending_op.claimed_at` (migration V26) and `Db::claim_next_due_op`. Select-and-claim is one transaction, and the query excludes any op whose uid another worker holds, so ordering still holds per node without holding it globally. Worker 0 keeps the idle chores. Stale claims are cleared in `Db::open`, which the single-writer `flock` makes sound |
|
||||||
|
| ✅ | §5 adaptive debounce — `Core::revision_debounce` widens the grace period toward the node's last measured upload wall-time, clamped to `[2s, 60s]`; only a *completed* upload counts as a measurement, so a fault cannot widen it permanently |
|
||||||
|
| ✅ | §5 cancel on supersede — `Core::cancel_upload` sets a per-node flag that the upload's `CountingReader` refuses the SDK's next block on. Signalled by `enqueue_staged_write`, `queue_trash` and `discard_queued_ops`, i.e. every path that either replaces those bytes or unlinks them. A cancelled upload returns `Ok` rather than a retry: its row and its blob already belong to the write that cancelled it |
|
||||||
|
|
||||||
|
Corrected while implementing: the audit's "a claim column and N=3–4 workers" needed
|
||||||
|
one thing it does not mention — with several workers, a supersede or a trash can
|
||||||
|
now delete a row that is *in flight*, which single-threading used to make
|
||||||
|
impossible. That is what makes the cancellation flag part of the parallel-drain
|
||||||
|
change rather than a separate throughput nicety.
|
||||||
|
|
||||||
|
Seventh batch — T8, in `proton-sdk-rs` (uncommitted there; reaches this client
|
||||||
|
only through a crates.io publish, so nothing here changes until that lands):
|
||||||
|
|
||||||
|
| Landed | Item |
|
||||||
|
|---|---|
|
||||||
|
| ✅ | T8 token lock — `Inner::tokens` is a `std::sync::RwLock<Arc<Tokens>>` read per request, with a separate `Inner::refresh` async mutex held across the refresh call. An ordinary request no longer queues behind an in-flight `auth/v4/refresh`; `an_ordinary_request_does_not_wait_for_an_in_flight_refresh` pins it against a deliberately slow loopback refresh |
|
||||||
|
| ✅ | T8 crypto off the reactor — the three remaining inline spots are on the blocking pool: `create_public_link`'s bcrypt + SRP verifier, `decrypt_invitation_name`'s share-key S2K, and the public-link handshake's `generate_proofs` + `unlock_share_key` (`public_link.rs::blocking`) |
|
||||||
|
| ✅ | T8 `list_blocks` pipelining — pages after the first are fetched concurrently in a window ramping 2→6, page offsets derived positionally (which the existing contiguity check already requires). A 4 GiB file is ~6 round trips instead of 21 |
|
||||||
|
| ✅ | T8 one 4 MiB copy per block removed — `ContentKey::decrypt_block` returns `Bytes` via `LiteralData::into_bytes` instead of `data().to_vec()`; `decrypt_block_blocking`, `digest_and_decrypt_block_blocking` and `RevisionReader::block_plaintext` carry `Bytes` through |
|
||||||
|
| n/a | T8's last bullet is client-side only: the SDK already exposes `RevisionReader::block_sizes()`, so consulting it instead of the hardcoded `BLOCK_SIZE = 1 << 22` is a change in this repo, not that one |
|
||||||
|
|
||||||
|
Note: `decrypt_block` returning `Bytes` is a breaking change to the SDK's public
|
||||||
|
crypto surface, so the publish that carries T8 needs the version bump to match.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 0. The one-sentence version
|
||||||
|
|
||||||
|
The subsystem designs are sound and unusually well documented; the defects are
|
||||||
|
almost all **placement** — correct work happening on the wrong thread, under the
|
||||||
|
wrong lock, or at the wrong granularity. Three exceptions destroy user bytes.
|
||||||
|
|
||||||
|
Two structural facts explain most of the severity:
|
||||||
|
|
||||||
|
1. **fuser's session loop is non-concurrent** and `Config::n_threads` is unset →
|
||||||
|
`unwrap_or(1)` (`fuser-0.17.0/src/session.rs:254`). One thread reads
|
||||||
|
`/dev/fuse`. Anything a handler does inline stalls *every* request on the
|
||||||
|
mount, including the 11 idle workers, which have nothing to receive.
|
||||||
|
2. **One `Mutex<Connection>`** (`pdfs-core/src/db/mod.rs:73`) for the whole
|
||||||
|
daemon, and `State` (`lib.rs:305`) is one mutex over the entire inode space —
|
||||||
|
and `State`'s methods write through to the DB *while the caller holds it*.
|
||||||
|
The `State → Db` edge turns a microsecond lock into a multi-second one.
|
||||||
|
|
||||||
|
Fixing (1) and (2) is worth more than every micro-optimisation below combined.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. Data loss — fix first
|
||||||
|
|
||||||
|
### D1. Sweep deletes staged blobs after a network round trip
|
||||||
|
`sweep.rs:169` — `discard_queued_ops` (`lib.rs:2610`) deletes the op row *and*
|
||||||
|
unlinks the staged blob, and runs *after* the `trash_nodes` call at
|
||||||
|
`sweep.rs:150-153`. The B71 interlock `duplicate_still_removable`
|
||||||
|
(`sweep.rs:193`) runs *before* it. A `close(2)` landing in that window stages
|
||||||
|
bytes that the sweep then destroys. Only survives today because `SweepMode`
|
||||||
|
defaults to report-only.
|
||||||
|
|
||||||
|
Fix: drop `discard_queued_ops` from `remove_conflict_copy` entirely. Unhook
|
||||||
|
locally and let the drain discover the node is trashed — `revision_conflict`
|
||||||
|
(`drain.rs:615`) already routes that to `keep_as_conflict_copy`, the correct
|
||||||
|
non-destructive outcome.
|
||||||
|
|
||||||
|
### D2. Conflict paths evict a `pending` entry that may belong to a newer write
|
||||||
|
`drain.rs:713` (`keep_as_conflict_copy`), `drain.rs:751` (`abandon_to_staging`)
|
||||||
|
both do `self.pending.lock().remove(uid)` unconditionally. `drain_revision`
|
||||||
|
deliberately does not — `drain.rs:890` removes only when `op_in_mem.path ==
|
||||||
|
blob`, precisely because a supersede may have replaced the entry mid-flight.
|
||||||
|
|
||||||
|
Consequences of the missing guard: reads fall back to a stale remote; the next
|
||||||
|
`remote_baseline` (`lib.rs:2195`) forks a spurious `(sync-conflict)` copy; and
|
||||||
|
the incomplete-write interlock at `lib.rs:2246` is silently disarmed, letting an
|
||||||
|
incomplete write gap-fill from a revision that no longer describes the file.
|
||||||
|
|
||||||
|
Fix: copy `drain_revision`'s `path == blob` guard into both sites.
|
||||||
|
|
||||||
|
### D3. No ENOSPC path anywhere
|
||||||
|
`cache.rs:891` `emergency_evict` has **zero callers** (verified by grep across
|
||||||
|
the workspace). `write(2)` returns `EIO`, not `ENOSPC` (`filesystem.rs:379`).
|
||||||
|
Worse: `stage_write` failing on a full disk leaves the scratch file with no
|
||||||
|
durability sidecar, so `discard_unmarked_scratch` deletes the only copy at next
|
||||||
|
mount — the one place the "never delete on a failure path" invariant breaks.
|
||||||
|
|
||||||
|
Fix: call `emergency_evict` on `StorageFull`, propagate `ENOSPC`, and
|
||||||
|
`mark_scratch_durable` before returning any staging error.
|
||||||
|
|
||||||
|
### D4. `record_pending_write` failure is only a `warn!`
|
||||||
|
`state.rs:925-931`. Its own doc comment (`:918-923`) says this row is the only
|
||||||
|
record that the file is as long as the caller was told. On ENOSPC the daemon
|
||||||
|
acknowledges the write and loses the size. Fix: fail the `release` with `ENOSPC`.
|
||||||
|
|
||||||
|
### D5. `rescue_scratch` fabricates a sidecar for a corrupt one
|
||||||
|
`cache.rs:836-861` invents `uid: "recovered~<name>"`, `complete: true`. That uid
|
||||||
|
parses, so `recover_fsynced_writes` enqueues an op against a nonexistent node
|
||||||
|
that retries forever, and `complete: true` is a lie that stops any gap-fill.
|
||||||
|
Fix: move the blob to `recovery/` *without* a sidecar — `recovered_writes`
|
||||||
|
already retains sidecar-less blobs for a human, which is the right outcome.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. Whole-mount freezes (single dispatch thread)
|
||||||
|
|
||||||
|
### F1. `release(2)` gap-fills over the network, inline
|
||||||
|
`filesystem.rs:687` → `lib.rs:2113` → `reads.rs:616` → `rt.block_on`. Closing a
|
||||||
|
2 GB file after a 4 KB edit downloads ~500 blocks with the dispatch loop stopped:
|
||||||
|
no `stat`, no `ls`, no `open`, no `read` anywhere on the mount. The comment at
|
||||||
|
`filesystem.rs:674-686` already concedes the placement is wrong.
|
||||||
|
|
||||||
|
Fix: the staged blob is allowed to be incomplete (`complete: false` is
|
||||||
|
first-class, and `drain_revision` at `drain.rs:851-869` already gap-fills off the
|
||||||
|
loop). Skip `fill_gaps` in `queue_revision` for partial writes; add the per-node
|
||||||
|
staging-in-flight barrier that `read`/`open` wait on, then move `finish_release`
|
||||||
|
to `Lane::Transfer`. Also: `fill_gaps` uses `read_range_remote`, bypassing the
|
||||||
|
block cache — a cache-aware fill makes editing a recently-read file ~free.
|
||||||
|
|
||||||
|
### F2. `open(2)` copies the whole staged blob, inline
|
||||||
|
`filesystem.rs:180` `std::fs::copy` of a possibly multi-GB pending blob, on the
|
||||||
|
dispatch thread. Fix: reflink (`FICLONE` → `copy_file_range` → `fs::copy`), and
|
||||||
|
route the write-open path through `Lane::Transfer`.
|
||||||
|
|
||||||
|
### F3. `fsync(2)` runs `sync_all` + sidecar write inline
|
||||||
|
`filesystem.rs:602`, `:641`. Apps that fsync per record stall the mount at their
|
||||||
|
barrier rate. No ordering constraint — the kernel waits for the reply either way.
|
||||||
|
Fix: `Lane::Transfer`.
|
||||||
|
|
||||||
|
### F4. Size-upgrade waiters occupy every worker in both lanes for 10 s
|
||||||
|
`filesystem.rs:75-88`, `:1069-1077` queue a `Lane::Meta` job that blocks on a
|
||||||
|
condvar up to `SizeUpgrade::WAIT = 10s` (`lib.rs:4292`). This defeats the premise
|
||||||
|
of the lane split (`workers.rs:39-52`: "metadata jobs are short"). Single-flight
|
||||||
|
is per folder, so `ls -l` across N cold folders fills all 11 threads. Also
|
||||||
|
`lib.rs:1801` spawns an unbounded thread per folder.
|
||||||
|
|
||||||
|
Fix: register the `Reply` on a completion list instead of blocking a worker; bound
|
||||||
|
the upgrade threads.
|
||||||
|
|
||||||
|
### F5. `mounts` lock held across FUSE teardown + thread join
|
||||||
|
`devices.rs:539`, `devices.rs:870`. Edition-2024 if-let chains keep the
|
||||||
|
`MutexGuard` alive through the body, so the lock spans `abort_fuse_connection` +
|
||||||
|
`umount_and_join`. If the secondary session is inside F1's gap-fill, the join
|
||||||
|
waits it out while the shutdown path blocks on the same lock → systemd SIGKILLs
|
||||||
|
mid-unmount, stranding the endpoint that `abort_fuse_connection` exists to avoid.
|
||||||
|
Fix: `let taken = self.mounts.lock().remove(&id);` then drop before teardown.
|
||||||
|
|
||||||
|
### F6. Notifier called while holding the State lock
|
||||||
|
`lib.rs:706-710` `for_each_mount`. `for_each_state` (`:689`) and
|
||||||
|
`flush_access_changes` (`:715`) both document and honour the opposite rule.
|
||||||
|
`inval_inode` → `invalidate_inode_pages2` can block on pages that this daemon's
|
||||||
|
own workers must fill — and those workers are blocked on the State lock the
|
||||||
|
notifier holds. `background.rs:226-234` walks every cached directory this way in
|
||||||
|
one hold. Fix: collect inodes under the lock, notify after dropping it.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. Throughput
|
||||||
|
|
||||||
|
### T1. No single-flight on block fetches — ~2× bandwidth and CPU, steady state
|
||||||
|
`reads.rs:475-526`. Nothing registers "(uid, mtime, size, bidx) in flight". With
|
||||||
|
`FUSE_ASYNC_READ` on, the demand read and the kernel's readahead land in the same
|
||||||
|
4 MiB block for 31 of every 32 reads, and both fetch and decrypt it in full.
|
||||||
|
|
||||||
|
The codebase already has the pattern (`ReaderSlot::Pending`, `reads.rs:164-277`)
|
||||||
|
and the SDK has `SingleFlight` used for node/parent-key loads. Best fixed in the
|
||||||
|
SDK's `RevisionReader` so every consumer benefits.
|
||||||
|
|
||||||
|
### T2. No prefetch for anything under 256 MiB
|
||||||
|
`reads.rs:541` calls `stream_readahead` only when `!cache_blocks`. Everything
|
||||||
|
else is a strictly serial block chain: fetch, serve 32 kernel reads, stall.
|
||||||
|
Derived ceiling on a 100 ms RTT link ≈ 24 MB/s against ~125 MB/s available. The
|
||||||
|
SDK's 10-way block concurrency is never exercised.
|
||||||
|
|
||||||
|
Fix: per-(ino, fh) sequential detection, prefetch depth ramping 2→8, reset on a
|
||||||
|
non-sequential offset. Give prefetch its own permit budget or `try_acquire` so it
|
||||||
|
never delays a demand read (SDK's semaphore is flat, `client.rs:126`).
|
||||||
|
|
||||||
|
### T3. Warm cached reads re-read and re-allocate 4 MiB per 128 KiB
|
||||||
|
`cache.rs:570-585`: per kernel read, 2 opens + a JSON parse + a SQLite write txn
|
||||||
|
+ a 4 MiB pread + a 4 MiB alloc. 128 MiB of traffic to deliver 4 MiB. ~60 MB/s
|
||||||
|
ceiling on a fully cached file.
|
||||||
|
|
||||||
|
Fix: serve the disk-cache path through an in-memory block LRU (the `StreamRing`
|
||||||
|
shape, currently used only when `cache_blocks == false`); fold `Meta` into the
|
||||||
|
block filename to kill the sidecar read; `pread` only the requested window.
|
||||||
|
|
||||||
|
### T4. A SQLite write on the read hot path
|
||||||
|
`cache.rs:581` `cache_accessed` = `UPDATE cache_entries SET last_accessed` under
|
||||||
|
the global connection mutex, 32× per 4 MiB read, contending with every `lookup`
|
||||||
|
and `readdir`. Reintroduces one layer down exactly the contention the `Lane::Meta`
|
||||||
|
reservation exists to prevent. Also `filesystem.rs:297` `is_pinned` per read.
|
||||||
|
|
||||||
|
Fix: batch LRU touches in memory, flush periodically; cache the pin bit.
|
||||||
|
|
||||||
|
### T5. `fsync` per cached block on the cold path
|
||||||
|
`cache.rs:605` `sync_all()` inside `store_block`, inline on the thread the kernel
|
||||||
|
read is waiting on. The cache is disposable and `cached_block` doesn't even
|
||||||
|
validate length. Fix: drop the fsync, validate by length like `valid_blob` does.
|
||||||
|
|
||||||
|
### T6. Kernel/mount caps left at fuser defaults
|
||||||
|
No `Filesystem::init` impl at all. `mount.rs:257-273` sets only
|
||||||
|
`FSName`/`Subtype`/`DefaultPermissions`.
|
||||||
|
|
||||||
|
| Knob | Now | Should be |
|
||||||
|
|---|---|---|
|
||||||
|
| `n_threads` / `clone_fd` | 1 / false | 4 / true (`FUSE_DEV_IOC_CLONE`, Linux 4.5+) |
|
||||||
|
| `max_background` | 16 | 64 |
|
||||||
|
| `max_readahead` | 128 KiB | 4 MiB (match `BLOCK_SIZE`) |
|
||||||
|
| `FUSE_PARALLEL_DIROPS` | off | on — kernel serialises concurrent lookups per dir |
|
||||||
|
| `FUSE_DO_READDIRPLUS` + `_AUTO` | off, no `readdirplus` impl | on — `ls -l` is readdir + N lookups today |
|
||||||
|
| `FUSE_ATOMIC_O_TRUNC`, `CACHE_SYMLINKS` | off | on |
|
||||||
|
| `FOPEN_KEEP_CACHE` | never set (`filesystem.rs:145`, `:214`) | set — the event poller already does the `inval_inode` this requires |
|
||||||
|
| `attr.blksize` | `512` (`filesystem.rs:1149`) | `1 << 20` — coreutils size their buffers from it |
|
||||||
|
| `statfs` | not implemented | implement — `df` and free-space preflights see garbage |
|
||||||
|
|
||||||
|
### T7. HTTP/2 is compiled out of the SDK
|
||||||
|
`proton-sdk-rs/Cargo.toml:27` uses `default-features = false` and omits
|
||||||
|
`http2`, which is in reqwest 0.13's default set. Verified: `h2` appears **zero**
|
||||||
|
times in `Cargo.lock`. Every concurrent block download pays its own TCP+TLS
|
||||||
|
handshake instead of multiplexing. **One-line fix, add `"http2"`.**
|
||||||
|
|
||||||
|
### T8. Other SDK hot spots
|
||||||
|
- Token mutex held across the refresh network call (`http.rs:503-524`); every
|
||||||
|
ordinary request takes the same lock just to *read* the token. Worst case ~2 min
|
||||||
|
of total API stall. Fix: `ArcSwap<Tokens>` for reads, separate refresh mutex.
|
||||||
|
- Three inline S2K/PGP decrypts on the reactor (`client.rs:1376-1378`, `:1281`,
|
||||||
|
`:6092`) — tens of ms each, while the rest of the crate correctly uses
|
||||||
|
`crypto::blocking`.
|
||||||
|
- `list_blocks` pages 50 at a time, sequentially (`transport.rs:152`): a 4 GiB
|
||||||
|
file is 21 serial round trips before the first byte.
|
||||||
|
- ~3 full 4 MiB copies per block delivered (`content.rs:350-366`, `revision.rs:361`).
|
||||||
|
- Client hardcodes `BLOCK_SIZE = 1 << 22` instead of consulting the SDK's
|
||||||
|
`block_sizes()`; any revision with a different geometry doubles fetch count.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. Database and IPC
|
||||||
|
|
||||||
|
### DB1. Break the `State → Db` edge
|
||||||
|
`state.rs:614`, `:688`, `:772`, `:854`, `:928`, `:281` all write to SQLite from
|
||||||
|
inside `State` methods, under the whole-mount inode lock. `lib.rs:1665-1695`
|
||||||
|
commits a 5000-row listing that way. `intern_batch`/`intern_from_db` already
|
||||||
|
prove the split works — extend it to the rest, and write through after `drop(st)`.
|
||||||
|
|
||||||
|
### DB2. Single connection throws away WAL's concurrent readers
|
||||||
|
Add a small pool of read-only connections with the same PRAGMAs and route every
|
||||||
|
`SELECT`-only method at them. The module's own justification (`mod.rs:6-7`,
|
||||||
|
"FUSE callbacks already serialize behind the State lock") stopped being true when
|
||||||
|
reads moved onto an 11-thread pool.
|
||||||
|
|
||||||
|
### DB3. A folder upsert re-indexes its whole subtree
|
||||||
|
`nodes.rs:532-608`. Renaming a 10k-descendant folder ≈ 50k statements, ~20k
|
||||||
|
recursive CTEs, holding both the State and DB locks. Fixes, in order: persist a
|
||||||
|
`path` column so `path_of` stops being a CTE; skip the reindex when name and
|
||||||
|
parent are unchanged (every re-listing pays it today); batch the FTS refresh;
|
||||||
|
hoist `node_is_indexable_tx` out of the per-descendant loop.
|
||||||
|
|
||||||
|
### DB4. Search is ~6000 statements per keystroke
|
||||||
|
`lib.rs:3064` takes up to 1000 candidates, then per candidate runs `path_of`
|
||||||
|
(CTE), `is_pinned` (CTE), and `mounted_search_path` → `sync_folder_list()` +
|
||||||
|
`path_relative_to` per sync folder (CTE). Fix: hoist `sync_folder_list` out of
|
||||||
|
the loop, batch pins via the existing `pinned_uids`, filter by score *before*
|
||||||
|
decorating, and persist `nodes.path`.
|
||||||
|
|
||||||
|
### DB5. `is_pinned`'s CTE has no depth cap and no cycle guard
|
||||||
|
`pins.rs:81-88`. Every other walk in the codebase caps (`utils.rs:56`,
|
||||||
|
`nodes.rs:312`, `nodes.rs:643`, `migrations.rs:580`). A `parent_uid` cycle —
|
||||||
|
which `nodes.rs:567-570` treats as data the API can hand you — makes `UNION ALL`
|
||||||
|
non-terminating *while holding the DB mutex*. Unrecoverable mount hang, one-line
|
||||||
|
fix.
|
||||||
|
|
||||||
|
### DB6. IPC can wedge the daemon two ways
|
||||||
|
- No `set_write_timeout` on the daemon side (`pdfs-fuse/src/control.rs:108` sets
|
||||||
|
only the read timeout; the *client* side gets both, `pdfs-core/src/control.rs:1390`).
|
||||||
|
64 clients that never read their reply exhaust `MAX_CONTROL_HANDLERS`
|
||||||
|
permanently.
|
||||||
|
- Client-supplied `limit` goes unclamped into `SQL LIMIT ?` (`control.rs:349`,
|
||||||
|
and `ListActivity`/`PhotoTimeline`/`AlbumPhotos`), with no cap on response size.
|
||||||
|
One request materialises the whole account under the mutex.
|
||||||
|
|
||||||
|
### DB7. Long handlers that don't ack-then-background
|
||||||
|
`VACUUM` (`maintenance.rs:183`), deep `integrity_check`, and `local_finish_scan`
|
||||||
|
(`db/local.rs:79-95`, a whole-index FTS trigram rebuild) all run under the
|
||||||
|
connection mutex — freezing the mount for tens of seconds. Every *other* long
|
||||||
|
handler already uses the ack-then-background pattern (`control.rs:452`, `:638`,
|
||||||
|
`:707`).
|
||||||
|
|
||||||
|
### DB8. Missing indexes (one migration)
|
||||||
|
```sql
|
||||||
|
CREATE INDEX idx_sync_entry_remote_uid ON sync_entry(remote_uid);
|
||||||
|
CREATE INDEX idx_pending_op_due ON pending_op(next_attempt_at, id);
|
||||||
|
CREATE INDEX idx_local_files_scan_gen ON local_files(scan_gen);
|
||||||
|
CREATE INDEX idx_nodes_parent_live ON nodes(parent_uid) WHERE trashed = 0;
|
||||||
|
```
|
||||||
|
`idx_pending_op_due` matters more than it looks: B70's tempfile parking puts long
|
||||||
|
runs of parked ops at the head of the queue, and `next_due_op`'s claim that it
|
||||||
|
"exits on the first row" (`ops.rs:462-465`) stops holding.
|
||||||
|
`activity_time` (`migrations.rs:302`) indexes `time DESC` while both queries order
|
||||||
|
by `id` — dead weight on every insert.
|
||||||
|
|
||||||
|
### DB9. PRAGMAs
|
||||||
|
Set and correct: `journal_mode=WAL`, `synchronous=NORMAL`, `foreign_keys`,
|
||||||
|
`journal_size_limit`, `busy_timeout`. Missing: `cache_size` (still the 2 MiB
|
||||||
|
default, against a `node_json`-heavy schema), `mmap_size`, `temp_store=MEMORY`
|
||||||
|
(every `ORDER BY … COLLATE NOCASE` and every recursive CTE spills to a file
|
||||||
|
today), `wal_autocheckpoint` (the committing thread — often the dispatch loop —
|
||||||
|
runs the checkpoint synchronously).
|
||||||
|
|
||||||
|
### DB10. Robustness gaps
|
||||||
|
No `SQLITE_CORRUPT`/`SQLITE_NOTADB` handling anywhere; a corrupt `cache.db` means
|
||||||
|
the daemon won't start. It is a pure cache — rename it aside and rebuild, loudly.
|
||||||
|
No single-instance lock: a hand-run `pdfs mount` alongside the systemd unit gives
|
||||||
|
two writers. Add `flock(LOCK_EX|LOCK_NB)` in `Db::open`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 5. Sequential drain, and other throughput ceilings
|
||||||
|
|
||||||
|
- **The drain is one thread, one op** (`drain.rs:130-191`, `next_due_op` is
|
||||||
|
`ORDER BY id LIMIT 1`) while the bulk uploader is 4-way (`upload.rs:7`). A
|
||||||
|
10 GiB upload blocks every queued rename and small write behind it. Fix: a claim
|
||||||
|
column and N=3–4 workers; ordering only needs to hold per uid.
|
||||||
|
- **A permanently failing op** never wedges the queue (the backoff is correct and
|
||||||
|
never dropping the op is right) but steals 30 s of the only drain thread every
|
||||||
|
5 minutes forever, invisibly. Surface `attempts`/`last_error` in `Status`.
|
||||||
|
- **`recover_fsynced_writes` does a `read_dir` on every idle drain iteration**
|
||||||
|
(`drain.rs:139`) for a directory that is empty ~always.
|
||||||
|
- **No adaptive debounce.** A file saved slower than 2 s but faster than its
|
||||||
|
upload never catches up. Scale the debounce toward the last measured upload
|
||||||
|
wall-time per uid, and cancel an in-flight upload on supersede.
|
||||||
|
- **Nothing bounds `staging/`.** Parked transient creates (`ops.rs:85`) are parked
|
||||||
|
forever; three abandoned 4 GiB browser downloads leave 12 GiB with no expiry and
|
||||||
|
no way for the user to see it. Report staging bytes + parked count + oldest age.
|
||||||
|
- **`own_sealed_revs` (`lib.rs:383`) is in-memory and unbounded** — a restart
|
||||||
|
re-opens the B70 conflict hole. One column persists it.
|
||||||
|
- **Orphaned staging blobs are unrecoverable by tooling.** Three paths stage the
|
||||||
|
only copy and return `EIO` with no durable reference (`lib.rs:2250`, `:2276`,
|
||||||
|
`:2297`); `hydrate_pending` walks the op table, `recover_fsynced_writes` walks
|
||||||
|
`recovery/`, nobody walks `staging/`. Add a startup reconcile.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 6. Confirmed sound — don't spend time here
|
||||||
|
|
||||||
|
Reader/key caching is single-flighted and LRU-bounded per file (`reads.rs:164-277`);
|
||||||
|
no per-block key derivation. Block decryption is off the reactor
|
||||||
|
(`revision.rs:38`). `readdir`/`lookup` are genuinely not N+1 — one uid
|
||||||
|
enumeration plus one batched `enumerate_nodes_light`, with S2K deferred to a
|
||||||
|
chunked single-flight. TTL=30 s is fine and could go higher given active
|
||||||
|
invalidation. **No lock-order inversion anywhere**; no `std::sync::Mutex`, no
|
||||||
|
`.lock().unwrap()` outside tests. Worker queues are unbounded *by design* and the
|
||||||
|
reasoning is right — backpressure onto the dispatch loop is the thing being
|
||||||
|
avoided. `write(2)` takes no lock and does no fsync. `fallocate`'s `PUNCH_HOLE`
|
||||||
|
branch correctly marks the range authored so gap-fill won't refill it. Migrations
|
||||||
|
are single-transaction with a hard newer-schema refusal. `enqueue_op`'s supersede
|
||||||
|
is transactional and cannot orphan the previous blob. `fsync` durability
|
||||||
|
semantics are correct and correctly documented.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 7. Suggested order
|
||||||
|
|
||||||
|
1. **One-line safety** — depth-cap `is_pinned` (DB5), `set_write_timeout` (DB6),
|
||||||
|
clamp IPC `limit`s (DB6), add `"http2"` (T7).
|
||||||
|
2. **Stop losing bytes** — D1, D2, D3, D4, D5.
|
||||||
|
3. **Off the dispatch loop** — `n_threads=4`+`clone_fd` (T6), F1, F2, F3, F5, F6.
|
||||||
|
4. **Break `State → Db`** (DB1) and stop the indexer monopolising the connection
|
||||||
|
(DB7).
|
||||||
|
5. **Cheap kernel/DB tuning batch** — rest of T6, T5, T4, DB8, DB9.
|
||||||
|
6. **Real throughput** — T1 (block single-flight, ideally in the SDK), T3
|
||||||
|
(in-memory block cache), T2 (sequential prefetch).
|
||||||
|
7. **Structural** — persist `nodes.path` (DB3/DB4), parallel drain, adaptive
|
||||||
|
debounce, staging reconcile + reporting.
|
||||||
Reference in new issue
Block a user