fix(core): retry rotated tokens the keyring refused

A failed keyring write after a refresh was only a warning, and the one
live refresh token then lived in memory only (B61). The sink now keeps
the rotation as unsaved and retries it in the background after 1 s, 5 s,
30 s and then every 5 minutes, until it is written or the blob has moved
on. The refusal is logged as an error naming `pdfs login`.

docs/BUGS.md B61.
This commit is contained in:
nvrl committed 2026-10-09 11:29:31 +02:00
1 parent bf43476e88
commit beb3b2e4e4
2 files changed
+126 -18

No files matched your search

+114 -16
View File
@@ -22,6 +22,8 @@
use std::collections::BTreeMap; use std::collections::BTreeMap;
use std::sync::Arc; use std::sync::Arc;
use std::sync::atomic::{AtomicBool, Ordering};
use std::time::Duration;
use base64::Engine as _; use base64::Engine as _;
use base64::engine::general_purpose::STANDARD as BASE64; use base64::engine::general_purpose::STANDARD as BASE64;
@@ -702,8 +704,21 @@ static TOKEN_SINKS: parking_lot::Mutex<BTreeMap<String, Arc<TokenSink>>> =
struct TokenSink { struct TokenSink {
session_id: String, session_id: String,
state: parking_lot::Mutex<SinkState>, state: parking_lot::Mutex<SinkState>,
/// Whether a thread is retrying a write the keyring refused.
retrying: AtomicBool,
} }
/// How long to wait before each new try at a write the keyring refused; the
/// last delay repeats until it succeeds. A locked or restarting keyring
/// usually comes back within seconds, and one that does not must not be asked
/// in a tight loop.
const KEYRING_RETRY: [Duration; 4] = [
Duration::from_secs(1),
Duration::from_secs(5),
Duration::from_secs(30),
Duration::from_secs(300),
];
struct SinkState { struct SinkState {
/// The refresh token the blob holds for this session, as far as this /// The refresh token the blob holds for this session, as far as this
/// session knows: the one it started from, or the last one it wrote. /// session knows: the one it started from, or the last one it wrote.
@@ -745,13 +760,59 @@ impl TokenSink {
stored_refresh, stored_refresh,
unsaved: None, unsaved: None,
}), }),
retrying: AtomicBool::new(false),
} }
} }
/// Take a new rotation and try to write it. /// Take a new rotation and try to write it. If the keyring refuses, keep
fn rotated(&self, tokens: Tokens) -> Result<()> { /// trying in the background: until then the only live refresh token is in
/// memory, and a stop that cannot write it either loses the session
/// (`docs/BUGS.md` B61).
fn rotated(self: &Arc<Self>, tokens: Tokens) -> Result<()> {
self.state.lock().unsaved = Some(tokens); self.state.lock().unsaved = Some(tokens);
self.flush() let written = self.flush();
if written.is_err() {
self.retry_in_background();
}
written
}
/// Retry the unsaved rotation on [`KEYRING_RETRY`] until it is written or
/// has nowhere left to go. One thread per sink; a newer rotation simply
/// replaces what it is retrying.
fn retry_in_background(self: &Arc<Self>) {
if self.retrying.swap(true, Ordering::SeqCst) {
return;
}
let sink = Arc::clone(self);
let spawned = std::thread::Builder::new()
.name("pdfs-keyring-retry".into())
.spawn(move || {
loop {
let delays = KEYRING_RETRY
.into_iter()
.chain(std::iter::repeat(KEYRING_RETRY[KEYRING_RETRY.len() - 1]));
for delay in delays {
std::thread::sleep(delay);
match sink.flush() {
Ok(()) => break,
Err(e) => tracing::debug!(error = %e, ?delay, "keyring still refuses"),
}
}
sink.retrying.store(false, Ordering::SeqCst);
// A rotation refused after the last try succeeded found
// this thread still running, and started none of its own.
if sink.state.lock().unsaved.is_none()
|| sink.retrying.swap(true, Ordering::SeqCst)
{
return;
}
}
});
if let Err(e) = spawned {
self.retrying.store(false, Ordering::SeqCst);
tracing::error!(error = %e, "could not start retrying the keyring write");
}
} }
/// Write the unsaved rotation, if there is one. A blob that moved on, or /// Write the unsaved rotation, if there is one. A blob that moved on, or
@@ -806,7 +867,11 @@ fn register_refresh_handler(session: &ProtonApiSession, stored_refresh: String)
.insert(session_id.to_owned(), Arc::clone(&sink)); .insert(session_id.to_owned(), Arc::clone(&sink));
session.http().set_on_tokens_refreshed(move |tokens| { session.http().set_on_tokens_refreshed(move |tokens| {
if let Err(e) = sink.rotated(tokens) { if let Err(e) = sink.rotated(tokens) {
tracing::warn!(error = %e, "failed to auto-persist refreshed tokens in keyring") tracing::error!(
error = %e,
"the keyring refused the refreshed session tokens; retrying. \
If the daemon stops first, the next start needs `pdfs login`"
)
} }
}); });
} }
@@ -946,27 +1011,32 @@ mod tests {
assert_eq!(stored.refresh_token, "r"); assert_eq!(stored.refresh_token, "r");
} }
/// The only test that touches the keyring, through `keyring-core`'s /// The keyring is one store per process, so the tests that use it,
/// in-memory store. /// through `keyring-core`'s in-memory store, take turns.
fn mock_keyring() -> parking_lot::MutexGuard<'static, ()> {
static KEYRING: parking_lot::Mutex<()> = parking_lot::Mutex::new(());
let turn = KEYRING.lock();
keyring_core::set_default_store(keyring_core::mock::Store::new().unwrap());
turn
}
fn stored_with(refresh: &str) -> StoredSession {
serde_json::from_str(&blob("").replace(r#""r""#, &format!("{refresh:?}"))).unwrap()
}
#[test] #[test]
fn a_sink_keeps_writing_its_own_rotations_and_stops_at_a_new_login() { fn a_sink_keeps_writing_its_own_rotations_and_stops_at_a_new_login() {
keyring_core::set_default_store(keyring_core::mock::Store::new().unwrap()); let _turn = mock_keyring();
let stored = |refresh: &str| { write_stored(&stored_with("r")).unwrap();
serde_json::from_str::<StoredSession>(
&blob("").replace(r#""r""#, &format!("{refresh:?}")),
)
.unwrap()
};
write_stored(&stored("r")).unwrap();
let sink = TokenSink::new("s", "r".into()); let sink = Arc::new(TokenSink::new("s", "r".into()));
sink.rotated(tokens("a2", "r2")).unwrap(); sink.rotated(tokens("a2", "r2")).unwrap();
sink.rotated(tokens("a3", "r3")).unwrap(); sink.rotated(tokens("a3", "r3")).unwrap();
assert_eq!(load().unwrap().refresh_token, "r3"); assert_eq!(load().unwrap().refresh_token, "r3");
// A new login replaces the blob; the old session's next rotation, and // A new login replaces the blob; the old session's next rotation, and
// its stop, leave it alone. // its stop, leave it alone.
let mut login = stored("fresh"); let mut login = stored_with("fresh");
login.session_id = "new".into(); login.session_id = "new".into();
write_stored(&login).unwrap(); write_stored(&login).unwrap();
sink.rotated(tokens("a4", "r4")).unwrap(); sink.rotated(tokens("a4", "r4")).unwrap();
@@ -975,4 +1045,32 @@ mod tests {
assert_eq!(now.session_id, "new"); assert_eq!(now.session_id, "new");
assert_eq!(now.refresh_token, "fresh"); assert_eq!(now.refresh_token, "fresh");
} }
/// B61: a rotation the keyring refused lived only in memory.
#[test]
fn a_rotation_the_keyring_refused_is_written_once_it_takes_it() {
let _turn = mock_keyring();
write_stored(&stored_with("r")).unwrap();
let sink = Arc::new(TokenSink::new("s", "r".into()));
let entry = keyring_entry().unwrap();
let cred = entry
.as_any()
.downcast_ref::<keyring_core::mock::Cred>()
.unwrap();
cred.set_error(keyring_core::Error::NoStorageAccess(
std::io::Error::other("locked").into(),
));
assert!(sink.rotated(tokens("a2", "r2")).is_err());
let deadline = std::time::Instant::now() + Duration::from_secs(5);
while load().unwrap().refresh_token != "r2" {
assert!(
std::time::Instant::now() < deadline,
"the retry never wrote the rotation"
);
std::thread::sleep(Duration::from_millis(50));
}
assert!(sink.state.lock().unsaved.is_none());
}
} }
+12 -2
View File
@@ -73,7 +73,6 @@ original wording.
| [B67](#b67--fresh-state-restore-can-silently-select-a-new-device-high-22) | Fresh-state restore can silently select a new device (HIGH-22) | An ambiguous device after a loss of local state is not asked about | | [B67](#b67--fresh-state-restore-can-silently-select-a-new-device-high-22) | Fresh-state restore can silently select a new device (HIGH-22) | An ambiguous device after a loss of local state is not asked about |
| [B66](#b66--local-only-pending-data-is-not-protected-at-shutdown-high-21) | Local-only pending data is not protected at shutdown (HIGH-21) | No shutdown protection and no `pdfs sync flush` | | [B66](#b66--local-only-pending-data-is-not-protected-at-shutdown-high-21) | Local-only pending data is not protected at shutdown (HIGH-21) | No shutdown protection and no `pdfs sync flush` |
| [B65](#b65--release-artifacts-lack-supply-chain-verification-med-11) | Release artifacts lack supply-chain verification (MED-11) | No SBOM, provenance or checksums | | [B65](#b65--release-artifacts-lack-supply-chain-verification-med-11) | Release artifacts lack supply-chain verification (MED-11) | No SBOM, provenance or checksums |
| [B61](#b61--rotated-single-use-credentials-can-be-lost-high-18) | Rotated single-use credentials can be lost (HIGH-18) | Keyring write failures are only logged |
| [B15](#b15--an-empty-but-listed-folder-and-duplicated-folder-uids) | An empty-but-`listed` folder, and duplicated folder uids | Cause not attributed | | [B15](#b15--an-empty-but-listed-folder-and-duplicated-folder-uids) | An empty-but-`listed` folder, and duplicated folder uids | Cause not attributed |
| [B10](#b10--glib-critical-when-the-launcher-closes-over-an-in-flight-open) | GLib critical when the launcher closes over an in-flight open | | | [B10](#b10--glib-critical-when-the-launcher-closes-over-an-in-flight-open) | GLib critical when the launcher closes over an in-flight open | |
@@ -88,6 +87,7 @@ original wording.
| [B68](#b68--profile-backup-cannot-be-written-to-the-device-root) | Profile backup cannot be written to the device root | Backup health is not shown in the app; the restore was not re-run | | [B68](#b68--profile-backup-cannot-be-written-to-the-device-root) | Profile backup cannot be written to the device root | Backup health is not shown in the app; the restore was not re-run |
| [B64](#b64--stable-publishing-lacks-data-safety-and-recovery-gates-crit-12) | Stable publishing lacks data-safety and recovery gates (CRIT-12) | Recovery approval before a release | | [B64](#b64--stable-publishing-lacks-data-safety-and-recovery-gates-crit-12) | Stable publishing lacks data-safety and recovery gates (CRIT-12) | Recovery approval before a release |
| [B63](#b63--release-version-identity-is-inconsistent-and-unenforced-high-20) | Release version identity is inconsistent and unenforced (HIGH-20) | Protocol version identity | | [B63](#b63--release-version-identity-is-inconsistent-and-unenforced-high-20) | Release version identity is inconsistent and unenforced (HIGH-20) | Protocol version identity |
| [B61](#b61--rotated-single-use-credentials-can-be-lost-high-18) | Rotated single-use credentials can be lost (HIGH-18) | A crash or SIGKILL before a retried write lands still loses it; the GUI is not told |
| [B57](#b57--event-cursor-can-advance-past-unapplied-state-high-14) | Event cursor can advance past unapplied state (HIGH-14) | Database and apply fault tests | | [B57](#b57--event-cursor-can-advance-past-unapplied-state-high-14) | Event cursor can advance past unapplied state (HIGH-14) | Database and apply fault tests |
| [B56](#b56--database-schema-version-parsing-fails-open-high-13) | Database schema version parsing fails open (HIGH-13) | Malformed schema versions | | [B56](#b56--database-schema-version-parsing-fails-open-high-13) | Database schema version parsing fails open (HIGH-13) | Malformed schema versions |
| [B12](#b12--cold-enumeration-is-slow-per-entry-and-goes-superlinear-past-500) | Cold enumeration is slow per entry, and goes superlinear past ~500 | The attribute-invalidation follow-up is unverified | | [B12](#b12--cold-enumeration-is-slow-per-entry-and-goes-superlinear-past-500) | Cold enumeration is slow per entry, and goes superlinear past ~500 | The attribute-invalidation follow-up is unverified |
@@ -4921,7 +4921,8 @@ daemon/tray/FUSE, upgrade, rollback, and uninstall without deleting user state.
### B61 — Rotated single-use credentials can be lost (HIGH-18) ### B61 — Rotated single-use credentials can be lost (HIGH-18)
**Status:** Open — 1.0 authentication/recovery blocker **Status:** Partly fixed. A process that dies before a retried write lands still loses the
rotation, and only the log says so.
**Found:** 2026-07-22, 1.0 authentication audit **Found:** 2026-07-22, 1.0 authentication audit
**Where:** `crates/pdfs-core/src/auth.rs`, refresh callback **Where:** `crates/pdfs-core/src/auth.rs`, refresh callback
@@ -4933,6 +4934,15 @@ keyring retains an invalid single-use refresh token, so restart loses login.
notify the user, and define a safe re-login/shutdown path. Test locked/full/ notify the user, and define a safe re-login/shutdown path. Test locked/full/
unavailable keyrings, callback races, repeated rotations, death, and restart. unavailable keyrings, callback races, repeated rotations, death, and restart.
**Fix (2026-10-09, issue #35).** A rotation the keyring refuses is kept as unsaved and retried
in the background after 1 s, 5 s, 30 s and then every 5 minutes, until it is written or the blob
has moved on (B201). The refusal is an error in the log naming `pdfs login`, and the daemon's stop
writes a rotation still unsaved, logging an error if that fails too. The re-login path is B201's:
an old session never writes into a newer login. Left: a crash or SIGKILL before a retry lands,
and telling the GUI.
**Test:** `auth::tests::a_rotation_the_keyring_refused_is_written_once_it_takes_it`.
--- ---
### B60 — Config writes are non-atomic and parse errors are overwritten (HIGH-17) ### B60 — Config writes are non-atomic and parse errors are overwritten (HIGH-17)