Every finished crawl marked itself interrupted

app/src/main/java/com/httrack/android/HTTrackActivity.java

@@ -1431,6 +1431,9 @@ protected void runInternal() {
       RandomAccessFile outLock = null;
       FileLock lock = null;
       File profile = null;
+      // Did the engine get as far as running, and if so did it leave anything to resume?
+      boolean engineRan = false;
+      boolean pendingWork = true;
       try {
         // Sanity checks
         if (parent == null) {
@@ -1496,7 +1499,9 @@ protected void runInternal() {
         setProgressLines(new String[] { string_starting_mirror });
 
         // Run engine
+        engineRan = true;
         final int code = engine.main(cargs);
+        pendingWork = leavesPendingWork(interrupted || engine.wasStopped(), code);
 
         // Result
         if (code == 0) {
@@ -1545,6 +1550,16 @@ protected void runInternal() {
           } catch (IOException io) {
           }
         }
+        // Stamp what the run actually was; a project the engine never touched keeps its marker.
+        if (engineRan) {
+          try {
+            setInterruptedProfile(pendingWork);
+          } catch (final IOException io) {
+            Log.w(getClass().getSimpleName(), "could not update the resume marker", io);
+          }
+        }
+        // Before the finished pane, whose own stopMirror() must not read as an interruption.
+        ended = true;
       }
 
       // Ensure we switch to the final pane
@@ -1591,15 +1606,10 @@ public void run() {
      */
     private synchronized void setInterruptedProfile(final boolean interrupted)
         throws IOException {
-      if (parent != null) {
-        if (interrupted) {
-          parent.setInterruptedProfile(true);
-        } else {
-          parent.setInterruptedProfile(false);
-        }
-      } else {
+      if (parent == null) {
         throw new IOException("parent has been detached");
       }
+      parent.setInterruptedProfile(interrupted);
     }
 
     /*
@@ -1622,12 +1632,14 @@ public boolean stopMirror(final boolean force) {
       }
       // Stop engine
       final boolean stopSent = engine.stop(force);
-      // If not yet stopped, mark as dirty
-      // ("Continue an interrupted mirror ...")
-      try {
-        setInterruptedProfile(stopSent);
-      } catch (final IOException io) {
-        Log.w(getClass().getSimpleName(), "could not lock file", io);
+      // Only a stop that lands on a live crawl leaves work behind: the finished pane asks for one
+      // too, and the engine answers it long after runInternal recorded the real outcome.
+      if (!ended) {
+        try {
+          setInterruptedProfile(true);
+        } catch (final IOException io) {
+          Log.w(getClass().getSimpleName(), "could not write the resume marker", io);
+        }
       }
       return stopSent;
     }
@@ -1851,19 +1863,74 @@ protected boolean hasCacheFile() {
     return profile != null && profile.exists();
   }
 
+  /**
+   * Does a crawl that ended this way leave the mirror resumable ?
+   *
+   * @param stopped
+   *          Was the crawl cut short, by the user or by a cap the engine enforces itself ?
+   * @param engineCode
+   *          The engine return code; nonzero means it gave up rather than finished.
+   * @return true if the project should reopen offering "Continue an interrupted download"
+   */
+  protected static boolean leavesPendingWork(final boolean stopped,
+      final int engineCode) {
+    // Errors are not a criterion: the engine returns 0 once it has drained its queue, however
+    // many links failed on the way, and there is nothing left to continue.
+    return stopped || engineCode != 0;
+  }
+
+  /**
+   * The marker a stopped or aborted crawl leaves behind.
+   *
+   * @param target
+   *          The project directory
+   * @return The lock file
+   */
+  protected static File getInterruptedLockFile(final File target) {
+    return new File(new File(target, "hts-cache"), "interrupted.lock");
+  }
+
+  /**
+   * Interrupted profile ?
+   *
+   * @param target
+   *          The project directory
+   * @return true if the mirror was interrupted
+   */
+  protected static boolean isInterruptedProfile(final File target) {
+    // The engine's own lock only outlives a crawl it never got to end, such as a killed process.
+    return new File(target, "hts-in_progress.lock").exists()
+        || getInterruptedLockFile(target).exists();
+  }
+
   /**
    * Interrupted profile ?
    * 
    * @return true if the mirror was interrupted
    */
   protected boolean isInterruptedProfile() {
     final File target = getTargetFile();
-    if (target != null) {
-      final File cache = new File(target, "hts-cache");
-      return new File(target, "hts-in_progress.lock").exists()
-          || new File(cache, "interrupted.lock").exists();
+    return target != null && isInterruptedProfile(target);
+  }
+
+  /**
+   * Set the "interrupted" flag.
+   *
+   * @param target
+   *          The project directory
+   * @param interrupted
+   *          Interrupted mirror ?
+   * @throws IOException
+   *           Upon I/O error.
+   */
+  protected static void setInterruptedProfile(final File target,
+      final boolean interrupted) throws IOException {
+    final File lock = getInterruptedLockFile(target);
+    if (interrupted) {
+      final FileWriter wr = new FileWriter(lock);
+      wr.close();
     } else {
-      return false;
+      lock.delete();
     }
   }
 
@@ -1878,14 +1945,10 @@ protected boolean isInterruptedProfile() {
   protected synchronized void setInterruptedProfile(final boolean interrupted)
       throws IOException {
     final File target = getTargetFile();
-    final File cache = new File(target, "hts-cache");
-    final File lock = new File(cache, "interrupted.lock");
-    if (interrupted) {
-      final FileWriter wr = new FileWriter(lock);
-      wr.close();
-    } else {
-      lock.delete();
+    if (target == null) {
+      throw new IOException("no project name defined!");
     }
+    setInterruptedProfile(target, interrupted);
   }
 
   /**

app/src/main/java/com/httrack/android/jni/HTTrackLib.java

@@ -109,6 +109,14 @@ public static int buildTopIndex(final File path, final File templatesPath) {
    */
   public native boolean stop(boolean force);
 
+  /**
+   * Was the last run cut short ? True for a stop() request, and for the size and time caps the
+   * engine enforces itself, which no return code of main() tells apart from a completion.
+   *
+   * @return true if the engine stopped before draining its queue
+   */
+  public native boolean wasStopped();
+
   /**
    * Default constructor.
    */

app/src/main/jni/htslibjni.c

@@ -827,6 +827,29 @@ Java_com_httrack_android_jni_HTTrackLib_stop(JNIEnv* env, jobject object,
   return stopped;
 }
 
+JNICALL jboolean
+Java_com_httrack_android_jni_HTTrackLib_wasStopped(JNIEnv* env, jobject object) {
+  HTTrackLib_context *const context = getNativeOpt(env, object);
+  jboolean stopped = JNI_FALSE;
+
+  if (context == NULL) {
+    throwRuntimeException(env, "null context");
+    return JNI_FALSE;
+  }
+
+  MUTEX_LOCK(context->lock);
+  /* hts_main2() returns 0 for nearly every abort, so ask the engine instead.
+     stop is a cap or a user stop; exit_xh is a fatal disk error, a full link
+     table, an aborting callback, or a rolled-back session. */
+  if (context->opt != NULL) {
+    stopped = context->opt->state.stop != 0
+        || hts_is_exiting(context->opt) != 0 ? JNI_TRUE : JNI_FALSE;
+  }
+  MUTEX_UNLOCK(context->lock);
+
+  return stopped;
+}
+
 static jint HTTrackLib_buildTopIndex(JNIEnv* env, jclass clazz, jstring opath,
                                      jstring otemplates) {
   if (opath != NULL && otemplates != NULL) {
@@ -929,7 +952,7 @@ jint HTTrackLib_main(JNIEnv* env, jobject object, jobjectArray stringArray) {
       }
 
       /* Unreference global option tab */
-      /* Nope - do this at destructor time */
+      /* Freed at destructor time, not here: wasStopped() reads opt after main returns. */
       /*MUTEX_LOCK(context->lock);
       hts_free_opt(context->opt);
       context->opt = NULL;

app/src/main/jni/htslibjni.h

@@ -48,6 +48,9 @@ JNIEXPORT jboolean JNICALL
 Java_com_httrack_android_jni_HTTrackLib_stop(JNIEnv* env, jobject object,
                                              jboolean force);
 
+JNIEXPORT jboolean JNICALL
+Java_com_httrack_android_jni_HTTrackLib_wasStopped(JNIEnv* env, jobject object);
+
 JNIEXPORT jint JNICALL
 Java_com_httrack_android_jni_HTTrackLib_buildTopIndex(JNIEnv* env, jclass clazz,
                                                       jstring opath, jstring otemplates);

app/src/test/java/com/httrack/android/InterruptedLockTest.java

@@ -0,0 +1,151 @@
+package com.httrack.android;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+/** hts-cache/interrupted.lock is what makes a project reopen on "Continue an interrupted
+ *  download", so it has to mean the crawl was cut short and nothing else. */
+public class InterruptedLockTest {
+  @Rule
+  public final TemporaryFolder tmp = new TemporaryFolder();
+
+  private File target;
+
+  @Before
+  public void setUp() throws Exception {
+    target = tmp.newFolder("project");
+    new File(target, "hts-cache").mkdirs();
+  }
+
+  /** The engine returns 0 once its queue is drained, whatever failed on the way. */
+  @Test
+  public void aRunThatReachesTheEndIsNotResumable() {
+    assertFalse("a crawl that ran to the end has nothing to continue",
+        HTTrackActivity.leavesPendingWork(false, 0));
+  }
+
+  @Test
+  public void aCrawlCutShortIsResumable() {
+    // A soft stop lets pending transfers finish, so the engine still returns 0; so does a size or
+    // time cap, which the engine applies by stopping itself. HTTrackLib.wasStopped() sees both.
+    assertTrue(HTTrackActivity.leavesPendingWork(true, 0));
+    assertTrue(HTTrackActivity.leavesPendingWork(true, -1));
+  }
+
+  @Test
+  public void anEngineThatGaveUpIsResumable() {
+    assertTrue(HTTrackActivity.leavesPendingWork(false, -1));
+    assertTrue(HTTrackActivity.leavesPendingWork(false, 1));
+  }
+
+  @Test
+  public void theMarkerRoundTrips() throws Exception {
+    assertFalse(HTTrackActivity.isInterruptedProfile(target));
+    HTTrackActivity.setInterruptedProfile(target, true);
+    assertTrue(HTTrackActivity.isInterruptedProfile(target));
+    HTTrackActivity.setInterruptedProfile(target, false);
+    assertFalse(HTTrackActivity.isInterruptedProfile(target));
+  }
+
+  /** The engine leaves this one behind only when it never got to end. */
+  @Test
+  public void theEnginesOwnLockAlsoMeansInterrupted() throws Exception {
+    assertTrue(new File(target, "hts-in_progress.lock").createNewFile());
+    assertTrue(HTTrackActivity.isInterruptedProfile(target));
+  }
+
+  /** What the finished pane does, end to end, for each way a crawl can end. */
+  private boolean reopensOnContinue(final boolean stoppedByUser, final int engineCode)
+      throws IOException {
+    HTTrackActivity.setInterruptedProfile(target,
+        HTTrackActivity.leavesPendingWork(stoppedByUser, engineCode));
+    return HTTrackActivity.isInterruptedProfile(target);
+  }
+
+  @Test
+  public void everyOutcomeStampsTheProjectItsOwnWay() throws Exception {
+    assertFalse("a completed crawl", reopensOnContinue(false, 0));
+    assertTrue("a stop, asked for or capped", reopensOnContinue(true, 0));
+    assertTrue("an engine that gave up", reopensOnContinue(false, -1));
+    assertFalse("a completed crawl again", reopensOnContinue(false, 0));
+  }
+
+  /** A project stopped last time and finished this time no longer offers to continue: the marker
+   *  clears itself, which is all that ever clears one left by an older build. */
+  @Test
+  public void aCleanRunClearsAStaleMarker() throws Exception {
+    HTTrackActivity.setInterruptedProfile(target, true);
+    assertFalse("a stale marker survived a clean run", reopensOnContinue(false, 0));
+  }
+
+  /** Runner.stopMirror needs a live AsyncTask, so the guard is pinned in the source instead. */
+  private static String stopMirrorBody() throws IOException {
+    final String source = TestSources.javaSource("HTTrackActivity");
+    // The last declaration is the runner's; the first is RunnerFragment's trunk to it.
+    final int from = source.lastIndexOf("public boolean stopMirror(final boolean force) {");
+    assertTrue("Runner.stopMirror is gone", from != -1);
+    final int to = source.indexOf("\n    }\n", from);
+    assertTrue("Runner.stopMirror is not closed where expected", to > from);
+    return source.substring(from, to);
+  }
+
+  @Test
+  public void aStopRequestOnlyEverWritesTheMarker() throws Exception {
+    final Matcher m = Pattern.compile("setInterruptedProfile\\(([^)]*)\\)")
+        .matcher(stopMirrorBody());
+    int calls = 0;
+    while (m.find()) {
+      calls++;
+      assertEquals("stopMirror must not clear the marker, nor pass a verdict it cannot make",
+          "true", m.group(1));
+    }
+    assertEquals("stopMirror no longer touches the marker at all", 1, calls);
+  }
+
+  @Test
+  public void aStopAfterTheCrawlEndedIsIgnored() throws Exception {
+    assertTrue("the finished pane's own stopMirror() would mark every project resumable",
+        stopMirrorBody().contains("if (!ended)"));
+  }
+
+  /** The verdict belongs to the run, so it is written where the run ends. */
+  @Test
+  public void theCrawlStampsItsOwnOutcome() throws Exception {
+    final String source = TestSources.javaSource("HTTrackActivity");
+    final int from = source.indexOf("protected void runInternal()");
+    final int to = source.indexOf("displayFinishedPanel(displayMessage, errorsCount,");
+    assertTrue("runInternal no longer bounded by its displayFinishedPanel call",
+        from != -1 && to > from);
+    final String body = source.substring(from, to);
+    assertTrue("runInternal must weigh the engine's own stop, not just the user's",
+        body.contains("leavesPendingWork(interrupted || engine.wasStopped(), code)"));
+    assertTrue("runInternal must write the verdict before the finished pane opens",
+        body.contains("setInterruptedProfile(pendingWork)"));
+    assertTrue("ended must be set before the finished pane asks for a stop",
+        body.contains("ended = true"));
+  }
+
+  /* The predicate below only ever sees what wasStopped() reports, so the engine's
+     two abort flags are pinned here: stop alone misses a fatal disk error. */
+  @Test
+  public void wasStoppedWeighsBothOfTheEnginesAbortFlags() throws Exception {
+    final String jni = TestSources.jniSource("htslibjni.c");
+    final int at = jni.indexOf("HTTrackLib_wasStopped");
+    assertTrue("wasStopped is gone", at > 0);
+    final String body = jni.substring(at, jni.indexOf("\n}", at));
+    assertTrue("wasStopped must read state.stop", body.contains("state.stop"));
+    assertTrue("wasStopped must also read exit_xh, which stop never sets",
+        body.contains("hts_is_exiting"));
+  }
+}