Skip to content

Fix split install issue#2898

Merged
mar-v-in merged 15 commits into
microg:masterfrom
DaVinci9196:fix_split_install
Jul 23, 2025
Merged

Fix split install issue#2898
mar-v-in merged 15 commits into
microg:masterfrom
DaVinci9196:fix_split_install

Conversation

@DaVinci9196

Copy link
Copy Markdown
Contributor

Extract the optimization and resubmit it to #2799

  • Download notifications for split packages should be dismissible.
  • After a split package is downloaded, clicking the notification should trigger installation (to handle cases where the install prompt can't appear due to a locked screen).
  • Prevent multiple notifications for the same split package download.
  • Remove notifications when the network is disconnected or the download fails.
  • Implement resumable downloads to save data usage.
  • Ensure device synchronization to avoid missing split package information.
  • Support account elevation to prevent failure in accessing split package data.

@mar-v-in mar-v-in added the 📦 Asset Modules Asset Packs, Play Asset Delivery and related marketing terms label May 14, 2025
@mar-v-in mar-v-in requested a review from fynngodau May 27, 2025 08:33
@mar-v-in mar-v-in added this to the 0.3.9 milestone Jun 11, 2025

@fynngodau fynngodau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these improvements. I think resumable downloads, and only downloading one split install package at a time, really make sense. However, I would prefer a different implementation approach: the way you are doing it is that the packages are first downloaded to a temporary directory, and then copied to the install session from this directory. (A previous iteration of the code also had this behavior, but we changed it to avoid occupying 2× the space of the installed package while installing it.)

I think it should be possible to instead resume downloads into the pending session. To do this:

  • We need a registry of session IDs, so we know which download corresponds to which session.
  • Sessions must not be discarded when download fails.
  • We don't need temporary files anymore.

Do you see any reason that speaks against implementing it like this?

Comment thread vending-app/src/main/java/org/microg/vending/ui/InstallProgressNotification.kt Outdated
Comment thread vending-app/src/main/java/org/microg/vending/ui/InstallProgressNotification.kt Outdated
Comment thread vending-app/src/main/java/org/microg/vending/delivery/Delivery.kt Outdated
Comment thread vending-app/src/main/java/org/microg/vending/delivery/Delivery.kt Outdated
Comment thread vending-app/src/main/java/org/microg/vending/delivery/Delivery.kt
Comment thread vending-app/src/main/kotlin/com/android/vending/installer/Install.kt Outdated
@DaVinci9196

Copy link
Copy Markdown
Contributor Author

Thanks for these improvements. I think resumable downloads, and only downloading one split install package at a time, really make sense. However, I would prefer a different implementation approach: the way you are doing it is that the packages are first downloaded to a temporary directory, and then copied to the install session from this directory. (A previous iteration of the code also had this behavior, but we changed it to avoid occupying 2× the space of the installed package while installing it.)

I think it should be possible to instead resume downloads into the pending session. To do this:

  • We need a registry of session IDs, so we know which download corresponds to which session.
  • Sessions must not be discarded when download fails.
  • We don't need temporary files anymore.

Do you see any reason that speaks against implementing it like this?

We temporarily store the downloaded files locally to avoid repeated downloads in case of issues, which could lead to unnecessary data usage for users.
Regarding your suggestion of resuming directly into the install session — isn’t there a limitation on how many active sessions the system allows?
That’s part of the reason we initially chose to manage downloads outside the session system.

@fynngodau

Copy link
Copy Markdown
Member

Regarding your suggestion of resuming directly into the install session — isn’t there a limitation on how many active sessions the system allows?

I don't know if there is such a limit. What do you expect the limit to be? What happens if it is exceeded? Do you expect to open many sessions, i.e. many partial and failed downloads? This would also mean a large storage consumption in either implementation, so it's not really desirable in either implementation.

@DaVinci9196

Copy link
Copy Markdown
Contributor Author

Regarding your suggestion of resuming directly into the install session — isn’t there a limitation on how many active sessions the system allows?

I don't know if there is such a limit. What do you expect the limit to be? What happens if it is exceeded? Do you expect to open many sessions, i.e. many partial and failed downloads? This would also mean a large storage consumption in either implementation, so it's not really desirable in either implementation.

This is just an extreme case because Marvin initially raised concerns about data usage with me. That’s why I chose this approach.
Regarding your suggestion of tracking through sessions, wouldn’t the sessions be lost if the app is killed?

@fynngodau

Copy link
Copy Markdown
Member

Regarding your suggestion of tracking through sessions, wouldn’t the sessions be lost if the app is killed?

The session will even be persisted across multiple boots, please refer to: https://developer.android.com/reference/android/content/pm/PackageInstaller#createSession(android.content.pm.PackageInstaller.SessionParams)

@DaVinci9196

Copy link
Copy Markdown
Contributor Author

Regarding your suggestion of tracking through sessions, wouldn’t the sessions be lost if the app is killed?

The session will even be persisted across multiple boots, please refer to: https://developer.android.com/reference/android/content/pm/PackageInstaller#createSession(android.content.pm.PackageInstaller.SessionParams)

So do we need to store the sessions locally to facilitate reuse later?
If we use this method, how can we track the split package download progress, and how can we implement resumable downloads?

@DaVinci9196

Copy link
Copy Markdown
Contributor Author

Regarding your suggestion of tracking through sessions, wouldn’t the sessions be lost if the app is killed?

The session will even be persisted across multiple boots, please refer to: https://developer.android.com/reference/android/content/pm/PackageInstaller#createSession(android.content.pm.PackageInstaller.SessionParams)

I think your solution still cannot meet the requirements for resumable downloads in special cases.

@fynngodau

Copy link
Copy Markdown
Member

Yes, as I mentioned, we could use a registry of session IDs, so we know which download corresponds to which session. Since we are session owner, we might also be able to use: https://developer.android.com/reference/kotlin/android/content/pm/PackageInstaller?hl=en#getStagedSessions() and compare the package name of the session.

I suppose we might need to store how many bytes we wrote to the session, if it's not possible to ask the session that, though it might be feasible to use: https://developer.android.com/reference/kotlin/android/content/pm/PackageInstaller.Session?hl=en#openRead(kotlin.String)

What special cases do you have in mind?

@DaVinci9196

DaVinci9196 commented Jun 17, 2025

Copy link
Copy Markdown
Contributor Author

Yes, as I mentioned, we could use a registry of session IDs, so we know which download corresponds to which session. Since we are session owner, we might also be able to use: https://developer.android.com/reference/kotlin/android/content/pm/PackageInstaller?hl=en#getStagedSessions() and compare the package name of the session.

I suppose we might need to store how many bytes we wrote to the session, if it's not possible to ask the session that, though it might be feasible to use: https://developer.android.com/reference/kotlin/android/content/pm/PackageInstaller.Session?hl=en#openRead(kotlin.String)

What special cases do you have in mind?

I tried using your method, but I kept getting the following error when calling openSession: java.lang.SecurityException: Caller has no access to session 2068134291. @fynngodau

@fynngodau

Copy link
Copy Markdown
Member

@DaVinci9196 Can you provide me with the code where this issue occurs? That will make it much easier for me to find out why that could be the case.

@DaVinci9196

Copy link
Copy Markdown
Contributor Author

@DaVinci9196 Can you provide me with the code where this issue occurs? That will make it much easier for me to find out why that could be the case.

Install.txt
I modified Install.kt to Install.txt and uploaded it。
The core code is as follows. An error occurs when calling openSession

val cacheSessionId = SessionStorage.getInstance(this).getSessionId(key)
        sessionId = if (cacheSessionId == -1) {
            packageInstaller.createSession(params)
        } else {
            cacheSessionId
        }
        for (info in packageInstaller.allSessions) {
            Log.d(TAG, "id=${info.sessionId}, createdBy=${info.installerPackageName}")
        }

        SessionStorage.getInstance(this).saveSessionId(key, sessionId)
        session = packageInstaller.openSession(sessionId)

@DaVinci9196

Copy link
Copy Markdown
Contributor Author

@DaVinci9196 Can you provide me with the code where this issue occurs? That will make it much easier for me to find out why that could be the case.

Install.txt I modified Install.kt to Install.txt and uploaded it。 The core code is as follows. An error occurs when calling openSession

val cacheSessionId = SessionStorage.getInstance(this).getSessionId(key)
        sessionId = if (cacheSessionId == -1) {
            packageInstaller.createSession(params)
        } else {
            cacheSessionId
        }
        for (info in packageInstaller.allSessions) {
            Log.d(TAG, "id=${info.sessionId}, createdBy=${info.installerPackageName}")
        }

        SessionStorage.getInstance(this).saveSessionId(key, sessionId)
        session = packageInstaller.openSession(sessionId)

@fynngodau Can you take a look? This is quite urgent.

@fynngodau

Copy link
Copy Markdown
Member

@DaVinci9196 I could not reproduce your problem, but I also do not have access to the SessionStorage class that you implemented. Hence I instead tried the following code (replacing lines 134–135 of Install.kt):

        sessionId = packageInstaller.createSession(params)
        session = packageInstaller.openSession(sessionId)
        packageInstaller.updateSessionAppLabel(sessionId, packageName)
        session.close()
        Log.d(TAG, "session closed")
        Log.d(TAG, packageInstaller.mySessions.joinToString(",") { sessionInfo -> sessionInfo.sessionId.toString() + " of package " + sessionInfo.appLabel })
        session = packageInstaller.openSession(sessionId)
        Log.d(TAG, "session reopened")

This worked without problems:

  • mySessions returned the currently pending session
  • I could access its metadata (this could be a good alternative to your SessionStorage implementation, as I would expect mySessions to always be in sync with whether the system has discarded the session due to timeout (after a day or so per docs))
  • I was able to reopen the session (but I did not try with closing and reopening the app)

Does the above code already produce a problem for you?

I noticed that you are never closing the session. I would suggest adding this to the finally block in your version of the code, maybe it helps.

@DaVinci9196

DaVinci9196 commented Jun 25, 2025

Copy link
Copy Markdown
Contributor Author

@DaVinci9196 I could not reproduce your problem, but I also do not have access to the SessionStorage class that you implemented. Hence I instead tried the following code (replacing lines 134–135 of Install.kt):

        sessionId = packageInstaller.createSession(params)
        session = packageInstaller.openSession(sessionId)
        packageInstaller.updateSessionAppLabel(sessionId, packageName)
        session.close()
        Log.d(TAG, "session closed")
        Log.d(TAG, packageInstaller.mySessions.joinToString(",") { sessionInfo -> sessionInfo.sessionId.toString() + " of package " + sessionInfo.appLabel })
        session = packageInstaller.openSession(sessionId)
        Log.d(TAG, "session reopened")

This worked without problems:

  • mySessions returned the currently pending session
  • I could access its metadata (this could be a good alternative to your SessionStorage implementation, as I would expect mySessions to always be in sync with whether the system has discarded the session due to timeout (after a day or so per docs))
  • I was able to reopen the session (but I did not try with closing and reopening the app)

Does the above code already produce a problem for you?

I noticed that you are never closing the session. I would suggest adding this to the finally block in your version of the code, maybe it helps.

This code cannot run on my phone and will report an error @fynngodau

@DaVinci9196

DaVinci9196 commented Jun 25, 2025

Copy link
Copy Markdown
Contributor Author

and
@fynngodau I see the following code in the Delivery.kt class. What will happen if these codes are not included?

pf=2&pf=3&pf=4&pf=5&pf=7&pf=8&pf=9&pf=10&da=4&bda=4&bf=4&fdcf=1&fdcf=2

I have found that &da=4 will cause problems when PayPal switches to voice. Can this be removed? @fynngodau

@DaVinci9196 DaVinci9196 requested a review from fynngodau July 1, 2025 07:48
@mar-v-in

Copy link
Copy Markdown
Member

@DaVinci9196 I tried with just the code fragment from #2898 (comment) in a minimal demo app on multiple devices, including P60 Pro with EMUI 14 and Mate X5 with Harmony OS 4 and it worked as expected and reopened the session on all of them. What device did you use for this?

Looking at the code, I believe the issue might be a concurrency issue. Note that installPackagesInternal is a suspendable function and does not have any mutexes or anything preventing it from being invoked more than once. This means that a second call to installPackagesInternal will fail if it happens for the same package as one that is already ongoing.

@DaVinci9196

Copy link
Copy Markdown
Contributor Author

@DaVinci9196 I tried with just the code fragment from #2898 (comment) in a minimal demo app on multiple devices, including P60 Pro with EMUI 14 and Mate X5 with Harmony OS 4 and it worked as expected and reopened the session on all of them. What device did you use for this?

Looking at the code, I believe the issue might be a concurrency issue. Note that installPackagesInternal is a suspendable function and does not have any mutexes or anything preventing it from being invoked more than once. This means that a second call to installPackagesInternal will fail if it happens for the same package as one that is already ongoing.

I have modified it according to this comment #2898 (comment), you can test it with the latest code

@mar-v-in mar-v-in left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor nits to be fixed.

Comment thread vending-app/src/main/java/org/microg/vending/delivery/Delivery.kt
@DaVinci9196 DaVinci9196 requested a review from mar-v-in July 16, 2025 13:06
Comment thread vending-app/src/main/kotlin/com/android/vending/installer/Install.kt Outdated
@DaVinci9196 DaVinci9196 requested a review from fynngodau July 18, 2025 03:53

@fynngodau fynngodau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements to this PR so far. However, it does not fully work yet from my tests.

Comment thread vending-app/src/main/kotlin/com/android/vending/installer/Install.kt Outdated
Comment thread vending-app/src/main/kotlin/com/android/vending/installer/Install.kt Outdated
@DaVinci9196 DaVinci9196 requested a review from fynngodau July 18, 2025 12:14
Comment thread vending-app/src/main/kotlin/com/android/vending/installer/Install.kt Outdated
@fynngodau

Copy link
Copy Markdown
Member

I have found that &da=4 will cause problems when PayPal switches to voice. Can this be removed?

My answer is: nobody really knows what these options mean. I see no regressions caused by removing it so I consider it ok.

@DaVinci9196 DaVinci9196 requested a review from fynngodau July 18, 2025 14:12
@quimodotcom

Copy link
Copy Markdown

0.3.9 here we come!

@mar-v-in

Copy link
Copy Markdown
Member

LGTM, Thanks @DaVinci9196 and @fynngodau for the work!

@mar-v-in mar-v-in merged commit 4099b31 into microg:master Jul 23, 2025
1 check passed
while (inputStream.read(buffer).also { bytesRead = it } != -1) {
total += bytesRead
}
inputStream.close()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mar-v-in
Wouldn't make sense to put inputStream.close() inside a finally block to be sure it is always closed even in case of unknown errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📦 Asset Modules Asset Packs, Play Asset Delivery and related marketing terms

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants