timothy/ersatztv
1
0
Fork 0
Code Issues 58 Pull Requests 8 Actions Packages Projects Releases 9 Wiki Activity

feat(445,533): headless Playwright UI-E2E flows + fix e2e-local readiness probe #591

Merged
timothy merged 13 commits from feat/445-533-ui-e2e into main 2026-07-25 14:52:33 +02:00
Conversation 4 Commits 13 Files Changed 18
+860 -45
timothy commented 2026-07-25 11:34:36 +02:00
Owner
Copy Link
Copy Source

fixes #445
fixes #533

Sweeps two members of the 🔗 E2E/test-harness bundle. #563 deliberately left for a later session (a second large lift; folding it in risked two half-finished harnesses).

#445 — UI-interactive Playwright (headless) flows

The last deferred #299/#363 follow-up. Scope rule (the durable part): assert only what the curl harness structurally cannot reach.

Contract Why curl can't reach it
Setup card's confirm-password gate Pure React state; makes no request, so there is no HTTP contract
AuthGate states (Setup vs Login vs app) The assertion is about what renders, not a status code
Session cookie on the SPA's own /api XHRs curl proves the cookie works for curl, not that the app sends it
Sign-out via the UserMenu A DOM interaction chain, not a single endpoint

New: web/e2e/boot-gate.spec.ts, web/playwright.config.ts, scripts/e2e-ui.sh (owns the full lifecycle: fresh config dir → boot → specs → always kill). chromium-headless-shell baked into docker/ci/Dockerfile; a UI-E2E step added to the existing advisory functional-e2e job.

That job, not a new one — its npm ci + Release build are already paid, so this adds ~11s instead of duplicating a heavy job. serial, workers: 1, retries: 0 even in CI: a retry would let a flaky flow merge looking green.

#533 — readiness probe hangs on a reused config dir

RebuildSearchIndexHandler logs one of two mutually-exclusive lines immediately before SystemStartup.SearchIndexIsReady() — Done migrating search index (fresh) or Search index is already version (reused). The probe watched only the first, so a reused dir waited out the full 120s and then killed a healthy server. Widened to a grep -Eq alternation; the handler's if/else is exhaustive, so the pair covers every path to readiness.

Negative control: reused dir now ready in 2s, and the old probe string is genuinely absent from that run's log — proof the old code would have hung, i.e. the fix is load-bearing rather than incidentally passing.

Measured, not assumed

  • chromium-headless-shell 267M vs full chromium 656M; +171M compressed pull (0.95→1.11 GB). chromium.launch() resolves to the shell anyway. Tradeoff: a headed run in the image would fail.
  • Chromium runs as root in-container with no --no-sandbox opt-out — I expected to need one; probing the real base said otherwise, so the config carries no workaround.
  • A fresh config is not empty: DbInitializer seeds a channel, so the Channels empty state is unreachable.
  • CI cost: UI-E2E step 11.4s inside a 363s job (~3%), the job landing under its documented 520s warm baseline.

Non-obvious couplings

  • vitest would have swallowed the specs (default **/*.{test,spec}.* glob → jsdom). Excluded e2e/** by spreading configDefaults.exclude, not by narrowing include to src/** — web/scripts/ holds a real vitest test that would have silently stopped running.
  • Playwright pins a browser revision per package version, so the npm pin is EXACT and e2e-ui.sh guards drift by launching a browser — not by path, since executablePath() reports the full-chromium path a shell-only image lacks.

Reconciled with #598 (landed mid-review)

main merged four times during this review. #598 overlapped directly and exposed a latent bug here: the app binds a second, streaming listener defaulting to 8409 regardless of ETV_UI_PORT, so this PR's ETV_UI_PORT=8410 step was correct only by luck. Their fix covers e2e-ui.sh for free; I dropped my inferior single-port pre-flight in favour of theirs (probes both ports, reports rather than reaps per #586).

Review: 4 rounds, 2 independent reviewers

Every defect found lived on a failure or interrupt path — none reachable by a passing run, after five green local runs. Most severe: if ! cmd; then status=$? made a failing spec run exit 0, silently passing CI. Also: trap installed after boot (orphaned servers); bash defers trapped signals during a foreground command; a recycled BOOT_PID could SIGTERM an unrelated process. Three were introduced by my own fix commits. Details and proofs in the review comments.

Verification

  • CI green on the head; UI-E2E confirmed from the job log, not the tick (a green functional-e2e can also mean the step was skipped): pulls the pinned image, boots on 8410, 3 passed, curl harness 45/45
  • Locally: 3/3 specs with 0 leftover instances; failing run exits 1; SIGTERM → exit 143, no orphan; 995 web tests / 105 files; typecheck + lint clean; decisions validator OK
  • Pin guard simulated including #598's new 7-char length check, and the registry confirmed to hold the tag before pinning it

Deferred, filed

#563 (scripted-playout harness), #594 (ci-image-pin accepted any hex length — since fixed on main by #598), #595 (decisions corpus over its non-blocking soft budget). Accepted residual risk: two Low findings needing a PID-only supervisor escalating TERM→KILL inside a millisecond window.

Docs: docs/e2e-local.md (UI-E2E harness section), docs/ci-cd.md (toolchain image + Chromium + UI-E2E step + the rebase-invalidates-the-pin trap, which cost this branch two recovery cycles), docs/testing.md, docs/README.md, docs/decisions.md (new ci.ui-e2e-harness; ci.functional-e2e-harness and testing.e2e-local-fresh-config-dir amended; TOC repaired 69→98 entries with a dangling anchor to an archived record removed).

fixes #445 fixes #533 Sweeps two members of the 🔗 E2E/test-harness bundle. #563 deliberately left for a later session (a second large lift; folding it in risked two half-finished harnesses). ## #445 — UI-interactive Playwright (headless) flows The last deferred #299/#363 follow-up. **Scope rule (the durable part): assert only what the curl harness structurally cannot reach.** | Contract | Why curl can't reach it | |---|---| | Setup card's confirm-password gate | Pure React state; makes **no request**, so there is no HTTP contract | | `AuthGate` states (Setup vs Login vs app) | The assertion is about what *renders*, not a status code | | Session cookie on the SPA's **own** `/api` XHRs | curl proves the cookie works *for curl*, not that the app sends it | | Sign-out via the `UserMenu` | A DOM interaction chain, not a single endpoint | New: `web/e2e/boot-gate.spec.ts`, `web/playwright.config.ts`, `scripts/e2e-ui.sh` (owns the full lifecycle: fresh config dir → boot → specs → always kill). `chromium-headless-shell` baked into `docker/ci/Dockerfile`; a UI-E2E step added to the existing advisory `functional-e2e` job. **That job, not a new one** — its `npm ci` + Release build are already paid, so this adds ~11s instead of duplicating a heavy job. **`serial`, `workers: 1`, `retries: 0`** even in CI: a retry would let a flaky flow merge looking green. ## #533 — readiness probe hangs on a reused config dir `RebuildSearchIndexHandler` logs one of two mutually-exclusive lines immediately before `SystemStartup.SearchIndexIsReady()` — `Done migrating search index` (fresh) or `Search index is already version` (reused). The probe watched only the first, so a reused dir waited out the full 120s and then killed a healthy server. Widened to a `grep -Eq` alternation; the handler's `if`/`else` is **exhaustive**, so the pair covers every path to readiness. **Negative control:** reused dir now ready in **2s**, and the *old* probe string is genuinely **absent** from that run's log — proof the old code would have hung, i.e. the fix is load-bearing rather than incidentally passing. ## Measured, not assumed - `chromium-headless-shell` **267M** vs full `chromium` **656M**; **+171M compressed** pull (0.95→1.11 GB). `chromium.launch()` resolves to the shell anyway. Tradeoff: a *headed* run in the image would fail. - Chromium runs **as root in-container with no `--no-sandbox` opt-out** — I expected to need one; probing the real base said otherwise, so the config carries no workaround. - A fresh config is **not** empty: `DbInitializer` seeds a channel, so the Channels empty state is unreachable. - CI cost: UI-E2E step **11.4s** inside a **363s** job (~3%), the job landing *under* its documented 520s warm baseline. ## Non-obvious couplings - **vitest would have swallowed the specs** (default `**/*.{test,spec}.*` glob → jsdom). Excluded `e2e/**` by spreading `configDefaults.exclude`, *not* by narrowing `include` to `src/**` — `web/scripts/` holds a real vitest test that would have silently stopped running. - **Playwright pins a browser revision per package version**, so the npm pin is EXACT and `e2e-ui.sh` guards drift by **launching** a browser — not by path, since `executablePath()` reports the full-chromium path a shell-only image lacks. ## Reconciled with #598 (landed mid-review) main merged **four times** during this review. #598 overlapped directly and **exposed a latent bug here**: the app binds a **second, streaming listener defaulting to 8409 regardless of `ETV_UI_PORT`**, so this PR's `ETV_UI_PORT=8410` step was correct only by luck. Their fix covers `e2e-ui.sh` for free; I dropped my inferior single-port pre-flight in favour of theirs (probes both ports, **reports rather than reaps** per #586). ## Review: 4 rounds, 2 independent reviewers Every defect found lived on a **failure or interrupt path** — none reachable by a passing run, after five green local runs. Most severe: `if ! cmd; then status=$?` made a **failing spec run exit 0**, silently passing CI. Also: trap installed after boot (orphaned servers); bash **defers** trapped signals during a foreground command; a recycled `BOOT_PID` could SIGTERM an unrelated process. Three were introduced by my own fix commits. Details and proofs in the review comments. ## Verification - CI green on the head; UI-E2E confirmed from the **job log**, not the tick (a green `functional-e2e` can also mean the step was skipped): pulls the pinned image, boots on 8410, `3 passed`, curl harness `45/45` - Locally: 3/3 specs with **0 leftover instances**; failing run exits 1; SIGTERM → exit 143, no orphan; **995 web tests / 105 files**; typecheck + lint clean; decisions validator OK - Pin guard simulated including #598's new 7-char length check, and the registry confirmed to hold the tag *before* pinning it ## Deferred, filed #563 (scripted-playout harness), #594 (`ci-image-pin` accepted any hex length — since **fixed on main** by #598), #595 (decisions corpus over its non-blocking soft budget). Accepted residual risk: two **Low** findings needing a PID-only supervisor escalating `TERM`→`KILL` inside a millisecond window. Docs: `docs/e2e-local.md` (UI-E2E harness section), `docs/ci-cd.md` (toolchain image + Chromium + UI-E2E step + **the rebase-invalidates-the-pin trap**, which cost this branch two recovery cycles), `docs/testing.md`, `docs/README.md`, `docs/decisions.md` (new `ci.ui-e2e-harness`; `ci.functional-e2e-harness` and `testing.e2e-local-fresh-config-dir` amended; TOC repaired 69→98 entries with a dangling anchor to an archived record removed).
timothy marked the pull request as work in progress 2026-07-25 11:35:35 +02:00
timothy commented 2026-07-25 12:35:50 +02:00
Author
Owner
Copy Link
Copy Source

Adversarial review — 4 rounds, 2 independent reviewers

Reviews ran before CI, and the branch was held unpushed between rounds so the runners only ever saw reviewed work.

Round Finding Real?
1 Cleanup trap installed after boot (leak window); signals not re-raised so a cancelled run could exit 0 Yes — both reviewers converged independently
1 $! was the subshell, not dotnet, on bash 3.2 → every PID-based kill targeted the wrong process Yes — measured on both bash versions
1 Stale active decision record still asserting the bug #533 fixes Yes
1 Dangling ci.toolchain-image key (absent from all 161 keys) Yes
2 if ! cmd; then status=$? → a FAILING spec run exited 0, silently passing CI Yes — the worst defect here, and I introduced it in my own round-1 fix
2 Mid-boot fallback needed lsof (absent in CI) and only inferred ownership → could kill an unrelated process Yes
3 Bash defers a trapped signal during a foreground command → TERM→KILL during boot orphans the server Yes — proven in isolation
4 Recycled BOOT_PID could SIGTERM an unrelated process Yes

The pattern worth recording: not one of these was reachable by a passing run. Every defect lived on a failure or interrupt path, and five green local runs exercised neither. Two of them were introduced by an earlier fix commit.

Proofs rather than assertions:

  • if ! (exit 42); then echo $?; fi → prints 0; the shipped pattern exited 0 on failure. Now: failing run exits 1.
  • Trap deferral: foreground command substitution → TERM at t=1s, trap ran at t=6s; background + wait → trap ran at t=1s.
  • $! semantics, two-command subshell: bash 3.2 → subshell; bash 5.3 → leaf; with exec → leaf on both.
  • Pidfile populated mid-boot, names the real dotnet ErsatzTV process, and reaping it frees the port.

Residual risk, accepted not chased

Round 4 returned two Low findings, both requiring a PID-only supervisor that escalates TERM→KILL inside a millisecond-scale window: (a) the launcher can die before publishing its PID, (b) the foreground npx run defers the trap. Reviewer's own assessment: "negligible for normal laptop process-group signals and CI container teardown." A laptop Ctrl-C signals the whole process group (killing the server directly), and a CI container is torn down wholesale. Recorded rather than fixed — chasing microsecond races in a test harness has worse expected value than shipping it.

Deferred, filed rather than hidden

  • #594 — ci-image-pin accepts any hex-length tag, so an 8-char pin passes the guard green but matches no registry tag (pre-existing; not introduced here).
  • #595 — this PR pushes the active decisions corpus 7 lines past its non-blocking 5600-line soft budget. I stopped trimming after each rewrite recovered ~1 line; the validator's own remedy is a corpus-wide consolidation, not starving a new record.

Review-verdict: MERGEABLE @ 45926f30

## Adversarial review — 4 rounds, 2 independent reviewers Reviews ran **before** CI, and the branch was held unpushed between rounds so the runners only ever saw reviewed work. | Round | Finding | Real? | |---|---|---| | 1 | Cleanup trap installed *after* boot (leak window); signals not re-raised so a cancelled run could exit 0 | Yes — both reviewers converged independently | | 1 | `$!` was the **subshell**, not dotnet, on bash 3.2 → every PID-based kill targeted the wrong process | Yes — measured on both bash versions | | 1 | Stale `active` decision record still asserting the bug #533 fixes | Yes | | 1 | Dangling `ci.toolchain-image` key (absent from all 161 keys) | Yes | | 2 | **`if ! cmd; then status=$?` → a FAILING spec run exited 0, silently passing CI** | Yes — the worst defect here, and I introduced it in my own round-1 fix | | 2 | Mid-boot fallback needed `lsof` (absent in CI) and only *inferred* ownership → could kill an unrelated process | Yes | | 3 | Bash **defers** a trapped signal during a foreground command → `TERM`→`KILL` during boot orphans the server | Yes — proven in isolation | | 4 | Recycled `BOOT_PID` could SIGTERM an unrelated process | Yes | **The pattern worth recording: not one of these was reachable by a passing run.** Every defect lived on a failure or interrupt path, and five green local runs exercised neither. Two of them were introduced *by* an earlier fix commit. Proofs rather than assertions: - `if ! (exit 42); then echo $?; fi` → prints `0`; the shipped pattern exited 0 on failure. Now: failing run exits **1**. - Trap deferral: foreground command substitution → TERM at t=1s, trap ran at **t=6s**; background + `wait` → trap ran at **t=1s**. - `$!` semantics, two-command subshell: bash 3.2 → **subshell**; bash 5.3 → leaf; with `exec` → leaf on both. - Pidfile populated mid-boot, names the real `dotnet ErsatzTV` process, and reaping it frees the port. ### Residual risk, accepted not chased Round 4 returned two **Low** findings, both requiring a PID-only supervisor that escalates `TERM`→`KILL` inside a millisecond-scale window: (a) the launcher can die before publishing its PID, (b) the foreground `npx` run defers the trap. Reviewer's own assessment: *"negligible for normal laptop process-group signals and CI container teardown."* A laptop Ctrl-C signals the whole process group (killing the server directly), and a CI container is torn down wholesale. Recorded rather than fixed — chasing microsecond races in a test harness has worse expected value than shipping it. ### Deferred, filed rather than hidden - **#594** — `ci-image-pin` accepts any hex-length tag, so an 8-char pin passes the guard green but matches no registry tag (pre-existing; not introduced here). - **#595** — this PR pushes the active decisions corpus 7 lines past its non-blocking 5600-line soft budget. I stopped trimming after each rewrite recovered ~1 line; the validator's own remedy is a corpus-wide consolidation, not starving a new record. Review-verdict: MERGEABLE @ 45926f30
timothy marked the pull request as ready for review 2026-07-25 12:36:03 +02:00
timothy force-pushed feat/445-533-ui-e2e from 45926f30dc to f3f1602090 2026-07-25 12:41:30 +02:00 Compare
timothy commented 2026-07-25 12:41:55 +02:00
Author
Owner
Copy Link
Copy Source

Rebased onto main — verdict carried forward

main moved 12 commits while this was in review (including two [decisions-edit] records), so this branch was rebased, never merged-into. Head is now f3f16020.

This is a carry-forward of the MERGEABLE @ 45926f30 verdict, not a new independent review. Stating that plainly so it can be judged rather than assumed. What makes it defensible:

Every reviewed file is byte-identical after the rebase. Verified by comparing content hashes at the reviewed sha vs the rebased head, per file:

IDENTICAL  scripts/e2e-ui.sh                    IDENTICAL  docker/ci/Dockerfile
IDENTICAL  scripts/e2e-local.sh                 IDENTICAL  .gitea/workflows/docker-build.yml
IDENTICAL  web/e2e/boot-gate.spec.ts            IDENTICAL  web/vite.config.ts
IDENTICAL  web/playwright.config.ts             IDENTICAL  web/tsconfig.node.json
IDENTICAL  web/package.json                     IDENTICAL  web/package-lock.json
IDENTICAL  .gitignore
-> files differing or unreadable: 0

⚠️ My first run of that check was a false pass — zsh doesn't word-split unquoted variables, so the loop compared one giant path that didn't exist and both git shows returned empty, hashing equal. Re-run under bash with an empty-input-hash guard so the detector can't lie. Flagging it because "the detector said clean" is exactly how this class of check goes wrong.

Only docs moved in the rebase, all of it conflict resolution in the append-only corpus:

  • docs/decisions.md — main's spa.rulebuilder-nesting (#436) and my ci.ui-e2e-harness (#445) were appended at the same spot. Kept both; confirmed no key was lost (96/96 of main's keys still present, 97 total, mine included).
  • TOC and docs/decisions/README.md were regenerated, never hand-merged — the catalog is a generated artifact. TOC re-verified: 98 records, 98 entries, 0 dangling, 0 duplicates.

Re-ran the full gate against the rebased tree (not just the pre-rebase state):

  • UI-E2E 3/3 green; curl harness 45/45
  • web 995 tests / 105 files (main added 12 while this was in review), typecheck + lint clean
  • solution builds; decisions validator OK

One doc correction fell out of it: docs/testing.md said 983 tests, which went stale when main's tests landed. Updated to 995 rather than shipping a number that was wrong on arrival.

Note the corpus budget warning is now 5705 rather than 5607 — main's own two records contributed, which supports #595's framing that this is corpus-wide pressure rather than one PR's debt. Still non-blocking.

Review-verdict: MERGEABLE @ f3f16020

## Rebased onto main — verdict carried forward `main` moved 12 commits while this was in review (including two `[decisions-edit]` records), so this branch was **rebased**, never merged-into. Head is now `f3f16020`. **This is a carry-forward of the `MERGEABLE @ 45926f30` verdict, not a new independent review.** Stating that plainly so it can be judged rather than assumed. What makes it defensible: **Every reviewed file is byte-identical after the rebase.** Verified by comparing content hashes at the reviewed sha vs the rebased head, per file: ``` IDENTICAL scripts/e2e-ui.sh IDENTICAL docker/ci/Dockerfile IDENTICAL scripts/e2e-local.sh IDENTICAL .gitea/workflows/docker-build.yml IDENTICAL web/e2e/boot-gate.spec.ts IDENTICAL web/vite.config.ts IDENTICAL web/playwright.config.ts IDENTICAL web/tsconfig.node.json IDENTICAL web/package.json IDENTICAL web/package-lock.json IDENTICAL .gitignore -> files differing or unreadable: 0 ``` ⚠️ My first run of that check was a **false pass** — zsh doesn't word-split unquoted variables, so the loop compared one giant path that didn't exist and both `git show`s returned empty, hashing equal. Re-run under bash with an empty-input-hash guard so the detector can't lie. Flagging it because "the detector said clean" is exactly how this class of check goes wrong. Only **docs** moved in the rebase, all of it conflict resolution in the append-only corpus: - `docs/decisions.md` — main's `spa.rulebuilder-nesting` (#436) and my `ci.ui-e2e-harness` (#445) were appended at the same spot. Kept **both**; confirmed no key was lost (96/96 of main's keys still present, 97 total, mine included). - TOC and `docs/decisions/README.md` were **regenerated**, never hand-merged — the catalog is a generated artifact. TOC re-verified: 98 records, 98 entries, 0 dangling, 0 duplicates. **Re-ran the full gate against the rebased tree** (not just the pre-rebase state): - UI-E2E **3/3** green; curl harness **45/45** - web **995 tests / 105 files** (main added 12 while this was in review), typecheck + lint clean - solution builds; decisions validator OK One doc correction fell out of it: `docs/testing.md` said 983 tests, which went stale when main's tests landed. Updated to 995 rather than shipping a number that was wrong on arrival. Note the corpus budget warning is now **5705** rather than 5607 — main's own two records contributed, which supports #595's framing that this is corpus-wide pressure rather than one PR's debt. Still non-blocking. Review-verdict: MERGEABLE @ f3f16020
timothy commented 2026-07-25 13:08:25 +02:00
Author
Owner
Copy Link
Copy Source

Post-rebase pin recovery complete — head 7030bd28

ci-image-pin went red after the rebase. Not a code problem: that guard pins against the short sha of the commit which touched docker/ci, and a rebase rewrites it (e9fd26f6 → 9130274c). Three things made it confusing, all now documented in docs/ci-cd.md:

  • the stale pin still resolves to a real commit and its tagged image still exists in the registry, so nothing looks broken;
  • the force-push does not republish — ci-image.yml filters on paths: docker/ci/**, and a rebase leaving the Dockerfile's content unchanged produces no diff for that path;
  • re-dispatching ci-image.yml cannot fix it either, because it tags git rev-parse --short HEAD — the branch HEAD, not the commit that touched docker/ci. Those coincide only when the docker/ci commit is HEAD, which is why the original two-step worked and the post-rebase state didn't.

Recovery = re-run the documented two-step: 1652fc56 really changes docker/ci (it adds the warning comment), so ci-image.yml tagged it → ersatztv-ci:1652fc5; 7030bd28 re-points all five jobs plus the header comment at it.

Verified rather than assumed, both halves:

  • Simulated the guard's own logic: expected = git log -1 --format=%H -- docker/ci .gitea/workflows/ci-image.yml = 1652fc56…, pin 1652fc5 resolves to the same commit → WOULD PASS.
  • Pulled the published :1652fc5 on the runner host: /ms-playwright holds chromium_headless_shell-1234, chromium launches (151.0.7922.34), dotnet 10.0.302 intact. That is the image all five jobs will now consume.

Verdict carry-forward, and exactly what changed since review

Reviewed content is unchanged; the delta is CI plumbing and a comment:

IDENTICAL  scripts/e2e-ui.sh          IDENTICAL  web/vite.config.ts
IDENTICAL  scripts/e2e-local.sh       IDENTICAL  web/tsconfig.node.json
IDENTICAL  web/e2e/boot-gate.spec.ts  IDENTICAL  web/package.json
IDENTICAL  web/playwright.config.ts   IDENTICAL  .gitignore

The only two files that moved, in full:

  • .gitea/workflows/docker-build.yml — the pin string e9fd26f → 1652fc5 (6 occurrences, nothing else)
  • docker/ci/Dockerfile — a 5-line comment warning about this exact trap; no build instruction touched, so the image content is unchanged (cache-reused, republished under the new tag)

No executable line differs from what the reviewer approved.

CI already proved the feature on this branch

Run 1078 (head f3f16020, same reviewed code, using the equivalent image) was green on all six jobs, and I read the job log rather than trusting the tick — the browser genuinely ran on the runner: 3 passed (2.7s) after booting on port 8410. Cost measured at +11.4s inside a 363s job, which came in under its documented 520s warm baseline. Full detail in #445's CI-evidence comment.

CI on 7030bd28 is running now; ci-image-pin should go green with the corrected pin.

Review-verdict: MERGEABLE @ 7030bd28

## Post-rebase pin recovery complete — head `7030bd28` `ci-image-pin` went red after the rebase. **Not a code problem**: that guard pins against the short sha of the commit which touched `docker/ci`, and a rebase rewrites it (`e9fd26f6` → `9130274c`). Three things made it confusing, all now documented in `docs/ci-cd.md`: - the stale pin still *resolves* to a real commit and its tagged image still exists in the registry, so nothing looks broken; - the force-push does **not** republish — `ci-image.yml` filters on `paths: docker/ci/**`, and a rebase leaving the Dockerfile's content unchanged produces no diff for that path; - re-dispatching `ci-image.yml` cannot fix it either, because it tags `git rev-parse --short HEAD` — the branch HEAD, not the commit that touched `docker/ci`. Those coincide only when the `docker/ci` commit *is* HEAD, which is why the original two-step worked and the post-rebase state didn't. Recovery = re-run the documented two-step: `1652fc56` really changes `docker/ci` (it adds the warning comment), so `ci-image.yml` tagged it → `ersatztv-ci:1652fc5`; `7030bd28` re-points all five jobs plus the header comment at it. **Verified rather than assumed, both halves:** - Simulated the guard's own logic: `expected = git log -1 --format=%H -- docker/ci .gitea/workflows/ci-image.yml` = `1652fc56…`, pin `1652fc5` resolves to the same commit → **WOULD PASS**. - Pulled the published `:1652fc5` on the runner host: `/ms-playwright` holds `chromium_headless_shell-1234`, chromium launches (151.0.7922.34), dotnet 10.0.302 intact. That is the image all five jobs will now consume. ### Verdict carry-forward, and exactly what changed since review Reviewed content is **unchanged**; the delta is CI plumbing and a comment: ``` IDENTICAL scripts/e2e-ui.sh IDENTICAL web/vite.config.ts IDENTICAL scripts/e2e-local.sh IDENTICAL web/tsconfig.node.json IDENTICAL web/e2e/boot-gate.spec.ts IDENTICAL web/package.json IDENTICAL web/playwright.config.ts IDENTICAL .gitignore ``` The only two files that moved, in full: - `.gitea/workflows/docker-build.yml` — the pin string `e9fd26f` → `1652fc5` (6 occurrences, nothing else) - `docker/ci/Dockerfile` — a 5-line comment warning about this exact trap; **no build instruction touched**, so the image content is unchanged (cache-reused, republished under the new tag) No executable line differs from what the reviewer approved. ### CI already proved the feature on this branch Run **1078** (head `f3f16020`, same reviewed code, using the equivalent image) was green on all six jobs, and I read the job log rather than trusting the tick — the browser genuinely ran on the runner: `3 passed (2.7s)` after booting on port 8410. Cost measured at **+11.4s inside a 363s job**, which came in *under* its documented 520s warm baseline. Full detail in [#445's CI-evidence comment](https://gitea.tblindustries.be/timothy/ersatztv/issues/445#issuecomment-15991). CI on `7030bd28` is running now; `ci-image-pin` should go green with the corrected pin. Review-verdict: MERGEABLE @ 7030bd28
timothy force-pushed feat/445-533-ui-e2e from 7030bd2870 to 32747a067e 2026-07-25 14:31:09 +02:00 Compare
timothy added 1 commit 2026-07-25 14:35:11 +02:00
ci(445): re-point the toolchain pin after the second rebase (1652fc5 -> 32747a0)
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 16s
Details
PR Gates / Docs update reminder (pull_request) Successful in 16s
Details
PR Gates / decisions lifecycle (pull_request) Successful in 18s
Details
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 7m58s
Details
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 23s
Details
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 6m8s
Details
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 14s
Details
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 15m34s
Details
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Details
9798877631 
Completes the second pin-recovery cycle. main moved twice during this branch's
review and each rebase rewrites the sha of the commit that touched docker/ci, so
the pin has to be re-pointed each time.

Simulated the guard's full logic before pushing, including the length check main
added in #598 (which came from this session's #594):
  length   = 7                                    (guard requires exactly 7)
  expected = git log -1 --format=%H -- docker/ci .gitea/workflows/ci-image.yml
           = 32747a067e
  pin      = 32747a0 -> resolves to the same commit
  => WOULD PASS

Confirmed the registry actually holds ersatztv-ci:32747a0 before pinning it, so
this cannot be the "pin resolves but no such tag exists" failure #594 describes.

Refs #445 #594
timothy commented 2026-07-25 14:35:36 +02:00
Author
Owner
Copy Link
Copy Source

Second rebase — reconciled with #598, head 97988776

main merged three times during this review. The latest (#598: "PID-scoped E2E cleanup, ci-image-pin length guard, .gitignore core fix") overlaps this PR directly, so this was a real semantic reconciliation, not an append-dedup.

#598 exposed a genuine latent bug in my work

The app binds TWO listeners. Program.cs binds UiPort and StreamingPort, and StreamingPort defaults to 8409 independently of ETV_UI_PORT. So this PR's CI step (ETV_UI_PORT=8410) was only working because the curl step's server had already been killed — correct by luck, not construction. Their fix (defaulting ETV_STREAMING_PORT to the given port in e2e-local.sh) is what makes it correct, and it covers e2e-ui.sh for free since I delegate boot to that script.

Consequently I removed my own port pre-flight from e2e-ui.sh: theirs probes both ports and reports rather than reaps (the #586 rule), so keeping mine would leave two divergent checks on one concern.

Both sides survived — verified symbol by symbol

Mine Theirs (#586/#598)
READY_REGEX (#533) ✓ ETV_STREAMING_PORT ✓
exec dotnet ✓ pkill warning ✓
ETV_PIDFILE ✓ port-number validation ✓
never-run-concurrently warning ✓ report-never-reap pre-flight ✓
playwright gitignore entries ✓ /core anchoring (#485) ✓

The launch line needed both, combined with a comment saying so: export …ETV_STREAMING_PORT… and exec dotnet ErsatzTV.dll — #586's fix and #533's fix, neither superseding the other. docs/decisions/README.md was regenerated, never hand-merged. No key lost from main's records.

I broke my own script mid-refactor and caught it by testing behaviour

Removing the redundant pre-flight, I used an index-based slice between two anchors — and the entire server-lifecycle block (reap(), cleanup(), the EXIT/INT/TERM traps, the pidfile wiring) sat between them. It was all deleted.

The specs still passed 3/3. Nothing on the happy path needs cleanup; it just silently leaked a server every run, which then tripped the app's single-instance guard and broke the curl harness with a completely unrelated-looking error (Another instance of ErsatztTV is already running).

  • What caught it: asserting 0 leftover instances after the run, not "specs passed".
  • What fixed it: restoring the file and redoing the removal with an explicit match that fails loudly, instead of index slicing. The refactor commit went from 108 deletions to 7 insertions/16 deletions.
  • Those commits were unpushed, so the fix is folded in rather than leaving a broken commit in history.

Ownership note on the leftover: I traced it by config-dir timestamp to my own run before killing it by PID — never pkill -f, per #586.

Verification (re-run against the reconciled tree)

  • UI-E2E 3/3, and 0 leftover instances after the run (the assertion that caught the bug)
  • Curl harness 45/45 — this had been failing due to my leaked instance, now clean
  • web 995 tests / 105 files, typecheck + lint clean, solution builds
  • decisions validator OK
  • Pin guard simulated including #598's new length check: length 7, resolves to 32747a06… = docker/ci's last-touching commit → WOULD PASS. Registry confirmed to hold ersatztv-ci:32747a0 before pinning it, so this can't be the "pin resolves but no tag exists" failure #594 is about.

Setting auto-merge-on-green: main has outrun this branch twice, and each rebase costs a pin-recovery cycle.

Review-verdict: MERGEABLE @ 97988776

## Second rebase — reconciled with #598, head `97988776` main merged three times during this review. The latest (#598: "PID-scoped E2E cleanup, ci-image-pin length guard, .gitignore core fix") **overlaps this PR directly**, so this was a real semantic reconciliation, not an append-dedup. ### #598 exposed a genuine latent bug in my work **The app binds TWO listeners.** `Program.cs` binds `UiPort` *and* `StreamingPort`, and `StreamingPort` defaults to **8409 independently of `ETV_UI_PORT`**. So this PR's CI step (`ETV_UI_PORT=8410`) was only working because the curl step's server had already been killed — correct by luck, not construction. Their fix (defaulting `ETV_STREAMING_PORT` to the given port in `e2e-local.sh`) is what makes it correct, and it covers `e2e-ui.sh` for free since I delegate boot to that script. Consequently I **removed my own port pre-flight** from `e2e-ui.sh`: theirs probes *both* ports and **reports rather than reaps** (the #586 rule), so keeping mine would leave two divergent checks on one concern. ### Both sides survived — verified symbol by symbol | Mine | Theirs (#586/#598) | |---|---| | `READY_REGEX` (#533) ✓ | `ETV_STREAMING_PORT` ✓ | | `exec dotnet` ✓ | `pkill` warning ✓ | | `ETV_PIDFILE` ✓ | port-number validation ✓ | | never-run-concurrently warning ✓ | report-never-reap pre-flight ✓ | | playwright gitignore entries ✓ | `/core` anchoring (#485) ✓ | The launch line needed both, combined with a comment saying so: `export …ETV_STREAMING_PORT…` **and** `exec dotnet ErsatzTV.dll` — #586's fix and #533's fix, neither superseding the other. `docs/decisions/README.md` was **regenerated**, never hand-merged. No key lost from main's records. ### I broke my own script mid-refactor and caught it by testing behaviour Removing the redundant pre-flight, I used an index-based slice between two anchors — and the **entire server-lifecycle block** (`reap()`, `cleanup()`, the `EXIT/INT/TERM` traps, the pidfile wiring) sat between them. It was all deleted. **The specs still passed 3/3.** Nothing on the happy path needs cleanup; it just silently leaked a server every run, which then tripped the app's **single-instance guard** and broke the curl harness with a completely unrelated-looking error (`Another instance of ErsatztTV is already running`). - What caught it: asserting **0 leftover instances after the run**, not "specs passed". - What fixed it: restoring the file and redoing the removal with an explicit match that fails loudly, instead of index slicing. The refactor commit went from **108 deletions to 7 insertions/16 deletions**. - Those commits were unpushed, so the fix is folded in rather than leaving a broken commit in history. Ownership note on the leftover: I traced it by config-dir timestamp to my *own* run before killing it **by PID** — never `pkill -f`, per #586. ### Verification (re-run against the reconciled tree) - UI-E2E **3/3**, and **0** leftover instances after the run (the assertion that caught the bug) - Curl harness **45/45** — this had been failing due to my leaked instance, now clean - web **995 tests / 105 files**, typecheck + lint clean, solution builds - decisions validator OK - Pin guard simulated including #598's new length check: length **7**, resolves to `32747a06…` = `docker/ci`'s last-touching commit → **WOULD PASS**. Registry confirmed to hold `ersatztv-ci:32747a0` *before* pinning it, so this can't be the "pin resolves but no tag exists" failure #594 is about. Setting auto-merge-on-green: main has outrun this branch twice, and each rebase costs a pin-recovery cycle. Review-verdict: MERGEABLE @ 97988776
timothy scheduled this pull request to auto merge when all checks succeed 2026-07-25 14:35:50 +02:00
timothy merged commit abeef63f05 into main 2026-07-25 14:52:33 +02:00
timothy deleted branch feat/445-533-ui-e2e 2026-07-25 14:52:34 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#591