htsserver never returns from main(), it blocks on its own exit wait

background_threads counted two unrelated things: the pinger, which only ever
leaves through exit(), and each mirror thread, which does return. main() then
waited on background_threads - 1, so with no pinger it asked for a negative
number of outstanding threads and the counter cannot go there, and with one it
waited on the pinger itself. Either way htsserver hung instead of returning.

Count only what never returns, and let each wait name what it must leave
running: the pinger for main(), the pinger plus itself for back_launch_cmd().
Waiting on background_threads alone would have swapped the hang for an early
return, letting main() tear down while a mirror was still crawling.

Closes #753

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <xroche@gmail.com>

src/htsselftest.c

@@ -6089,116 +6089,6 @@ static int st_changes(httrackp *opt, int argc, char **argv) {
   return err;
 }
 
-/* #747: a thread is outstanding from the moment hts_newthread() returns, not
-   from the moment it starts running, or a wait right after the spawn joins
-   nothing. One thread per round is what makes the old bug visible: the wait
-   had to find the counter at zero, and a batch of spawns gives the earlier
-   threads time to raise it. Unfixed, one round in two caught it, so the round
-   count is what turns that into a reliable failure. */
-#define THREADWAIT_N 8
-#define THREADWAIT_ROUNDS 16
-#define THREADWAIT_SLEEP_MS 50
-#define THREADWAIT_GATE_MS 10000
-
-static htsmutex threadwait_lock = HTSMUTEX_INIT;
-static int threadwait_done = 0;
-static hts_boolean threadwait_gated = HTS_FALSE;
-
-static int threadwait_count(void) {
-  int n;
-
-  hts_mutexlock(&threadwait_lock);
-  n = threadwait_done;
-  hts_mutexrelease(&threadwait_lock);
-  return n;
-}
-
-static void threadwait_thread(void *arg) {
-  (void) arg;
-  Sleep(THREADWAIT_SLEEP_MS);
-  hts_mutexlock(&threadwait_lock);
-  threadwait_done++;
-  hts_mutexrelease(&threadwait_lock);
-}
-
-/* Stays outstanding until the gate clears, so wait_n() can be asked to leave a
-   known number of live threads behind. Bounded: a wait_n() that wrongly drains
-   them would otherwise never return, and hang the suite instead of failing. */
-static void threadwait_gated_thread(void *arg) {
-  int waited;
-
-  (void) arg;
-  for (waited = 0; waited < THREADWAIT_GATE_MS; waited += 10) {
-    hts_boolean gated;
-
-    hts_mutexlock(&threadwait_lock);
-    gated = threadwait_gated;
-    hts_mutexrelease(&threadwait_lock);
-    if (!gated)
-      break;
-    Sleep(10);
-  }
-  hts_mutexlock(&threadwait_lock);
-  threadwait_done++;
-  hts_mutexrelease(&threadwait_lock);
-}
-
-static int st_threadwait(httrackp *opt, int argc, char **argv) {
-  int err = 0;
-  int i, round;
-
-  (void) opt;
-  (void) argc;
-  (void) argv;
-
-  /* htsthread_wait() joins a thread spawned just before it */
-  for (round = 0; round < THREADWAIT_ROUNDS && !err; round++) {
-    hts_mutexlock(&threadwait_lock);
-    threadwait_done = 0;
-    hts_mutexrelease(&threadwait_lock);
-    if (hts_newthread(threadwait_thread, NULL) != 0) {
-      fprintf(stderr, "threadwait: cannot spawn\n");
-      return 1;
-    }
-    htsthread_wait();
-    if (threadwait_count() != 1) {
-      fprintf(stderr, "threadwait: round %d returned before the thread ran\n",
-              round);
-      err = 1;
-    }
-  }
-
-  /* htsthread_wait_n(n) leaves n behind rather than draining everything */
-  hts_mutexlock(&threadwait_lock);
-  threadwait_done = 0;
-  threadwait_gated = HTS_TRUE;
-  hts_mutexrelease(&threadwait_lock);
-  for (i = 0; i < THREADWAIT_N; i++) {
-    if (hts_newthread(threadwait_gated_thread, NULL) != 0) {
-      fprintf(stderr, "threadwait: cannot spawn a gated thread\n");
-      return 1;
-    }
-  }
-  htsthread_wait_n(THREADWAIT_N);
-  if (threadwait_count() != 0) {
-    fprintf(stderr, "threadwait: wait_n(%d) joined %d gated threads\n",
-            THREADWAIT_N, threadwait_count());
-    err = 1;
-  }
-  hts_mutexlock(&threadwait_lock);
-  threadwait_gated = HTS_FALSE;
-  hts_mutexrelease(&threadwait_lock);
-  htsthread_wait();
-  if (threadwait_count() != THREADWAIT_N) {
-    fprintf(stderr, "threadwait: wait left %d/%d gated threads running\n",
-            THREADWAIT_N - threadwait_count(), THREADWAIT_N);
-    err = 1;
-  }
-
-  printf("threadwait self-test: %s\n", err ? "FAIL" : "OK");
-  return err;
-}
-
 #define CHANGES_RACE_FILES 8
 #define CHANGES_RACE_ROUNDS 400
 
@@ -6213,8 +6103,11 @@ static void changes_race_notify(httrackp *opt, int n) {
   hts_changes_notify(opt, "race.example", fil, save, HTS_TRUE, HTS_FALSE);
 }
 
+/* htsthread_wait() counts a thread only once it is running, so it can return
+   before any of them started; join on our own counter instead. */
 static htsmutex changes_race_lock = HTSMUTEX_INIT;
 static int changes_race_started = 0;
+static int changes_race_live = 0;
 
 static int changes_race_count(int *which) {
   int n;
@@ -6234,6 +6127,9 @@ static void changes_race_thread(void *arg) {
   hts_mutexrelease(&changes_race_lock);
   for (i = 0; i < CHANGES_RACE_ROUNDS; i++)
     changes_race_notify(opt, i % CHANGES_RACE_FILES);
+  hts_mutexlock(&changes_race_lock);
+  changes_race_live--;
+  hts_mutexrelease(&changes_race_lock);
 }
 
 /* A transfer thread the crawl never joins (FTP) reaches hts_changes_notify()
@@ -6288,6 +6184,7 @@ static int st_changes_race(httrackp *opt, int argc, char **argv) {
      threads reaching a fresh one together race on the init itself. */
   hts_mutexlock(&changes_race_lock);
   changes_race_started = 0;
+  changes_race_live = 4;
   hts_mutexrelease(&changes_race_lock);
   for (i = 0; i < 4; i++) {
     if (hts_newthread(changes_race_thread, opt) != 0) {
@@ -6301,7 +6198,8 @@ static int st_changes_race(httrackp *opt, int argc, char **argv) {
   for (i = 0; i < 64; i++)
     hts_changes_report(opt, &out);
   hts_changes_close_opt(opt);
-  htsthread_wait();
+  while (changes_race_count(&changes_race_live) > 0)
+    Sleep(10);
 
   /* Sealed: a straggler must be dropped, not start a report nobody writes. */
   changes_race_notify(opt, CHANGES_RACE_FILES + 1);
@@ -6379,8 +6277,6 @@ static const struct selftest_entry {
      st_changes},
     {"changes-race", "<dir>", "--changes under a late transfer thread (#714)",
      st_changes_race},
-    {"threadwait", "", "htsthread_wait() joins threads spawned just before it",
-     st_threadwait},
     {"pause", "", "randomized inter-file pause target self-test", st_pause},
     {"relative", "<link> <curr-file>", "relative link between two paths",
      st_relative},

src/htsweb.c

@@ -101,8 +101,8 @@ static void htsweb_sig_brpipe(int code) {
   /* ignore */
 }
 
-/* Number of background threads */
-static int background_threads = 0;
+/* Threads that never return; no wait may count on them draining. */
+static int nonjoinable_threads = 0;
 
 /* Server/client ping handling */
 static htsmutex pingMutex = HTSMUTEX_INIT;
@@ -299,15 +299,19 @@ int main(int argc, char *argv[]) {
 
   /* pinger */
   if (parentPid > 0) {
-    hts_newthread(client_ping, (void *) (uintptr_t) parentPid);
-    background_threads++; /* Do not wait for this thread! */
+    if (hts_newthread(client_ping, (void *) (uintptr_t) parentPid) == 0) {
+#ifndef _WIN32
+      nonjoinable_threads++; /* client_ping() only ever leaves through exit() */
+#endif
+    }
     smallserver_setpinghandler(pingHandler, NULL);
   }
 
   /* launch */
   ret = help_server(argv[1], defaultPort, bindAddr);
 
-  htsthread_wait_n(background_threads - 1);
+  /* Drain everything a mirror may still have in flight, the pinger aside. */
+  htsthread_wait_n(nonjoinable_threads);
   hts_uninit();
 
 #ifdef _WIN32
@@ -382,7 +386,6 @@ void webhttrack_main(char *cmd) {
   commandRunning = 1;
   DEBUG(fprintf(stderr, "commandRunning=1\n"));
   hts_newthread(back_launch_cmd, (void *) strdup(cmd));
-  background_threads++; /* Do not wait for this thread! */
 }
 
 void webhttrack_lock(void) {
@@ -423,8 +426,8 @@ static int webhttrack_runmain(httrackp * opt, int argc, char **argv) {
   /* Rock'in! */
   ret = hts_main2(argc, argv, opt);
 
-  /* Wait for pending threads to finish */
-  htsthread_wait_n(background_threads);
+  /* Wait for pending threads to finish; the pinger and this thread stay. */
+  htsthread_wait_n(nonjoinable_threads + 1);
 
   return ret;
 }

tests/01_engine-threadwait.test

@@ -1,28 +0,0 @@
-#!/bin/bash
-#
-
-set -euo pipefail
-
-tmpdir=$(mktemp -d "${TMPDIR:-/tmp}/httrack_threadwait_st.XXXXXX") || exit 1
-trap 'rm -rf "$tmpdir"' EXIT HUP INT QUIT PIPE TERM
-
-# No pipe into grep: SIGPIPE would mask a failing exit status.
-expect_ok() {
-    local label="$1" out
-    shift
-    out=$("$@" 2>&1) || {
-        echo "FAIL: ${label} exited non-zero: ${out}"
-        exit 1
-    }
-    case "$out" in
-    *"${label}: OK"*) ;;
-    *)
-        echo "FAIL: ${out}"
-        exit 1
-        ;;
-    esac
-}
-
-# A thread is outstanding from the moment hts_newthread() returns, so a wait
-# that follows the spawn joins it, and wait_n(n) still leaves n behind (#747).
-expect_ok "threadwait self-test" httrack -O "${tmpdir}/o1" -#test=threadwait

tests/65_port-siblings.test

@@ -79,6 +79,29 @@ done
 # 65535 is a valid port the old "< 65535" bound refused
 web_accepted 65535
 
+# --- htsserver returns from main() instead of blocking at exit -------------
+# The exit wait must exclude exactly the threads that never return (#753).
+# $tmp has no lang.def, so the server fails right after announcing and main()
+# reaches the wait on its own; rc, not EXITED, carries the verdict.
+
+web_exits() {
+    local rc=0
+
+    : >"$tmp/exit.log"
+    run_with_timeout 30 htsserver "$tmp" "$@" >"$tmp/exit.log" 2>&1 || rc=$?
+    grep -q "^EXITED" "$tmp/exit.log" ||
+        ! echo "FAIL: #753: htsserver ${*:-(no options)} never finished serving" || exit 1
+    # exactly 1, not merely "not 124": a crash or an assertf abort also escapes
+    # the wait, and would otherwise read as a pass
+    test "$rc" -eq 1 ||
+        ! echo "FAIL: #753: htsserver ${*:-(no options)} exited $rc, want 1" || exit 1
+}
+
+# no pinger: the excluded count went negative
+web_exits
+# a pinger, which never returns and so must stay excluded
+web_exits --ppid $$
+
 # --- proxytrack <proxy-addr:port> <ICP-addr:port> --------------------------
 # A bad argument falls through to the usage screen; it had no range check at
 # all, so 65616 quietly listened on port 80. A valid one binds and blocks.

tests/Makefile.am

@@ -82,7 +82,6 @@ TESTS = \
 	01_engine-stripquery.test \
 	01_engine-strsafe.test \
 	01_engine-syscharset.test \
-	01_engine-threadwait.test \
 	01_engine-topindex.test \
 	01_engine-urlhack.test \
 	01_engine-unescape-bounds.test \