Repository navigation
fix(sync): recover credentials after secure storage failures - #617
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70fd6da130
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| override fun getAll(): Map<String, *> = access { it.all } | ||
| override fun getString(key: String?, defValue: String?): String? = access { it.getString(key, defValue) } |
There was a problem hiding this comment.
When the Keystore is unavailable, these reads now throw instead of using the previous in-memory fallback, but SettingsBackupRestoreSection.loadBackupSyncPreferenceSnapshot() still calls getLegacyPlayHistoryUpdateModeName() and isDataSaverMode() without handling SyncStorageUnavailableException. Its caller is an unguarded LaunchedEffect, so opening the backup/sync settings during a storage outage produces an uncaught exception and crashes the app, rather than allowing the user to reach the newly added credential-recovery UI. Handle the unavailable-storage result in this loader as well, without treating it as an empty saved configuration.
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 8.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check 标题准确概括了在安全存储故障后恢复同步凭据这一主要变更，且简洁明确。 Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 8.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 34 functions across 40 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
WebDavStorage.kt:49-54
modules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/webdav/WebDavStorage.kt:49-54
Minor · Data Integrity & Integration · Quick win WebDAV 凭据保存未确认落盘，但界面会报告成功
本 PR 让 GitHub 的
saveToken/saveRepository使用requirePersistence = true。WebDAV 的saveConfiguration仍然调用默认的editTarget，并通过apply()异步写盘。
WebDavSyncViewModel.validateAndSaveConfiguration在saveConfiguration返回后会设置isConfigured = true并显示webdav_validate_success。如果磁盘写入失败，apply()不会抛出异常。界面仍显示成功，但应用重启后服务器地址和密码会丢失。
WebDavSyncCredentialRecoveryTest中模拟saveConfiguration抛出IOException的路径在生产代码中无法发生。请与 GitHub 保持一致，并要求提交成功。🐛 建议修复
modules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/secure/RecoverableSyncPreferences.kt: 1
93-97:Minor · Data Integrity & Integration · Quick win 拒绝旧编辑器时不应使当前健康会话失效
use在access { }内部抛出SyncStorageUnavailableException。access的catch会把任何异常当作存储故障处理。它会执行session++、注销监听器并清空current。下一次访问会重新打开存储。由于openedBefore == true，invalidateSessions会再次递增KEY_SYNC_MUTATION_VERSION和KEY_CONFIGURATION_GENERATION。触发条件：后台线程在故障前取得编辑器，恢复后再调用
putX/commit。结果是已恢复的健康会话被再次作废。正在进行的同步会失去captureSyncMetadataGuard的有效性，完成时间无法写入。*IfMutationVersion写入也会因版本变化而失败。旧编辑器每调用一次stage，这个过程就会重复一次。旧编辑器检查只表示调用方使用了过期句柄。底层存储并没有发生故障。请在
access外部完成会话比较，或者在catch中跳过这类拒绝，不重置current。🐛 建议修复
-
-
- Source mode: Auto
- Repository: cwuom/NeriPlayer
- Branch: fix/sync-credential-recovery
- File: .coderabbit.yaml
- Commit: 70fd6da
- Layers: Repository YAML
- Review profile: QUIET
- Plan: Advanced
- Run ID:
51036ec6-4efa-425b-a664-204df74f4a8f
-
app/src/main/java/moe/ouom/neriplayer/core/startup/sync/StartupSyncScheduler.ktapp/src/main/java/moe/ouom/neriplayer/core/startup/sync/StartupSyncWarningCoordinator.ktapp/src/main/java/moe/ouom/neriplayer/ui/screen/tab/settings/dialog/SettingsGitHubDialogs.ktapp/src/main/java/moe/ouom/neriplayer/ui/screen/tab/settings/dialog/SettingsWebDavDialogs.ktapp/src/main/java/moe/ouom/neriplayer/ui/sync/upgrade/SyncProtocolUpgradeEntry.ktapp/src/main/java/moe/ouom/neriplayer/ui/sync/upgrade/SyncProtocolUpgradeViewModel.ktapp/src/main/java/moe/ouom/neriplayer/ui/viewmodel/GitHubSyncViewModel.ktapp/src/main/java/moe/ouom/neriplayer/ui/viewmodel/WebDavSyncViewModel.ktapp/src/test/java/moe/ouom/neriplayer/core/startup/sync/StartupSyncSchedulerFlowTest.ktapp/src/test/java/moe/ouom/neriplayer/core/startup/sync/StartupSyncWarningCoordinatorTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/screen/tab/settings/dialog/SettingsGitHubCredentialDialogTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/screen/tab/settings/dialog/SettingsWebDavCredentialStorageTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/sync/upgrade/SyncProtocolStartupStorageFailureTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/sync/upgrade/SyncProtocolUpgradeFailureTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/viewmodel/GitHubSyncCredentialRecoveryTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/viewmodel/SyncSettingsActionsTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/viewmodel/SyncViewModelCancellationTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/viewmodel/SyncViewModelCompletionTimeTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/viewmodel/SyncViewModelTargetBindingTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/viewmodel/WebDavSyncConfigurationOrderingTest.ktapp/src/test/java/moe/ouom/neriplayer/ui/viewmodel/WebDavSyncCredentialRecoveryTest.ktmodules/common/src/main/res/values-en/strings_sync_backup.xmlmodules/common/src/main/res/values-zh/strings_sync_backup.xmlmodules/common/src/main/res/values/strings_sync_backup.xmlmodules/local/src/main/java/moe/ouom/neriplayer/data/playlist/favorite/FavoritePlaylistRepository.ktmodules/local/src/main/java/moe/ouom/neriplayer/data/sync/github/GitHubSyncBackend.ktmodules/local/src/main/java/moe/ouom/neriplayer/data/sync/host/SyncProtocolUpgradeRepository.ktmodules/local/src/test/java/moe/ouom/neriplayer/data/config/ConfigFileManagerExportTest.ktmodules/local/src/test/java/moe/ouom/neriplayer/data/playlist/favorite/FavoritePlaylistRepositoryEditingTest.ktmodules/local/src/test/java/moe/ouom/neriplayer/data/sync/host/SyncBackendTransportTest.ktmodules/local/src/test/java/moe/ouom/neriplayer/data/sync/host/SyncProtocolUpgradeRepositoryTest.ktmodules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/github/SecureTokenStorage.ktmodules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/secure/EncryptedSyncPreferences.ktmodules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/secure/RecoverableSyncPreferences.ktmodules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/secure/SyncPreferenceRecovery.ktmodules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/state/GitHubSyncConfigurationStore.ktmodules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/state/SyncCompletionTimeStore.ktmodules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/state/SyncStoreKeys.ktmodules/sync/src/main/java/moe/ouom/neriplayer/data/sync/store/webdav/WebDavStorage.ktmodules/sync/src/test/java/moe/ouom/neriplayer/data/sync/store/secure/RecoverableSyncPreferencesTest.ktmodules/sync/src/test/java/moe/ouom/neriplayer/data/sync/store/secure/SyncPreferenceRecoveryTest.ktmodules/sync/src/test/java/moe/ouom/neriplayer/data/sync/store/state/SyncConfigurationStorageTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
-
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31465383f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| override fun getAll(): Map<String, *> = access { it.all } | ||
| override fun getString(key: String?, defValue: String?): String? = access { it.getString(key, defValue) } |
There was a problem hiding this comment.
When the Keystore is unavailable, playing a non-local song now causes PlayHistoryRepository.record() to throw from storage.removeRecentPlayDeletion(song.identityKey()) after saving the history entry. updateRememberedPlaybackPosition() has the same unguarded call. Both run in scope.launch under a SupervisorJob without an exception handler, so the new throwing reads produce an uncaught exception and crash the app—even when cloud sync is not configured. Handle SyncStorageUnavailableException at these playback-history boundaries while preserving the saved local history and existing durable deletion state.
Useful? React with 👍 / 👎.
| override fun onFailure(error: Throwable) { | ||
| if (error is TokenExpiredException) storage.clearToken() | ||
| } | ||
| override fun onFailure(error: Throwable) = Unit |
There was a problem hiding this comment.
When an automatic GitHub sync receives a 401, removing the backend's token clearing leaves both isConfigured() and isAutoSyncEnabled() true. The replacement pause logic exists only in GitHubSyncViewModel.authenticationFailed(), which background workers never invoke: createGitHubWorkerHost() delegates to SyncRepositoryWorkerHost.handleFailure(), which only reports the failure. Consequently, subsequent favorite/history changes continue scheduling requests with the expired token, and SyncWorkerFailurePolicy.github() requests a notification for every authentication failure, even with silent failures enabled. Preserve the credentials, but add a configuration-guarded pause to the background authentication-failure path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de5b185ba1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val lyricOverridesVersion: StateFlow<Long> | ||
| get() = lyricOverrides.version |
There was a problem hiding this comment.
If secure storage is unavailable when the player initializes, accessing this property initializes SyncLyricOverrideStore, whose notificationMarker() reads the unavailable preferences and throws. In PlayerManagerLifecycleExtensions.observeCommittedLyricOverrides(), this happens before collection starts, so the outer catch logs the error and permanently exits the observer. Recovering credentials later does not restart that observer, meaning subsequently synchronized lyric overrides no longer update the current song or playback queue until the player is restarted. Keep the notification flow obtainable without reading encrypted state, or retry the subscription after storage recovery.
Useful? React with 👍 / 👎.
描述 / Description
Secure-storage failures could leave GitHub/WebDAV sync stuck in a cached fallback, while stale token-validation state prevented saving a replacement token. A late GitHub 401 could also erase newer credentials and repository settings.
类型 / Type
修复或解决的问题 / Issues Fixed or Closed by This PR
Reported directly: token replacement could not recover sync, and the sync database version dialog could keep failing after retry. Opening backup settings or mutating playback history during a secure-storage outage could also crash. Background GitHub authentication failures could repeatedly schedule requests and notifications.
清单 / Checklist
其他信息 / Additional Information
Validation:
The complete CI Gradle gate passed locally on
370b4dfc0with JDK 21: 15,207 JVM cases, 0 failures/errors and 7 skipped; lint, CRAP and dependency checks passed.75 focused history/backend/worker regressions passed after reproducing the failures. Coverage includes saved history and durable deletion preservation, recovery retries, typed exception boundaries, automatic-sync pause, stale 401 responses across storage instances and unavailable storage while pausing.
The directory-refresh regression passed in five independent executions (15 cases). The fixture waits for its own operation and waiter to be released before asserting state or restoring globals, using the original deadline and assertions.
26 focused settings regressions passed, including unavailable reads, deferred legacy migration, partial preference snapshots, cancellation, recovery UI access, failed saves and import/read interleavings with a controlled IO dispatcher.
The settings-card color failure was reproduced in CI on
370b4dfc0. The test-only follow-up16bc12f1ereads committed Activity-window pixels instead of a screenshot of the display, retaining all RGB/layout assertions and timeouts. Three independent API 35 ARM64 runs passed all 54 cases with no skips (swangle, 1080x2400, 420 dpi); debug/test APK builds and app lint passed.The storage follow-up
de5b185bareproduced both review findings before the fix. Its 19 focused cases then passed; the broader local gate passed 1,436 cases with 0 failures/errors and 5 opt-in skips, plus sync/app lint, sync CRAP, adapter CRAP and dependency checks.Complete remote Android CI passed on
16bc12f1e(run 41099337431): all 11 validation jobs succeeded, including the previously failing card-color case; the instrumentation inventory records 1,658 passed and 43 skipped.The lyric-observer follow-up
6f2df24b3reproduced the subscription failure before the fix. All 30 focused cases passed; the wider gate passed 1,406 cases with 0 failures/errors and 5 existing opt-in skips (1,052 sync, 333 adapter and 21 playback lyric cases). Sync lint, CRAP, dependency checks and adapter CRAP passed. Coverage includes strict initial read failures, recovery on the same subscription, failed-commit silence, successful retries, duplicate suppression and exception/cancellation propagation.The test-only sheet-dismissal follow-up
e3495f15caddresses the 3,000 ms dismissal timeout in CI on6f2df24b3(run 41109387596). It advances the frozen virtual clock through the hide animation before the original 3,000 ms wall-clock removal wait, retaining all gesture, geometry and callback assertions. Three independent API 35 ARM64 runs passed 27 cases with no skips (swangle, 1080x2400, 420 dpi); test APK assembly and app lint passed. The unmodified target passed once locally, so the slow CI timing was not reproduced locally. Complete remote Android CI passed one3495f15c2fc3da6639c7470c9372d1ed3b2c7ed(run 41119700108): all 11 required jobs succeeded; the Telegram upload was skipped by its branch condition. The inventory verifier confirmed complete, disjoint instrumentation routes with 1,663 passed and 41 skipped cases. Both historical failures actually executed successfully: all 9MoreOptionsSheetPredictiveBackTestcases and all 16MusicServerSettingsLayoutTestcases passed without skips.Secure-storage outages were simulated in JVM/Robolectric tests. A real TEE outage was not reproduced on the physical device.