ADFA-4954 fix(kolibri): re-attach went silent, phase latched, margin truncated

Review fixes on the two commits below. Two of these break scenarios the ZIM
client never meets, which is why copying its shape carried them in unnoticed.

attach() went completely silent on a reused instance. `finished` short-circuits
both pollOnce() and fail(), so re-attaching after a seed had completed produced no
polling, no error and no callback — on the one method whose entire purpose is
recovering after the service restarts. Run state is now cleared on entry, before
the id check, and an empty id fails with its real cause instead of degrading into
ten 404s and "lost contact with the content service".

The indexing flag latched. RestContentClient can latch it because it handles one
ZIM; a Kolibri job walks several channels and re-enters processing once per
channel, so onIndexing() only ever fired for the first. Cleared on entering
downloading rather than only set on leaving it.

SeedPlan.requiredBytes divided before multiplying, truncating the remainder before
it was scaled. Real impact is ~108 bytes on a multi-gigabyte decision, so this is
small — but the test could not have caught it either way: it used 1000, which is
divisible by 100 and therefore identical under both orderings. Added a case with
1099, which distinguishes them (1100 truncated vs 1208 scaled), and one at
catalog scale to pin the absence of overflow now that the multiply comes first.

Also seed() with an empty plan now routes through done() so the double-callback
guard is armed rather than left open.

Not fixed, and noted rather than silently carried: `listener` and `pollErrors` are
non-volatile and touched from IO threads. The race is real but benign, and both
are inherited verbatim from RestContentClient — correcting one client and not the
other would be worse than the race. Worth its own pass over both.

Verified: all seven files parse; the arithmetic checked numerically against the
values the new tests assert. NOT COMPILED — no javac in the sandbox.

Author: luisguzman-adfa

controller/app/src/main/java/org/iiab/controller/kolibri/data/KolibriRestClient.java

@@ -131,10 +131,12 @@ private static String firstBlocker(JSONObject readyResponse) {
      */
     public void seed(@NonNull SeedPlan plan, @NonNull Listener l) {
         this.listener = l;
+        resetRunState();
         if (plan.isEmpty()) {
             // Nothing queued is a success, not an error: the caller drained an
-            // empty wishlist.
-            main.post(l::onDone);
+            // empty wishlist. Routed through done() so the double-callback guard is
+            // armed rather than left open.
+            done();
             return;
         }
         AppExecutors.get().io().execute(() -> {
@@ -154,13 +156,37 @@ public void seed(@NonNull SeedPlan plan, @NonNull Listener l) {
         });
     }
 
-    /** Re-attaches to a job started earlier, e.g. after the service was restarted. */
+    /**
+     * Re-attaches to a job started earlier, e.g. after the service was restarted.
+     *
+     * <p>Resets the run state first, and that ordering matters: {@code finished}
+     * short-circuits both {@link #pollOnce()} and {@link #fail(String)}, so on an
+     * instance that already completed a seed this method would otherwise go
+     * completely silent — no polling, no error, no callback.
+     */
     public void attach(@NonNull String existingJobId, @NonNull Listener l) {
         this.listener = l;
-        this.jobId = existingJobId;
+        resetRunState();
+
+        String id = existingJobId.trim();
+        if (id.isEmpty()) {
+            // Fail with the real cause. Left to the poller, an empty id would 404
+            // ten times and surface as "lost contact with the content service",
+            // pointing at the network instead of at the missing id.
+            fail("no job to re-attach to");
+            return;
+        }
+        this.jobId = id;
         main.postDelayed(pollTask, POLL_MS);
     }
 
+    /** Clears the per-run flags so the instance can start or re-attach again. */
+    private void resetRunState() {
+        finished = false;
+        indexing = false;
+        pollErrors = 0;
+    }
+
     /** The id of the in-flight job, or null. Persist it to survive a restart. */
     public String jobId() {
         return jobId;
@@ -208,6 +234,12 @@ private void pollOnce() {
 
             switch (phase) {
                 case "downloading":
+                    // Cleared on the way in, not only set on the way out: a job that
+                    // seeds several channels re-enters processing once per channel,
+                    // and a latching flag would announce the phase for the first
+                    // channel only. The ZIM client can latch because it handles one
+                    // item; this one cannot.
+                    indexing = false;
                     if (percent >= 0) {
                         final int p = percent;
                         final String rate = formatRate(speed);

controller/app/src/main/java/org/iiab/controller/kolibri/domain/SeedPlan.java

@@ -173,7 +173,11 @@ public long requiredBytes() {
      */
     public long requiredBytes(int marginPercent) {
         int margin = Math.max(100, marginPercent);
-        return estimatedBytes() / 100L * margin;
+        // Multiply first: dividing by 100 up front truncates the remainder before it
+        // is scaled, losing up to ~99 bytes per call. No overflow risk — the whole
+        // public catalog is ~775 GB (8.3e11), and 8.3e11 * 150 is 1.2e14 against a
+        // long ceiling of 9.2e18.
+        return estimatedBytes() * margin / 100L;
     }
 
     /**

controller/app/src/test/java/org/iiab/controller/kolibri/domain/SeedPlanTest.java

@@ -122,6 +122,30 @@ public void requiredBytesAddsHeadRoom() {
         assertEquals(1500L, p.requiredBytes(150));
     }
 
+    @Test
+    public void requiredBytesScalesTheRemainderInsteadOfTruncatingIt() {
+        // A size divisible by 100 cannot tell "* margin / 100" from "/ 100 * margin",
+        // so the original test passed while the second, wrong, ordering was in place.
+        // 1099 does distinguish them: truncating first gives 1100, scaling gives 1208.
+        SeedPlan p = SeedPlan.of(
+                Collections.singletonList(ChannelSelection.wholeChannel(STORYBOOK)),
+                sizes(STORYBOOK, 1099L));
+        assertEquals(1208L, p.requiredBytes());
+        assertEquals(1648L, p.requiredBytes(150));
+    }
+
+    @Test
+    public void requiredBytesDoesNotOverflowOnACatalogSizedPlan() {
+        // The whole public catalog is ~775 GB; multiplying before dividing has to stay
+        // inside long. 8.3e11 * 150 is 1.2e14, well under the 9.2e18 ceiling.
+        SeedPlan p = SeedPlan.of(
+                Collections.singletonList(ChannelSelection.wholeChannel(STORYBOOK)),
+                sizes(STORYBOOK, 775L * GB));
+        long required = p.requiredBytes(150);
+        assertTrue("overflowed to a negative", required > 0L);
+        assertEquals(775L * GB / 100L * 150L, required);
+    }
+
     @Test
     public void aMarginBelowOneHundredIsClampedNotHonoured() {
         // Planning to use less space than the content needs is never right.