Reduce worker_manager memory allocation#4435
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors remove the injected Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
|
Great change proposal! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pubspec.yaml (1)
180-184: Pin the temporaryworker_managerfork to an immutable commit.A branch ref can drift on the next dependency resolve, so a clean
flutter pub getis not guaranteed to use the same code that produced the memory profile in this PR. Please switchrefto a commit SHA and verifypubspec.lockcaptures that exact revision.♻️ Suggested change
worker_manager: git: url: https://github.com/linagora/worker_manager.git - ref: hotfix/worker-init-memory-leak + ref: <commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pubspec.yaml` around lines 180 - 184, Replace the mutable branch ref for the worker_manager dependency with the specific commit SHA that was used to produce the profile: change the worker_manager git ref from hotfix/worker-init-memory-leak to the immutable commit hash (use the commit SHA from the upstream fork), run flutter pub get to update dependencies, and verify pubspec.lock records that exact git revision for the worker_manager entry; ensure you update the ref value under the worker_manager git block in pubspec.yaml and confirm pubspec.lock shows the same SHA.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/mailbox/data/network/mailbox_isolate_worker.dart`:
- Around line 73-82: markAsMailboxRead currently sends the full List<EmailId> on
each progress update, causing costly cross-isolate copies; change it to send
just an int count. Update the call to workerManager.executeWithPort to use
generics <List<EmailId>, int> (or the equivalent signature expecting an int
progress) and replace the onMessage handler to treat the incoming value as an
int and pass that into UpdatingMarkAsMailboxReadState as countRead; likewise
update the worker entrypoint (_handleMarkAsMailboxReadAction) to emit the
progress as an int (accumulated count) instead of sending the List<EmailId>.
Ensure references to markAsMailboxRead, _handleMarkAsMailboxReadAction,
onProgressController, and UpdatingMarkAsMailboxReadState are adjusted to use the
int progress pattern consistent with moveFolderContent.
In `@lib/features/thread/data/network/thread_isolate_worker.dart`:
- Around line 59-67: The current executeWithPort call is serializing the full
List<EmailId> back to the UI isolate each batch; change the protocol to send
only the processed count: update the generic type arguments on
workerManager.executeWithPort from List<EmailId>, List<EmailId> to
List<EmailId>, int (or simply int as the return payload), modify the worker
entrypoint _emptyMailboxFolderAction to send the batch count (an int) over the
SendPort instead of the accumulated List<EmailId>, and update the onMessage
handler to accept an int (use the value directly as processed count) and keep
emitting EmptyingFolderState(mailboxId, value, totalEmails). Ensure any type
annotations and return expectations for ThreadIsolateWorker::emptyMailboxFolder
/ _emptyMailboxFolderAction are adjusted accordingly.
---
Nitpick comments:
In `@pubspec.yaml`:
- Around line 180-184: Replace the mutable branch ref for the worker_manager
dependency with the specific commit SHA that was used to produce the profile:
change the worker_manager git ref from hotfix/worker-init-memory-leak to the
immutable commit hash (use the commit SHA from the upstream fork), run flutter
pub get to update dependencies, and verify pubspec.lock records that exact git
revision for the worker_manager entry; ensure you update the ref value under the
worker_manager git block in pubspec.yaml and confirm pubspec.lock shows the same
SHA.
🪄 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: bd83ebcf-b31e-4bf5-9bd7-0422dc54b9d5
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
lib/features/mailbox/data/network/mailbox_isolate_worker.dartlib/features/thread/data/network/thread_isolate_worker.dartlib/features/upload/data/network/file_uploader.dartlib/main/bindings/network/network_bindings.dartlib/main/bindings/network/network_isolate_binding.dartlib/main/main_entry.dartpubspec.yamltest/features/identity_creator/presentation/identity_creator_controller_test.dart
💤 Files with no reviewable changes (3)
- lib/main/bindings/network/network_bindings.dart
- lib/main/bindings/network/network_isolate_binding.dart
- test/features/identity_creator/presentation/identity_creator_controller_test.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4435. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/upload/data/network/file_uploader.dart`:
- Around line 64-76: The non-web worker path doesn't propagate the cancelToken
so uploads can't be aborted; propagate the cancelToken into the isolate flow by
adding it to UploadFileArguments (used by _buildUploadClosure) and then use that
token inside the worker handler (_handleUploadAttachmentAction) when calling
_dioClient.post so the request is cancellable; if your worker/executeWithPort
API cannot cancel running isolates, detect a non-null cancelToken and run the
upload on the main isolate instead (i.e., avoid the worker.path) so the existing
cancelToken-based cancellation logic is honored.
🪄 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: ec2c4637-36e4-4146-b4b5-51e2aec807c3
📒 Files selected for processing (3)
lib/features/mailbox/data/network/mailbox_isolate_worker.dartlib/features/thread/data/network/thread_isolate_worker.dartlib/features/upload/data/network/file_uploader.dart
| final args = UploadFileArguments( | ||
| _dioClient, | ||
| _fileUtils, | ||
| uploadId, | ||
| fileInfo, | ||
| uploadUri, | ||
| rootIsolateToken, | ||
| ); | ||
| return await worker.workerManager.executeWithPort<Attachment, Success>( | ||
| _buildUploadClosure(args), | ||
| onMessage: (value) { | ||
| log('FileUploader::uploadAttachment(): onUpdateProgress: $value'); | ||
| onSendController?.add(Right(value)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n[1] FileUploader cancelToken usage\n'
rg -n -C2 --type=dart '\bcancelToken\b|executeWithPort|dioClient\.post\(' lib/features/upload/data/network/file_uploader.dart
printf '\n[2] Upload flows that pass or observe cancellation\n'
rg -n -C2 --type=dart 'uploadAttachment\(|CancelToken|isCancelled|\.cancel\(' lib/features/uploadRepository: linagora/tmail-flutter
Length of output: 8102
Non-web uploads ignore cancelToken in the isolate-backed request flow.
The method accepts cancelToken (line 46) and the web branch passes it to _dioClient.post() (line 189), but the non-web worker path does not. At line 110, _handleUploadAttachmentAction() calls dioClient.post() without a cancellation token. This allows callers to cancel uploads and update local state while the HTTP request continues in the worker isolate and completes on the server.
Options: implement cancellation via a dedicated worker-cancel channel or fall back to the main isolate for cancellable uploads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/upload/data/network/file_uploader.dart` around lines 64 - 76,
The non-web worker path doesn't propagate the cancelToken so uploads can't be
aborted; propagate the cancelToken into the isolate flow by adding it to
UploadFileArguments (used by _buildUploadClosure) and then use that token inside
the worker handler (_handleUploadAttachmentAction) when calling _dioClient.post
so the request is cancellable; if your worker/executeWithPort API cannot cancel
running isolates, detect a non-null cancelToken and run the upload on the main
isolate instead (i.e., avoid the worker.path) so the existing cancelToken-based
cancellation logic is honored.
There was a problem hiding this comment.
Implement a dedicated worker-cancel channel is costly, and fall back to main isolate just for the cancel capability affect the main isolate performance.
There was a problem hiding this comment.
@tddang-linagora, that's a fair trade-off given the substantial memory gains demonstrated in the profiling images (Android Dart/Flutter heap drops from ~375 MB to ~109 MB, iOS from ~297 MB to ~105 MB). Introducing cross-isolate cancellation plumbing or forcing the main isolate to handle uploads just to support cancellation would offset those wins.
A minimal mitigation would be adding a short comment near the executeWithPort call so future maintainers understand the deliberate limitation:
// Note: cancelToken is not propagated into the worker isolate.
// Cross-isolate cancellation is not supported here due to cost/complexity;
// cancelling the upload task via the caller will not abort the in-flight HTTP request.
return await worker.workerManager.executeWithPort<Attachment, Success>(
_buildUploadClosure(args),
onMessage: (value) {
log('FileUploader::uploadAttachment(): onUpdateProgress: $value');
onSendController?.add(Right(value));
},
)This keeps the code self-documenting without adding any runtime overhead. Up to you whether you want to add it.
✏️ Learnings added
Learnt from: tddang-linagora
Repo: linagora/tmail-flutter PR: 4435
File: lib/features/upload/data/network/file_uploader.dart:64-76
Timestamp: 2026-04-07T03:15:49.896Z
Learning: In linagora/tmail-flutter, the non-web upload path in `lib/features/upload/data/network/file_uploader.dart` deliberately does NOT propagate `cancelToken` into the `worker.workerManager.executeWithPort` isolate call. This is an accepted trade-off: implementing a worker-cancel channel was considered too costly, and falling back to the main isolate for cancellable uploads was rejected due to main-isolate performance impact. Do not flag the missing cancelToken propagation as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4362
File: lib/features/login/domain/exceptions/authentication_exception.dart:54-62
Timestamp: 2026-03-09T03:46:24.006Z
Learning: In tmail-flutter, `DownloadAttachmentHasTokenExpiredException` (lib/features/login/domain/exceptions/authentication_exception.dart) intentionally does NOT override `props` to include `refreshToken`, for security reasons — the token must not be leaked via equality checks, debug output, or crash reporters like Sentry.
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4422
File: lib/features/mailbox_dashboard/presentation/mixin/setup_preferences_setting_mixin.dart:24-30
Timestamp: 2026-03-27T11:00:56.451Z
Learning: In linagora/tmail-flutter, the `collapseThreads` getter in `lib/features/mailbox_dashboard/presentation/mixin/setup_preferences_setting_mixin.dart` does NOT require a server Session capability check. It is enabled purely based on two conditions: (1) the in-app thread setting is enabled (`isThreadDetailEnabled == true`) and (2) `forceEmailQuery` is active (`PlatformInfo.isWeb && AppConfig.isForceEmailQueryEnabled`). Do not flag the absence of a session capability gate as a bug for this getter.
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4380
File: docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md:265-314
Timestamp: 2026-03-17T10:08:56.785Z
Learning: In linagora/tmail-flutter, the thread-aware interactors introduced in ADR-0072 (e.g., MoveThreadInteractor, MarkAsThreadReadInteractor in docs/adr/0072-thread-aware-bulk-actions-email-list-collapse-threads.md) are intentionally designed to call EmailRepository directly, rather than delegating to existing email interactors (e.g., MoveMultipleEmailToMailboxInteractor, MarkAsMultipleEmailReadInteractor). This is a deliberate choice to avoid affecting old logic. Do not suggest refactoring them to reuse existing interactors.
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4315
File: core/lib/utils/app_logger.dart:140-144
Timestamp: 2026-02-09T07:57:07.651Z
Learning: For the tmail-flutter repository, ensure that logTrace calls are used selectively for diagnostic purposes related to Sentry submissions and not for regular production logging. During reviews, verify that trace-level logs are placed only where they meaningfully aid diagnosis, avoid noisy production reports, and ensure no sensitive data is logged.
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mailbox_isolate_worker.dart | 9.10 → 9.39 | Complex Conditional, Excess Number of Function Arguments |
Absence of Expected Change Pattern
- tmail-flutter/lib/features/mailbox/data/datasource_impl/mailbox_datasource_impl.dart is usually changed with: tmail-flutter/lib/features/mailbox/data/datasource_impl/mailbox_cache_datasource_impl.dart, tmail-flutter/lib/features/mailbox/data/datasource/mailbox_datasource.dart, tmail-flutter/lib/features/mailbox/data/repository/mailbox_repository_impl.dart
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.
|
Note: The memory usage of Twake Mail can even be reduced further by 1 isolate. |
2 participants
Issue
worker_manageris creating a lot of idle forever-living isolations without providing a way to dispose any of them, making the memory allocation of mobile is really high, even when those isolations are not in use.Solution
Upgrade
worker_managerto latest version, which provides the ability to create an isolation when needed and dispose when done.Resolved
Mobile's memory usage is reduced by ~2/3. No improvement/negative impact for web
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
Tests