TF-4606 Fix cannot download attachment#4608
Conversation
…ailureAction and exportAllAttachmentsFailureAction
…enOidc and persistOneTokenOidc
…rupted box recovery scenario
|
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:
WalkthroughThis PR adds guarded recovery around TokenOidc cache reads/writes (try/catch, safe clear), introduces MemoryTokenOidcCacheClient and StubTokenOidcCacheClient for tests, and adds tests for cache recovery and persistence. It also adds ExportAttachmentRequest and ExportAllAttachmentsRequest models, refactors ExportAttachmentInteractor and ExportAllAttachmentsInteractor to accept requests and use fallback tokens with repair persistence, updates controller wiring to build those requests, exposes AuthorizationInterceptors.currentToken, and forwards failure.exception to export failure actions. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
How about we should support boxItem.flush() inside Future<void> insertItem(
String key,
T newObject, {
bool isolated = true,
bool flushAfterWrite = false,
}) {
log('$runtimeType::insertItem:encryption: $encryption - key = $key - isolated = $isolated');
return Future.sync(() async {
if (isolated) {
final boxItem = await openIsolatedBox();
await boxItem.put(key, newObject);
// Flush pending writes to reduce the risk of losing recently written
// encrypted data if the app is terminated immediately after a write.
// Improves durability; does NOT guarantee atomic persistence. No-op on
// web (IndexedDB writes are already transactional).
if (flushAfterWrite) await boxItem.flush();
} else {
final boxItem = await openBox();
await boxItem.put(key, newObject);
if (flushAfterWrite) await boxItem.flush();
}
}).catchError((error) {
throw error;
});
} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/login/data/local/token_oidc_cache_manager.dart`:
- Around line 43-72: persistOneTokenOidc currently wraps both
_tokenOidcCacheClient.insertItem(...) and _removeStaleTokens(...) in one
try/catch so any error from the initial insert triggers full box recovery;
change this so the insert is performed outside the recovery catch and only
_removeStaleTokens is wrapped to trigger recovery on failure. Concretely: call
await _tokenOidcCacheClient.insertItem(tokenOIDC.tokenIdHash, ...) first and let
AppBaseException rethrow as before; then wrap only await
_removeStaleTokens(keepKey: ...) in a try/catch that on non-AppBaseException
logs and calls _recoverBoxWithToken(tokenOIDC). Also update _recoverBoxWithToken
to not silently swallow a failed re-insert (make it propagate or surface the
error after logging) so insertItem failures aren’t ignored; refer to
persistOneTokenOidc, _removeStaleTokens, _recoverBoxWithToken, _safelyClearBox
and insertItem to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65dfe16a-e9dc-4585-8ea3-74bacd045cd4
📒 Files selected for processing (4)
lib/features/download/presentation/controllers/download_controller.dartlib/features/login/data/local/token_oidc_cache_manager.darttest/features/login/data/local/memory_token_oidc_cache_client.darttest/features/login/data/local/token_oidc_cache_manager_test.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4608. |
After analyzing, Why it doesn't fix cross-isolate raceThe confirmed failure mode is two independent Why it may worsen partial write on process kill
The proposed code's own comment acknowledges this: "does NOT guarantee atomic persistence". API design concernWith Correct directionThe real fix requires either removing the shared-file write path altogether or using a storage backend with OS-level atomic writes. That's tracked as a follow-up PR — migrating the OIDC token to Flutter Secure Storage (iOS Keychain / Android Keystore), where each |
|
How about we should have the fallback token with token which in the memory (interceptor)? It means if we failed to get token in Hive, we try one more time with token in memory to download? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…e Primitive Obsession
…to fix CodeScene Primitive Obsession
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…d ExportAllAttachmentsInteractor
…chmentsInteractor
Agree. applied |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dab246
changed the title
… in export interactors
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/features/download/domain/usecase/export_attachment_interactor_test.dart (1)
80-181: ⚡ Quick winAssert the
AccountRequestcontents, not just the emitted success/failure.These cases currently pass even if the interactor builds the wrong auth material, because
downloadRepository.exportAttachment(any, any, any, any, any)accepts anyAccountRequest. Please addverify(...)/captureAnyassertions for the OIDC stored-token path, the fallback-token path, and the basic-auth path so the tests prove the new wiring rather than only the stream shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/features/download/domain/usecase/export_attachment_interactor_test.dart` around lines 80 - 181, Add assertions that verify the exact AccountRequest passed to downloadRepository.exportAttachment instead of allowing any; after each test call to interactor.execute(buildRequest(...)) add verify/capture on downloadRepository.exportAttachment and assert the captured AccountRequest's auth fields: for OIDC path assert token equals OIDCFixtures.newTokenOidc, for fallback-token path assert token equals provided fallback token when authenticationOIDCRepository.getStoredTokenOIDC throws, and for basic-auth path assert credentials equal AuthenticationInfoCache('user@example.com','secret') (referencing exportAttachment, interactor.execute, authenticationOIDCRepository.getStoredTokenOIDC, credentialRepository.getAuthenticationInfoStored, accountRepository.getCurrentAccount).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@lib/features/login/data/network/interceptors/authorization_interceptors.dart`:
- Line 75: The currentToken getter can leak stale OIDC tokens after
setBasicAuthorization() switches auth type; update the getter to return _token
only when _authenticationType == AuthenticationType.oidc (otherwise return null)
so ExportAttachmentRequest/ExportAllAttachmentsRequest cannot pick up an OIDC
token during basic auth; alternatively, if you prefer state mutation, clear
_token and _configOIDC inside setBasicAuthorization() after setting
_authenticationType/_authorization—pick one approach and apply it to the symbols
currentToken, _token, _authenticationType, setBasicAuthorization(), and
_configOIDC.
---
Nitpick comments:
In
`@test/features/download/domain/usecase/export_attachment_interactor_test.dart`:
- Around line 80-181: Add assertions that verify the exact AccountRequest passed
to downloadRepository.exportAttachment instead of allowing any; after each test
call to interactor.execute(buildRequest(...)) add verify/capture on
downloadRepository.exportAttachment and assert the captured AccountRequest's
auth fields: for OIDC path assert token equals OIDCFixtures.newTokenOidc, for
fallback-token path assert token equals provided fallback token when
authenticationOIDCRepository.getStoredTokenOIDC throws, and for basic-auth path
assert credentials equal AuthenticationInfoCache('user@example.com','secret')
(referencing exportAttachment, interactor.execute,
authenticationOIDCRepository.getStoredTokenOIDC,
credentialRepository.getAuthenticationInfoStored,
accountRepository.getCurrentAccount).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d39ae13-0413-4a0b-a7ec-c13287a33fc3
📒 Files selected for processing (11)
lib/features/download/domain/model/export_all_attachments_request.dartlib/features/download/domain/model/export_attachment_request.dartlib/features/download/domain/usecase/export_all_attachments_interactor.dartlib/features/download/domain/usecase/export_attachment_interactor.dartlib/features/download/presentation/extensions/download_attachment_download_controller_extension.dartlib/features/login/data/local/token_oidc_cache_manager.dartlib/features/login/data/network/interceptors/authorization_interceptors.darttest/features/download/domain/usecase/export_all_attachments_interactor_test.darttest/features/download/domain/usecase/export_attachment_interactor_test.darttest/features/login/data/local/stub_token_oidc_cache_clients.darttest/features/login/data/local/token_oidc_cache_manager_test.dart
✅ Files skipped from review due to trivial changes (1)
- test/features/download/domain/usecase/export_all_attachments_interactor_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/login/data/local/token_oidc_cache_manager.dart
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/features/download/domain/usecase/export_all_attachments_interactor.dart (1)
72-78: ⚡ Quick winPrefer
Objectoverdynamicfor the error parameter.The
catchErrorcallback parameter should useObjectinstead ofdynamicto maintain static type safety, consistent with Effective Dart guidance and this codebase's convention for exception parameters.♻️ Suggested fix
_authenticationOIDCRepository.persistTokenOIDC(fallbackToken).catchError( - (dynamic repairError) => logError( + (Object repairError) => logError( 'ExportAllAttachmentsInteractor::_getTokenOidc(): ' 'failed to repair token storage | error=${repairError.runtimeType}', exception: repairError, ), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/download/domain/usecase/export_all_attachments_interactor.dart` around lines 72 - 78, The catchError callback uses a dynamic parameter; change it to Object to follow project conventions and preserve static type safety: update the lambda parameter in the call to _authenticationOIDCRepository.persistTokenOIDC(fallbackToken).catchError from (dynamic repairError) => ... to (Object repairError) => ... and ensure the existing usage of repairError (e.g., repairError.runtimeType and exception: repairError) still compiles with Object.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/features/download/domain/usecase/export_all_attachments_interactor.dart`:
- Around line 72-78: The catchError callback uses a dynamic parameter; change it
to Object to follow project conventions and preserve static type safety: update
the lambda parameter in the call to
_authenticationOIDCRepository.persistTokenOIDC(fallbackToken).catchError from
(dynamic repairError) => ... to (Object repairError) => ... and ensure the
existing usage of repairError (e.g., repairError.runtimeType and exception:
repairError) still compiles with Object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b5c9315-d90a-4d89-a6c7-e6372303fd73
📒 Files selected for processing (4)
lib/features/download/domain/usecase/export_all_attachments_interactor.dartlib/features/download/domain/usecase/export_attachment_interactor.darttest/features/download/domain/usecase/export_all_attachments_interactor_test.darttest/features/download/domain/usecase/export_attachment_interactor_test.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- test/features/download/domain/usecase/export_all_attachments_interactor_test.dart
- lib/features/download/domain/usecase/export_attachment_interactor.dart
- test/features/download/domain/usecase/export_attachment_interactor_test.dart
…mic, assert AccountRequest in tests
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…cation in export interactors
…cancelToken, fallbackToken
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…nt key divergence
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
2 participants
Problem
#4606
Two symptoms were reported:
Both stem from the same root cause: the Hive box storing the OIDC token becomes corrupted (cross-isolate race between the main app and the FCM background isolate), and the app had no recovery mechanism.
Root Cause
flowchart TD A[FCM isolate + main app write to same Hive file concurrently] A --> B[Partial write — AES block boundary violated] B --> C[getStoredTokenOIDC throws RangeError] C --> D1[Download fails — toast shown] C --> D2[Box never cleared] D2 --> E[App restart → same error → logout loop]Download fails:
ExportAttachmentInteractorcallsgetStoredTokenOIDC. TheRangeErrorpropagates toDownloadController→ "download failed" toast. Box is left corrupt.Logout loop on restart:
GetStoredTokenOidcInteractorreads the same corrupt box →GetStoredTokenOidcFailure→isNotSignedIn()→clearDataAndGoToLoginPage(). Every restart repeats.Secondary —
persistOneTokenOidcpropagates corruption: When Dio refreshes an expired token,_removeStaleTokenscallsgetMapItems()→toMap(). A corrupt neighbour entry causestoMap()to crash before pruning completes, propagatingUnknownRemoteExceptionto the in-flight request.Minor — wrong argument to failure handlers:
DownloadControllerwas passing theExportAttachmentFailurewrapper instead offailure.exception, soCancelDownloadFileExceptionwas never matched — cancelled downloads showed a spurious "download failed" toast.Fix
Three defence layers applied in depth.
Layer 1 —
TokenOidcCacheManagerclears the corrupted box on first detectiongetTokenOidc: catches any non-AppBaseException(AES decryption error,RangeError, etc.), logs to Sentry, clears the box, throwsNotFoundStoredTokenException.persistOneTokenOidc: writes the new token first (crash-safe), then prunes stale entries. If pruning crashes on a corrupt neighbour entry, recovers by clearing the box and re-inserting only the new valid token.→ Eliminates the logout loop on restart. Does not prevent the current-session download failure on its own.
Layer 2 — Export interactors fall back to the in-memory token and repair storage
After Layer 1 clears the box, the valid OIDC token is still held in memory by
AuthorizationInterceptors._token. Layer 2 uses it.flowchart TD A[ExportAttachmentInteractor needs OIDC token] --> B{getStoredTokenOIDC accountCacheKey} B -- success --> C[Use storage token] B -- throws --> D{fallbackToken present?} D -- no --> E[Rethrow → download fails] D -- yes --> F[Use in-memory token from AuthorizationInterceptors] F --> G[persistTokenOIDCAt accountCacheKey — fire & forget] G -- success --> H[Box healed at correct key — token survives app restart] G -- fails --> I[logError to Sentry] F --> J[Download proceeds normally]ExportAttachmentRequestandExportAllAttachmentsRequestcarry an optionalfallbackTokenfield, populated fromAuthorizationInterceptors.currentTokenat the call site. Both interactors resolve the token via_getTokenOidc(accountCacheKey, fallbackToken: request.fallbackToken).Why
persistTokenOIDCAt(accountCacheKey, ...)matters:GetStoredTokenOidcInteractorlooks up the token at startup usingpersonalAccount.id(AccountCache.id) as the Hive key.persistTokenOIDCstores underfallbackToken.tokenIdHash. These can diverge if a prior 401 token refresh updated_tokenin memory but_updateCurrentAccountfailed beforesetCurrentAccountran — leaving_token.tokenIdHash ≠ currentAccount.id. Repairing underfallbackToken.tokenIdHashwould write to a key startup never reads, causing logout.persistTokenOIDCAtstores at the explicitaccountCacheKeythat startup will use.persistTokenOIDCAtis fire-and-forget (not awaited) so the download is never blocked by a storage repair. On repair failure,logErrorwith the exception is sent to Sentry.→ Download succeeds in the current session even when storage is corrupt.
→ Box is healed at the correct key so the token survives an app restart without requiring re-login.
Layer 3 —
DownloadControllerpasses the correct exception to failure handlers→
CancelDownloadFileExceptioncheck works correctly; cancelled downloads no longer show a "download failed" toast.Outcome
accountCacheKey_tokenhash diverges fromcurrentAccount.id+ restartpersistOneTokenOidcthrows → request failsSummary by CodeRabbit
New Features
Enhancements
Tests