Compare commits

...
Author SHA1 Message Date
nvrl 5b7f17bdaf Merge branch 'fix/db-errors-not-offline' into fix/diagnose-coverage
# Conflicts:
#	docs/BUGS.md
2026-10-31 19:26:38 +01:00
nvrl 73068f5868 Merge branch 'fix/mount-failure-surfaced' into fix/db-errors-not-offline
# Conflicts:
#	docs/BUGS.md
2026-10-31 19:26:36 +01:00
nvrl a617b2b410 fix(cli): have diagnose report failing writes, a pause and folders in error
diagnose dropped most of the Status reply and never looked at the synced
folders, so it said "No problems found" while a folder would not sync
(B268, issue #53). It now warns about writes that keep failing, a write
a day old, a pause, folders in error or conflict, and a control socket
nothing listens on.
2026-10-31 10:57:34 +01:00
nvrl bdbfc0bfd4 fix(core): read database errors handed to from_api as internal, not offline
from_api took every error that was not a ProtonError for a transport
failure. Pin, unpin, the searches and node loads hand it pdfs_core
errors from the cache and the database, so a failure there said
"You're offline" (B266, the rest of B205). The source chain now
decides, and only an error with neither type reads as offline.
2026-10-31 10:53:25 +01:00
3 changed files with 218 additions and 22 deletions

No files matched your search

+154 -5
View File
@@ -3446,6 +3446,79 @@ fn cmd_sync_limit(up: Option<u64>, down: Option<u64>) -> Result<()> {
})?) })?)
} }
/// How long the oldest write may wait to upload before `diagnose` calls it
/// out. Under a day it is a slow link or a big file; past it, something is
/// holding the queue.
const STAGED_STALE_SECS: u64 = 86_400;
/// `diagnose` findings for the queue and the pause, from the `Status` reply.
/// `paused_for` is how long a timed pause still runs, in seconds.
fn sync_health(
failing_ops: u64,
failing_error: Option<&str>,
staged_bytes: u64,
staged_oldest_secs: u64,
paused: bool,
paused_for: Option<i64>,
) -> Vec<(DiagnoseLevel, &'static str, String)> {
let mut findings = Vec::new();
if failing_ops > 0 {
findings.push((
DiagnoseLevel::Warn,
" failing writes",
match failing_error {
Some(error) => format!("{failing_ops} keep failing; the last error: {error}"),
None => format!("{failing_ops} keep failing"),
},
));
}
if staged_bytes > 0 && staged_oldest_secs >= STAGED_STALE_SECS {
findings.push((
DiagnoseLevel::Warn,
" oldest unsent write",
format!(
"{} old, {} only on this computer",
human_secs(staged_oldest_secs),
human_bytes(staged_bytes)
),
));
}
if paused {
findings.push((
DiagnoseLevel::Warn,
" sync",
match paused_for {
Some(left) if left > 0 => {
format!("paused for another {}", human_secs(left as u64))
}
_ => "paused until resumed (`pdfs sync resume`)".to_string(),
},
));
}
findings
}
/// `diagnose` findings for the synced folders that need someone to look.
fn folder_health(
folders: &[pdfs_core::control::SyncFolderInfo],
) -> Vec<(DiagnoseLevel, String, String)> {
folders
.iter()
.filter_map(|folder| {
let what = match folder.state.as_str() {
"error" => "sync error; `pdfs sync list` and the activity feed say more",
"conflict" => "conflict copies to look at",
_ => return None,
};
Some((
DiagnoseLevel::Warn,
format!(" synced folder [{}]", folder.id),
format!("{}: {what}", folder.local_path),
))
})
.collect()
}
fn unix_now() -> i64 { fn unix_now() -> i64 {
std::time::SystemTime::now() std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH) .duration_since(std::time::UNIX_EPOCH)
@@ -3782,21 +3855,26 @@ fn cmd_diagnose() -> Result<()> {
let socket_exists = socket.exists(); let socket_exists = socket.exists();
let socket_kind_ok = let socket_kind_ok =
std::fs::symlink_metadata(&socket).is_ok_and(|m| m.file_type().is_socket()); std::fs::symlink_metadata(&socket).is_ok_and(|m| m.file_type().is_socket());
// A stale socket file with no listener is the common failure, so the reply
// is what decides this rather than the file's existence.
let status = control_request(CtlRequest::Status);
report.finding( report.finding(
if socket_exists && !socket_kind_ok { if socket_exists && !socket_kind_ok {
DiagnoseLevel::Fail DiagnoseLevel::Fail
} else if socket_exists { } else if socket_exists && status.is_ok() {
DiagnoseLevel::Ok DiagnoseLevel::Ok
} else { } else {
DiagnoseLevel::Warn DiagnoseLevel::Warn
}, },
" control socket", " control socket",
socket.display().to_string(), if socket_exists && socket_kind_ok && status.is_err() {
format!("{} (left over; nothing listens on it)", socket.display())
} else {
socket.display().to_string()
},
); );
// A stale socket file with no listener is the common failure, so the reply
// is what decides this rather than the file's existence.
let mut daemon_responding = false; let mut daemon_responding = false;
match control_request(CtlRequest::Status) { match status {
Ok(CtlResponse::Status { Ok(CtlResponse::Status {
mountpoint, mountpoint,
online, online,
@@ -3804,6 +3882,12 @@ fn cmd_diagnose() -> Result<()> {
tokens_unsaved, tokens_unsaved,
pending_uploads, pending_uploads,
pending_changes, pending_changes,
failing_ops,
failing_error,
staged_bytes,
staged_oldest_secs,
paused,
paused_until,
version, version,
.. ..
}) => { }) => {
@@ -3878,6 +3962,27 @@ fn cmd_diagnose() -> Result<()> {
format!("{pending} waiting to upload") format!("{pending} waiting to upload")
}, },
); );
// The rest of the daemon's state that the report used to drop:
// writes that keep failing, a pause, a folder in error. Each of
// them stops changes from reaching Drive while everything above
// reads as fine (`docs/BUGS.md` B268).
for (level, check, detail) in sync_health(
failing_ops,
failing_error.as_deref(),
staged_bytes,
staged_oldest_secs,
paused,
paused_until.map(|until| until - unix_now()),
) {
report.finding(level, check, detail);
}
if let Ok(CtlResponse::SyncFolders { items }) =
control_request(CtlRequest::ListSyncFolders)
{
for (level, check, detail) in folder_health(&items) {
report.finding(level, &check, detail);
}
}
match await_integrity_check() { match await_integrity_check() {
Ok(CtlResponse::CacheReport { Ok(CtlResponse::CacheReport {
integrity_problems, integrity_problems,
@@ -4075,6 +4180,50 @@ mod pause_tests {
mod diagnose_tests { mod diagnose_tests {
use super::*; use super::*;
/// A healthy queue says nothing; writes that keep failing, an old unsent
/// write and a pause each say so (`docs/BUGS.md` B268).
#[test]
fn a_stuck_queue_or_a_pause_is_reported() {
assert!(sync_health(0, None, 0, 0, false, None).is_empty());
assert!(sync_health(0, None, 4096, STAGED_STALE_SECS - 1, false, None).is_empty());
let checks: Vec<&str> = sync_health(
2,
Some("upload a.txt: quota"),
4096,
STAGED_STALE_SECS,
true,
Some(600),
)
.into_iter()
.map(|(level, check, _)| {
assert!(matches!(level, DiagnoseLevel::Warn));
check.trim()
})
.collect();
assert_eq!(checks, ["failing writes", "oldest unsent write", "sync"]);
}
/// Issue #53: a folder that would not sync, and `diagnose` found nothing.
#[test]
fn a_synced_folder_in_error_is_reported() {
let folder = |id: i64, state: &str| pdfs_core::control::SyncFolderInfo {
id,
local_path: format!("/home/u/f{id}"),
remote_uid: "vol~link".into(),
mode: "mirror".into(),
state: state.into(),
last_sync: 0,
pending_mode: None,
progress: None,
paused: false,
};
let findings =
folder_health(&[folder(1, "idle"), folder(2, "error"), folder(3, "syncing")]);
assert_eq!(findings.len(), 1);
assert_eq!(findings[0].1.trim(), "synced folder [2]");
assert!(findings[0].2.starts_with("/home/u/f2: sync error"));
}
#[test] #[test]
fn finding_severity_is_counted_centrally() { fn finding_severity_is_counted_centrally() {
let mut report = DiagnoseReport::default(); let mut report = DiagnoseReport::default();
+45 -9
View File
@@ -160,13 +160,14 @@ impl CoreError {
/// telling the user their connection is down beats telling them the /// telling the user their connection is down beats telling them the
/// server refused. The SDK's other failures say what went wrong instead /// server refused. The SDK's other failures say what went wrong instead
/// (see [`proton_kind`]). /// (see [`proton_kind`]).
///
/// Callers also hand it this crate's [`Error`] from the cache and the
/// database, which is not a transport error at all. Taking those for
/// "offline" told the user their connection was down when a pin failed on
/// the database (`docs/BUGS.md` B266), so the source chain is searched for
/// either type before falling back.
pub fn from_api(e: &(dyn std::error::Error + 'static), what: &str) -> Self { pub fn from_api(e: &(dyn std::error::Error + 'static), what: &str) -> Self {
let kind = match e.downcast_ref::<proton_sdk::ProtonError>() { Self::new(api_kind_of(e), format!("{what}: {e}"))
Some(proton) => proton_kind(proton),
// Not the SDK's at all: a timeout around the call, or the socket.
None => ErrorKind::Offline,
};
Self::new(kind, format!("{what}: {e}"))
} }
/// Prefix the message while keeping the classification. /// Prefix the message while keeping the classification.
@@ -208,7 +209,13 @@ impl From<serde_json::Error> for CoreError {
impl From<Error> for CoreError { impl From<Error> for CoreError {
fn from(e: Error) -> Self { fn from(e: Error) -> Self {
let kind = match &e { Self::new(core_kind(&e), e.to_string())
}
}
/// What this crate's [`Error`] means to a front-end.
fn core_kind(e: &Error) -> ErrorKind {
match e {
Error::NotLoggedIn Error::NotLoggedIn
| Error::ReloginRequired | Error::ReloginRequired
| Error::Locked | Error::Locked
@@ -216,11 +223,26 @@ impl From<Error> for CoreError {
| Error::HumanVerificationRequired(_) => ErrorKind::Denied, | Error::HumanVerificationRequired(_) => ErrorKind::Denied,
Error::Proton(proton) => proton_kind(proton), Error::Proton(proton) => proton_kind(proton),
_ => ErrorKind::Internal, _ => ErrorKind::Internal,
};
Self::new(kind, e.to_string())
} }
} }
/// The kind for [`CoreError::from_api`]: the first SDK or crate error in `e`'s
/// source chain decides it.
fn api_kind_of(e: &(dyn std::error::Error + 'static)) -> ErrorKind {
let mut cause = Some(e);
while let Some(err) = cause {
if let Some(proton) = err.downcast_ref::<proton_sdk::ProtonError>() {
return proton_kind(proton);
}
if let Some(core) = err.downcast_ref::<Error>() {
return core_kind(core);
}
cause = err.source();
}
// Not the SDK's at all: a timeout around the call, or the socket.
ErrorKind::Offline
}
/// What a failed SDK call means to a front-end. /// What a failed SDK call means to a front-end.
/// ///
/// Only a transport failure is the connection. The rest happened on this /// Only a transport failure is the connection. The rest happened on this
@@ -414,6 +436,20 @@ mod tests {
assert_ne!(e.kind, ErrorKind::Offline); assert_ne!(e.kind, ErrorKind::Offline);
} }
/// A pin or a search that fails on the database is not the connection, so
/// it must not say "You're offline" (`docs/BUGS.md` B266). An SDK error
/// wrapped in this crate's type classifies as the SDK error does.
#[test]
fn a_database_error_handed_to_from_api_is_not_offline() {
let db = Error::Other("database is locked".into());
assert_eq!(CoreError::from_api(&db, "pin").kind, ErrorKind::Internal);
let wrapped = Error::Proton(api(C::DoesNotExist));
assert_eq!(
CoreError::from_api(&wrapped, "load node").kind,
ErrorKind::NotFound
);
}
/// `Error` is the plumbing type; converting it must classify as sharply as /// `Error` is the plumbing type; converting it must classify as sharply as
/// the direct path, or the two disagree about the same failure. /// the direct path, or the two disagree about the same failure.
#[test] #[test]
+19 -8
View File
@@ -72,9 +72,7 @@ original wording.
| [B271](#b271--one-failed-listing-makes-a-large-first-sync-start-over-from-nothing) | One failed listing makes a large first sync start over from nothing | Not fixed | | [B271](#b271--one-failed-listing-makes-a-large-first-sync-start-over-from-nothing) | One failed listing makes a large first sync start over from nothing | Not fixed |
| [B270](#b270--one-file-left-in-a-mirror-folder-clears-the-wipe-guard-for-all-the-others) | One file left in a mirror folder clears the wipe guard for all the others | Not fixed | | [B270](#b270--one-file-left-in-a-mirror-folder-clears-the-wipe-guard-for-all-the-others) | One file left in a mirror folder clears the wipe guard for all the others | Not fixed |
| [B269](#b269--a-restore-that-skipped-every-folder-reports-success) | A restore that skipped every folder reports success | Not fixed | | [B269](#b269--a-restore-that-skipped-every-folder-reports-success) | A restore that skipped every folder reports success | Not fixed |
| [B268](#b268--pdfs-diagnose-says-no-problems-found-while-sync-is-stuck) | `pdfs diagnose` says "No problems found" while sync is stuck | Not fixed |
| [B267](#b267--a-key-failure-at-a-start-from-a-warm-cache-shows-as-offline-for-good) | A key failure at a start from a warm cache shows as offline for good | Not fixed | | [B267](#b267--a-key-failure-at-a-start-from-a-warm-cache-shows-as-offline-for-good) | A key failure at a start from a warm cache shows as offline for good | Not fixed |
| [B266](#b266--pin-unpin-search-and-node-loads-report-a-database-error-as-offline) | Pin, unpin, search and node loads report a database error as "offline" | Not fixed |
| [B265](#b265--address-keys-whose-token-is-for-an-inactive-account-key-never-open) | Address keys whose token is for an inactive account key never open | Needs an SDK change; not reproduced | | [B265](#b265--address-keys-whose-token-is-for-an-inactive-account-key-never-open) | Address keys whose token is for an inactive account key never open | Needs an SDK change; not reproduced |
| [B134](#b134--a-file-deleted-online-while-the-drain-checks-its-write-is-kept-as-a-conflict-copy) | A file deleted online while the drain checks its write is kept as a conflict copy | Only with `local_first` off | | [B134](#b134--a-file-deleted-online-while-the-drain-checks-its-write-is-kept-as-a-conflict-copy) | A file deleted online while the drain checks its write is kept as a conflict copy | Only with `local_first` off |
| [B91](#b91--the-daemon-hung-overnight-and-nothing-could-say-what-it-was-waiting-on) | The daemon hung overnight, and nothing could say what it was waiting on | The cause is not diagnosed; the daemon now reports where it is stuck and is restarted | | [B91](#b91--the-daemon-hung-overnight-and-nothing-could-say-what-it-was-waiting-on) | The daemon hung overnight, and nothing could say what it was waiting on | The cause is not diagnosed; the daemon now reports where it is stuck and is restarted |
@@ -88,6 +86,7 @@ original wording.
| Entry | Title | What is left | | Entry | Title | What is left |
|---|---|---| |---|---|---|
| [B268](#b268--pdfs-diagnose-says-no-problems-found-while-sync-is-stuck) | `pdfs diagnose` says "No problems found" while sync is stuck | The sync engine's state is not checked |
| [B263](#b263--a-mount-that-can-never-succeed-retries-every-5-s-and-tells-only-the-journal) | A mount that can never succeed retries every 5 s and tells only the journal | Errors no retry fixes back off like network errors | | [B263](#b263--a-mount-that-can-never-succeed-retries-every-5-s-and-tells-only-the-journal) | A mount that can never succeed retries every 5 s and tells only the journal | Errors no retry fixes back off like network errors |
| [B262](#b262--watching-a-large-synced-folder-holds-up-the-sync-engine-and-a-failed-watch-is-never-retried) | Watching a large synced folder holds up the sync engine, and a failed watch is never retried | The first pass still waits for the watch; a failed watch is not shown in the app | | [B262](#b262--watching-a-large-synced-folder-holds-up-the-sync-engine-and-a-failed-watch-is-never-retried) | Watching a large synced folder holds up the sync engine, and a failed watch is never retried | The first pass still waits for the watch; a failed watch is not shown in the app |
| [B259](#b259--the-sync-engine-can-die-or-hang-and-nothing-shows-it-until-a-restart) | The sync engine can die or hang, and nothing shows it until a restart | No pass deadline; an ended engine is reported, not restarted; the trigger in #53 is not known | | [B259](#b259--the-sync-engine-can-die-or-hang-and-nothing-shows-it-until-a-restart) | The sync engine can die or hang, and nothing shows it until a restart | No pass deadline; an ended engine is reported, not restarted; the trigger in #53 is not known |
@@ -111,6 +110,7 @@ a live account since.
| Entry | Title | | Entry | Title |
|---|---| |---|---|
| [B266](#b266--pin-unpin-search-and-node-loads-report-a-database-error-as-offline) | Pin, unpin, search and node loads report a database error as "offline" |
| [B264](#b264--a-mount-that-fails-after-starting-its-workers-leaves-them-running) | A mount that fails after starting its workers leaves them running | | [B264](#b264--a-mount-that-fails-after-starting-its-workers-leaves-them-running) | A mount that fails after starting its workers leaves them running |
| [B261](#b261--a-folder-that-has-never-synced-reads-up-to-date) | A folder that has never synced reads "up to date" | | [B261](#b261--a-folder-that-has-never-synced-reads-up-to-date) | A folder that has never synced reads "up to date" |
| [B272](#b272--after-a-folder-is-removed-and-added-again-its-file-events-go-to-the-old-id) | After a folder is removed and added again, its file events go to the old id | | [B272](#b272--after-a-folder-is-removed-and-added-again-its-file-events-go-to-the-old-id) | After a folder is removed and added again, its file events go to the old id |
@@ -484,7 +484,7 @@ exit path.
### B268 — `pdfs diagnose` says "No problems found" while sync is stuck ### B268 — `pdfs diagnose` says "No problems found" while sync is stuck
**Status:** Open **Status:** Partly fixed. The sync engine's own state is not checked yet.
**Found:** 2026-10-31, issue #53: a folder that never synced, and `diagnose` found nothing. **Found:** 2026-10-31, issue #53: a folder that never synced, and `diagnose` found nothing.
**Where:** `diagnose` in `crates/pdfs-cli/src/main.rs` **Where:** `diagnose` in `crates/pdfs-cli/src/main.rs`
@@ -497,8 +497,16 @@ folders or for `Diagnostics`, so a folder in error and a stalled worker are not
control socket file with no daemon behind it is also reported `[ok]`; only the next line says control socket file with no daemon behind it is also reported `[ok]`; only the next line says
the daemon does not answer. the daemon does not answer.
**Fix.** Not done. Warn on failing or parked writes, a pause, old staged data and folders in **Fix.** `diagnose` warns about writes that keep failing (with the last error), a write that
error, and report the sync engine's state once B259 exposes it. has waited a day or more to upload, a pause, and each synced folder in `error` or `conflict`
(`sync_health` and `folder_health` in `crates/pdfs-cli/src/main.rs`). A control socket file
that nothing listens on is a warning, not `[ok]`. Parked uploads are left out: a file still
wearing a download's temporary name is normal. Not done: the sync engine's state, which
`Diagnostics` reports once B259's change is in.
**Test:** `a_stuck_queue_or_a_pause_is_reported` and `a_synced_folder_in_error_is_reported` in
`pdfs-cli`. On 2026-10-31 the new `diagnose` against a healthy 3.6.0 daemon still reported
"No problems found".
--- ---
@@ -525,7 +533,7 @@ report the others as their own state through `Status`.
### B266 — Pin, unpin, search and node loads report a database error as "offline" ### B266 — Pin, unpin, search and node loads report a database error as "offline"
**Status:** Open **Status:** Fixed, not verified live
**Found:** 2026-10-31, code audit after issue #38. B205 fixed the photo upload path only. **Found:** 2026-10-31, code audit after issue #38. B205 fixed the photo upload path only.
**Where:** `CoreError::from_api` in `crates/pdfs-core/src/error.rs`; its callers in **Where:** `CoreError::from_api` in `crates/pdfs-core/src/error.rs`; its callers in
`crates/pdfs-fuse/src/lib.rs` (`pin`, `unpin`, the searches, `load node`) `crates/pdfs-fuse/src/lib.rs` (`pin`, `unpin`, the searches, `load node`)
@@ -538,8 +546,11 @@ it was written for SDK calls, where that means a transport error. These callers
`pdfs_core::Error` values from the cache and the database. A `ProtonError` wrapped in `pdfs_core::Error` values from the cache and the database. A `ProtonError` wrapped in
`pdfs_core::Error::Proton` also misses the downcast, since the source chain is not walked. `pdfs_core::Error::Proton` also misses the downcast, since the source chain is not walked.
**Fix.** Not done. Use `CoreError::internal` at the database callers, and walk the source **Fix.** `from_api` walks the source chain. The first `ProtonError` or `pdfs_core::Error` in it
chain in `from_api` before falling back. decides the kind, the same way `From<Error>` does, and only an error with neither, a transport
error, reads as offline. That covers every caller at once, the database ones included.
**Test:** `a_database_error_handed_to_from_api_is_not_offline` in `pdfs-core`.
--- ---