Merge pull request #89 from narrrl/fix/78-retain-unsaved-session-tokens

fix(core): retain unsaved session tokens

crates/pdfs-core/src/auth.rs

@@ -996,21 +996,15 @@ impl TokenSink {
         }
     }
 
-    /// Write the unsaved rotation, if there is one. A blob that moved on, or
-    /// is gone with a logout, drops it: there is nothing it may still go into.
+    /// Write the unsaved rotation, if there is one. A blob that moved on drops
+    /// it. A missing blob is retried: some Secret Service providers also report
+    /// locked or temporarily hidden items as absent. Never recreate it here.
     fn flush(&self) -> Result<()> {
         let mut state = self.state.lock();
         let Some(tokens) = state.unsaved.clone() else {
             return Ok(());
         };
-        let mut stored = match load() {
-            Ok(stored) => stored,
-            Err(Error::NotLoggedIn) => {
-                state.settle();
-                return Ok(());
-            }
-            Err(e) => return Err(e),
-        };
+        let mut stored = load()?;
         match take_rotation(
             &mut stored,
             &self.session_id,
@@ -1306,6 +1300,47 @@ mod tests {
         }
     }
 
+    /// oo7 0.7.0 reports a locked collection's items as absent (upstream #614).
+    /// Treating that as logout loses the only live tokens (B284, issue #78).
+    #[test]
+    fn a_temporarily_missing_session_does_not_discard_a_rotation() {
+        let _turn = mock_keyring();
+        write_stored(&stored_with("r")).unwrap();
+        let sink = Arc::new(TokenSink::new("s", "r".into()));
+        let entry = keyring_entry().unwrap();
+        entry
+            .as_any()
+            .downcast_ref::<keyring_core::mock::Cred>()
+            .unwrap()
+            .set_error(keyring_core::Error::NoEntry);
+
+        assert!(sink.rotated(tokens("a2", "r2")).is_err());
+        assert!(sink.state.lock().unsaved.is_some());
+        assert_eq!(load().unwrap().refresh_token, "r");
+
+        sink.flush().unwrap();
+        assert_eq!(load().unwrap().refresh_token, "r2");
+        assert!(sink.state.lock().unsaved.is_none());
+    }
+
+    #[test]
+    fn retrying_a_missing_session_never_recreates_it_or_overwrites_a_new_login() {
+        let _turn = mock_keyring();
+        let sink = TokenSink::new("s", "r".into());
+        sink.state.lock().unsaved = Some(tokens("a2", "r2"));
+
+        assert!(matches!(sink.flush(), Err(Error::NotLoggedIn)));
+        assert!(matches!(load(), Err(Error::NotLoggedIn)));
+        assert!(sink.state.lock().unsaved.is_some());
+
+        let mut login = stored_with("fresh");
+        login.session_id = "new".into();
+        write_stored(&login).unwrap();
+        sink.flush().unwrap();
+        assert!(sink.state.lock().unsaved.is_none());
+        assert_eq!(load().unwrap().refresh_token, "fresh");
+    }
+
     #[test]
     fn an_unsaved_rotation_goes_only_into_the_blob_it_was_refused_for() {
         let unsaved = UnsavedRotation {

docs/BUGS.md

@@ -109,6 +109,7 @@ a live account since.
 
 | Entry | Title |
 |---|---|
+| [B284](#b284--a-keyring-entry-reported-missing-discards-refreshed-session-tokens) | A keyring entry reported missing discards refreshed session tokens |
 | [B283](#b283--videos-have-no-thumbnail-outside-photos-and-a-ts-recording-looks-like-source-code) | Videos have no thumbnail outside Photos, and a `.ts` recording looks like source code |
 | [B282](#b282--a-file-kept-offline-is-downloaded-again-after-a-change-made-elsewhere) | A file kept offline is downloaded again after a change made elsewhere |
 | [B281](#b281--a-file-in-a-synced-folder-cannot-be-kept-offline) | A file in a synced folder cannot be kept offline |
@@ -386,6 +387,34 @@ a live account since.
 
 Newest first.
 
+### B284 — A keyring entry reported missing discards refreshed session tokens
+
+**Status:** Fixed (unverified)
+**Found:** 2026-11-14, investigating [issue #78](https://github.com/narrrl/proton-drive-linux/issues/78)
+after the reporter's oo7 0.7.0 retest ruled out the original persistence fix as a sufficient
+explanation. See [the investigation and isolated reproducer](ISSUE-78.md).
+**Where:** `crates/pdfs-core/src/auth.rs`, `TokenSink::flush`
+
+**Repro:** Refresh a session while the keyring lookup reports `NoEntry` for its existing blob.
+oo7 0.7.0 does this for a locked collection (upstream #614), reproduced with fake tokens on a
+private D-Bus connection. pdfs dropped the pending rotation and returned success, leaving the
+spent refresh token in the saved blob without triggering B61's retry or kernel recovery paths.
+The running session can still work, but restarting it exposes the stale saved credentials.
+
+**Fix:** Propagate the missing-session error so the rotation stays pending and is retried.
+Persistence still requires the matching existing blob; a retry never recreates a deleted
+session or overwrites a new login. This does not make a locked keyring writable, and pending
+tokens still need to reach it before power-off to survive a cold boot.
+
+**Verified:** The two regression tests
+`auth::tests::a_temporarily_missing_session_does_not_discard_a_rotation` and
+`auth::tests::retrying_a_missing_session_never_recreates_it_or_overwrites_a_new_login`
+failed before the fix and pass afterward. The oo7 fixture confirms its locked lookup behavior
+and repeated binary `SetSecret` persistence. The reporter's complete Fedora cold-boot sequence
+has not been reproduced; issue #78's precise trigger remains unconfirmed.
+
+---
+
 ### B283 — Videos have no thumbnail outside Photos, and a `.ts` recording looks like source code
 
 **Status:** Fixed, not verified live

docs/ISSUE-78.md

@@ -0,0 +1,91 @@
+# Issue #78: session loss on Fedora 45
+
+Investigation on 2026-11-14 of [issue #78](https://github.com/narrrl/proton-drive-linux/issues/78),
+using its report and comments through `gh`, pdfs 3.7.0/current source, and the oo7 0.7.0 tag.
+
+## What is established
+
+The reporter still saw the problem with oo7 0.7.0 on November 7. The original
+[SetSecret persistence fix](https://github.com/linux-credentials/oo7/commit/7fb11dd111c4be8fec01996e3caa31c44fa962b3)
+is present in that release. In an isolated test, three successive binary `SetSecret` updates
+survived reopening the keyring file. Repeating the original persistence explanation is therefore
+insufficient.
+
+A different, reproducible failure exists at the boundary between oo7 and pdfs:
+
+1. oo7 0.7.0's `Collection::search_inner_items` returns an empty list for a locked collection.
+   `SearchItems` consequently returns **both** the unlocked and locked lists empty even when the
+   matching item is on disk. The isolated test reproduces this with pdfs's exact lookup attributes.
+   This is upstream [oo7 #614](https://github.com/linux-credentials/oo7/issues/614).
+2. `dbus-secret-service-keyring-store` 1.0.1 maps an empty search to `keyring_core::Error::NoEntry`;
+   pdfs maps that to `Error::NotLoggedIn`. The client cannot offer an unlock when no locked item
+   is reported.
+3. In pdfs 3.7.0 and the code before this change, `TokenSink::flush` treated `NotLoggedIn` as a
+   logout, cleared the pending rotation and returned success. It skipped the error path that
+   retains the new tokens in the kernel keyring and retries their persistence. The code was
+   unchanged between v3.7.0 and this investigation.
+4. If a token refresh encounters that reply, the saved blob retains a spent refresh token. The
+   running process can keep working with the new tokens in memory; a restart then exposes the
+   stale saved credentials. A regression test failed before the fix and passes afterward.
+
+The same unsafe assumption also applies to transient empty lookups such as the race reported in
+[oo7 #652](https://github.com/linux-credentials/oo7/issues/652), fixed in 0.7.1. That issue's
+replacement race was not independently reproduced here; pdfs normally updates existing items
+with `SetSecret`, so it must not be assumed to encounter the same-item `CreateItem` race.
+
+## Change and limits
+
+`TokenSink::flush` now propagates a missing-session error instead of discarding the rotation.
+The existing retry, kernel-keyring recovery and unsaved-token status paths can then run. Once
+the collection is unlocked and readable, the pending rotation is saved. A genuinely deleted
+session is never recreated: flushing still requires a readable blob with the matching session
+ID and expected refresh token. A new login causes the old rotation to be discarded as before.
+
+This is a confirmed pdfs defect (B284), **not a confirmed complete explanation of the reporter's
+cold-boot failure**. The issue has no oo7 logs or record showing that its collection was locked
+when a token rotated. A locked keyring only at startup can hide a login, but does not by itself
+explain `InvalidRefreshToken`. The token-loss path requires a successful refresh while the saved
+entry is unreadable.
+
+The fix does not make a locked keyring writable. If it stays unreadable until power-off, tokens
+retained in process/kernel memory still cannot survive a cold boot. No new disk credential store
+or credential format is introduced.
+
+Other upstream Fedora-upgrade issues were checked: [oo7 #651](https://github.com/linux-credentials/oo7/issues/651)
+creates duplicate placeholder/migrated collections and refuses writes; [oo7 #662](https://github.com/linux-credentials/oo7/issues/662)
+can resolve the default alias to a locked collection. Both have fixes in
+[0.7.1](https://github.com/linux-credentials/oo7/releases/tag/0.7.1), released November 10,
+after the reporter's November 7 retest. Neither can be assigned as this report's cause without
+matching runtime evidence.
+
+## Reproduce without a Proton account or desktop keyring
+
+The oo7 fixture uses fake tokens, a temporary disk keyring and a private peer-to-peer D-Bus
+connection. It asserts the observed 0.7.0 behavior, including the incorrect empty locked lookup;
+passing means that behavior was reproduced. It does not simulate Proton refresh or a full boot.
+
+From the pdfs repository, choose an unused temporary checkout path:
+
+```sh
+git clone --depth 1 --branch 0.7.0 https://github.com/linux-credentials/oo7.git /tmp/pdfs-oo7-repro
+git -C /tmp/pdfs-oo7-repro apply "$PWD/docs/repro/oo7-0.7.0-issue78.patch"
+cargo test --manifest-path /tmp/pdfs-oo7-repro/Cargo.toml \
+  -p oo7-daemon --lib --features test-util issue78 -- --nocapture
+```
+
+Observed output:
+
+```text
+oo7 0.7.0: locked session is on disk but SearchItems returns (0, 0)
+oo7 0.7.0: all three binary SetSecret updates survived reopening the disk keyring
+test item::tests::issue78_locked_lookup_hides_saved_tokens ... ok
+```
+
+The pdfs side is covered by `cargo test -p pdfs-core auth::tests`, specifically
+`a_temporarily_missing_session_does_not_discard_a_rotation` and
+`retrying_a_missing_session_never_recreates_it_or_overwrites_a_new_login`.
+
+Remaining live validation: reproduce on a Fedora 45 GNOME session across logout/shutdown,
+correlating oo7 lock/write errors with pdfs refresh and retry logs, and checking the running
+daemon's package version rather than just the CLI version. The investigation did not use a real
+Proton account, alter a desktop keyring, or post to either GitHub issue.

docs/repro/oo7-0.7.0-issue78.patch

@@ -0,0 +1,53 @@
+diff --git a/server/src/item/tests.rs b/server/src/item/tests.rs
+index 99c25dc..2ba705d 100644
+--- a/server/src/item/tests.rs
++++ b/server/src/item/tests.rs
+@@ -5,6 +5,48 @@ use tokio_stream::StreamExt;
+
+ use crate::{service::PrompterType, tests::TestServiceSetup};
+
++// Investigation for proton-drive-linux #78. Fake tokens and a private p2p bus;
++// no desktop keyring or Proton account is used.
++#[tokio::test]
++async fn issue78_locked_lookup_hides_saved_tokens() -> Result<(), Box<dyn std::error::Error>> {
++    let setup =
++        TestServiceSetup::with_login_keyring(oo7::Secret::from("test-password-long-enough"))
++            .await?;
++    let collection = setup.default_collection().await?;
++    let attributes = [("service", "proton-drive-linux"), ("username", "session")];
++    let secret = setup.create_dbus_secret(oo7::Secret::blob(b"fake-refresh-r1"))?;
++    collection
++        .create_item("pdfs fake session", &attributes, &secret, false, None)
++        .await?;
++
++    let (unlocked, locked) = setup.service_api.search_items(&attributes).await?;
++    assert_eq!((unlocked.len(), locked.len()), (1, 0));
++    setup.lock_collection(collection).await?;
++    assert!(collection.is_locked().await?);
++
++    let (unlocked, locked) = setup.service_api.search_items(&attributes).await?;
++    // This is the problematic reply, not the Secret Service behavior expected
++    // by keyring-store: the saved item is neither unlocked nor reported locked.
++    assert_eq!((unlocked.len(), locked.len()), (0, 0));
++    assert_eq!(setup.login_keyring().await?.items().await?.len(), 1);
++    eprintln!("oo7 0.7.0: locked session is on disk but SearchItems returns (0, 0)");
++
++    setup.unlock_collection(collection).await?;
++    let (unlocked, locked) = setup.service_api.search_items(&attributes).await?;
++    assert_eq!((unlocked.len(), locked.len()), (1, 0));
++
++    // Exercise the binary SetSecret path used by pdfs's keyring-store. Reopen
++    // the disk keyring after each update, rather than trusting a memory read.
++    for refresh in [b"fake-refresh-r2", b"fake-refresh-r3", b"fake-refresh-r4"] {
++        let secret = setup.create_dbus_secret(oo7::Secret::blob(refresh))?;
++        unlocked[0].set_secret(&secret).await?;
++        let items = setup.login_keyring().await?.items().await?;
++        assert_eq!(items[0].secret().as_bytes(), refresh);
++    }
++    eprintln!("oo7 0.7.0: all three binary SetSecret updates survived reopening the disk keyring");
++    Ok(())
++}
++
+ #[tokio::test]
+ async fn label_property() -> Result<(), Box<dyn std::error::Error>> {
+     let setup = TestServiceSetup::plain_session(true).await?;

scripts/publish-wiki.sh

@@ -91,7 +91,7 @@ SECTIONS = [
 ]
 
 # Too long or too internal for the wiki; links to them go to GitHub.
-EXCLUDED = {"BUGS.md", "CHANGELOG.md", "SDK-0.11.0.md"}
+EXCLUDED = {"BUGS.md", "CHANGELOG.md", "ISSUE-78.md", "SDK-0.11.0.md"}
 
 # Files outside docs/ whose links are checked but not published.
 CHECKED = ["README.md", "CONTRIBUTING.md", "SECURITY.md"]