feat: download remaining old lazy splits on new release failure or timeout#250
feat: download remaining old lazy splits on new release failure or timeout#250Yash02Rajput wants to merge 1 commit intomainfrom
Conversation
|
Review changes with |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughBoth files enhance failure handling and retry mechanisms for lazy package downloads. The Android UpdateTask now tracks package update state and differentiates between updating the current package versus updating lazy-splits for new releases when failures occur. The iOS ApplicationManager adds a dedicated retry method and integrates it across multiple download paths to gracefully handle failed lazy downloads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
airborne_sdk_android/airborne/src/main/java/in/juspay/airborne/ota/UpdateTask.kt (1)
239-259:⚠️ Potential issue | 🟠 MajorLogic gap confirmed: lazy splits may be downloaded for incomplete packages when important splits fail after timeout.
When
updateTimedOut=trueandpresultisUpdate.Package.Failed:
newReleaseFailedevaluates tofalse(not preventing lazy download)shouldDownloadNewLazyevaluates totrue(allowing lazy download despite failed important splits)The code will attempt to download and persist lazy splits for the new package even though the important splits failed. A new
TempWriteris created at line 709, and results are persisted viasaveDownloadedPackages(line 589–591), but the package remains incomplete and unusable without the important splits.The proposed fix is sound:
Recommended fix
- val shouldDownloadNewLazy = fetched != null && !newReleaseFailed && - (presult is Update.Package.Finished || updateTimedOut.get()) + val shouldDownloadNewLazy = fetched != null && !newReleaseFailed && + presult is Update.Package.FinishedThis ensures lazy splits are only downloaded when important splits succeeded, preventing wasted downloads of unusable packages.
However, clarify the design intent: if timeout-triggered partial progress is meant to support future retries with separately-retried important splits, document this explicitly; otherwise, apply the fix to avoid persisting incomplete packages.
🤖 Fix all issues with AI agents
In
`@airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/ApplicationManager/AJPApplicationManager.m`:
- Around line 736-743: The code mutates and iterates the NSMutableArray
strongSelf.downloadedLazy from concurrent callbacks in singleDownloadHandler
(called by downloadLazyPackageResources:), causing a data race; fix by
serializing all accesses to downloadedLazy—wrap the loop that reads/mutates
downloadedLazy and any other reads (the earlier read at line ~750) with a single
synchronization mechanism (e.g., a dedicated serial dispatch_queue, NSLock
property, or `@synchronized`(self.downloadedLazy)) and use the same lock/queue
consistently wherever downloadedLazy is read or written (references:
downloadedLazy, singleDownloadHandler, downloadLazyPackageResources:,
AJPLazyResource.isDownloaded). Ensure you acquire the lock before
iterating/modifying and release after the update.
🧹 Nitpick comments (3)
airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/ApplicationManager/AJPApplicationManager.m (2)
1341-1387: Eliminate duplication:retryFailedLazyDownloadsandretryFailedLazyDownloadsWithCompletion:are nearly identical.The new method duplicates the body of
retryFailedLazyDownloads(lines 1297–1339) almost verbatim. Consider consolidating by having the original method delegate to the new one:♻️ Proposed refactor
- (void)retryFailedLazyDownloads { - NSMutableArray<AJPLazyResource *> *failedDownloads = [NSMutableArray array]; - // ... duplicated body ... + [self retryFailedLazyDownloadsWithCompletion:nil]; }This keeps a single implementation path and reduces maintenance burden.
721-753: Verify: nestedweakSelf/strongSelfre-declarations shadow the outer capture.Lines 727–730 and 745–748 re-declare
__strong AJPApplicationManager* strongSelf = weakSelf;inside the inner blocks, shadowing the outerstrongSelffrom line 717. While this is correct Objective-C (each block creates a new scope), mixing two scopes with the same variable name is easy to mis-read. TheweakSelfnil-checks on lines 727 and 745 correctly guard the inner usage, so this is functionally fine but worth noting for readability.airborne_sdk_android/airborne/src/main/java/in/juspay/airborne/ota/UpdateTask.kt (1)
231-231: Non-null assertionpresult!!is safe but unidiomatic — the enclosingifalready checkspresult != null.Since
presultis a localvar, Kotlin's smart cast doesn't apply across theelse ifboundary. The!!works but aletor a localvalwould be cleaner.♻️ Suggested alternative
- } else if (presult != null && fetched.pkg.lazy.isEmpty()) { - saveDownloadedPackages(presult!!, fetched.pkg) + } else if (presult != null && fetched.pkg.lazy.isEmpty()) { + presult?.let { saveDownloadedPackages(it, fetched.pkg) } }
| for (NSUInteger i = 0; i < strongSelf.downloadedLazy.count; i++) { | ||
| AJPLazyResource *lazyResource = strongSelf.downloadedLazy[i]; | ||
| if ([lazyResource.filePath isEqualToString:resource.filePath]) { | ||
| lazyResource.isDownloaded = status; | ||
| strongSelf.downloadedLazy[i] = lazyResource; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Data race: concurrent mutation of downloadedLazy without synchronization.
singleDownloadHandler is invoked concurrently from multiple GCD blocks dispatched inside downloadLazyPackageResources:. Each callback iterates and mutates strongSelf.downloadedLazy (an NSMutableArray) without any lock or @synchronized block. This can cause crashes or corrupted state.
🔒 Proposed fix — synchronize access to downloadedLazy
+ `@synchronized`(strongSelf.downloadedLazy) {
for (NSUInteger i = 0; i < strongSelf.downloadedLazy.count; i++) {
AJPLazyResource *lazyResource = strongSelf.downloadedLazy[i];
if ([lazyResource.filePath isEqualToString:resource.filePath]) {
lazyResource.isDownloaded = status;
strongSelf.downloadedLazy[i] = lazyResource;
break;
}
}
+ }Apply the same synchronization at line 750 where strongSelf.downloadedLazy is read.
📝 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.
| for (NSUInteger i = 0; i < strongSelf.downloadedLazy.count; i++) { | |
| AJPLazyResource *lazyResource = strongSelf.downloadedLazy[i]; | |
| if ([lazyResource.filePath isEqualToString:resource.filePath]) { | |
| lazyResource.isDownloaded = status; | |
| strongSelf.downloadedLazy[i] = lazyResource; | |
| break; | |
| } | |
| } | |
| `@synchronized`(strongSelf.downloadedLazy) { | |
| for (NSUInteger i = 0; i < strongSelf.downloadedLazy.count; i++) { | |
| AJPLazyResource *lazyResource = strongSelf.downloadedLazy[i]; | |
| if ([lazyResource.filePath isEqualToString:resource.filePath]) { | |
| lazyResource.isDownloaded = status; | |
| strongSelf.downloadedLazy[i] = lazyResource; | |
| break; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/ApplicationManager/AJPApplicationManager.m`
around lines 736 - 743, The code mutates and iterates the NSMutableArray
strongSelf.downloadedLazy from concurrent callbacks in singleDownloadHandler
(called by downloadLazyPackageResources:), causing a data race; fix by
serializing all accesses to downloadedLazy—wrap the loop that reads/mutates
downloadedLazy and any other reads (the earlier read at line ~750) with a single
synchronization mechanism (e.g., a dedicated serial dispatch_queue, NSLock
property, or `@synchronized`(self.downloadedLazy)) and use the same lock/queue
consistently wherever downloadedLazy is read or written (references:
downloadedLazy, singleDownloadHandler, downloadLazyPackageResources:,
AJPLazyResource.isDownloaded). Ensure you acquire the lock before
iterating/modifying and release after the update.
Yash02Rajput
force-pushed
the
download-old-lazy-splits-on-release-failure-or-timeout
branch
2 times, most recently
from
d3c0f73 to
ffa79dc
Compare
airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/ApplicationManager/AJPApplicationManager.m
Show resolved
Hide resolved
Yash02Rajput
force-pushed
the
download-old-lazy-splits-on-release-failure-or-timeout
branch
from
ffa79dc to
b794bf2
Compare
Yash02Rajput
force-pushed
the
download-old-lazy-splits-on-release-failure-or-timeout
branch
from
b794bf2 to
39050f9
Compare
Yash02Rajput
force-pushed
the
download-old-lazy-splits-on-release-failure-or-timeout
branch
from
608373f to
39050f9
Compare
2 participants
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements