Merge pull request 'test(api): let a shutdown's closed connections leave pg_stat_activity' (#100) from fix/flaky-shutdown-connection-check into main

Reviewed-on: #100
Reviewed-by: Zayin_Admin <[email protected]>

CLAUDE.md

@@ -108,6 +108,13 @@ your own memory of a prior session** — treat stale assumptions as a bug to cat
 - Never run a database migration automatically (on import, on startup, in a test that isn't
   explicitly a migration test, or as a side effect of an unrelated command). Migrations are always
   an explicit, operator-invoked action.
+- Report what you find. A defect, flaky test, documentation/ADR conflict, or CI problem outside the
+  task at hand gets an issue in the repository it belongs to, as soon as you find it. Search for an
+  existing issue first; something fixed in the same PR goes in that PR's description instead. Be
+  descriptive: what you saw, the evidence (run and job links, commit, file and line), how to
+  reproduce it, the impact, and options, and never a secret. Label it `found-by-claude` (an agent
+  that cannot apply labels ends the body with a line saying so), leave it unassigned, and list what
+  you filed when you report back.
 
 ## Canonical verification commands
 

apps/api/tests/api-http.integration.ts

@@ -208,6 +208,29 @@ async function activeConnectionCount(databaseName: string): Promise<number> {
   }
 }
 
+/** How long a shutdown's closed connections may take to leave `pg_stat_activity`. Far below pg's
+ * 10 s idle timeout, so a connection that is genuinely still open is still caught. */
+const SHUTDOWN_SETTLE_MS = 3_000;
+
+/**
+ * The connections left on `databaseName` once a shutdown has had a moment to land.
+ *
+ * PostgreSQL ends a backend asynchronously: `pool.end()` resolves once the client side has closed,
+ * and the server can still list that backend for a moment afterwards. On a loaded CI runner that
+ * moment was long enough for an immediate count to see one. So this waits, briefly, for the count to
+ * reach zero; a leaked pooled client would stay idle and listed far past the deadline.
+ */
+async function connectionsLeftAfterShutdown(databaseName: string): Promise<number> {
+  const deadline = Date.now() + SHUTDOWN_SETTLE_MS;
+  for (;;) {
+    const count = await activeConnectionCount(databaseName);
+    if (count === 0 || Date.now() >= deadline) {
+      return count;
+    }
+    await new Promise((resolve) => setTimeout(resolve, 50));
+  }
+}
+
 async function createDisposableDatabase(label: string): Promise<{ name: string; url: string }> {
   const name = makeDatabaseName(label);
   await createDatabase(name);
@@ -1349,7 +1372,7 @@ describe.sequential("live API — signup, sign-in, principal, sign-out", () => {
     await running.shutdown();
 
     await expect(fetch(origin, { signal: AbortSignal.timeout(1_000) })).rejects.toBeTruthy();
-    const remainingConnections = await activeConnectionCount(database.name);
+    const remainingConnections = await connectionsLeftAfterShutdown(database.name);
     expect(remainingConnections).toBe(0);
 
     await dropDatabase(database.name);
@@ -2230,7 +2253,7 @@ describe.sequential("live API — platform owner and delegated grant management"
 
   it("shuts down cleanly, closing the one shared pool", async () => {
     await running.shutdown();
-    const remainingConnections = await activeConnectionCount(database.name);
+    const remainingConnections = await connectionsLeftAfterShutdown(database.name);
     expect(remainingConnections).toBe(0);
     await dropDatabase(database.name);
   });
@@ -2837,7 +2860,7 @@ describe.sequential("live API — installed module permission end-to-end", () =>
 
   it("shuts down cleanly, then restarting with the module absent rejects new grants for its former permission, returns NOT_FOUND for its former route, while the historical grant remains listable", async () => {
     await running.shutdown();
-    const remainingConnections = await activeConnectionCount(database.name);
+    const remainingConnections = await connectionsLeftAfterShutdown(database.name);
     expect(remainingConnections).toBe(0);
     // Exactly one stop, matching the exactly-one register/start already asserted above.
     expect(recorder.events.filter((event) => event === "calendar:stop")).toHaveLength(1);

apps/api/tests/module-database.integration.ts

@@ -195,6 +195,29 @@ async function activeConnectionCount(databaseName: string): Promise<number> {
   }
 }
 
+/** How long a shutdown's closed connections may take to leave `pg_stat_activity`. Far below pg's
+ * 10 s idle timeout, so a connection that is genuinely still open is still caught. */
+const SHUTDOWN_SETTLE_MS = 3_000;
+
+/**
+ * The connections left on `databaseName` once a shutdown has had a moment to land.
+ *
+ * PostgreSQL ends a backend asynchronously: `pool.end()` resolves once the client side has closed,
+ * and the server can still list that backend for a moment afterwards. On a loaded CI runner that
+ * moment was long enough for an immediate count to see one. So this waits, briefly, for the count to
+ * reach zero; a leaked pooled client would stay idle and listed far past the deadline.
+ */
+async function connectionsLeftAfterShutdown(databaseName: string): Promise<number> {
+  const deadline = Date.now() + SHUTDOWN_SETTLE_MS;
+  for (;;) {
+    const count = await activeConnectionCount(databaseName);
+    if (count === 0 || Date.now() >= deadline) {
+      return count;
+    }
+    await new Promise((resolve) => setTimeout(resolve, 50));
+  }
+}
+
 async function createDisposableDatabase(label: string): Promise<{ name: string; url: string }> {
   const name = makeDatabaseName(label);
   await createDatabase(name);
@@ -760,7 +783,7 @@ describe.sequential("live API — module database access through ModuleRuntimeSe
 
     await running.shutdown();
 
-    const remainingConnections = await activeConnectionCount(database.name);
+    const remainingConnections = await connectionsLeftAfterShutdown(database.name);
     expect(remainingConnections).toBe(0);
 
     const eventsAfterShutdown = readRecorder();

scripts/ci-sweep.sh

@@ -5,6 +5,11 @@
 #   bash scripts/ci-sweep.sh --stale [--max-age-seconds N]   any CI stack older than N (default 3 h)
 #   bash scripts/ci-sweep.sh --job                           this job's own stacks, at any age
 #
+# `--stale --prefix P` narrows the sweep to stacks whose names begin with P, which must itself begin
+# with the CI prefix: it can only ever narrow a sweep, never widen one. scripts/test-ci-sweep.sh
+# confines its short thresholds with it, because the daemon is shared and another job's live stack
+# may be on it right now.
+#
 # Every CI script already removes its own stack in an EXIT trap, but a trap never runs when the runner
 # kills the job container: a cancelled or superseded job (the server honours `concurrency`), or
 # the runner's own time limit. So each workflow job that starts stacks ends with `--job` in an
@@ -31,9 +36,10 @@ source "$repository_root/scripts/lib/ci-project.sh"
 compose_label="com.docker.compose.project"
 mode=""
 max_age_seconds="${EKKLESIA_CI_SWEEP_MAX_AGE_SECONDS:-10800}"
+narrow_prefix=""
 
 usage() {
-  echo "usage: ci-sweep.sh --stale [--max-age-seconds N] | --job" >&2
+  echo "usage: ci-sweep.sh --stale [--max-age-seconds N] [--prefix P] | --job" >&2
   exit 2
 }
 
@@ -48,11 +54,17 @@ while (($# > 0)); do
     max_age_seconds="$2"
     shift
     ;;
+  --prefix)
+    [[ "${2:-}" == "$EKKLESIA_CI_PREFIX"?* ]] || usage
+    narrow_prefix="$2"
+    shift
+    ;;
   *) usage ;;
   esac
   shift
 done
 [[ -n "$mode" ]] || usage
+[[ -z "$narrow_prefix" || "$mode" == stale ]] || usage
 
 if [[ "$mode" == job ]]; then
   [[ -n "${GITHUB_RUN_ID:-}" ]] || {
@@ -62,7 +74,7 @@ if [[ "$mode" == job ]]; then
   prefix="$(ci_job_prefix)"
   cutoff=""
 else
-  prefix="$EKKLESIA_CI_PREFIX"
+  prefix="${narrow_prefix:-$EKKLESIA_CI_PREFIX}"
   cutoff="$(($(date +%s) - max_age_seconds))"
 fi
 

scripts/test-ci-sweep.sh

@@ -4,8 +4,10 @@
 #
 #   bash scripts/test-ci-sweep.sh
 #
-# Every resource it creates is named and labeled for this run alone, under fake run identifiers, so
-# it can never touch a real stack; and it removes all of them on exit, whatever happens.
+# Every resource it creates is named and labeled for this run alone, under fake run identifiers, and
+# every sweep it runs is confined to those identifiers, so it can never touch a real stack: the
+# runner's daemon is shared, and another job's live stack may be on it right now. It removes all of
+# its own resources on exit, whatever happens.
 
 set -Eeuo pipefail
 
@@ -91,10 +93,14 @@ source "$repository_root/scripts/lib/ci-project.sh"
 # Fake run identifiers: nothing here can match a real run's stacks.
 export GITHUB_RUN_ID="sweeptest${nonce}"
 export GITHUB_RUN_ATTEMPT="1"
+# Every stack this test sweeps begins with this; nothing it sweeps reaches past it.
+own_run="$(ci_name_safe "ekklesia-ci-${GITHUB_RUN_ID}-")"
 mine="$(EKKLESIA_CI_JOB_KEY=alpha ci_project_name mine)"
 sibling="$(EKKLESIA_CI_JOB_KEY=beta ci_project_name sibling)"
 foreign="ekklesia-sweep-control-${nonce}"
-for project in "$mine" "$sibling" "$foreign"; do
+# Stands in for another run's live stack on the shared daemon — a module's integration job, say.
+bystander="$(GITHUB_RUN_ID="bystander${nonce}" EKKLESIA_CI_JOB_KEY=gamma ci_project_name live)"
+for project in "$mine" "$sibling" "$foreign" "$bystander"; do
   make_stack "$project"
 done
 # A job container connected to this job's stack network, exactly as compose-job-network.sh does.
@@ -115,12 +121,19 @@ check '! env -u GITHUB_RUN_ID bash scripts/ci-sweep.sh --job >/dev/null 2>&1' \
   "--job without GITHUB_RUN_ID fails"
 
 echo "--- --stale removes only CI stacks older than its threshold"
-bash scripts/ci-sweep.sh --stale
+bash scripts/ci-sweep.sh --stale --prefix "$own_run"
 check 'project_exists "$sibling"' "a young CI stack survives the default threshold"
-bash scripts/ci-sweep.sh --stale --max-age-seconds 0
+bash scripts/ci-sweep.sh --stale --max-age-seconds 0 --prefix "$own_run"
 check '! project_exists "$sibling"' "a CI stack older than the threshold is removed"
 check '! staging_exists "$sibling"' "its staging directory too"
 check 'project_exists "$foreign"' "a stack outside the CI prefix is never removed, at any age"
+check 'project_exists "$bystander"' \
+  "another run's stack is never removed by a narrowed sweep, even past the threshold"
+echo "--- --prefix only ever narrows the sweep"
+check '! bash scripts/ci-sweep.sh --stale --prefix "ekklesia-" >/dev/null 2>&1' \
+  "a prefix outside the CI prefix is refused"
+check '! bash scripts/ci-sweep.sh --job --prefix "$own_run" >/dev/null 2>&1' \
+  "--prefix with --job is refused"
 
 if ((failures > 0)); then
   echo "test-ci-sweep: ${failures} check(s) failed." >&2

tests/ci-scripts.test.ts

@@ -128,6 +128,34 @@ describe("CI scripts on a shared Docker daemon", () => {
     expect(workflow.jobs["deploy-smoke"]?.env?.EKKLESIA_CI_JOB_KEY).toContain("matrix.topology");
   });
 
+  it("never sweep another run's live stack early, not even from the sweep's own self-test", async () => {
+    // The workflow's stale sweeps keep the default threshold, which is far above any job's run time.
+    const workflow = await source(".github/workflows/ci.yml");
+    const workflowSweeps = workflow
+      .split("\n")
+      .filter((line) => line.includes("ci-sweep.sh --stale"));
+    expect(workflowSweeps.length).toBeGreaterThan(0);
+    for (const line of workflowSweeps) {
+      expect(line).not.toContain("--max-age-seconds");
+    }
+
+    // The self-test does use short thresholds, so every stale sweep it runs is confined to its own
+    // fake run. Unconfined, `--max-age-seconds 0` removed every CI stack on the shared daemon —
+    // including a module repository's integration stack mid-run.
+    const selfTest = await source("scripts/test-ci-sweep.sh");
+    expect(selfTest).toMatch(/^own_run="\$\(ci_name_safe "ekklesia-ci-\$\{GITHUB_RUN_ID\}-"\)"$/mu);
+    expect(selfTest.indexOf('export GITHUB_RUN_ID="sweeptest')).toBeLessThan(
+      selfTest.indexOf("own_run="),
+    );
+    const selfTestSweeps = selfTest
+      .split("\n")
+      .filter((line) => /^\s*bash scripts\/ci-sweep\.sh --stale/u.test(line));
+    expect(selfTestSweeps.length).toBeGreaterThan(0);
+    for (const line of selfTestSweeps) {
+      expect(line).toContain('--prefix "$own_run"');
+    }
+  });
+
   it("choose the deploy smoke's edge subnet from those not in use, and choose again on a race", async () => {
     const deploy = await source("scripts/smoke-deploy.sh");
     expect(deploy).not.toMatch(/^octet=\$\(\(RANDOM/mu);