Survive the Android idle stop of the Kolibri server service - #15104
Survive the Android idle stop of the Kolibri server service#15104rtibblesbot wants to merge 4 commits into
Conversation
Build Artifacts
Smoke test screenshot |
ad64305 to
19c1224
Compare
|
The PR body now carries an explicit acceptance-criteria table: what each criterion is covered by in the automated suites, and what is left for a human on a device. None of the five is ticked, and the reason is the same for all of them. I retried the emulator before writing that off, on both AVDs and every GPU mode. One thing worth knowing before reading the test tree: No code changed in this pass: 32 Android JVM tests and 15 Chaquopy Python tests green, spotless clean. @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
1f43037 to
0437f02
Compare
|
No reviewer feedback has landed yet — no reviews and no inline comments on the PR — so nothing here responds to a reviewer. One change in this pass, from a gate that read It is Robolectric configuration, and it has to exist: 32 Android JVM tests, 15 Chaquopy Python tests, Acceptance criteria are unchanged and still all five unverified: they are on-device outcomes, nothing is attached over @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
rtibbles
left a comment
There was a problem hiding this comment.
A couple of concerns, no manual testing as yet.
| private static final String LOOPBACK_IP = "127.0.0.1"; | ||
|
|
||
| /** Reversed from {@code kolibri:core:initialize_app}; built in Python by app_initialize_url. */ | ||
| private static final String INITIALIZE_PATH = "/api/device/initialize/"; |
There was a problem hiding this comment.
Why is this hard coded here? As noted by your comment this is built by a function, so it shouldn't be hard coded.
There was a problem hiding this comment.
Fixed in a971c27. The activity now hands WebViewLocation the URL it is about to load (noteInitializeUrl), and save recognises the endpoint by that path — so the path comes from app_initialize_url across the bridge rather than being reproduced in Java. Also more correct than the prefix match was: it matches the real path including this device's app key, not any URL that happens to start /api/device/initialize/.
The ordering makes it safe: loadIfOriginChanged is the only place a URL is loaded, so the path is recorded before any navigation the WebView could report back.
There was a problem hiding this comment.
No further change — this was fixed in the reworked commit and is still in place at 73fb07b: WebViewActivity passes the URL it is about to load to WebViewLocation.noteInitializeUrl, so the endpoint path comes from app_initialize_url over the bridge, not from Java.
| @@ -0,0 +1,172 @@ | |||
| package org.learningequality.Kolibri; | |||
There was a problem hiding this comment.
I feel like the tests are excessive/duplicative here. Integration tests as much as possible are preferred, rather than individual unit tests covering mocked behavior.
There was a problem hiding this comment.
Agreed — rewritten in cd9e173, seven tests down to four, each a run through a lifecycle rather than a poke at one callback.
The four are: a page surviving the stop and a restart on its own port (the issue's scenario, opening with the cold-start load that used to have its own test); a page whose server never went away being thawed on return (the onStart gate, which exists because LiveData does not re-deliver an unchanged value); a restart that could not rebind reloading at the new origin; and a page that died against the stopped server being reloaded rather than left as the WebView's error page. onStop_freezesTheWebView and the two standalone thaw tests are gone — they were beats inside those journeys.
Two things I could not make fully integration-shaped, so flagging rather than hiding:
loadIfOriginChangedis still called directly. The observer reaches it viaget_initialize_urlover Chaquopy, which has no JVM implementation —Python.getInstance()throwsUnsatisfiedLinkErroroff-thread. The tests call it with the URL Python would have returned; the class javadoc says so.WebResourceRequest/WebResourceErrorare still Mockito mocks in the failed-load path. They are framework interfaces with no constructible stub in Robolectric, and onlyisForMainFrame()is read.
Neither stubs any of our own behaviour. The end-to-end coverage this cannot reach is the on-device verification in the PR description, which still needs a human on Android 13+.
There was a problem hiding this comment.
Still in place after the history squash — the folded tests are now in 6235e2a.
There was a problem hiding this comment.
No further change — the rewrite is still in place at 73fb07b: four tests, each walking a lifecycle rather than poking one callback. This push adds no Android tests.
cd9e173 to
d7cd77e
Compare
rtibbles
left a comment
There was a problem hiding this comment.
Code wise, this looks much cleaner, and accords with the changes.
Tighten up the commit history to a few commits.
Please write very concise reviewer guidance for manual QA in the PR body - not a long winded mea culpa about why you couldn't do it - the guidance should be suitable for QA reviewers, not engineers.
d7cd77e to
6235e2a
Compare
|
Both done. History is three commits now, one per move: record the port and route, rebind it on restart, freeze the WebView and gate the reload. Same tree as before — The acceptance-criteria section in the PR body is gone, replaced by a four-step Manual QA run written for a QA reviewer: import a CSV from Google Drive, check the filename and 28 Android JVM tests and 15 Python tests green, spotless clean. @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
6235e2a to
d89702c
Compare
|
@rtibblesbot manually tested on 2 physical Android devices and the issue is still not fixed - I'm still getting the Logs (including the relevant logcat logs): logs.zip I confirm the second part of the reviewer guidance that after force stopping the app and reopening it it lands on the page I was last on. |
|
The logs pin it down — thanks. The Android side works through all three idle stops in Port 56493 was rebound every time and the page was never reloaded. The frontend is what wedges. At 13:38:33 one request failed with a network error, and Fixed in 73fb07b: visible retries after 5 s, hidden keeps 600 s, and the 502/504/511 branch at 60 s becomes reachable again. Re-testing: the snackbar may still flash on return from the picker, but it should clear within ~5 s and show "Successfully reconnected!". If it sticks, please grab the logs again — a request is still failing on resume that I cannot account for from these. @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
npm Package VersionsWarning The following packages have changed files but no version bump:
If these changes affect published code, consider bumping the version. |
|
I went back through Both idle stops came back clean. The server was stopped at 13:36:36 and 13:37:56, rebound port 56493 both times, and the page was left in place. There is not a single console error in the log between 13:35:32 and 13:38:33 — so returning from the two long picker sessions produced no failed request at all. The freeze is doing its job. The failure came from a short round trip, not the restart. At 13:38:30 the picker opened again and you were back two seconds later — no idle stop, and the server had been up since 13:38:26: So one request failed on resume against a server that was already running, and the inverted On the re-test, the moment to watch is when the file lands, not when you come back from a long background. If the message appears there and clears within about five seconds with "Successfully reconnected!", the wedge is gone and only that one request is left to chase. One ask if it does need chasing: @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
The path was saved from onPageFinished, which only ever sees the hash present at document load, and the origin was thrown away entirely. Give that knowledge one home: WebViewLocation holds the loopback port the page was loaded on, the path with its query and fragment, and the same-origin comparison a restart needs. The port is a process static rather than a preference. A remembered port must not outlive the process — a cold start still has to elect an ephemeral one — and a static is per-process for free.
AndroidKolibriProcessBus called super().__init__() with no arguments, so BaseKolibriProcessBus's port=0 default elected a fresh ephemeral port on every start. The service now hands start_server the port the page was loaded on, and the bus binds it when it is still free. The decision is made by binding rather than by probing for a listener: port_is_available_on_host only detects listeners, so a port the kernel has handed to another app's outbound connection reads as free and cheroot then dies on EADDRINUSE with nothing to fall back to. Binding also answers at once instead of burning the probe's five seconds of retries.
A backgrounded page keeps polling, and one failed request is enough to wedge it: any error with no response arrives as status 0, which trips the heartbeat's disconnect handling, shows the snackbar and cancels every subsequent request for ten minutes. The WebView is now paused from onStop and thawed only once the server is serving again, so the page issues nothing while the server is away. loadInitializeUrl then ran on every serverReady, making any restart a full reload. It now loads only when there is no live page or the origin actually moved; a restart that rebound the same port leaves the page as it was. A main-frame load error clears the live page, so a document that died against the stopped server does not keep the user on an error page. Robolectric needs the merged manifest to drive the activity, which makes it honour android:name=".App" — and App.onCreate starts Chaquopy, which has no native library on the JVM. robolectric.properties pins a stock Application, restoring the instance the existing tests already got.
monitorDisconnect read pageVisible the opposite way round from its own comment: a page the user was looking at took MAX_RECONNECT_TIME, so one failed request left "Will try to reconnect in 10:00" on screen with every subsequent request cancelled by the client interceptor for ten minutes. The 502/504/511 overload branch was unreachable for the same reason. Freezing the WebView stops the page issuing requests across the idle stop, but it cannot make that window zero — QA on a physical device still hit one failed request on return, and this is what turned it into a wedged page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
73fb07b to
23e4c86
Compare
Summary
Returning to Kolibri from a system file picker showed the disconnection snackbar with the picked file gone and
Continueunable to proceed: Android stops the server service after about a minute of idle, and it came back on a fresh ephemeral port, reloading the WebView and taking the page state with it. The WebView is now frozen while the activity is backgrounded, and the restarting server rebinds the port the page was loaded on, so a restart on that port leaves the open page exactly as it was. When that port can no longer be bound, the server elects an ephemeral one and the WebView reloads at the new origin, landing on the same path and hash route.The freeze narrows the window in which the page can hit a stopped server, but it cannot close it, and QA on hardware showed one failed request was still enough to wedge the page for ten minutes:
monitorDisconnectinpackages/kolibri/heartbeat.jsreadpageVisiblethe opposite way round from its own comment, giving a page you are looking at the backoff meant for a hidden one. That is the one-character frontend change in this PR.References
Fixes #15094.
First seen: #15050 (comment)
Manual QA
Needs a physical device on Android 13 or later — the idle stop this fixes does not fire reliably on an emulator. Put a users CSV on Google Drive on the device first.
@pcenov ran this on two devices and hit the disconnected message at step 2; step 4 passed. The heartbeat fix above landed after that run, so steps 1–3 need re-testing. Their logs put the failure at the moment the file comes back from the picker, not at the return from the background — so step 2 is the one to watch. The message may still flash there, but it should clear within about five seconds and say "Successfully reconnected!"; if it stays, please attach the logs again, including
kolibri.txtgrabbed after the run.Known gap, worth a look while you are there: an HTML5 or EPUB resource left open in step 3 loads from a second port that this change does not restore, so it may need a refresh after the app has been away. Flagged as a follow-up rather than fixed here.
Reviewer guidance
platforms/android/app/src/main/java/org/learningequality/Kolibri/WebViewActivity.java:452— the same-origin gate trustsmainFrameLoadFailed; check no path leaves a dead document counting as live and so skips the reload the user needs.platforms/android/app/src/main/python/main.py:59—_resolve_server_portbinds to decide, rather than usingport_is_available_on_host: that probes for a listener, so a port the kernel has since handed to another app's outbound connection reads as free and cheroot then dies onEADDRINUSEwith nothing to fall back to. Binding also answers at once instead of burning the probe's five seconds of retries.platforms/android/app/src/main/python/main.py:108—ZipContentServerPluginbinds its own ephemeral port, which this change does not restore, so a page left open on an HTML5 or EPUB item keepszipcontentiframe URLs on the old port. Follow-up, or does it belong here?platforms/android/app/src/test/resources/robolectric.propertiesis Robolectric configuration rather than a test, and lives there because that is the only classpath location Robolectric reads it from.includeAndroidResourcesmakes Robolectric honourandroid:name=".App", andApp.onCreatecallsPython.start(...), which throwsUnsatisfiedLinkErroron the JVM; pinning a stockApplicationrestores the instance the existing tests already got. Removing the file fails 23 of the 28 JVM tests, which is how I confirmed it is load-bearing. Robolectric has no@Config-free alternative — theapplicationoverride exists only as a properties file or as an annotation repeated on every test class.Deviations from the issue spec
WebViewLocation.lastPort). The issue says to persist it alongsidePREF_LAST_PATH; a persisted port would outlive the process and be tried on a cold start, which the issue's own out-of-scope note rules out.doUpdateVisitedHistoryfires on each same-document navigation, so the saved fragment is already current when the activity stops — noonStopcapture needed, and none is added. If that callback turns out not to fire on a real device,onStopis where the capture would go.onStartand the server-ready observer.onStartthaws only when the server is already up; a restart is thawed by the observer instead, becauseloadUrlon a paused WebView is unreliable and LiveData does not re-deliver an unchanged value on returning to STARTED.AI usage
Used Claude Code to implement a pre-approved plan test-first, including the Robolectric and pytest coverage, and to trace the disconnect back through the heartbeat's response interceptor. Verified with 28 Android JVM unit tests, 15 Chaquopy Python tests, the
heartbeatJest suite, spotless and prek — but not on a device, as above.@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
🟡 Waiting for feedback
Last updated: 2026-07-31 12:31 UTC