fix(flutter): stop deleteAllModels from reporting success when listing fails - #608
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesModel deletion
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
sdk/runanywhere-flutter/packages/runanywhere/lib/public/capabilities/runanywhere_downloads.dart (1)
274-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both lookup outcomes.
Test that a failed
StorageInfoResultreturnssuccess: false, preserveserrorMessage, and does not calldeleteProto. Test that a successful result forwards everymodelIdtoStorageDeleteRequest.🤖 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 `@sdk/runanywhere-flutter/packages/runanywhere/lib/public/capabilities/runanywhere_downloads.dart` around lines 274 - 288, Add regression tests for the download deletion flow around getStorageInfoResult and DartBridgeStorage.instance.deleteProto: verify failed storage lookup returns success: false, preserves the provided errorMessage, and never calls deleteProto; verify successful lookup forwards every modelId from info.models into StorageDeleteRequest.
🤖 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
`@sdk/runanywhere-flutter/packages/runanywhere/lib/public/capabilities/runanywhere_downloads.dart`:
- Around line 274-283: Update deleteAllModels around getStorageInfoResult so
exceptions from the storage lookup are caught and returned as
StorageDeleteResult(success: false), using the exception message for
errorMessage with an appropriate fallback. Keep the initialization check outside
this catch and preserve the existing handling for unsuccessful infoResult
responses.
---
Nitpick comments:
In
`@sdk/runanywhere-flutter/packages/runanywhere/lib/public/capabilities/runanywhere_downloads.dart`:
- Around line 274-288: Add regression tests for the download deletion flow
around getStorageInfoResult and DartBridgeStorage.instance.deleteProto: verify
failed storage lookup returns success: false, preserves the provided
errorMessage, and never calls deleteProto; verify successful lookup forwards
every modelId from info.models into StorageDeleteRequest.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7336c06c-d684-4552-b978-ffc9bd3cdf7a
📒 Files selected for processing (1)
sdk/runanywhere-flutter/packages/runanywhere/lib/public/capabilities/runanywhere_downloads.dart
| final infoResult = await getStorageInfoResult( | ||
| StorageInfoRequest(includeModels: true), | ||
| ); | ||
| if (!infoResult.success) { | ||
| return StorageDeleteResult( | ||
| success: false, | ||
| errorMessage: infoResult.errorMessage.isNotEmpty | ||
| ? infoResult.errorMessage | ||
| : 'Failed to list downloaded models', | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return a failed result when the lookup throws.
list() catches exceptions from getStorageInfoResult at Lines 341-353. If the native storage lookup throws, execution never reaches the !infoResult.success branch. deleteAllModels() then throws instead of returning StorageDeleteResult(success: false).
Catch the lookup exception and map its message to errorMessage. Keep the initialization check outside the catch.
Proposed fix
- final infoResult = await getStorageInfoResult(
- StorageInfoRequest(includeModels: true),
- );
+ late final StorageInfoResult infoResult;
+ try {
+ infoResult = await getStorageInfoResult(
+ StorageInfoRequest(includeModels: true),
+ );
+ } catch (e) {
+ return StorageDeleteResult(
+ success: false,
+ errorMessage: e.toString(),
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final infoResult = await getStorageInfoResult( | |
| StorageInfoRequest(includeModels: true), | |
| ); | |
| if (!infoResult.success) { | |
| return StorageDeleteResult( | |
| success: false, | |
| errorMessage: infoResult.errorMessage.isNotEmpty | |
| ? infoResult.errorMessage | |
| : 'Failed to list downloaded models', | |
| ); | |
| late final StorageInfoResult infoResult; | |
| try { | |
| infoResult = await getStorageInfoResult( | |
| StorageInfoRequest(includeModels: true), | |
| ); | |
| } catch (e) { | |
| return StorageDeleteResult( | |
| success: false, | |
| errorMessage: e.toString(), | |
| ); | |
| } | |
| if (!infoResult.success) { | |
| return StorageDeleteResult( | |
| success: false, | |
| errorMessage: infoResult.errorMessage.isNotEmpty | |
| ? infoResult.errorMessage | |
| : 'Failed to list downloaded models', | |
| ); |
🤖 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
`@sdk/runanywhere-flutter/packages/runanywhere/lib/public/capabilities/runanywhere_downloads.dart`
around lines 274 - 283, Update deleteAllModels around getStorageInfoResult so
exceptions from the storage lookup are caught and returned as
StorageDeleteResult(success: false), using the exception message for
errorMessage with an appropriate fallback. Keep the initialization check outside
this catch and preserve the existing handling for unsuccessful infoResult
responses.
|
The The job fails on a single ktlint violation in a file this PR does not touch: It was introduced by #604 (merged 2026-07-30) and is present on I've opened a one-line fix for it separately so this and the other affected PRs can go green — I'll link it here shortly. Not re-pushing this branch to chase an unrelated failure. |
|
The one-line fix for that ktlint violation is #609 — merging it should turn |
8fd2c48 to
1604ed2
Compare
|
Both failures came from the base, and that base is now fixed: #609 merged and This branch was still sitting on the pre-#609 Re-gated after the rebase: |
|
The rebase did fix the lint failures — That test asserts a wall-clock budget for coroutine cancellation to propagate across 5 consumers on val torn = withTimeoutOrNull(250) { waitFor { unregisterCount.get() == 1 } } ?: false
...
assertTrue("cancel-to-native unregister fired in ${elapsed}ms (budget 250ms)", elapsed < 250)A 250 ms deadline on a shared CI runner is inherently timing-sensitive. Supporting evidence that it is intermittent rather than broken: Not pushing a code change for this. Re-triggering the workflow with an identical diff; if it flakes again, a maintainer re-run should clear it. |
1604ed2 to
afffd51
Compare
|
Gentle nudge on this one, since #611, #612 and #613 have since merged past it. This branch is rebased on current Recap of the bug in one line: Happy to rebase again or adjust the approach if you would rather handle the failed-lookup case differently. |
afffd51 to
e9a2c6b
Compare
|
Rebased onto current
The fix now propagates the lookup's own error instead of synthesising one: final infoResult = await getStorageInfoResult(
StorageInfoRequest(includeModels: true),
);
if (infoResult.hasError()) {
return StorageDeleteResult(error: infoResult.error);
}The defect itself is unchanged and still reproduces at
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…g fails deleteAllModels() built its delete request from list(), which reports a failed lookup as an empty list. When the storage lookup failed, the delete request went out with no model ids, commons had nothing to do, and the caller got a StorageDeleteResult with success: true even though no model was deleted - a silent no-op on a destructive operation. Read the models through getStorageInfoResult() instead, which carries success/error_message, and surface a failed lookup as StorageDeleteResult(success: false) with the underlying error message. list() keeps its tolerant empty-list contract for display callers (e.g. the example app's storage view); only the destructive path now requires a successful lookup before acting. Signed-off-by: ayaangazali <ayaangazali.work@gmail.com>
e9a2c6b to
738ea7d
Compare
|
Thanks @ayaangazali, solid catch! |
What
RunAnywhereDownloads.deleteAllModels()built its delete request fromlist(), which reports a failed storage lookup as an empty list. When the lookup failed, the request went out with no model ids, commons had nothing to delete, and the caller receivedStorageDeleteResult(success: true)having deleted nothing.This changes only the destructive path to read through
getStorageInfoResult(), which carriessuccess/error_message(storage_types.proto), and to surface a failed lookup asStorageDeleteResult(success: false)with the underlying message.Why
A silent no-op is the worst outcome for a destructive call: a caller that asks to free space is told it succeeded while every model is still on disk, and there is nothing in the return value to retry on.
list()already logs the failure, but the log is not visible to the caller and the[]is indistinguishable from "no models are downloaded".list()keeps its tolerant empty-list contract — display callers such as the example app's storage view (storage_view.dart) legitimately want a list they can render. Only the delete path now requires a successful lookup before it acts.Scope
Testing
From
sdk/runanywhere-flutter/packages/runanywhere(Flutter 3.44.6):flutter pub get— OKflutter analyze --no-pub—No issues found!Rebased onto current
main(3becbae55) and re-gated after the rebase.🤖 Generated with Claude Code
Summary by CodeRabbit