timothy
71706f3849
Second review round. Codex returned BLOCKED @ e50a2624 with three High findings;
all three were real and all three are fixed. One of them was severe and was
introduced by my OWN previous "fix" commit.
## HIGH 1 — a FAILING spec run exited 0, silently passing CI
`if ! npx playwright test; then status=$?; ... exit "$status"; fi`
Under `!` negation bash sets `$?` to the LOGICAL NEGATION of the command's
status, so inside the failure branch `$?` reads 0 — the script exited 0 on a
failing run. Verified: `if ! (exit 42); then echo $?; fi` prints 0.
A UI-E2E harness that reports success when its specs fail is worse than no
harness. My five green local runs could never have caught this: the bug lives
only on the failure path. Introduced by the log-tail improvement in e50a2624.
Fixed with `set +e` / read `$?` / `set -e`, then exit that status explicitly.
Verified: a deliberately-failing run (`--grep ZZZ_NOPE`) now exits 1.
## HIGH 2 + 3 — the pre-PID fallback needed lsof and only GUESSED ownership
The mid-boot fallback reaped "whatever LISTENS on $PORT", which was wrong twice:
- it needed `lsof`, which is ABSENT from the CI toolchain image (verified
directly in the published image) — so it silently no-opped precisely where
it was needed;
- it INFERRED ownership from the earlier pre-flight rather than proving it, so
a process that grabbed the port after the pre-flight — or a real instance on
a shared host — could be killed. Reaping someone else's server is worse than
the leak it was meant to fix.
Replaced with an opt-in `ETV_PIDFILE`: e2e-local.sh writes the PID the instant
it forks, BEFORE its readiness wait, which is exactly the window a mid-boot
signal lands in. A pidfile we asked for PROVES ownership, needs no external
tool, and works in CI. The port-based kill is gone; the lsof pre-flight remains
as a friendly local check only.
Verified: the pidfile is populated while still mid-boot (readiness not yet
reached), names the real `dotnet ErsatzTV` process (not a subshell — which also
re-confirms the `exec` fix), and killing that PID alone frees the port.
Also dropped the `seq` dependency inside the trap (shell arithmetic instead),
addressing the other reviewer's busybox concern.
## Docs-reviewer finding — my stated reasoning was wrong
I justified amending `testing.e2e-local-fresh-config-dir` rather than superseding
it partly on "renaming the heading trips CI". That's a true statement that does
NOT bear on the choice: a supersession relocates to `archive/` with the heading
INTACT (verified: archive/api.md keeps the #72 heading verbatim). Corrected to
the actual reasons — the Rule never reversed, and the key is cited from
docs/handoffs/chicorytv-issue-queue.md plus two docs/superpowers/ files, which a
supersession would aim at an archived, stale-labelled record.
## Gotcha found by accident, now documented
A flawed test of mine booted two e2e-local.sh instances concurrently and the
first mysteriously failed to become ready. Cause: every run `rm -rf`s and
re-copies the SAME build-output wwwroot, so a second run yanks the static files
out from under a still-starting first instance. Documented in both the script
header and docs/e2e-local.md, because the symptom (readiness timeout, or /app
404ing) looks nothing like a shared-directory race. CI is unaffected — its curl
and UI-E2E steps are sequential.
## Budget: filed, not shaved
This PR pushes the active decisions corpus 7 lines past its 5600-line soft
budget (main was under). I trimmed my records repeatedly and each rewrite
recovered ~1 line, because the content is load-bearing; continuing would have
meant deleting useful rationale from a new convention record to hit an arbitrary
cap. The validator's own remedy is "schedule a consolidation", so that is filed
as #595 rather than paid for by starving the record. Non-blocking warning.
Also filed #594 for the pre-existing `ci-image-pin` any-hex-length weakness.
## Verification
- failing run exits 1 (was 0); passing run still 3/3 green
- pidfile written mid-boot, names the real dotnet proc, reap frees the port
- SIGTERM mid-run: exit 143, no orphan listener or process
- curl harness unaffected: 45/45 PASS; ETV_PIDFILE unset => unchanged behaviour
- decisions validator OK; zero orphaned processes after the full gate
Refs #445 #533 #594 #595
182 lines
9.8 KiB
Bash
Executable File
182 lines
9.8 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
# scripts/e2e-local.sh — launch a local ErsatzTV instance for live E2E verification.
|
|
#
|
|
# See docs/e2e-local.md for the full recipe and the "why" behind each step. This script assumes
|
|
# `dotnet build ErsatzTV.sln` and (if the SPA changed) `cd web && npm run build` have ALREADY been
|
|
# run — it only does the wwwroot copy + launch + ready-wait, since rebuilding on every invocation
|
|
# is slow and this is meant to be re-run often during a debugging session.
|
|
#
|
|
# Usage:
|
|
# scripts/e2e-local.sh [CONFIG_DIR]
|
|
#
|
|
# DO NOT run two instances of this script concurrently, even on different ports. Every run
|
|
# `rm -rf`s and re-copies the SAME build-output wwwroot (below), so a second run yanks the static
|
|
# files out from under a server that is still starting — the symptom is a readiness timeout or a
|
|
# 404-ing /app on the first instance, which looks nothing like a shared-directory race. Run them
|
|
# sequentially (the CI job's curl and UI-E2E steps do exactly that).
|
|
#
|
|
# CONFIG_DIR defaults to a fresh `mktemp -d` if omitted. Prefer a fresh dir per run: leftover
|
|
# channels/schedules/DB rows bleed state between runs and corrupt assertions. Reuse no longer
|
|
# *hangs* the readiness probe (ersatztv#533 — see READY_REGEX below), but the state-bleed reason to
|
|
# avoid it stands.
|
|
#
|
|
# On success, prints:
|
|
# PID=<pid>
|
|
# PORT=<port>
|
|
# CONFIG_DIR=<dir>
|
|
# LOG=<log file path>
|
|
# and exits 0, leaving the server RUNNING in the background.
|
|
#
|
|
# THE CALLER OWNS THE PRINTED PID. Stop the server with `kill "$PID"` using the PID printed above,
|
|
# and free the port before starting another run.
|
|
#
|
|
# NEVER `pkill -f "dotnet ErsatzTV.dll"` (ersatztv#586, `testing.e2e-cleanup-scope-by-pid`). This
|
|
# machine is shared by parallel sessions running this same binary, and a pattern kill reaps all of
|
|
# them — silently truncating another run's output into plausible-but-wrong data rather than failing
|
|
# loudly. Choosing a different port does NOT make a pattern kill safe: it matches on the command
|
|
# line, not the port. Kill the PID you started; if some other process holds the port, report it and
|
|
# move to `ETV_UI_PORT=<other>` (the pre-flight below prints the offending PID for you, and also
|
|
# moves the streaming listener — see the ETV_STREAMING_PORT note below, which is why that actually
|
|
# works here but not when you launch the DLL by hand).
|
|
#
|
|
# This script deliberately does NOT trap-and-kill on exit, unlike scripts/e2e-ui.sh: its contract is
|
|
# to hand a running instance back to its caller, so an EXIT trap would kill the server the moment the
|
|
# launcher returned. e2e-ui.sh is the lifecycle OWNER and traps; this is the launcher and does not.
|
|
|
|
set -euo pipefail
|
|
|
|
REPO_ROOT="$(git rev-parse --show-toplevel)"
|
|
# ETV_BUILD_CONFIG selects which build output to launch (Debug for local dev; the CI functional-E2E
|
|
# job builds Release). Must match the `dotnet build --configuration` you ran beforehand.
|
|
BUILD_CONFIG="${ETV_BUILD_CONFIG:-Debug}"
|
|
BUILD_DIR="$REPO_ROOT/ErsatzTV/bin/$BUILD_CONFIG/net10.0"
|
|
PORT="${ETV_UI_PORT:-8409}"
|
|
# The app binds TWO listeners: Program.cs does `ListenAnyIP(UiPort)` and, when they differ,
|
|
# `ListenAnyIP(StreamingPort)` — and SystemEnvironment.cs defaults StreamingPort to 8409
|
|
# INDEPENDENTLY of ETV_UI_PORT. So `ETV_UI_PORT=8420` alone still binds 8409 and dies at startup if
|
|
# another session holds it, which is the exact dead end that produced the #586 pattern-kill. Default
|
|
# the streaming port to whatever port this run was given so "move to a free port" actually works; an
|
|
# explicit ETV_STREAMING_PORT still wins. CI sets ETV_UI_PORT=8409, so this is a no-op there.
|
|
export ETV_STREAMING_PORT="${ETV_STREAMING_PORT:-$PORT}"
|
|
|
|
# Reject a non-numeric port rather than let it fail silently in the WORST possible way: `lsof -ti :abc`
|
|
# fails a service-name lookup so the pre-flight below skips it, and SystemEnvironment.cs's int.TryParse
|
|
# then falls back to 8409 — so a typo'd port makes the app bind the very port the pre-flight just
|
|
# certified as irrelevant, and the run dies against the foreign holder with exactly the confusing
|
|
# framing this script exists to prevent.
|
|
for port_var in ETV_UI_PORT:"$PORT" ETV_STREAMING_PORT:"$ETV_STREAMING_PORT"; do
|
|
case "${port_var#*:}" in
|
|
''|*[!0-9]*)
|
|
echo "error: ${port_var%%:*} must be a number, got '${port_var#*:}'. A non-numeric port is" >&2
|
|
echo " silently ignored by the app, which then falls back to binding 8409." >&2
|
|
exit 1
|
|
;;
|
|
esac
|
|
done
|
|
# Readiness marker. RebuildSearchIndexHandler logs one of TWO mutually-exclusive lines immediately
|
|
# before it calls SystemStartup.SearchIndexIsReady(), and which one depends on the config dir:
|
|
# fresh config -> "Done migrating search index in {Duration}" (the index actually migrated)
|
|
# reused config -> "Search index is already version {Version}" (nothing to migrate)
|
|
# Matching only the first made this probe hang forever on a REUSED config dir and then kill a
|
|
# perfectly healthy server (ersatztv#533). The handler's if/else is exhaustive, so this alternation
|
|
# covers every path to readiness — do not narrow it back to one line.
|
|
READY_REGEX="Done migrating search index|Search index is already version"
|
|
TIMEOUT_SECS=120
|
|
|
|
# Pre-flight (before mktemp, so a refused run leaks no config dir): name the process holding the
|
|
# port rather than letting this fail at startup with `process N exited before becoming ready` and a
|
|
# log tail — a framing that reads like a broken build instead of "something else is already
|
|
# listening" (ersatztv#586). A foreign listener is REPORTED, never reaped: on a shared machine it is
|
|
# very likely another session's harness mid-run. Both bound ports are probed, since a free UI port
|
|
# with a busy streaming port fails just as hard. `command -v` guard: lsof is absent from some
|
|
# containers, and this diagnostic must never itself be the reason a run fails.
|
|
if command -v lsof >/dev/null 2>&1; then
|
|
for probe_port in $(printf '%s\n%s\n' "$PORT" "$ETV_STREAMING_PORT" | sort -u); do
|
|
if lsof -ti :"$probe_port" >/dev/null 2>&1; then
|
|
echo "error: port $probe_port is already in use by PID(s): $(lsof -ti :"$probe_port" | tr '\n' ' ')" >&2
|
|
echo " That process is NOT yours. Do NOT 'pkill -f \"dotnet ErsatzTV.dll\"' — on this shared" >&2
|
|
echo " machine that reaps other sessions' servers mid-run (ersatztv#586)." >&2
|
|
echo " Re-run on a free port instead. Set BOTH, or the app still binds 8409:" >&2
|
|
echo " ETV_UI_PORT=8420 ETV_STREAMING_PORT=8420 $0${1:+ \"$1\"}" >&2
|
|
echo " (this script defaults ETV_STREAMING_PORT to ETV_UI_PORT, so ETV_UI_PORT=8420 alone" >&2
|
|
echo " is enough when you invoke it directly — set both when launching the app by hand.)" >&2
|
|
exit 1
|
|
fi
|
|
done
|
|
fi
|
|
|
|
CONFIG_DIR="${1:-$(mktemp -d)}"
|
|
mkdir -p "$CONFIG_DIR"
|
|
|
|
if [ ! -d "$BUILD_DIR" ]; then
|
|
echo "error: $BUILD_DIR does not exist — run 'dotnet build ErsatzTV.sln${BUILD_CONFIG:+ --configuration $BUILD_CONFIG}' first" >&2
|
|
exit 1
|
|
fi
|
|
|
|
if [ ! -d "$REPO_ROOT/ErsatzTV/wwwroot/app" ]; then
|
|
echo "warning: $REPO_ROOT/ErsatzTV/wwwroot/app does not exist — the SPA hasn't been built" \
|
|
"('cd web && npm run build'). The /app UI will 404 until it exists AND the server is" \
|
|
"(re)started after copying it in." >&2
|
|
fi
|
|
|
|
echo "Copying wwwroot into build output (static middleware resolves its file root at startup;" \
|
|
"a running process will never see files added later)..."
|
|
# rm first: with an existing destination dir, `cp -R src dst` copies INTO it (dst/wwwroot/...),
|
|
# silently leaving a previous run's stale assets in place.
|
|
rm -rf "$BUILD_DIR/wwwroot"
|
|
cp -R "$REPO_ROOT/ErsatzTV/wwwroot" "$BUILD_DIR/wwwroot"
|
|
|
|
LOG_FILE="$(mktemp)"
|
|
echo "Launching dotnet ErsatzTV.dll (log: $LOG_FILE, config: $CONFIG_DIR, port: $PORT)..."
|
|
|
|
# `exec` matters: without it, whether `$!` is the dotnet process or the wrapping SUBSHELL depends on
|
|
# bash's last-command optimization — bash 5.x collapses it to the leaf, but bash 3.2 (stock macOS
|
|
# /bin/bash, which `#!/usr/bin/env bash` picks up without Homebrew bash on PATH) reports the
|
|
# subshell. Callers then kill the subshell, the real server is orphaned holding the port, and every
|
|
# PID-based liveness check reports success. `exec` replaces the subshell with dotnet, so `$!` is the
|
|
# server on every bash.
|
|
(
|
|
cd "$BUILD_DIR"
|
|
# All three exported together, then `exec`: ETV_STREAMING_PORT is #586's (the app binds a
|
|
# second listener that otherwise defaults to 8409 regardless of ETV_UI_PORT); `exec` is #533's
|
|
# (without it `$!` is this SUBSHELL, not dotnet, on bash 3.2 — every PID-based kill then
|
|
# targets the wrong process). Both are required; neither supersedes the other.
|
|
export ETV_CONFIG_FOLDER="$CONFIG_DIR" ETV_UI_PORT="$PORT" ETV_STREAMING_PORT="$ETV_STREAMING_PORT"
|
|
exec dotnet ErsatzTV.dll
|
|
) >"$LOG_FILE" 2>&1 &
|
|
PID=$!
|
|
|
|
# Publish the PID immediately, BEFORE the readiness wait below, for a caller that must be able to
|
|
# reap the server even though it never got to parse this script's stdout — e.g. killed while this
|
|
# script is still waiting for readiness. That window is exactly where a leak would otherwise happen.
|
|
# Opt-in: unset means "behave exactly as before". scripts/e2e-ui.sh sets it before booting.
|
|
if [ -n "${ETV_PIDFILE:-}" ]; then
|
|
printf '%s\n' "$PID" >"$ETV_PIDFILE"
|
|
fi
|
|
|
|
echo "Waiting up to ${TIMEOUT_SECS}s for readiness ('$READY_REGEX')..."
|
|
elapsed=0
|
|
until grep -Eq "$READY_REGEX" "$LOG_FILE" 2>/dev/null; do
|
|
if ! kill -0 "$PID" 2>/dev/null; then
|
|
echo "error: process $PID exited before becoming ready. Log tail:" >&2
|
|
tail -n 40 "$LOG_FILE" >&2
|
|
exit 1
|
|
fi
|
|
|
|
if [ "$elapsed" -ge "$TIMEOUT_SECS" ]; then
|
|
echo "error: timed out after ${TIMEOUT_SECS}s waiting for readiness. Log tail:" >&2
|
|
tail -n 40 "$LOG_FILE" >&2
|
|
kill "$PID" 2>/dev/null || true
|
|
exit 1
|
|
fi
|
|
|
|
sleep 1
|
|
elapsed=$((elapsed + 1))
|
|
done
|
|
|
|
echo "Server ready."
|
|
echo "PID=$PID"
|
|
echo "PORT=$PORT"
|
|
echo "CONFIG_DIR=$CONFIG_DIR"
|
|
echo "LOG=$LOG_FILE"
|