f00dfde0c574b9dd6797d25dbcd706d89aecdf4e
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 timothy
|
352aa70634 |
refactor(445): drop e2e-ui.sh's port pre-flight, now owned by e2e-local.sh (#586/#598)
main's #598 added a port pre-flight to scripts/e2e-local.sh while this branch was in review, and theirs is strictly better than the one I had here: - it probes BOTH bound ports. The app binds a SECOND (streaming) listener that defaults to 8409 regardless of ETV_UI_PORT, so my single-port check could certify a port free while the run still died binding 8409. That also means this PR's CI step (ETV_UI_PORT=8410) was only working because the curl step's server had already been killed — #598's fix, defaulting ETV_STREAMING_PORT to the given port, is what makes it correct rather than lucky. A real latent bug in my work, surfaced by their change. - it REPORTS rather than reaps, which is the #586 rule. Keeping mine would leave two divergent port checks on the same concern. Also records the app's SINGLE-INSTANCE guard, which is independent of ports: a stray instance blocks a run whatever port you choose, so 'pick another port' is not a workaround. Kill the leftover BY PID (#586). Refs #445 #586 |
||
|
timothy
|
54919e4730 |
fix(445): pidfile-first cleanup ordering + clear BOOT_PID after wait
Codex round-3 verdict was BLOCKED @ 29bc5119 with three High findings. All three are real; all three are fixed. They are narrower than the previous rounds — two are sub-millisecond races between adjacent statements — but the fixes are cheap and make the ordering correct rather than lucky. **Recycled-PID kill (the one that genuinely mattered).** `BOOT_PID` stayed set after `wait` had already reaped the launcher. A stale PID is not merely useless: the OS can RECYCLE that number during a long spec run, and cleanup would then SIGTERM an unrelated process. This is the same "never kill something you did not prove is yours" failure that got the earlier port-based reap deleted — it came back in a different disguise. Cleared immediately after `wait` returns. **Cleanup ordering.** cleanup() killed e2e-local.sh BEFORE reading the pidfile, so a signal landing between the server fork and the pidfile write could kill the launcher and lose the only handle on an already-running server. The pidfile is now read FIRST; stopping the launcher is best-effort and never the primary handle. **Fork/publish sliver.** A signal can also land between `... &` and `BOOT_PID=$!`, or between e2e-local.sh's fork and its pidfile write — the pidfile reads empty, then populates microseconds later, and the server is orphaned. cleanup now does a bounded grace re-read (~1s) when it has no PID. Verified this does NOT cost anything on a genuinely server-less abort: a bad ETV_BUILD_CONFIG still exits in 1s rather than sitting out the loop. ## Verification - passing run 3/3; failing run exits 1 - server-less abort exits in 1s (grace loop stays bounded) - targeted SIGTERM: exit 143, no listener left - zero orphaned processes after the full gate Refs #445 |
||
|
timothy
|
e8ad79623f |
fix(445): make the boot wait interruptible so a mid-boot signal can't orphan the server
Codex round-2 verdict was BLOCKED @ 2a5f26c0 on one High finding, which was
correct and is the subtlest defect in this whole change.
## The defect
Bash DEFERS a trapped signal while it waits on a FOREGROUND command. The boot was
OUT="$(ETV_UI_PORT=... scripts/e2e-local.sh "$CONFIG_DIR")"
so a TERM arriving during the (up to 120s) readiness wait did not run `on_signal`
until boot completed — and a supervisor escalating TERM->KILL means the trap
never runs at all, orphaning the server on its port. The pidfile added in the
previous commit only helps IF the trap runs; this is the case where it doesn't.
Proved the mechanism in isolation rather than asserting it, since the real boot
is only ~2s locally and kept winning the race:
foreground command substitution : TERM at t=1s -> trap ran at t=6s (deferred)
background job + `wait` : TERM at t=1s -> trap ran at t=1s (prompt)
## The fix
Boot in the background and `wait` on it. `wait` is interruptible, so the handler
runs immediately; by then e2e-local.sh has already written $ETV_PIDFILE, so
cleanup can reap a server whose PID this script has not yet parsed. Cleanup also
now stops the backgrounded e2e-local.sh itself, so it cannot sit waiting on a
server we just killed, and removes its temp output file.
Boot diagnostics are preserved on the failure path (verified with a deliberately
bad ETV_BUILD_CONFIG: the underlying "bin/<config> does not exist" error is
surfaced, along with the exit status, which is now propagated rather than
flattened to 1).
## Verification
- mechanism: deferred-vs-prompt trap proven as above
- passing run 3/3 green; failing run (--grep ZZZ_NOPE) exits 1
- boot-failure path prints e2e-local.sh's real error and its exit status
- targeted SIGTERM: exit 143, no listener, no stray process
- curl harness unaffected: 45/45 PASS
- no orphaned processes after the full gate (an earlier count of 1 was a
graceful shutdown still in flight, not a leak — re-checked clean)
Refs #445
|
||
|
timothy
|
71706f3849 |
fix(445): silent-pass on spec failure + prove server ownership via pidfile [decisions-edit]
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
|
||
|
timothy
|
0f1951340e |
fix(445,533): harden e2e-ui lifecycle + retire stale #533 decision record [decisions-edit]
Addresses the adversarial review round. Codex returned BLOCKED on the harness lifecycle contract; the second (cold) reviewer independently flagged the same trap-ordering defect, which is what made it credible. **Trap installed AFTER boot -> signal mid-boot orphans the server.** The window between `e2e-local.sh` returning and `trap ... EXIT INT TERM` had no handler, so a Ctrl-C/TERM there (or the PID-parse bail-out) left dotnet holding the port — violating the script's own "always kills the server" contract. The trap is now installed BEFORE boot. When the PID is not yet known, cleanup falls back to reaping whatever LISTENS on our port; attribution is sound because the pre-flight proved that port free moments earlier. **Signals were not re-raised.** A TERM landing beside a passing Playwright run could exit 0, reporting success for a cancelled run. INT/TERM now clean up and re-raise, so the wrapper dies BY the signal. Verified, not assumed: SIGTERM mid-run -> wrapper exits 143 (128+15) and leaves zero listeners and zero stray ErsatzTV processes. `e2e-local.sh` backgrounded a MULTI-command subshell, so `$!` is the subshell — not dotnet — wherever bash does not collapse it. Measured both ways: bash 3.2.57 (stock macOS /bin/bash), 2-command subshell : $! = SUBSHELL bash 5.3.15 (homebrew) : $! = leaf with `exec` (both versions) : $! = leaf Consequence on stock-macOS bash: every PID-based kill/liveness check targeted the wrong process, the escalation silently no-opped, and the server leaked. Fixed at the source with `exec`, which benefits all consumers (the CI step's trap and the e2e-functional.sh pairing), not just e2e-ui.sh. My first attempt to test this was WRONG and would have cleared the finding: a single-command subshell is collapsed on both versions. Only the 2-command form reproduces it. `testing.e2e-local-fresh-config-dir` was still `status: active` asserting the bug 'wait for either line' option today", "widening the probe ... is tracked as #533". `READY_LINE` existed ONLY in that record; nowhere in code. Amended rather than superseded: the operative rule (use a fresh config dir) is unchanged and still correct — only its RATIONALE moved from "the probe hangs" to "state bleed". Heading deliberately left alone: the validator matches records by `## HEADING`, so renaming fails CI as an "unlogged removal"; an explicit note now tells the reader the heading is historical. - Boot-failure diagnostics were lost: `OUT="$(...)"` aborts under `set -e` before the print. Now `if ! OUT=$(...)` so the output is shown. - Playwright failures surfaced no server-side evidence (nothing uploads the traces in web/e2e/.output). The server log tail is now printed on failure. - `lsof -ti :PORT` also matched outbound/TIME_WAIT sockets -> false positives. Now `-sTCP:LISTEN`. Documented honestly that the CI image ships no lsof, so the pre-flight is a local-developer guard only. - Predictable `/tmp/etv-pw-probe.$$` -> `mktemp` (symlink-redirect on a shared host). - Suggested escape-hatch port was 8411, which is scripts/security-scan.sh's default; 8409/8410 are prod/ersatztv-test on jazz. Now suggests 8419 and names the conflicts. - boot-gate.spec.ts comment overclaimed: after logout the browser holds NO cookie, so that assertion cannot prove stamp rotation (the curl harness does, by replaying the same cookie). Comment corrected to what it actually proves. Deliberately NOT changed: `ci-image-pin`'s regex accepts any hex length rather than the exact 7 chars ci-image.yml publishes (pre-existing guard weakness, not introduced here — filed as a follow-up rather than widened in this PR). Also trimmed the two records I had bloated: the active decisions corpus went over its 5600-line budget as a result of this PR (baseline on main was under), so the overflow was mine to pay down, not to pass on. - SIGTERM mid-run: exit 143, no orphan listener/process - curl harness unaffected by the `exec` change: 45/45 PASS - UI-E2E 2x clean; typecheck + lint clean; decisions validator OK, no size warning Refs #445 #533 |
||
|
timothy
|
d8c0b3e752 |
feat(445,533): headless Playwright UI-E2E flows + fix e2e-local readiness probe [decisions-edit]
Adds the last deferred #299/#363 follow-up: the flows that CANNOT be expressed
as curl calls. Scope rule (the durable part) — assert only what the curl
harness structurally cannot reach:
1. client-side form validation (the Setup confirm-password gate is pure React
state and makes no request, so there is no HTTP contract to assert)
2. AuthGate's RENDERED states (Setup vs Login vs app)
3. the session cookie authenticating the SPA's OWN /api XHRs — curl proves the
cookie works for curl, not that the app sends it
4. sign-out through the UserMenu back to the login gate
New: web/e2e/boot-gate.spec.ts, web/playwright.config.ts, scripts/e2e-ui.sh
(owns the whole lifecycle: fresh config dir -> boot -> specs -> always kill).
Runs as a second step of the EXISTING advisory `functional-e2e` job rather than
a new job: the dominant cost there is `npm ci` + the Release build, both already
done, so this adds ~5s instead of duplicating a heavy job. It boots its own
fresh instance on port 8410 because the first spec asserts the one-shot Setup
gate that the curl step has already claimed on its config dir.
Determinism (the issue asked for it explicitly): `serial`, `workers: 1`,
`retries: 0` even in CI — a retry would let a flaky flow merge looking green.
Measured 5 consecutive clean runs, ~2s each.
Pins all five `container:` jobs to the toolchain image built by the preceding
commit, which bakes `chromium-headless-shell`.
Non-obvious coupling fixed: vitest's default include glob would have collected
web/e2e/*.spec.ts and run it under jsdom. Excluded `e2e/**` by spreading
`configDefaults.exclude` rather than narrowing `include` to `src/**`, because
web/scripts/ holds a real vitest test an src-only include would silently stop
running.
`RebuildSearchIndexHandler` logs one of two mutually-exclusive lines just before
`SystemStartup.SearchIndexIsReady()`:
fresh config -> "Done migrating search index in {Duration}"
reused config -> "Search index is already version {Version}"
The probe watched only the first, so a reused dir waited out the full 120s
timeout and then killed a perfectly healthy server. Widened to a `grep -Eq`
alternation; the handler's if/else is exhaustive, so the pair covers every path
to readiness.
Verified with a negative control: on a reused dir the server is ready in 2s via
the "already version" line, and the OLD probe string is genuinely ABSENT from
that run's log — so the old code would have hung, i.e. the fix is load-bearing
rather than incidentally passing.
The "prefer a fresh config dir" guidance stays: that guards state bleed, which
is a separate concern from the probe hanging.
- `wait "$PID"` in the cleanup trap was a NO-OP: the server is a grandchild
(launched in e2e-local.sh's subshell, which then exits), so `wait` fails
instantly and was swallowed by `|| true` — cleanup did not actually ensure the
port was released, exactly what its comment claimed. Replaced with a bounded
`kill -0` poll, then SIGKILL.
- Added a port pre-flight check: previously an occupied port surfaced as a 120s
readiness timeout that reads like a broken build. Now fails in 0s naming the
PIDs, and warns against blanket-killing `dotnet ErsatzTV.dll` (that reaps
other sessions' servers).
- UI-E2E: 5x clean (3 specs, ~2s); back-to-back runs pass with no manual cleanup
- curl harness unaffected by the boot-script change: 45/45 PASS
- web: 983 tests / 105 files green; typecheck + lint clean
- vitest collection verified: excludes web/e2e, still collects web/scripts
- Dockerfile sequence + browser launch validated verbatim in a container on the
real amd64 base before committing; chromium launches as root with NO sandbox
opt-out needed
- decisions validator green; catalog regenerated
- docs/decisions.md TOC repaired: it had drifted to 69 of 97 records and held a
dangling anchor to the #72 record that #415 superseded into archive/.
Regenerated with a generator validated against the 68 existing anchors (0
mismatches) -> 97/97, no dangling, no duplicates.
Docs: docs/e2e-local.md (new "UI-E2E harness" section), docs/ci-cd.md (toolchain
image + UI-E2E step), docs/testing.md, docs/README.md, docs/decisions.md
(new `ci.ui-e2e-harness` record; `ci.functional-e2e-harness` amended — its Rule
said "curl-only", now accurate).
Refs #445 #533
|