Skip to content

Fix split install issue #2898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Jul 23, 2025
Merged

Fix split install issue #2898

merged 15 commits into from
Jul 23, 2025

Conversation

DaVinci9196
Copy link
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
Copy link
Member

@fynngodau fynngodau left a comment

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?

@DaVinci9196
Copy link
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
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
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
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
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
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
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
Copy link
Contributor Author

DaVinci9196 commented Jun 17, 2025

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
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
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
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
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
Copy link
Contributor Author

DaVinci9196 commented Jun 25, 2025

@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
Copy link
Contributor Author

DaVinci9196 commented Jun 25, 2025

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
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
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

Copy link
Member

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

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.

@DaVinci9196 DaVinci9196 requested a review from mar-v-in July 16, 2025 13:06
@DaVinci9196 DaVinci9196 requested a review from fynngodau July 18, 2025 03:53
Copy link
Member

@fynngodau fynngodau left a comment

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.

@DaVinci9196 DaVinci9196 requested a review from fynngodau July 18, 2025 12:14
@fynngodau
Copy link
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

0.3.9 here we come!

@mar-v-in
Copy link
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
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.

@DaVinci9196 DaVinci9196 deleted the fix_split_install branch July 24, 2025 02:47
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