From 03c8fb23c881e479d8c913f30105a19c72152761 Mon Sep 17 00:00:00 2001 From: Rob Lazzurs Date: Fri, 16 Oct 2026 21:31:11 +0000 Subject: [PATCH 1/3] Run the account's loop on its own thread so iCloud calls stop queueing behind fetches Every call into Python was serialised behind PythonLock because FindMy.py's sync AppleAccount enters its event loop per call with run_until_complete. A first import walks thousands of key indices inside a single fetch_location, so the iCloud screen waited minutes for its turn (#126). account_loop.attach now swaps the account's _evt_loop for a stand-in that hands coroutines to the real loop, running forever on a thread of its own. Same loop, same session, same identity. The account is still used by one operation at a time: request_pet swaps the login state out across a GSA round trip, so a report request running beside it would read a missing dsid. Ordinary calls take the account's turn for their whole length; a location fetch inside interleaving() takes it once per fetch_raw_reports, so an iCloud step waits for one request rather than the whole walk. PythonICloudService no longer takes PythonLock. Location fetches still do, against each other. The "waiting for the location check" caption is gone with the wait it described. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 32 ++ .../FetchFromICloudBackPressTest.java | 7 +- .../FetchFromICloudActivity.java | 17 +- .../python/PythonAppleService.java | 5 +- .../opentagviewer/python/PythonLock.java | 38 +-- .../python/icloud/PythonICloudService.java | 20 +- app/src/main/python/account_loop.py | 273 +++++++++++++++ app/src/main/python/icloud_bridge.py | 17 +- app/src/main/python/main.py | 15 +- app/src/main/res/values-de/strings.xml | 1 - app/src/main/res/values-en/strings.xml | 1 - app/src/main/res/values-fr/strings.xml | 1 - app/src/main/res/values-ja/strings.xml | 1 - app/src/main/res/values-ko/strings.xml | 1 - app/src/main/res/values-nl/strings.xml | 1 - app/src/main/res/values-ru/strings.xml | 1 - app/src/main/res/values-zh-rCN/strings.xml | 1 - app/src/main/res/values-zh-rTW/strings.xml | 1 - app/src/main/res/values/strings.xml | 1 - app/src/test/python/test_account_loop.py | 312 ++++++++++++++++++ app/src/test/python/test_icloud_bridge.py | 22 +- 21 files changed, 707 insertions(+), 61 deletions(-) create mode 100644 app/src/main/python/account_loop.py create mode 100644 app/src/test/python/test_account_loop.py diff --git a/AGENTS.md b/AGENTS.md index 5fe25e58..1001685f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -694,6 +694,38 @@ already sent you, and then reasoning from a guess while you wait. had a phone number blacked out by hand, and the pixels either side of it were not. Quote the error text, never the surrounding screen, and never re-upload the image. +### 21. The account takes turns. It is never used by two things at once + +The signed-in account's asyncio loop runs on a thread of its own (`app/src/main/python/account_loop.py`), +so the iCloud screen no longer waits behind a location fetch. Before +[#126](https://github.com/parawanderer/OpenTagViewer/issues/126) every call into Python held +`PythonLock`, and a first import held it for minutes inside a single `fetch_location`. + +**That is interleaving, not concurrency, and it must stay that way.** FindMy.py's async account is +not safe to use from two operations at once. `request_pet`, which opening a keychain session calls, +swaps the account's login state out for a whole GSA round trip and restores it in a `finally`; a +report request running in that window reads a `dsid` that is not there. A 401 inside +`fetch_raw_reports` re-authenticates the same way. So every operation takes the account's turn: +an ordinary call for its whole length, a location fetch inside `account_loop.interleaving()` once +per request to Apple. An iCloud step waits for one request, not for the walk. + +Three things follow: + +- **Every account `main` hands Java is attached** (`account_loop.attach`, straight after it is built + and before its first call - the loop has to be idle to be handed over). `icloud_bridge.openSession` + refuses an account that is not, because without the thread the two flows would drive one loop + with `run_until_complete` at once - the failure the Java lock existed for. +- **Only location fetches go inside `interleaving()`.** They are known to touch the account in + `fetch_raw_reports` alone, which is wrapped to take the turn. Anything else in that block runs + with no exclusion at all. +- **Location fetches still take `PythonLock` against each other.** Two side by side would double the + request rate for the same keys (rule 6). The iCloud service is the only caller that dropped it. + +`test_account_loop.py` runs against the real pinned `AppleAccount` rather than a fake, because the +whole arrangement leans on two private details of it - `_evt_loop.run_until_complete` and the +fetcher looking `fetch_raw_reports` up on the instance - and a fake keeps both names after a rename +(rule 3). + --- ## Building and testing diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudBackPressTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudBackPressTest.java index e9dcded5..55a2fd82 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudBackPressTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudBackPressTest.java @@ -245,10 +245,11 @@ public void leavingClosesTheSession() { * the in-app arrow and the device's own back button alike - on the reasoning that there is * nothing to go back to mid-call. That reasoning holds for an unlock and for nothing else. * - *

What it produced in practice: every call into Python is serialised through + *

What it produced in practice: every call into Python was serialised through * {@code PythonLock}, because the iCloud flow and the location fetch drive the same asyncio - * loop, and a first import holds that lock for minutes walking key indices for tags - * with no alignment record. Opening this screen during that wait left the user on a spinner + * loop, and a first import held that lock for minutes walking key indices for tags + * with no alignment record. Issue #126 shortened that wait to one request to Apple, which is + * still a call genuinely in flight - so leaving has to keep working. Opening this screen during that wait left the user on a spinner * they could not leave by any means, reading "Looking for a device that can unlock it" - * which was not what was happening and, for an account already linked, never would be. * diff --git a/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java index a4bf3945..7f931343 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/FetchFromICloudActivity.java @@ -49,7 +49,6 @@ import dev.wander.android.opentagviewer.python.icloud.ICloudFetch; import dev.wander.android.opentagviewer.python.icloud.ICloudService; import dev.wander.android.opentagviewer.python.icloud.KeychainMembership; -import dev.wander.android.opentagviewer.python.PythonLock; import dev.wander.android.opentagviewer.python.icloud.RecoverableDevice; import dev.wander.android.opentagviewer.ui.CodeChipSpan; import dev.wander.android.opentagviewer.ui.RecoverableDeviceIcon; @@ -230,9 +229,9 @@ private void onBackWithin() { if (this.isShowing(R.id.icloud_loading_container)) { // **Only an unlock is worth trapping somebody for.** Swallowing every back press // while the loading step was up meant that a screen waiting on PythonLock - which a - // first import holds for minutes, walking key indices for tags with no alignment - - // could not be left at all. Neither the in-app arrow nor the device's own back - // button did anything, and the wait looked like a hang. + // first import held for minutes, walking key indices for tags with no alignment, + // until issue #126 - could not be left at all. Neither the in-app arrow nor the + // device's own back button did anything, and the wait looked like a hang. // // An unlock attempt is different: it is already talking to Apple, and attempts are // probably a limited resource on their end, so abandoning one mid-flight risks @@ -282,12 +281,14 @@ private void start() { // by the membership check further down: an app that has already joined resumes as the // member it is and never sees the device list. Saying otherwise up front tells somebody // who just linked their account that the app is hunting for hardware to unlock with, - // when the thing it is actually waiting for is its turn to talk to Python. + // when what it is actually doing is opening the account. + // + // That is also all it is doing while a location check runs. This used to say it was + // waiting for one, because it was - for minutes, on a first import - until the two + // started taking turns between requests to Apple (issue #126). // // askForADevice() sets that message, at the point it becomes true. - this.showWaiting(PythonLock.isBusy() - ? R.string.icloud_loading_waiting_for_location_check - : R.string.icloud_loading_opening_account); + this.showWaiting(R.string.icloud_loading_opening_account); if (this.icloud != null) { this.icloud.close(); diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAppleService.java b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAppleService.java index 6ce38090..7ba8565e 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAppleService.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAppleService.java @@ -92,10 +92,9 @@ public static PythonAppleService getInstance() { } /** - * Whether a call into Python is currently in progress. + * Whether a fetch or an account change is currently in progress. * - *

Delegates to {@link PythonLock}, which is where the lock moved when the iCloud flow - * started driving the same event loop. Kept here because the periodic refresh asks this + *

Delegates to {@link PythonLock}. Kept here because the periodic refresh asks this * service, and where the lock lives is not its business. */ public static boolean isBusy() { diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonLock.java b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonLock.java index cda507bf..a76ec215 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonLock.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonLock.java @@ -4,21 +4,22 @@ import java.util.concurrent.locks.ReentrantLock; /** - * The one lock that every call into Python must be made under. + * One location fetch, or one change to which account is signed in, at a time. * - *

FindMy.py's synchronous {@code AppleAccount} wraps an async one and drives it with a single - * asyncio event loop. RxJava schedules our work on a thread pool, and the periodic refresh in - * {@code MapsActivity} fires every 60 seconds regardless of whether the previous fetch has - * finished. Two threads calling {@code run_until_complete} on the same loop fails with - * "RuntimeError: This event loop is already running" and then keeps failing. + *

RxJava schedules our work on a thread pool, and the periodic refresh in {@code MapsActivity} + * fires every 60 seconds regardless of whether the previous fetch has finished. A fetch that + * takes longer than the refresh interval is entirely normal for an accessory with no alignment + * yet, so two fetches overlapping is not a rare race. Running them side by side would double the + * request rate against Apple for the same keys, which is the account-flagging risk rule 6 is + * about, and have both write back alignment for the same tags. * - *

A fetch that takes longer than the refresh interval is entirely normal for an accessory with - * no alignment yet, so this is not a rare race. - * - *

It lives here, rather than inside one service, because it is not one service's lock. - * The iCloud flow drives the same event loop - deliberately, since a second account would - * be a second device to Apple - so a location fetch and a keychain unlock collide exactly as two - * location fetches would. A second private lock in a second class would be no lock at all. + *

It is no longer what keeps the asyncio loop from being driven twice at once. It used + * to be: FindMy.py's synchronous account entered its event loop per call, and two threads in + * {@code run_until_complete} failed with "This event loop is already running". The loop now + * runs on its own thread and the account is shared out in turns on the Python side - see + * {@code account_loop.py}. That is why the iCloud flow does not take this lock any more: it used + * to wait for a whole fetch, which on a first import was minutes (issue #126), and now waits for + * one request at most. * *

Serialising alone is not enough: the periodic refresh would still queue up behind a slow * fetch, one entry per minute, and then fire the whole stale backlog at once when it finally @@ -32,13 +33,12 @@ private PythonLock() { } /** - * Run one call into Python, with nothing else in there at the same time. + * Run one call into Python, with no other fetch or account change in there at the same time. * *

Scope it to the call and nothing more. The reason this is a method taking the - * work, rather than a lock to take and release, is the iCloud flow: it is several calls with - * a person answering a dialog between them, and holding this across one of those waits would - * stop every location refresh in the app until they got round to typing. Each step takes it, - * finishes, and gives it back; the waiting happens in Java with nothing held. + * work, rather than a lock to take and release, is that anything held across a wait for a + * person - a sign-in code, a passcode dialog - would stop every location refresh in the app + * until they got round to typing. */ public static T holding(final Callable work) throws Exception { LOCK.lock(); @@ -50,7 +50,7 @@ public static T holding(final Callable work) throws Exception { } /** - * Whether a call into Python is currently in progress. + * Whether a fetch or an account change is currently in progress. * *

Advisory only. A caller that acts on this can still be beaten to the lock, which is * harmless: it just waits, exactly as it did before. diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/icloud/PythonICloudService.java b/app/src/main/java/dev/wander/android/opentagviewer/python/icloud/PythonICloudService.java index 243e726d..2a28fd98 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/python/icloud/PythonICloudService.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/icloud/PythonICloudService.java @@ -12,7 +12,6 @@ import java.util.List; import dev.wander.android.opentagviewer.python.PythonAppleAccount; -import dev.wander.android.opentagviewer.python.PythonLock; import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Observable; import io.reactivex.rxjava3.schedulers.Schedulers; @@ -195,10 +194,7 @@ public Observable> records(final List beaconIds) @Override public void close() { try { - PythonLock.holding(() -> { - this.session.callAttr("close"); - return null; - }); + this.session.callAttr("close"); } catch (Exception e) { // Callers close from a `finally`. Throwing here would replace whatever real failure // sent them there with a confusing one about closing. @@ -211,11 +207,12 @@ public void close() { * *

Two things happen here and both matter. * - *

The Python lock is taken for the call and given straight back. The iCloud flow and - * the app's periodic location refresh drive the same asyncio event loop, and two threads in - * {@code run_until_complete} on one loop fails permanently. Scoping it to the call rather - * than the flow is what keeps a passcode dialog from freezing every location update in the - * app until the user types. + *

No {@code PythonLock} here, and that is the point of issue #126. The iCloud flow + * and the location fetch share one account and one asyncio loop, and taking the Java lock + * made this wait for a whole fetch - minutes, on a first import. The loop now runs on a thread + * of its own and the account is shared out in turns on the Python side, a fetch giving way + * between requests to Apple, so a step here waits for one request at most. See + * {@code account_loop.py}. * *

A reported failure becomes an exception; a raised one is not expected. The bridge * returns failures as values precisely so their message is never empty, so anything actually @@ -223,8 +220,7 @@ public void close() { * {@link ICloudFailure#UNKNOWN} with whatever text it had. */ private JSONObject answered(final String step, final Object... arguments) throws Exception { - final String returned = PythonLock.holding( - () -> this.session.callAttr(step, arguments).toString()); + final String returned = this.session.callAttr(step, arguments).toString(); final JSONObject answer = new JSONObject(returned); diff --git a/app/src/main/python/account_loop.py b/app/src/main/python/account_loop.py new file mode 100644 index 00000000..5b2c2760 --- /dev/null +++ b/app/src/main/python/account_loop.py @@ -0,0 +1,273 @@ +""" +The account's event loop, run on a thread of its own instead of being borrowed by each caller. + +FindMy.py's synchronous `AppleAccount` drives its async account by calling +`self._evt_loop.run_until_complete(...)` from whichever thread asked. A loop can only be run by +one thread at a time, so Java serialised every call into Python behind `PythonLock` - and a +first import, which walks thousands of key indices inside a *single* `fetch_location`, held it +for minutes. The iCloud screen waited for the whole walk before it could say anything to Apple +(issue #126). + +:func:`attach` swaps the account's `_evt_loop` for an :class:`AccountLoop`, whose +`run_until_complete` hands the coroutine to the real loop - now running forever on its own +thread - and waits for the answer. **Same loop, same aiohttp session, same identity**: rule 11 is +untouched, because only *who drives* the loop changes, not what is on it. + +**The account is still used by one thing at a time, and that is deliberate.** FindMy.py's async +account is not safe to use concurrently, and not only in theory: `request_pet`, which opening a +keychain session calls, swaps the account's login state out for the length of a GSA round trip +and puts it back in a `finally`. Anything reading `dsid` or `service_tokens` in that window gets +a `KeyError` or an `InvalidStateError`. A 401 inside `fetch_raw_reports` re-authenticates the +same way. So every operation takes a turn on the account, and what changed is the size of a +turn: + +- an ordinary call takes one turn for its whole length - the exclusion it always had, measured + in coroutines rather than Java calls +- a location fetch inside :func:`interleaving` takes a turn **per request to Apple** instead, + by way of a wrapped `fetch_raw_reports`, and nothing between requests touches the account + +So a call that arrives during a long walk waits for one request, not for the walk. Between +requests the fetch is only deriving keys and decrypting, which needs nothing the account holds. +""" + +from __future__ import annotations + +import asyncio +import concurrent.futures +import contextvars +import threading +import traceback +from contextlib import contextmanager +from typing import Any, Awaitable, Callable, Coroutine, Iterator, TypeVar + +_T = TypeVar("_T") + +# How long closing waits for cancelled calls to unwind, and then for the thread to stop. Both +# are about a coroutine that will not let go, which should never happen; the bound is there so +# that it cannot hang whoever is closing the account. +_SHUTDOWN_TIMEOUT_SECONDS = 10.0 + +# Set on the calling thread by `interleaving`, read by `run_until_complete` on the same thread. +_interleaving = threading.local() + +# Whether the running task already holds the account's turn. A context variable rather than a +# task identity, so that it is inherited by anything the holder spawns - which is part of the same +# operation, and would otherwise deadlock waiting for a turn its own parent is holding. +_holdingTurn: contextvars.ContextVar[bool] = contextvars.ContextVar( + "holdingTurn", default=False) + + +@contextmanager +def interleaving() -> Iterator[None]: + """ + Let the account calls made on this thread take turns per request rather than per call. + + For location fetches, and only for them: they are the long calls, and the only ones whose + requests are known to be separable - `fetch_raw_reports` is the one place they touch the + account. Anything else made inside this block would run without any exclusion at all. + """ + previous = getattr(_interleaving, "on", False) + _interleaving.on = True + try: + yield + finally: + _interleaving.on = previous + + +class AccountLoop: + """ + Stands in for an account's `_evt_loop`, which FindMy.py uses for nothing but + `run_until_complete`. + + Deliberately not a general proxy for the loop. Anything that reached past these methods for + `run_forever` or `stop` would be fighting the thread that owns it, so they are not offered. + """ + + def __init__(self, loop: asyncio.AbstractEventLoop) -> None: + self._loop = loop + self._turn = asyncio.Lock() + # Guards `_closed` against a submission racing a close: anything submitted before the + # flag flips is on the loop by the time the close cancels what is there. + self._state = threading.Lock() + self._closed = False + self._thread = threading.Thread( + target=self._runForever, name="findmy-account-loop", daemon=True) + self._thread.start() + + def _runForever(self) -> None: + asyncio.set_event_loop(self._loop) + self._loop.run_forever() + + @property + def loop(self) -> asyncio.AbstractEventLoop: + return self._loop + + def is_running(self) -> bool: + return not self._closed and self._loop.is_running() + + def is_closed(self) -> bool: + return self._closed or self._loop.is_closed() + + def run_until_complete(self, coro: Coroutine[Any, Any, _T]) -> _T: + """What FindMy.py's sync wrapper calls: run this on the account's loop, in its turn.""" + try: + if getattr(_interleaving, "on", False): + return self._submit(coro) + + return self._submit(self.inTurn(lambda: coro)) + finally: + # A no-op once it has run. For one refused, or cancelled while still queued for the + # turn, it is what stops "coroutine was never awaited" landing in the log as if it + # were a bug. + coro.close() + + async def inTurn(self, makeCall: Callable[[], Awaitable[_T]]) -> _T: + """ + Make and await a call while holding the account's turn, or straight through if this + task already holds it. + + **A factory, so the call is made inside the turn.** FindMy.py checks the login state + when an account method is *called*, not when it is awaited - and `request_pet` holding + the turn is exactly when that state is temporarily wrong. + + **Reentrant on purpose.** An ordinary call that reaches `fetch_raw_reports` - a sync + `fetch_location` made outside `interleaving` - holds the turn already, and an + `asyncio.Lock` taken twice by one holder waits on itself forever. + """ + if _holdingTurn.get(): + return await makeCall() + + async with self._turn: + holding = _holdingTurn.set(True) + try: + return await makeCall() + finally: + _holdingTurn.reset(holding) + + def _submit(self, coro: Coroutine[Any, Any, _T]) -> _T: + if threading.current_thread() is self._thread: + # Blocking the loop's own thread on something only that thread can run never ends. + # An exception is a far better way to meet this than a frozen app. + coro.close() + raise RuntimeError( + "A synchronous account call was made from the account's own event loop thread") + + with self._state: + if self._closed: + coro.close() + raise RuntimeError("This account has been closed") + + future = asyncio.run_coroutine_threadsafe(coro, self._loop) + + try: + return future.result() + except concurrent.futures.CancelledError: + # Only `close` cancels anything here, so this is the account going away under a call + # that was waiting for it - a reason worth more than a bare CancelledError. + raise RuntimeError( + "The account was closed while this call was waiting for it") from None + + def shutDown(self) -> bool: + """ + Cancel whatever is still waiting, stop the loop, and stop its thread. + + :return: whether the thread stopped, which is what makes the loop safe to hand back. + """ + with self._state: + if self._closed: + return not self._thread.is_alive() + self._closed = True + + try: + asyncio.run_coroutine_threadsafe( + _cancelEverythingElse(), self._loop).result(_SHUTDOWN_TIMEOUT_SECONDS) + except Exception: + print(f"Account loop: calls still waiting did not unwind:\n{traceback.format_exc()}") + + self._loop.call_soon_threadsafe(self._loop.stop) + self._thread.join(_SHUTDOWN_TIMEOUT_SECONDS) + + if self._thread.is_alive(): + print("Account loop: its thread did not stop; leaving the loop where it is.") + return False + + return True + + +async def _cancelEverythingElse() -> None: + current = asyncio.current_task() + others = [task for task in asyncio.all_tasks() if task is not current] + + for task in others: + task.cancel() + + await asyncio.gather(*others, return_exceptions=True) + + +def attach(account: Any) -> bool: + """ + Run this account's loop on a thread of its own, from now until :func:`detach`. + + Call it once, straight after the account is built and before anything else uses it: the + loop has to be idle to be handed to another thread. + + :return: whether it is attached. False leaves the account as FindMy.py built it, driven by + whoever calls it - and :func:`icloud_bridge.openSession` refuses such an account, because + the iCloud flow is only safe beside a location fetch when this is in place. + """ + current = getattr(account, "_evt_loop", None) + asyncAccount = getattr(account, "_asyncacc", None) + + if isinstance(current, AccountLoop): + return True + + if current is None or asyncAccount is None: + print( + "FindMy.py's account internals have changed: no _evt_loop or _asyncacc, so the" + " account's loop cannot be given a thread of its own.") + return False + + if current.is_running() or current.is_closed(): + print("Not attaching the account loop: it is already running somewhere, or closed.") + return False + + attached = AccountLoop(current) + _takeTurnsPerRequest(asyncAccount, attached) + account._evt_loop = attached + + return True + + +def _takeTurnsPerRequest(asyncAccount: Any, attached: AccountLoop) -> None: + """ + Make each report request take the account's turn on its own. + + `fetch_raw_reports` is the one place a location fetch touches the account: it reads the + service token, asks Anisette for headers, and on a 401 re-authenticates. Every walk through + key indices in FindMy.py's report fetcher comes through here, one request at a time, so this + is the boundary where something else can be let in. + + Set on the instance, which is where the fetcher looks it up (`self._account.fetch_raw_reports`). + """ + original = asyncAccount.fetch_raw_reports + + async def fetch_raw_reports(*args: Any, **kwargs: Any) -> Any: + return await attached.inTurn(lambda: original(*args, **kwargs)) + + asyncAccount.fetch_raw_reports = fetch_raw_reports + + +def detach(account: Any) -> None: + """ + Stop the account's loop thread and hand the loop back, once the account is closed. + + Handed back rather than left behind so a closed account behaves exactly as it did before + this module existed - including `Closable.__del__`, which closes on whatever loop it finds. + """ + attached = getattr(account, "_evt_loop", None) + + if not isinstance(attached, AccountLoop): + return + + if attached.shutDown(): + account._evt_loop = attached.loop diff --git a/app/src/main/python/icloud_bridge.py b/app/src/main/python/icloud_bridge.py index 2b85beb8..f47b67b8 100644 --- a/app/src/main/python/icloud_bridge.py +++ b/app/src/main/python/icloud_bridge.py @@ -42,6 +42,7 @@ import traceback from typing import Any +import account_loop import identity as app_identity from exporter import icloud from exporter.identity import written_by_opentagviewer @@ -344,9 +345,10 @@ class ICloudSession: :meth:`close` when the screen is finished with - in a `finally`, because two of these steps hold sockets and an abandoned session leaks them for the life of the process. - **Not thread-safe, and it cannot be made so.** Every step runs a coroutine on the account's - own event loop, and a loop cannot be driven from two threads at once. Java calls these from - a single Rx chain. + **Not thread-safe with itself.** The steps build on each other's state, and Java calls them + from a single Rx chain. Beside a location fetch it is safe, and does not wait for one to + finish: the account's loop runs on its own thread and each step takes its turn on the + account between the fetch's requests - see :mod:`account_loop`. """ def __init__(self, account: Any, asyncAccount: Any, loop: Any) -> None: @@ -911,4 +913,13 @@ def openSession(account: Any) -> ICloudSession | None: " iCloud flow cannot be driven from the account the app is signed in with.") return None + if not isinstance(loop, account_loop.AccountLoop): + # Java no longer makes these steps wait behind a location fetch, which is only safe + # with the loop on its own thread. Without it, both would run `run_until_complete` on + # one loop at once - "This event loop is already running", for good. + print( + "The account's loop is not on a thread of its own, so the iCloud flow cannot run" + " beside a location fetch. See account_loop.attach.") + return None + return ICloudSession(account, asyncAccount, loop) diff --git a/app/src/main/python/main.py b/app/src/main/python/main.py index 5bab609f..0431c81a 100644 --- a/app/src/main/python/main.py +++ b/app/src/main/python/main.py @@ -30,6 +30,7 @@ SyncSecondFactorMethod ) +import account_loop import identity as app_identity @@ -318,6 +319,8 @@ def loginSync(email: str, password: str, anisetteServerUrl: str, timeout=LOGIN_TIMEOUT_SECONDS, **app_identity.deviceIdsForNewSession(localAnisette), ) + # Before the first call, while the loop is still idle - see account_loop.attach. + account_loop.attach(acc) state = acc.login(email, password) @@ -839,6 +842,7 @@ def getAccount( acc = AppleAccount.from_json(data) _preferLocalAnisette(acc, localAnisette) + account_loop.attach(acc) print(f"Restored account, login state: {acc.login_state}") @@ -975,6 +979,9 @@ def closeAccount(account: AppleAccount) -> bool: loop.run_until_complete(account.close()) print("Closed the account's HTTP session and connector.") + # After the close, which takes its turn like any other call: the loop's thread is only + # stopped once nothing on it still needs the session. + account_loop.detach(account) return True except Exception: # Discarding this account regardless, so a failure here changes nothing the caller does. @@ -1959,7 +1966,10 @@ def getLastReports( # which meant no beacon's updated alignment was persisted - so every later # fetch started from the same wide range again and never converged. try: - fetched = _fetchReportsForAccessory(account, airtag, start_dt, now_dt) + # Turns per request rather than per fetch, so the iCloud screen is not kept + # waiting for a whole key walk - see account_loop. + with account_loop.interleaving(): + fetched = _fetchReportsForAccessory(account, airtag, start_dt, now_dt) reports = fetched.reports or [] except Exception: failures += 1 @@ -2097,7 +2107,8 @@ def getReports( width_before = _isAlignmentWide(airtag, start_dt, end_dt) try: - reports = _fetchReportsInRange(account, airtag, start_dt, end_dt) or [] + with account_loop.interleaving(): + reports = _fetchReportsInRange(account, airtag, start_dt, end_dt) or [] except Exception: failures += 1 print(f"Fetch failed for {beaconId}, continuing with the rest: " diff --git a/app/src/main/res/values-de/strings.xml b/app/src/main/res/values-de/strings.xml index 6bda6f93..0406b6eb 100644 --- a/app/src/main/res/values-de/strings.xml +++ b/app/src/main/res/values-de/strings.xml @@ -247,7 +247,6 @@ Du kannst sie später wieder verknüpfen. Dafür brauchst du den Gerätecode dei Die Verknüpfung mit deinem Apple-Konto konnte nicht aufgehoben werden. Bitte versuche es erneut. Keine App auf diesem Telefon kann eine Karte öffnen Apple-Konto wird geöffnet … - Warten auf den Abschluss der Standortprüfung … Protokolle exportieren Protokolldatei wurde exportiert Protokolldatei konnte nicht exportiert werden diff --git a/app/src/main/res/values-en/strings.xml b/app/src/main/res/values-en/strings.xml index 9bd596cf..e391d1b4 100644 --- a/app/src/main/res/values-en/strings.xml +++ b/app/src/main/res/values-en/strings.xml @@ -247,7 +247,6 @@ You can link again later. That needs your Apple device passcode. Could not unlink your Apple account. Please try again. No app on this phone can open a map Opening your Apple account… - Waiting for the location check to finish… Export Logs Log file has been exported successfully Could not export the log file diff --git a/app/src/main/res/values-fr/strings.xml b/app/src/main/res/values-fr/strings.xml index 86ce02ed..e3f89dfb 100644 --- a/app/src/main/res/values-fr/strings.xml +++ b/app/src/main/res/values-fr/strings.xml @@ -248,7 +248,6 @@ Vous pourrez réassocier votre compte plus tard. Le code de votre appareil Apple Impossible de dissocier votre compte Apple. Veuillez réessayer. Aucune application de ce téléphone ne peut ouvrir une carte Ouverture de votre compte Apple… - En attente de la fin de la recherche de position… Exporter les journaux Le fichier journal a bien été exporté Impossible d’exporter le fichier journal diff --git a/app/src/main/res/values-ja/strings.xml b/app/src/main/res/values-ja/strings.xml index 89f8366e..782894bd 100644 --- a/app/src/main/res/values-ja/strings.xml +++ b/app/src/main/res/values-ja/strings.xml @@ -247,7 +247,6 @@ Apple アカウントの連携を解除できませんでした。もう一度お試しください。 この端末には地図を開けるアプリがありません Apple アカウントを開いています… - 位置情報の確認が終わるのを待っています… ログを書き出す ログファイルを書き出しました ログファイルを書き出せませんでした diff --git a/app/src/main/res/values-ko/strings.xml b/app/src/main/res/values-ko/strings.xml index 868a1313..9ac42b60 100644 --- a/app/src/main/res/values-ko/strings.xml +++ b/app/src/main/res/values-ko/strings.xml @@ -247,7 +247,6 @@ Apple 계정 연결을 해제하지 못했습니다. 다시 시도해 주세요. 이 휴대전화에는 지도를 열 수 있는 앱이 없습니다 Apple 계정을 여는 중… - 위치 확인이 끝나기를 기다리는 중… 로그 내보내기 로그 파일을 내보냈습니다 로그 파일을 내보내지 못했습니다 diff --git a/app/src/main/res/values-nl/strings.xml b/app/src/main/res/values-nl/strings.xml index d2e116d2..abf4ca86 100644 --- a/app/src/main/res/values-nl/strings.xml +++ b/app/src/main/res/values-nl/strings.xml @@ -247,7 +247,6 @@ Je kunt later opnieuw koppelen. Daarvoor heb je de toegangscode van je Apple-app Kan de koppeling met je Apple-account niet opheffen. Probeer het opnieuw. Geen app op deze telefoon kan een kaart openen Je Apple-account wordt geopend… - Wachten tot de locatiecontrole klaar is… Logboeken exporteren Logbestand is geëxporteerd Kon het logbestand niet exporteren diff --git a/app/src/main/res/values-ru/strings.xml b/app/src/main/res/values-ru/strings.xml index 96a992cb..52ff2691 100644 --- a/app/src/main/res/values-ru/strings.xml +++ b/app/src/main/res/values-ru/strings.xml @@ -247,7 +247,6 @@ Не удалось отвязать учётную запись Apple. Повторите попытку. На этом телефоне нет приложения, способного открыть карту Открытие вашей учётной записи Apple… - Ожидание завершения проверки местоположения… Экспорт журналов Файл журнала экспортирован Не удалось экспортировать файл журнала diff --git a/app/src/main/res/values-zh-rCN/strings.xml b/app/src/main/res/values-zh-rCN/strings.xml index 5f1810a5..ca2ff1c2 100644 --- a/app/src/main/res/values-zh-rCN/strings.xml +++ b/app/src/main/res/values-zh-rCN/strings.xml @@ -247,7 +247,6 @@ 无法取消关联你的 Apple 账户，请重试。 此手机上没有可以打开地图的应用 正在打开您的 Apple 账户… - 正在等待位置检查完成… 导出日志 日志文件已导出 无法导出日志文件 diff --git a/app/src/main/res/values-zh-rTW/strings.xml b/app/src/main/res/values-zh-rTW/strings.xml index 9bf37589..e106d31e 100644 --- a/app/src/main/res/values-zh-rTW/strings.xml +++ b/app/src/main/res/values-zh-rTW/strings.xml @@ -247,7 +247,6 @@ 無法取消連結你的 Apple 帳戶，請重試。 此手機上沒有可以開啟地圖的應用程式 正在開啟您的 Apple 帳戶… - 正在等待位置檢查完成… 匯出日誌 日誌檔案已匯出 無法匯出日誌檔案 diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 5f33053d..37660928 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -281,7 +281,6 @@ You can link again later. That needs your Apple device passcode. Could not unlink your Apple account. Please try again. No app on this phone can open a map Opening your Apple account… - Waiting for the location check to finish… Export Logs Log file has been exported successfully Could not export the log file diff --git a/app/src/test/python/test_account_loop.py b/app/src/test/python/test_account_loop.py new file mode 100644 index 00000000..703da034 --- /dev/null +++ b/app/src/test/python/test_account_loop.py @@ -0,0 +1,312 @@ +""" +The account's event loop on its own thread - issue #126. + +**Against the real `AppleAccount`, not a fake.** Everything here depends on two private details +of the pinned FindMy.py: that the sync wrapper drives its async account through +`self._evt_loop.run_until_complete`, and that the report fetcher reaches Apple through +`self._account.fetch_raw_reports`, looked up on the instance. A fake would keep both names after +a rename and keep passing (rule 3). Only the network is replaced: `fetch_raw_reports` and +`request_pet` are swapped for coroutines that record when they ran. +""" + +from __future__ import annotations + +import asyncio +import json +import os +import threading +import time +from datetime import datetime, timedelta, timezone + +import pytest + +from findmy import FindMyAccessory +from findmy.reports import AppleAccount, RemoteAnisetteProvider + +import account_loop +import main + +# Long enough that a walk of several requests takes visibly longer than one request does, and +# short enough that the suite does not notice. +REQUEST_SECONDS = 0.05 + + +class Apple: + """ + Stands in for Apple's side of the two calls that matter, and remembers how they overlapped. + + `request_pet` is the dangerous one to run beside a fetch: it swaps the account's login state + out for the length of a round trip. So the thing to prove is that it never runs while a report + request is in flight - and, the point of the issue, that it does not wait for the whole walk. + """ + + def __init__(self) -> None: + self.requests = 0 + self.inFlight = 0 + self.overlapped = False + self.firstRequest = threading.Event() + self.requestsWhenPetAsked: int | None = None + + async def fetch_raw_reports(self, devices): + self.inFlight += 1 + try: + self.firstRequest.set() + await asyncio.sleep(REQUEST_SECONDS) + self.requests += 1 + return [] + finally: + self.inFlight -= 1 + + async def request_pet(self): + if self.inFlight: + self.overlapped = True + self.requestsWhenPetAsked = self.requests + await asyncio.sleep(REQUEST_SECONDS) + if self.inFlight: + self.overlapped = True + return "pet" + + +@pytest.fixture +def apple(): + return Apple() + + +@pytest.fixture +def account(apple): + """A real account, attached the way `main` attaches one, with Apple replaced.""" + made = AppleAccount(RemoteAnisetteProvider("https://example.invalid")) + + # Before attaching, as the real method is: attach wraps whatever it finds there. + made._asyncacc.fetch_raw_reports = apple.fetch_raw_reports + made._asyncacc.request_pet = apple.request_pet + + assert account_loop.attach(made) + yield made + main.closeAccount(made) + + +def anAccessoryWithAWalkAhead() -> FindMyAccessory: + """ + An accessory with no alignment, paired long enough ago that finding it takes several requests. + + Apple answers nothing here, so `fetch_location` walks the whole history - the first-import + case the issue is about, in miniature. + """ + return FindMyAccessory( + master_key=os.urandom(28), + skn=os.urandom(32), + sks=os.urandom(32), + paired_at=datetime.now(tz=timezone.utc) - timedelta(days=10), + ) + + +def fetchingInTheBackground(account, interleaved: bool): + """Start a location fetch on another thread, the way Java's Rx pool would.""" + failures: list[BaseException] = [] + + def fetch(): + try: + if interleaved: + with account_loop.interleaving(): + account.fetch_location(anAccessoryWithAWalkAhead()) + else: + account.fetch_location(anAccessoryWithAWalkAhead()) + except BaseException as e: # noqa: BLE001 - reported to the test, not swallowed + failures.append(e) + + thread = threading.Thread(target=fetch) + thread.start() + return thread, failures + + +class TestAnICloudCallDuringALongFetch: + def test_it_waits_for_one_request_rather_than_the_whole_walk(self, account, apple): + fetching, failures = fetchingInTheBackground(account, interleaved=True) + assert apple.firstRequest.wait(5) + + assert account.request_pet() == "pet" + fetching.join(30) + + assert not failures + assert apple.requests > 2, "the walk was too short to show anything" + assert apple.requestsWhenPetAsked is not None + assert apple.requestsWhenPetAsked < apple.requests, ( + "request_pet ran only once the whole walk had finished - which is issue #126") + + def test_it_never_runs_while_a_request_is_in_flight(self, account, apple): + """ + `request_pet` rewrites the login state across an await, so a report request running + beside it would read a session that is half somebody else's. + """ + fetching, failures = fetchingInTheBackground(account, interleaved=True) + assert apple.firstRequest.wait(5) + + account.request_pet() + fetching.join(30) + + assert not failures + assert not apple.overlapped + + def test_outside_interleaving_a_call_still_holds_the_account_for_its_whole_length( + self, account, apple): + """ + Anything not known to be separable keeps the exclusion it always had. Also proves this + test's own setup can tell the difference - the walk really is long enough to be waited + behind. + """ + fetching, failures = fetchingInTheBackground(account, interleaved=False) + assert apple.firstRequest.wait(5) + + account.request_pet() + fetching.join(30) + + assert not failures + assert apple.requestsWhenPetAsked == apple.requests + + +class TestTheLoopItself: + def test_it_runs_on_a_thread_of_its_own(self, account): + attached = account._evt_loop + + assert isinstance(attached, account_loop.AccountLoop) + assert attached.is_running() + assert threading.current_thread() is not attached._thread + + def test_attaching_twice_is_harmless(self, account): + attached = account._evt_loop + + assert account_loop.attach(account) + assert account._evt_loop is attached + + def test_calls_from_two_threads_at_once_both_complete(self, account, apple): + """The failure the Java lock existed to prevent: "This event loop is already running".""" + results: list[str] = [] + threads = [threading.Thread(target=lambda: results.append(account.request_pet())) + for _ in range(4)] + + for thread in threads: + thread.start() + for thread in threads: + thread.join(10) + + assert results == ["pet"] * 4 + + def test_a_sync_call_from_the_loop_thread_fails_instead_of_hanging(self, account): + async def callingBackIn(): + return account.request_pet() + + with pytest.raises(RuntimeError, match="own event loop thread"): + account._evt_loop.run_until_complete(callingBackIn()) + + def test_an_account_without_the_internals_is_left_alone(self): + class Moved: + pass + + assert not account_loop.attach(Moved()) + + def test_a_loop_that_is_already_running_is_not_taken(self): + account = AppleAccount(RemoteAnisetteProvider("https://example.invalid")) + + async def attachFromInside(): + account._evt_loop = asyncio.get_running_loop() + return account_loop.attach(account) + + loop = asyncio.new_event_loop() + try: + assert not loop.run_until_complete(attachFromInside()) + finally: + loop.close() + + +class TestClosing: + def test_closing_stops_the_thread_and_hands_the_loop_back(self, apple): + account = AppleAccount(RemoteAnisetteProvider("https://example.invalid")) + account_loop.attach(account) + attached = account._evt_loop + + assert main.closeAccount(account) + + assert not attached._thread.is_alive() + assert account._evt_loop is attached.loop + assert not account._evt_loop.is_running() + + def test_a_call_waiting_its_turn_is_told_the_account_went_away(self, account, apple): + fetching, _ = fetchingInTheBackground(account, interleaved=False) + assert apple.firstRequest.wait(5) + + waiting: list[BaseException] = [] + + def askForAPet(): + try: + account.request_pet() + except BaseException as e: # noqa: BLE001 + waiting.append(e) + + asker = threading.Thread(target=askForAPet) + asker.start() + time.sleep(REQUEST_SECONDS) + + account._evt_loop.shutDown() + asker.join(10) + fetching.join(10) + + assert not asker.is_alive(), "a waiting call hung when the account closed" + assert len(waiting) == 1 + assert isinstance(waiting[0], RuntimeError) + assert "closed" in str(waiting[0]) + + # Hands the loop back so the fixture can close the session on it, as `closeAccount` + # does in the app - which closes before it detaches, never after. + account_loop.detach(account) + + def test_a_call_after_closing_fails_instead_of_hanging(self, account): + account._evt_loop.shutDown() + + with pytest.raises(RuntimeError, match="closed"): + account._evt_loop.run_until_complete(asyncio.sleep(0)) + + account_loop.detach(account) + + +class TestWhereTheAppAttachesIt: + """`main` has to attach every account it hands Java, or the iCloud flow refuses to open.""" + + def test_a_restored_account_comes_back_attached(self): + from findmy.reports.state import LoginState + + stored = AppleAccount(RemoteAnisetteProvider("https://ani.example.com")).to_json() + stored["login"]["state"] = LoginState.LOGGED_IN.value + + restored = main.getAccount(json.dumps(stored)) + try: + assert isinstance(restored._evt_loop, account_loop.AccountLoop) + finally: + main.closeAccount(restored) + + def test_a_new_sign_in_is_attached_before_it_logs_in(self, monkeypatch): + """Before, because the loop has to be idle to be handed over - see `attach`.""" + from findmy.reports.state import LoginState + + loggedInOn: list[str] = [] + + def building(*args, **kwargs): + made = AppleAccount(RemoteAnisetteProvider("https://ani.example.com")) + + async def login(username, password): + loggedInOn.append(threading.current_thread().name) + return LoginState.LOGGED_IN + + made._asyncacc.login = login + return made + + monkeypatch.setattr(main, "AppleAccount", building) + monkeypatch.setattr(main, "_anisetteProvider", lambda *a, **k: object()) + monkeypatch.setattr(main.app_identity, "identityForNewSession", lambda _: {}) + monkeypatch.setattr(main.app_identity, "deviceIdsForNewSession", lambda _: {}) + + signedIn = main.loginSync("someone@example.com", "hunter2", "https://ani.example") + try: + assert loggedInOn == ["findmy-account-loop"] + finally: + main.closeAccount(signedIn["account"]) diff --git a/app/src/test/python/test_icloud_bridge.py b/app/src/test/python/test_icloud_bridge.py index db33aef9..c1bb1291 100644 --- a/app/src/test/python/test_icloud_bridge.py +++ b/app/src/test/python/test_icloud_bridge.py @@ -26,6 +26,7 @@ from findmy.errors import InvalidCredentialsError, UnauthorizedError +import account_loop import icloud_bridge import identity as app_identity from exporter import icloud @@ -194,8 +195,15 @@ class FakeAsyncAccount: @pytest.fixture def loop(): + """ + The loop as the app's account has it: running on a thread of its own. + + Not a bare loop, because `openSession` refuses one - see `account_loop.attach`. + """ made = asyncio.new_event_loop() - yield made + attached = account_loop.AccountLoop(made) + yield attached + attached.shutDown() made.close() @@ -241,6 +249,18 @@ class Moved: assert icloud_bridge.openSession(Moved()) is None + def test_it_refuses_an_account_whose_loop_is_not_on_its_own_thread(self): + """ + Java lets these steps run beside a location fetch now, which is only safe once + `account_loop.attach` has given the loop a thread. Without it, both would drive one loop + with `run_until_complete` at once. + """ + bare = asyncio.new_event_loop() + try: + assert icloud_bridge.openSession(FakeAccount(bare)) is None + finally: + bare.close() + def test_nothing_can_be_done_before_the_client_is_open(self, loop): made = icloud_bridge.openSession(FakeAccount(loop)) From e0a5301139a9f4c16a7988535d1f653af9fc51b4 Mon Sep 17 00:00:00 2001 From: Rob Lazzurs Date: Fri, 16 Oct 2026 21:44:43 +0000 Subject: [PATCH 2/3] Stop the loop thread of an account dropped without being closed A failed sign-in or an abandoned code screen never reaches closeAccount. That used to cost two sockets until the collector ran; with the loop on its own thread it would also leave a thread idling forever. A finaliser on the account now lets whatever is already on the loop finish - including the close FindMy.py's own finaliser schedules - and then stops the thread. Co-Authored-By: Claude Opus 5.5 --- app/src/main/python/account_loop.py | 34 ++++++++++++++++++++++++ app/src/test/python/test_account_loop.py | 13 +++++++++ 2 files changed, 47 insertions(+) diff --git a/app/src/main/python/account_loop.py b/app/src/main/python/account_loop.py index 5b2c2760..c9d0eb90 100644 --- a/app/src/main/python/account_loop.py +++ b/app/src/main/python/account_loop.py @@ -37,6 +37,7 @@ import contextvars import threading import traceback +import weakref from contextlib import contextmanager from typing import Any, Awaitable, Callable, Coroutine, Iterator, TypeVar @@ -167,6 +168,29 @@ def _submit(self, coro: Coroutine[Any, Any, _T]) -> _T: raise RuntimeError( "The account was closed while this call was waiting for it") from None + def stopOnceIdle(self) -> None: + """ + Stop the thread once whatever is already on the loop has finished, without waiting here. + + For an account dropped without being closed - a failed sign-in, a code screen somebody + walked away from - which used to cost two sockets until the collector ran, and would now + also cost a thread idling forever. Called from a finaliser, on whatever thread the + collector happens to be on, so it must not block. + + **Finished, not cancelled.** FindMy.py's own finaliser has just scheduled the session's + `close` on this loop, and cancelling it would leak the very sockets it is closing. + """ + with self._state: + if self._closed: + return + self._closed = True + + try: + self._loop.call_soon_threadsafe(self._loop.create_task, _finishThenStop()) + except RuntimeError: + # The loop is closed already, so there is no thread left to stop. + pass + def shutDown(self) -> bool: """ Cancel whatever is still waiting, stop the loop, and stop its thread. @@ -204,6 +228,14 @@ async def _cancelEverythingElse() -> None: await asyncio.gather(*others, return_exceptions=True) +async def _finishThenStop() -> None: + current = asyncio.current_task() + others = [task for task in asyncio.all_tasks() if task is not current] + + await asyncio.gather(*others, return_exceptions=True) + asyncio.get_running_loop().stop() + + def attach(account: Any) -> bool: """ Run this account's loop on a thread of its own, from now until :func:`detach`. @@ -234,6 +266,8 @@ def attach(account: Any) -> bool: attached = AccountLoop(current) _takeTurnsPerRequest(asyncAccount, attached) account._evt_loop = attached + # Holds the loop, never the account, or the account could never be collected to fire it. + weakref.finalize(account, attached.stopOnceIdle) return True diff --git a/app/src/test/python/test_account_loop.py b/app/src/test/python/test_account_loop.py index 703da034..a96652b6 100644 --- a/app/src/test/python/test_account_loop.py +++ b/app/src/test/python/test_account_loop.py @@ -12,6 +12,7 @@ from __future__ import annotations import asyncio +import gc import json import os import threading @@ -231,6 +232,18 @@ def test_closing_stops_the_thread_and_hands_the_loop_back(self, apple): assert account._evt_loop is attached.loop assert not account._evt_loop.is_running() + def test_an_account_dropped_without_closing_does_not_leave_its_thread_behind(self): + """A failed sign-in, or a code screen walked away from, never reaches `closeAccount`.""" + account = AppleAccount(RemoteAnisetteProvider("https://example.invalid")) + account_loop.attach(account) + thread = account._evt_loop._thread + + del account + gc.collect() + thread.join(5) + + assert not thread.is_alive() + def test_a_call_waiting_its_turn_is_told_the_account_went_away(self, account, apple): fetching, _ = fetchingInTheBackground(account, interleaved=False) assert apple.firstRequest.wait(5) From 2b585378d43b33a3b13fb221ec3d7da9b20b1334 Mon Sep 17 00:00:00 2001 From: Rob Lazzurs Date: Fri, 16 Oct 2026 22:15:37 +0000 Subject: [PATCH 3/3] Attach the iCloud test accounts, and let attach degrade rather than raise The emulator suite's two iCloud bridge classes built accounts with a bare event loop, which openSession now refuses. They attach it the way main does. attach no longer assumes fetch_raw_reports exists or that the account can be weakly referenced. Without the method the loop still gets its thread and a fetch inside interleaving() takes one whole turn, as before; an exception there would fail the restore and sign the user out. Co-Authored-By: Claude Opus 5.5 --- .../icloud/OpeningARealICloudSessionTest.java | 12 ++-- app/src/debug/python/icloud_test_double.py | 27 ++++++--- app/src/main/python/account_loop.py | 29 ++++++++-- app/src/test/python/test_account_loop.py | 55 +++++++++++++++++++ 4 files changed, 108 insertions(+), 15 deletions(-) diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/OpeningARealICloudSessionTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/OpeningARealICloudSessionTest.java index 745c372b..ba65958e 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/OpeningARealICloudSessionTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/icloud/OpeningARealICloudSessionTest.java @@ -4,6 +4,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import com.chaquo.python.Kwarg; import com.chaquo.python.PyObject; @@ -108,19 +109,22 @@ public void anaccountTheBridgeCannotUseYieldsNullRatherThanACrash() { * *

The account is assembled here rather than mocked at the Java seam, which is the whole * point: {@code openSession} looks for the two private attributes FindMy.py's account carries, - * and a {@code SimpleNamespace} with those is enough to get a real {@code ICloudSession} back - * across the real bridge. No Apple account, no network, and no fake standing where the bug was. + * and a {@code SimpleNamespace} with those - attached, as {@code main} attaches every account - + * is enough to get a real {@code ICloudSession} back across the real bridge. No Apple account, no network, and no fake standing where the bug was. */ @Test public void asessionThatOpensIsHandedBackRatherThanLostInConversion() { final Python python = Python.getInstance(); - // What `openSession` guards on: an async account and a loop to drive it. Nothing is - // called on either here - opening a session only stores them. + // What `openSession` guards on: an async account and a loop to drive it, with the loop + // on its own thread as `main` leaves every account (issue #126). Nothing is called on + // either here - opening a session only stores them. final PyObject account = python.getModule("types").callAttr( "SimpleNamespace", new Kwarg("_asyncacc", python.getModule("types").callAttr("SimpleNamespace")), new Kwarg("_evt_loop", python.getModule("asyncio").callAttr("new_event_loop"))); + assertTrue("the account's loop could not be given its own thread", + python.getModule("account_loop").callAttr("attach", account).toBoolean()); assertNotNull("a session that opened must reach Java, not be lost converting it", PythonICloudService.openFor(new PythonAppleAccount(account))); diff --git a/app/src/debug/python/icloud_test_double.py b/app/src/debug/python/icloud_test_double.py index 400e76ce..531aba0f 100644 --- a/app/src/debug/python/icloud_test_double.py +++ b/app/src/debug/python/icloud_test_double.py @@ -27,6 +27,7 @@ from types import SimpleNamespace from typing import Any +import account_loop from exporter import icloud _originals: dict[str, Any] = {} @@ -245,13 +246,25 @@ def anAccount() -> Any: asserting it reached the join cannot pass against code that never read the session at all: that path falls back to `identity.LEGACY_SERIAL`, and the two must not be the same string. """ - return SimpleNamespace( - _asyncacc=SimpleNamespace( - serial=SESSION_SERIAL, - identity=SimpleNamespace( - model="iPhone17,1", os_name="iPhone OS", os_version="18.1", - os_build="22B83", cfnetwork="1568.100.1", darwin="24.1.0")), - _evt_loop=asyncio.new_event_loop()) + account = _Account() + account._asyncacc = SimpleNamespace( + serial=SESSION_SERIAL, + identity=SimpleNamespace( + model="iPhone17,1", os_name="iPhone OS", os_version="18.1", + os_build="22B83", cfnetwork="1568.100.1", darwin="24.1.0")) + account._evt_loop = asyncio.new_event_loop() + + # As `main` does for every account it hands Java, or `openSession` refuses it. + account_loop.attach(account) + + return account + + +class _Account: + """A plain class rather than a SimpleNamespace, so `attach` can watch it for collection.""" + + _asyncacc: Any + _evt_loop: Any def whatReachedTheAccount() -> dict[str, Any]: diff --git a/app/src/main/python/account_loop.py b/app/src/main/python/account_loop.py index c9d0eb90..17ee0262 100644 --- a/app/src/main/python/account_loop.py +++ b/app/src/main/python/account_loop.py @@ -91,6 +91,10 @@ def __init__(self, loop: asyncio.AbstractEventLoop) -> None: # flag flips is on the loop by the time the close cancels what is there. self._state = threading.Lock() self._closed = False + # Whether report requests take turns of their own. Until they do, a fetch inside + # `interleaving` must take one turn for its whole length like anything else, or it would + # run with no exclusion at all. + self._turnsPerRequest = False self._thread = threading.Thread( target=self._runForever, name="findmy-account-loop", daemon=True) self._thread.start() @@ -112,7 +116,7 @@ def is_closed(self) -> bool: def run_until_complete(self, coro: Coroutine[Any, Any, _T]) -> _T: """What FindMy.py's sync wrapper calls: run this on the account's loop, in its turn.""" try: - if getattr(_interleaving, "on", False): + if self._turnsPerRequest and getattr(_interleaving, "on", False): return self._submit(coro) return self._submit(self.inTurn(lambda: coro)) @@ -266,8 +270,14 @@ def attach(account: Any) -> bool: attached = AccountLoop(current) _takeTurnsPerRequest(asyncAccount, attached) account._evt_loop = attached - # Holds the loop, never the account, or the account could never be collected to fire it. - weakref.finalize(account, attached.stopOnceIdle) + + try: + # Holds the loop, never the account, or the account could never be collected to fire it. + weakref.finalize(account, attached.stopOnceIdle) + except TypeError: + # Only an object that cannot be weakly referenced, which FindMy.py's account can be. + # The thread then lives until `detach`, which is a leak and not a fault. + print("Account loop: this account cannot be watched for collection; close it explicitly.") return True @@ -282,13 +292,24 @@ def _takeTurnsPerRequest(asyncAccount: Any, attached: AccountLoop) -> None: is the boundary where something else can be let in. Set on the instance, which is where the fetcher looks it up (`self._account.fetch_raw_reports`). + + **Missing is survivable, so it is not an error.** Attaching still gives the loop its thread, + and fetches then take one turn each, exactly as they did before this module. Raising instead + would fail the restore, and a failed restore signs the user out. """ - original = asyncAccount.fetch_raw_reports + original = getattr(asyncAccount, "fetch_raw_reports", None) + + if original is None: + print( + "FindMy.py's account has no fetch_raw_reports, so location fetches cannot give way" + " between requests; each will hold the account until it finishes.") + return async def fetch_raw_reports(*args: Any, **kwargs: Any) -> Any: return await attached.inTurn(lambda: original(*args, **kwargs)) asyncAccount.fetch_raw_reports = fetch_raw_reports + attached._turnsPerRequest = True def detach(account: Any) -> None: diff --git a/app/src/test/python/test_account_loop.py b/app/src/test/python/test_account_loop.py index a96652b6..975a0c5e 100644 --- a/app/src/test/python/test_account_loop.py +++ b/app/src/test/python/test_account_loop.py @@ -18,6 +18,7 @@ import threading import time from datetime import datetime, timedelta, timezone +from types import SimpleNamespace import pytest @@ -323,3 +324,57 @@ async def login(username, password): assert loggedInOn == ["findmy-account-loop"] finally: main.closeAccount(signedIn["account"]) + + +class TestWhenTheInternalsMove: + """Degrading, never raising: a failed attach in `getAccount` would sign the user out.""" + + def test_an_account_without_fetch_raw_reports_still_gets_its_thread(self): + account = SimpleNamespace(_asyncacc=SimpleNamespace(), _evt_loop=asyncio.new_event_loop()) + + assert account_loop.attach(account) + try: + assert account._evt_loop.is_running() + assert not account._evt_loop._turnsPerRequest + finally: + account._evt_loop.shutDown() + + def test_without_it_a_fetch_inside_interleaving_still_takes_a_whole_turn(self, apple): + """Otherwise the fetch would run beside `request_pet` with no exclusion at all.""" + made = AppleAccount(RemoteAnisetteProvider("https://example.invalid")) + + async def closing(): + pass + + # The real sync wrapper over an async account that has lost the method. + made._asyncacc = SimpleNamespace(request_pet=apple.request_pet, close=closing) + + async def walk(devices): + # The fetcher's own loop over requests, standing in for a method FindMy.py renamed. + for _ in range(10): + await apple.fetch_raw_reports(devices) + return [] + + assert account_loop.attach(made) + try: + fetching_failures: list[BaseException] = [] + + def fetch(): + try: + with account_loop.interleaving(): + made._evt_loop.run_until_complete(walk([])) + except BaseException as e: # noqa: BLE001 + fetching_failures.append(e) + + fetching = threading.Thread(target=fetch) + fetching.start() + assert apple.firstRequest.wait(5) + + made.request_pet() + fetching.join(30) + + assert not fetching_failures + assert not apple.overlapped + assert apple.requestsWhenPetAsked == apple.requests + finally: + main.closeAccount(made)