chore(586,594,485): PID-scoped E2E cleanup, ci-image-pin length guard, .gitignore core fix
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 11s
PR Gates / decisions lifecycle (pull_request) Successful in 12s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 21s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 15s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 6m22s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 21m9s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 5m55s

Three independent CI/repo-hygiene fixes swept together; disjoint file sets.

fixes #586 — E2E cleanup is scoped by PID, never a pattern-wide pkill
  - New decision record `testing.e2e-cleanup-scope-by-pid`.
  - docs/e2e-local.md states the constraint where a BRIEF-WRITER sees it (the
    #586 root cause was a delegation gap, not agent error).
  - scripts/e2e-local.sh: reviewed against e2e-ui.sh's trap lifecycle and
    deliberately does NOT adopt it — its contract is to hand a running instance
    back to its caller, so an EXIT trap would kill the server the instant the
    launcher returned (both callers use `OUT="$(e2e-local.sh ...)"`). Recorded.
  - Instead it gains what actually prevents the incident: an lsof pre-flight
    that NAMES a foreign listener's PID rather than letting Kestrel fail its
    bind and surface as "process N exited before becoming ready".
  - Pre-flight probes BOTH bound ports, and ETV_STREAMING_PORT now defaults to
    ETV_UI_PORT. Program.cs binds a second listener whose port defaults to 8409
    independently of ETV_UI_PORT, so `ETV_UI_PORT=8420` alone still bound 8409
    and died against a foreign holder — i.e. the documented escape hatch was a
    dead end that led straight back to the confusion behind the pattern kill.

fixes #594 — ci-image-pin accepts any hex length
  - Length is a separate invariant from correctness: the resolve/staleness
    checks compare resolved shas, so an 8-char pin of the right commit passes
    green while matching NO registry tag, and all five container: jobs then die
    at image-pull with `manifest unknown` (reads like a registry outage).
  - Guard fails at the gate and prints the exact tag to use. Verified against
    doctored pins: 7 green; 6/8/10 red.
  - Uses a literal 7 rather than a derived `--short=7`: in a full clone git may
    widen an ambiguous abbreviation, demanding a pin ci-image.yml can never
    publish. Escape hatch documented inline.
  - Also fixes a pre-existing misdiagnosis: zero pins reported "MORE THAN ONE".
  - docs/ci-cd.md documents the 7-char rule and `git rev-parse --short=7 HEAD`.

fixes #485 — .gitignore `core` silently ignored `*/Core/` files
  - A bare `core` matched any path component named `core`; case-insensitively
    on macOS that swallowed every `*/Core/` SOURCE dir, so new untracked files
    were dropped by `git add -A` while tracked ones stayed fine — a clean local
    build and a CI checkout that fails to compile.
  - Now `/core` + `/core.[0-9]*`, both anchored (an unanchored `core.[0-9]*`
    would re-introduce the same silent-exclusion class this fixes).
  - Verified by diffing the full ignored-file set before/after: identical, and
    the three real Core/ dirs are trackable without -f.

Docs updated in-PR: docs/e2e-local.md, docs/ci-cd.md, docs/decisions/
workflow-process.md (+ regenerated catalog), docs/handoffs/chicorytv-issue-queue.md.

Follow-ups filed: #596 (the same shared-host reap in the Playwright-MCP
recovery record) and the ci-image.yml `--short=7` publisher-side fix, which
cannot ride this PR — editing ci-image.yml re-points ci-image-pin's `expected`
at this commit and reds the gate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-07-25 12:46:13 +02:00
co-authored by Claude Opus 5
parent 0952078c2b
commit c8e79f49f4
8 changed files with 263 additions and 12 deletions
+63 -5
View File
@@ -97,9 +97,51 @@ skip rubric uses (kickoff workflow lore).
provisions the admin and disables the browser setup-claim.) The machine key still works for curl seeding
regardless. Driving this with Playwright: fill the Setup/Login form before asserting any authed screen.
6. **Tear down**: kill the `dotnet ErsatzTV.dll` process and confirm the port is freed
(`lsof -i :8409` should return nothing) before starting another run — a stray process holding
the port will make the next run's health check hang or fail confusingly.
6. **Tear down**: kill the `dotnet ErsatzTV.dll` process **by the PID you captured at launch**, and
confirm the port is freed (`lsof -i :8409` should return nothing) before starting another run — a
stray process holding the port will make the next run's health check hang or fail confusingly.
> ### ⚠️ Kill by PID. Never `pkill -f` a pattern.
>
> This machine is shared by parallel sessions, and **several of them run this same binary at
> once**. `pkill -f "dotnet ErsatzTV.dll"` reaps every one of them, not just yours.
>
> ```bash
> kill "$PID" # ✅ the PID e2e-local.sh printed
> pkill -f "dotnet ErsatzTV.dll" # ❌ NEVER — reaps other sessions' servers
> ```
>
> **Filtering by port does not make a pattern kill safe** — `pkill -f` matches the *command line*,
> not the port, so it hits every instance whatever port each one chose. Nor are the ports actually
> separated: the CI `functional-e2e` step exports `ETV_UI_PORT=8409`, the **same** port local runs
> use. (8410 is the `ersatztv-test` *container* on jazz — a different thing; the ui-E2E step from
> #445 does use it.) Scope by PID, not by pattern and not by port.
>
> **If the port is already busy, that process is not yours — diagnose, don't reap.** Report it and
> move to another port; `scripts/e2e-local.sh` pre-flights this for you and prints the foreign PID:
> ```bash
> lsof -ti :8409 # who is holding it
> ETV_UI_PORT=8420 scripts/e2e-local.sh # your run, out of the way
> ```
>
> **Launching the DLL by hand? Set both ports.** `Program.cs` binds a second listener on
> `ETV_STREAMING_PORT`, which defaults to **8409 regardless of `ETV_UI_PORT`** — so
> `ETV_UI_PORT=8420 dotnet ErsatzTV.dll` still binds 8409 and dies against a foreign holder:
> ```bash
> ETV_UI_PORT=8420 ETV_STREAMING_PORT=8420 dotnet ErsatzTV.dll
> ```
> `scripts/e2e-local.sh` defaults `ETV_STREAMING_PORT` to whatever port you gave it, so through the
> script `ETV_UI_PORT=8420` alone is sufficient.
>
> Why this is a hard rule rather than a preference: killing another session's harness mid-run does
> **not** fail loudly. It truncates that run's output into plausible-looking-but-wrong data — the
> failure class that survives review. A near-miss of exactly this shape is recorded in
> `docs/decisions/workflow-process.md` → `testing.e2e-cleanup-scope-by-pid` (ersatztv#586).
>
> **Writing a brief for a delegated agent? Put this constraint in it.** The #586 incident was a
> delegation gap, not agent error: the brief specified a fresh config dir but said nothing about
> process cleanup, so the agent invented a reasonable-looking pattern kill. An omitted rule is not
> an unenforced rule — it is a rule replaced by whatever plausible default the agent reaches for.
## Playwright MCP screenshots
@@ -123,8 +165,24 @@ scripts/e2e-local.sh [CONFIG_DIR]
- Copies `ErsatzTV/wwwroot` → `ErsatzTV/bin/<config>/net10.0/wwwroot`.
- Launches `dotnet ErsatzTV.dll` in the background with `ETV_CONFIG_FOLDER` set.
- Waits (up to 120s) for the `Done migrating search index` log line.
- Prints the PID and port, then **exits leaving the server running** — the caller is responsible
for killing the PID when done (`kill <PID>`).
- **Pre-flights both bound ports** (`ETV_UI_PORT` and `ETV_STREAMING_PORT`): if something is already
listening, it fails immediately naming the offending PID, rather than letting Kestrel fail its bind
a moment later and surface as `process N exited before becoming ready` plus a log tail — a framing
that reads like a broken build. It **reports, never reaps** — on this shared machine a foreign
listener is most likely another session's harness mid-run. Re-run with `ETV_UI_PORT=<other>`.
- **Defaults `ETV_STREAMING_PORT` to `ETV_UI_PORT`** so that re-run actually works: the app binds a
second listener that otherwise defaults to 8409 no matter what the UI port is. An explicit
`ETV_STREAMING_PORT` still wins, and CI (which uses 8409) is unaffected.
- Prints the PID and port, then **exits leaving the server running** — the caller owns that PID and
is responsible for killing it when done (`kill "$PID"`, never a `pkill -f` pattern — see the
teardown warning in step 6).
- **Does not trap-and-kill on exit, by design** — unlike `scripts/e2e-ui.sh`, which owns its
instance's whole lifecycle and kills the server from an `EXIT INT TERM` trap. The two scripts sit
on opposite sides of that contract on purpose: this one is the *launcher* and hands a running
instance to its caller (an `EXIT` trap here would kill the server the instant it returned, breaking
every caller including `e2e-ui.sh` and the CI `functional-e2e` step); `e2e-ui.sh` is a *lifecycle
owner* and traps. If you write a new harness that boots and then finishes on its own, follow
`e2e-ui.sh`: capture the PID and trap.
- **`ETV_BUILD_CONFIG`** selects which build output to launch (`Debug` default for local dev; the
CI `functional-e2e` job sets `Release`). It must match the `dotnet build --configuration` you ran
first — the script only copies `wwwroot` + launches; it does not build.