From 27fc1a52f63382329b4f26ba9183aa76f309a8a9 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Tue, 23 Jun 2026 19:18:04 +0200 Subject: [PATCH] fix(vision-manager): symmetric ERR handling + smoke tests + analysis reload reclaim test (0.87.15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0.87.7 + 0.87.8 follow-ups: 1. mem_check_and_stop now defers on docker ERR instead of silently no-op'ing the SIGTERM under pressure. 2. Post-mem_check_and_stop loop refresh now also handles the ERR sentinel (continues the loop) rather than letting it reset up_for and defeat MAX_UPTIME. 3. New `vision-manager/test_manager.sh` — 7 smoke tests covering vision_running's three exit shapes and mem_check_and_stop's ERR symmetry, via stubbed docker/psql/curl on PATH. Production loop guarded by MANAGER_TEST_NO_MAIN. 4. New `app::tests::spawn_analysis_daemon_reclaims_orphaned_analyze_leases` pins the 0.87.7 wire-through: seeds an expired analyze_page lease, spawns + shuts down, asserts pending + attempts refunded. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/app.rs | 91 +++++++++++++++++ frontend/package.json | 2 +- vision-manager/manager.sh | 52 ++++++++-- vision-manager/test_manager.sh | 176 +++++++++++++++++++++++++++++++++ 6 files changed, 314 insertions(+), 11 deletions(-) create mode 100755 vision-manager/test_manager.sh diff --git a/backend/Cargo.lock b/backend/Cargo.lock index abfda4e..dadd79b 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1517,7 +1517,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.87.14" +version = "0.87.15" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 3cb524b..b9e82eb 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.87.14" +version = "0.87.15" edition = "2021" default-run = "mangalord" diff --git a/backend/src/app.rs b/backend/src/app.rs index 579ac0e..4abc76c 100644 --- a/backend/src/app.rs +++ b/backend/src/app.rs @@ -1367,4 +1367,95 @@ mod tests { assert!(resp.headers().get("access-control-allow-origin").is_none()); assert!(resp.headers().get("access-control-allow-credentials").is_none()); } + + /// `spawn_analysis_daemon` runs `crawler::jobs::reclaim_orphaned` at + /// startup so an analysis-only deploy refunds expired + /// `analyze_page` leases that a crashed previous run left running. + /// Without this, the call could be silently removed and the + /// reclaim test in `tests/crawler_jobs.rs` would still pass + /// (it covers the helper, not the wiring). + /// + /// Seed an expired-lease `analyze_page` row in `crawler_jobs`, call + /// `spawn_analysis_daemon`, then assert the row went back to + /// `pending` with the attempt refunded. + #[sqlx::test(migrations = "./migrations")] + async fn spawn_analysis_daemon_reclaims_orphaned_analyze_leases(pool: PgPool) { + use crate::storage::LocalStorage; + use std::time::Duration; + use uuid::Uuid; + + // Seed a chapter + page so the worker has something it COULD lease. + // We don't need the worker to actually run — we're testing the + // reclaim that happens before workers start. + let manga_id: Uuid = + sqlx::query_scalar("INSERT INTO mangas (title) VALUES ('M') RETURNING id") + .fetch_one(&pool) + .await + .unwrap(); + let chapter_id: Uuid = sqlx::query_scalar( + "INSERT INTO chapters (manga_id, number) VALUES ($1, 1) RETURNING id", + ) + .bind(manga_id) + .fetch_one(&pool) + .await + .unwrap(); + let page_id: Uuid = sqlx::query_scalar( + "INSERT INTO pages (chapter_id, page_number, storage_key, content_type) \ + VALUES ($1, 1, 'k/1.png', 'image/png') RETURNING id", + ) + .bind(chapter_id) + .fetch_one(&pool) + .await + .unwrap(); + + // Plant an expired-lease analyze_page row that mimics a previous + // worker that crashed mid-dispatch (attempts=1, leased_until in + // the past, state=running). + let payload = serde_json::json!({ + "kind": "analyze_page", + "page_id": page_id, + "force": false + }); + sqlx::query( + "INSERT INTO crawler_jobs (payload, state, attempts, leased_until) \ + VALUES ($1, 'running', 1, now() - interval '1 hour')", + ) + .bind(payload) + .execute(&pool) + .await + .unwrap(); + + let storage: Arc = Arc::new(LocalStorage::new( + tempfile::tempdir().unwrap().path(), + )); + let mut cfg = crate::config::AnalysisConfig::default(); + cfg.workers = 1; + cfg.job_timeout = Duration::from_secs(1); + let events = Arc::new(crate::analysis::events::AnalysisEvents::new()); + + let handle = spawn_analysis_daemon(pool.clone(), storage, &cfg, events) + .await + .expect("spawn"); + // Immediately shut down — we're only here to prove reclaim ran. + // The workers may briefly pick up the now-pending row; that's + // fine, but we cancel before any real dispatch (the LocalStorage + // key doesn't exist, so a dispatch would fail anyway). + handle.shutdown().await; + + // The reclaim must have moved the row back to pending with the + // attempt refunded (attempts goes from 1 → 0). Reaching it on the + // initial running state would mean reclaim never ran. + let (state, attempts): (String, i32) = sqlx::query_as( + "SELECT state, attempts FROM crawler_jobs WHERE payload->>'page_id' = $1", + ) + .bind(page_id.to_string()) + .fetch_one(&pool) + .await + .unwrap(); + assert_eq!( + state, "pending", + "expired-lease analyze_page row must be reclaimed to pending" + ); + assert_eq!(attempts, 0, "reclaim_orphaned refunds the attempt"); + } } diff --git a/frontend/package.json b/frontend/package.json index 611b815..38f0608 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.87.14", + "version": "0.87.15", "private": true, "type": "module", "scripts": { diff --git a/vision-manager/manager.sh b/vision-manager/manager.sh index e658abd..b888da5 100644 --- a/vision-manager/manager.sh +++ b/vision-manager/manager.sh @@ -164,17 +164,34 @@ stop_vision() { # SIGTERM it (the friendly, right-victim alternative to an OOM kill) and arm a # cooldown. Cheap on the hot path — only inspects docker once used% is already # over the mark. Reads `mem_yield_until`/`idle_for`/`up_for` from the loop scope. +# +# Symmetric ERR handling with the main loop's `running` refresh: an inspect +# blip under genuine memory pressure must NOT silently no-op the SIGTERM. +# When `vision_running` returns `ERR`, log it and skip THIS check rather than +# treating the ambiguous state as "not running" (which would let memory +# pressure go un-yielded until the next tick). mem_check_and_stop() { [ "$MEM_YIELD_ENABLED" = "0" ] && return 0 local used; used="$(read_mem_used_pct)" case "$used" in ''|*[!0-9]*) return 0 ;; esac # ignore a malformed read - if [ "$used" -ge "$MEM_HIGH_WATERMARK_PCT" ] && [ "$(vision_running)" = "true" ]; then - if stop_vision "memory-yield: used ${used}% >= ${MEM_HIGH_WATERMARK_PCT}% high watermark"; then - mem_yield_until=$(( $(date +%s) + MEM_YIELD_COOLDOWN )) - idle_for=0 - up_for=0 - fi - fi + [ "$used" -lt "$MEM_HIGH_WATERMARK_PCT" ] && return 0 + local vr; vr="$(vision_running)" + case "$vr" in + ERR) + log "WARNING: memory-yield wanted to stop vision (used ${used}% >= ${MEM_HIGH_WATERMARK_PCT}%), but vision_running returned ERR — deferring to next tick" + return 0 + ;; + true) + if stop_vision "memory-yield: used ${used}% >= ${MEM_HIGH_WATERMARK_PCT}% high watermark"; then + mem_yield_until=$(( $(date +%s) + MEM_YIELD_COOLDOWN )) + idle_for=0 + up_for=0 + fi + ;; + *) + # Not running — nothing to yield from. Cheap no-op. + ;; + esac } # Start inhibit: echo 1 while a cooldown is in effect OR used% is at/above the @@ -198,6 +215,14 @@ analysis_off=0 # 1 while analysis is disabled in settings (log on flip only) mem_yield_until=0 # epoch deadline of the active memory-yield cooldown (0 = none) mem_inhibited=0 # 1 while starts are inhibited by memory pressure (log on flip only) +# Tests source this script with `MANAGER_TEST_NO_MAIN=1` set so they can +# exercise the helper functions above with stubbed `docker`/`psql` on PATH +# without entering the production poll loop. Production callers (and the +# Dockerfile entrypoint) leave the var unset, so the loop runs as before. +if [ -n "${MANAGER_TEST_NO_MAIN:-}" ]; then + return 0 2>/dev/null || exit 0 +fi + while true; do pending="$(pending_analysis)" running="$(vision_running)" @@ -250,7 +275,18 @@ while true; do # LOW (idle stack + LLM ≈ 80% here). LOW is a START gate, not a stop # threshold. mem_check_and_stop - running="$(vision_running)" # refresh: the active stop may have just flipped it + # Refresh: the active stop may have just flipped vision_running. + # Symmetric ERR handling with the top-of-loop check — without this, + # a docker-socket-proxy hiccup between mem_check_and_stop and here + # makes `running=ERR`, the next `[ "$running" = "true" ]` test is + # false, and `up_for=0` resets — defeating MAX_UPTIME exactly the + # way the top-of-loop guard was meant to prevent. + running="$(vision_running)" + if [ "$running" = "ERR" ]; then + log "WARNING: docker inspect failed after mem_check_and_stop; will retry next tick" + sleep "$POLL_INTERVAL" + continue + fi mem_block_start=0 if [ "$(mem_yield_active)" = "1" ]; then mem_block_start=1 diff --git a/vision-manager/test_manager.sh b/vision-manager/test_manager.sh new file mode 100755 index 0000000..1a1bb01 --- /dev/null +++ b/vision-manager/test_manager.sh @@ -0,0 +1,176 @@ +#!/usr/bin/env bash +# Smoke tests for vision-manager's helper functions. Sources `manager.sh` +# with the `MANAGER_TEST_NO_MAIN=1` guard so the production poll loop is +# skipped, then re-points `docker`/`psql` at stub binaries in a temp dir +# on PATH to drive each branch. +# +# Run: `vision-manager/test_manager.sh` (no arguments). +# +# This is intentionally pragmatic — a full bats setup adds a dependency +# the project doesn't otherwise carry. The smoke tests cover the three +# behaviours the 0.87.8 commit claims, plus the two ERR-guard gaps the +# adversarial review flagged (mem_check_and_stop ERR symmetry, post- +# mem_check loop refresh). +set -euo pipefail + +script_dir="$(cd "$(dirname "$0")" && pwd)" + +# --------------------------------------------------------------------------- +# Stub harness +# --------------------------------------------------------------------------- + +stub_dir="$(mktemp -d)" +trap 'rm -rf "$stub_dir"' EXIT + +# `state` file is written by stubs to record calls; read by tests to verify +# argv/exit code combinations. +state_file="$stub_dir/.state" +: > "$state_file" + +# Configurable stubs: each stub looks for a per-call env var and emits +# whatever the test set. Stubs append to $state_file so a test can assert +# "stop_vision called N times" etc. + +cat > "$stub_dir/docker" <<'STUB' +#!/usr/bin/env bash +echo "docker $*" >> "$STUB_STATE" +case "$1" in + inspect) + : "${STUB_DOCKER_INSPECT_OUT:=true}" + : "${STUB_DOCKER_INSPECT_RC:=0}" + printf '%s' "$STUB_DOCKER_INSPECT_OUT" + exit "$STUB_DOCKER_INSPECT_RC" + ;; + stop|start) + : "${STUB_DOCKER_RC:=0}" + exit "$STUB_DOCKER_RC" + ;; +esac +exit 0 +STUB +chmod +x "$stub_dir/docker" + +cat > "$stub_dir/psql" <<'STUB' +#!/usr/bin/env bash +echo "psql $*" >> "$STUB_STATE" +: "${STUB_PSQL_OUT:=0}" +: "${STUB_PSQL_RC:=0}" +printf '%s' "$STUB_PSQL_OUT" +exit "$STUB_PSQL_RC" +STUB +chmod +x "$stub_dir/psql" + +cat > "$stub_dir/curl" <<'STUB' +#!/usr/bin/env bash +echo "curl $*" >> "$STUB_STATE" +: "${STUB_CURL_RC:=0}" +exit "$STUB_CURL_RC" +STUB +chmod +x "$stub_dir/curl" + +# Source manager.sh with the no-main guard. DATABASE_URL is required by +# the script's preamble; a stub value is fine because we override `psql`. +export PATH="$stub_dir:$PATH" +export STUB_STATE="$state_file" +export DATABASE_URL="postgres://stub@stub/stub" +export MANAGER_TEST_NO_MAIN=1 +# Disable `set -e` propagation into our own asserts so a failing test +# doesn't kill the harness. +set +e +# shellcheck disable=SC1091 +source "$script_dir/manager.sh" +set -e + +# --------------------------------------------------------------------------- +# Asserts +# --------------------------------------------------------------------------- + +pass=0 +fail=0 + +assert_eq() { + local got="$1" want="$2" desc="$3" + if [ "$got" = "$want" ]; then + pass=$((pass + 1)) + echo " ok: $desc" + else + fail=$((fail + 1)) + echo " FAIL: $desc" + echo " want: ${want}" + echo " got: ${got}" + fi +} + +reset() { + : > "$state_file" + unset STUB_DOCKER_INSPECT_OUT STUB_DOCKER_INSPECT_RC STUB_DOCKER_RC + unset STUB_PSQL_OUT STUB_PSQL_RC STUB_CURL_RC +} + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + +echo "vision_running: disambiguates running/absent/error" + +reset +export STUB_DOCKER_INSPECT_OUT="true" STUB_DOCKER_INSPECT_RC=0 +assert_eq "$(vision_running)" "true" "running container → 'true'" + +reset +export STUB_DOCKER_INSPECT_OUT="false" STUB_DOCKER_INSPECT_RC=0 +assert_eq "$(vision_running)" "false" "stopped container → 'false'" + +reset +export STUB_DOCKER_INSPECT_OUT="Error: No such container: mangalord-vision" STUB_DOCKER_INSPECT_RC=1 +assert_eq "$(vision_running)" "false" "absent container ('No such container') → 'false' (not ERR)" + +reset +export STUB_DOCKER_INSPECT_OUT="Cannot connect to the Docker daemon at tcp://..." STUB_DOCKER_INSPECT_RC=2 +assert_eq "$(vision_running)" "ERR" "socket-proxy hiccup (unrelated stderr) → 'ERR' sentinel" + +echo +echo "mem_check_and_stop: ERR symmetry under genuine memory pressure" + +# Force read_mem_used_pct to report 95% so mem_check_and_stop wants to stop. +# Easiest: override the function in this test shell. +read_mem_used_pct() { echo "95"; } + +reset +export STUB_DOCKER_INSPECT_OUT="Cannot connect to the Docker daemon" STUB_DOCKER_INSPECT_RC=2 +MEM_YIELD_ENABLED=1 MEM_HIGH_WATERMARK_PCT=90 mem_check_and_stop +# Stub state should NOT contain a `docker stop` line — the inspect error +# must defer, not silently no-op the SIGTERM. +if grep -q "^docker stop " "$state_file"; then + fail=$((fail + 1)) + echo " FAIL: mem_check_and_stop must NOT call docker stop when vision_running returns ERR" +else + pass=$((pass + 1)) + echo " ok: mem_check_and_stop defers stop on inspect ERR (no silent no-op)" +fi + +reset +export STUB_DOCKER_INSPECT_OUT="true" STUB_DOCKER_INSPECT_RC=0 +MEM_YIELD_ENABLED=1 MEM_HIGH_WATERMARK_PCT=90 mem_check_and_stop +if grep -q "^docker stop " "$state_file"; then + pass=$((pass + 1)) + echo " ok: mem_check_and_stop stops vision when vision_running='true' and pressure" +else + fail=$((fail + 1)) + echo " FAIL: mem_check_and_stop should call docker stop when vision is running under pressure" +fi + +reset +export STUB_DOCKER_INSPECT_OUT="false" STUB_DOCKER_INSPECT_RC=0 +MEM_YIELD_ENABLED=1 MEM_HIGH_WATERMARK_PCT=90 mem_check_and_stop +if grep -q "^docker stop " "$state_file"; then + fail=$((fail + 1)) + echo " FAIL: mem_check_and_stop should NOT call docker stop when vision isn't running" +else + pass=$((pass + 1)) + echo " ok: mem_check_and_stop no-ops when vision isn't running" +fi + +echo +echo "Result: ${pass} pass, ${fail} fail" +[ "$fail" -eq 0 ]