Skip to content

perf: fix Android AAB cleanup and mapping cache #653

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.

Sign up for GitHub

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
jamieQ merged 2 commits into from
Aug 7, 2026
+10 −2
Merged

perf: fix Android AAB cleanup and mapping cache #653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
995b844
fix: bind APK temp directory cleanup
jamieQ Aug 7, 2026
31f7c5e
fix: cache parsed AAB dex mapping
jamieQ Aug 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/launchpad/artifacts/android/aab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import shutil
import tempfile

from functools import partial
from pathlib import Path
from typing import Callable

Expand Down Expand Up @@ -101,7 +102,7 @@ def get_primary_apks(self, device_spec: DeviceSpec = DeviceSpec()) -> list[APK]:
APK(
new_apk_path,
self.get_dex_mapping(),
cleanup=lambda: shutil.rmtree(tmp_dir),
cleanup=partial(shutil.rmtree, tmp_dir),
)
)

Expand Down Expand Up @@ -148,7 +149,8 @@ def get_dex_mapping(self) -> DexMapping | None:
dex_mapping_file = dex_mapping_files[0]
with open(dex_mapping_file, "rb") as f:
dex_mapping_buffer = f.read()
return DexMapping(dex_mapping_buffer)
self._dex_mapping = DexMapping(dex_mapping_buffer)
return self._dex_mapping

@sentry_sdk.trace
def get_app_icon(self) -> bytes | None:
Comment on lines 149 to 156

@sentry sentry Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The caching in get_dex_mapping is incomplete. When no proguard.map file is found, the None result is not stored, causing repeated expensive filesystem searches on subsequent calls.
Severity: MEDIUM

Suggested Fix

To ensure the None result is cached, use a sentinel value. Initialize self._dex_mapping to a unique sentinel object. In get_dex_mapping, if no mapping file is found, explicitly set self._dex_mapping = None before returning. This ensures the filesystem search runs only once, and subsequent calls will immediately return the cached None value.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/launchpad/artifacts/android/aab.py#L149-L156

Potential issue: The `get_dex_mapping` method is designed to cache the result of a
filesystem search for a `proguard.map` file. However, when no mapping file is found, the
method returns `None` but fails to update the `self._dex_mapping` instance variable. As
a result, if the method is called multiple times, such as within the `get_primary_apks`
loop, the expensive `rglob` filesystem search is re-executed on every call. This defeats
the purpose of caching for the common scenario where an AAB does not contain a Proguard
mapping file, leading to unnecessary performance degradation.

Did we get this right? 👍 / 👎 to inform future reviews.

@jamieQ jamieQ Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't yet have any definitive empirical evidence the filesystem searches are particularly expensive. This might be a reasonable thing to do, but this change was intentionally kept pretty minimal.

Expand Down
6 changes: 6 additions & 0 deletions tests/unit/artifacts/android/test_aab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,9 @@ def test_get_app_icon_rejects_path_traversal(self, test_aab: AAB) -> None:
with patch.object(test_aab, "get_manifest", return_value=malicious_manifest):
with pytest.raises(UnsafePathError):
test_aab.get_app_icon()

def test_get_dex_mapping_caches_mapping(self, test_aab: AAB) -> None:
dex_mapping = test_aab.get_dex_mapping()

assert dex_mapping is not None
assert test_aab.get_dex_mapping() is dex_mapping
Loading