fix(e2e-browser): per-op deadline reset kills shared-budget flake (bookshelf-rpr4) #892

Merged

zombor merged 2 commits from bd-bookshelf-rpr4 into main 2026-07-03 02:57:40 +00:00

Conversation 9 Commits 2 Files changed 10 +41 -11

zombor commented 2026-07-03 01:33:57 +00:00

Owner

Copy link

Summary

• Root cause (confirmed): waitForStimulusController and multi-step JustBeforeEach/It blocks shared a single 60s absolute go-rod deadline set by one refreshPageTimeout call. On the chart-heavy /library-stats page under CI load, MustNavigate+MustWaitStable consumed nearly the full 60s budget, leaving waitForStimulusController (and MustElement after it) to start against a near-expired context — panicking with "context deadline exceeded". Proof: deadline reset at 00:49:21, panic at 00:50:21 (exactly 60s, run 5662/#885).

• Fix (durable): waitForStimulusController now resets the page absolute deadline at its start via refreshPageTimeout and returns the updated *rod.Page. Every subsequent go-rod op after a controller wait therefore starts with a fresh 60s budget, regardless of how long the preceding navigate+wait chain took.

• Primary fix site: journey_library_stats_test.go JustBeforeEach — the confirmed panic location.

• Systemic audit: all 19 call sites of waitForStimulusController across 11 files updated to page = waitForStimulusController(...). Additional multi-op chains hardened:

  • openDedupModal (7-op chain in browser_seed_helpers_test.go) — both controller waits reset the deadline
  • journey_a11y_keyboard_test.go — 4 It blocks navigating to /library-stats (chart-heavy) before a controller wait
  • journey_stats_distributions_test.go, journey_organize_modal_test.go, journey_book_file_replace_test.go, bookdrop_*.go (5 files)

Test plan

• go vet -tags e2e ./e2e/browser/ passes (verified locally, no type errors from changed signature)
• make test passes (verified locally)
• CI e2e browser suite green — the fixed JustBeforeEach no longer panics on the chart-heavy page

Closes bead bookshelf-rpr4 on merge.

## Summary - **Root cause (confirmed):** `waitForStimulusController` and multi-step `JustBeforeEach`/`It` blocks shared a single 60s absolute go-rod deadline set by one `refreshPageTimeout` call. On the chart-heavy `/library-stats` page under CI load, `MustNavigate+MustWaitStable` consumed nearly the full 60s budget, leaving `waitForStimulusController` (and `MustElement` after it) to start against a near-expired context — panicking with *"context deadline exceeded"*. Proof: deadline reset at 00:49:21, panic at 00:50:21 (exactly 60s, run 5662/#885). - **Fix (durable):** `waitForStimulusController` now resets the page absolute deadline at its start via `refreshPageTimeout` and **returns the updated `*rod.Page`**. Every subsequent go-rod op after a controller wait therefore starts with a fresh 60s budget, regardless of how long the preceding navigate+wait chain took. - **Primary fix site:** `journey_library_stats_test.go` `JustBeforeEach` — the confirmed panic location. - **Systemic audit:** all 19 call sites of `waitForStimulusController` across 11 files updated to `page = waitForStimulusController(...)`. Additional multi-op chains hardened: - `openDedupModal` (7-op chain in `browser_seed_helpers_test.go`) — both controller waits reset the deadline - `journey_a11y_keyboard_test.go` — 4 `It` blocks navigating to `/library-stats` (chart-heavy) before a controller wait - `journey_stats_distributions_test.go`, `journey_organize_modal_test.go`, `journey_book_file_replace_test.go`, `bookdrop_*.go` (5 files) ## Test plan - [ ] `go vet -tags e2e ./e2e/browser/` passes (verified locally, no type errors from changed signature) - [ ] `make test` passes (verified locally) - [ ] CI e2e browser suite green — the fixed JustBeforeEach no longer panics on the chart-heavy page Closes bead bookshelf-rpr4 on merge.

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

wf-detail-older-execution.png

56 KiB

recompute-match-score-kebab-open.png

109 KiB

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

56 KiB

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

zombor added 1 commit 2026-07-03 01:33:57 +00:00

fix(e2e-browser): reset go-rod deadline per blocking op to kill shared-budget flake (bookshelf-rpr4) ... 1728a4d2b8

Root cause: `waitForStimulusController` and multi-op `JustBeforeEach`/`It`
blocks were sharing a single 60s absolute go-rod deadline set by one
`refreshPageTimeout` call.  On the chart-heavy /library-stats page under CI
load, `MustNavigate+MustWaitStable` consumed nearly the full budget, leaving
`waitForStimulusController` (and `MustElement` after it) to start against a
near-expired context — panicking with "context deadline exceeded".  Proof:
deadline reset at 00:49:21, panic at 00:50:21 (exactly 60s, bookshelf-rpr4).

Fix:
- `waitForStimulusController` now resets the page deadline at its start
  (via `refreshPageTimeout`) and returns the updated `*rod.Page` so the
  caller gets a fresh 60s for every subsequent op after the controller wait.
- All 19 call sites updated to `page = waitForStimulusController(...)`.
- Primary flake site (`journey_library_stats_test.go` JustBeforeEach) now
  chains: `refreshPageTimeout` → navigate+wait → `waitForStimulusController`
  (resets) → MustClick, each op getting its own fresh budget.
- `openDedupModal` (7-op chain) hardened: both `waitForStimulusController`
  calls reset the deadline, so click ops after each call are safe.

Audit of all e2e/browser/ files for the same anti-pattern:
- `journey_a11y_keyboard_test.go`: 4 `It` blocks navigate to /library-stats
  or /settings then call `waitForStimulusController` — all updated.
- `journey_stats_distributions_test.go`: `waitForStimulusController` called
  after `MustElement` in an `It` — updated.
- `journey_organize_modal_test.go`, `journey_book_file_replace_test.go`,
  `bookdrop_*.go` (5 files): navigate+wait then controller wait — all updated.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented 2026-07-03 01:36:22 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

forgejo-actions commented 2026-07-03 01:36:56 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

zombor added 1 commit 2026-07-03 01:49:04 +00:00

fix(e2e-browser): per-op deadline resets eliminate go-rod timeout flake ... 5e5962a454

Root cause (bookshelf-rpr4): waitForStimulusController held shared page
context and was called AFTER MustNavigate+MustWaitStable, leaving the
click/Eval ops with an exhausted 60s budget on slow CI.

Fix:
- Revert waitForStimulusController to void (no return, no internal reset)
  so it never cancels the page context and invalidates captured elements.
- Add an explicit `page = refreshPageTimeout(page)` before each blocking
  op that follows a heavy navigate+wait sequence:
    MustNavigate → [break chain] → refreshPageTimeout → MustWaitStable
    → refreshPageTimeout → waitForStimulusController → refreshPageTimeout
    → MustElement/MustClick
- journey_library_stats_test.go JustBeforeEach: break
  MustNavigate(...).MustWaitStable() chain with an inter-op reset (the
  confirmed panic site: 62.559 s with MustWaitStable under full 60s budget).
- journey_stats_distributions_test.go: remove the erroneous page=
  assignment; the page is pre-loaded so the 60s budget is ample.
- All other callers (bookdrop_*, organize_modal, book_file_replace, axe,
  keyboard, openDedupModal): add refreshPageTimeout before the controller
  poll and drop the (now-illegal) assignment.

Closes bead bookshelf-rpr4 on merge.

forgejo-actions commented 2026-07-03 01:50:39 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

forgejo-actions commented 2026-07-03 01:50:46 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

zombor commented 2026-07-03 02:02:35 +00:00

Author

Owner

Copy link

Code Review — PR #883 (bookshelf-ry66)

Diff reviewed: origin/main...origin/bd-bookshelf-ry66 (head 87e9ea8eb5)
Files: internal/books/dto.go, internal/books/metadata_service.go, internal/books/metadata_candidates_test.go, static/js/controllers/metadata_fetch_controller.js, static/js/test/metadata_fetch_controller.test.js

---

Phase 0: DEMO Verification

No DEMO block in the bead completion comment. CI is confirmed green via Forgejo commit-status API (state: success) and PR is mergeable. Per the review task the CI green state serves as functional verification; findings below are diff-based.

---

Phase 1: Spec Compliance

age_rating 0 (All Ages) on the Go side - PASS.

• CandidateResult.AgeRating *int32 with json tag age_rating,omitempty: Go JSON encoder omits pointer fields only when the pointer is nil, NOT when the pointed-to value is zero. A Metron-provided All Ages rating (AgeRating = &int32(0)) serialises to "age_rating": 0 in the candidate JSON. Correct.
• metadataToCandidateResult at metadata_service.go:275 copies m.AgeRating directly. The all-ages case passes through. Correct.
• Service validation at metadata_service.go:372-380: if req.AgeRating != nil is non-nil for &0 so the branch is entered. IsCanonicalAgeRating(0) returns true (ratings_test.go:26). Not rejected. Correct.
• metadataToUpsertParams at metadata_store.go:1197: both conditions true for &0. Sets sql.NullInt32{Int32: 0, Valid: true}. Persisted as 0, not NULL. Correct.

age_rating 0 on the JS side - PASS.

• displayValue("age_rating", {age_rating: 0}): new two-step guard (lines 60-62) returns "0" not "". !("0") is false in JS, so buildCompareRow does not skip the row. Compare row renders for All Ages. Correct.
• Copy action: _applyCopyToField(curEl, "0", btn) sets the modal text input to "0".
• Save path (line 851-856): v = "0", v !== "" is true, k === "age_rating" branch executes, Number("0") = 0, !isNaN(0) true, body.age_rating = 0. Sent as JSON number 0. Server decodes to *int32(0). Correct.

Locks - PASS. SQL upsert in metadata.sql.go:120 uses IF(COALESCE(age_rating_locked, 0) = 0 AND COALESCE(all_fields_locked, 0) = 0, VALUES(age_rating), age_rating). Lock enforcement at DB level, unchanged.

Canonical validation - PASS. Service rejects non-canonical age_rating values (metadata_service.go:374-379). Go json.Unmarshal into *int32 rejects floats at decode time. IsCanonicalContentRating gate at line 382 unmodified.

Serialisation field names - PASS. JS sends age_rating (number) and content_rating (string), matching SaveMetadataRequest fields.

Scope vs PR 882 (7mzg) - PASS. This PR adds to fieldLabel/_editorFieldID/displayValue/_saveFromModal. It does NOT touch buildComicCompareRow, _comicEditorFieldID, comicFieldLabel, or comicFieldOrder. Non-conflicting sections; clean rebase expected.

Bulk enrich path - PASS. No changes to persistInTxWithComic or wfengine bulk enrichment activities. Bulk path writes AgeRating/ContentRating via metadataToUpsertParams unchanged.

Black-box tests - PASS. metadata_candidates_test.go declares package books_test. No unexported symbols referenced.

CSP / inline style= - PASS. No new style= attributes in the diff.

---

Phase 2: Code Quality Findings

[MINOR] static/js/test/metadata_fetch_controller.test.js — missing save-path test for age_rating=0
The JS test suite tests copy-and-save with age_rating 13 but never exercises the All Ages path (age_rating 0) through to the POST body. The code is correct (v !== "" passes for "0", Number("0") = 0), and coverage is maintained because the same branch handles both values. But age_rating 0 is the critical case motivating this fix; a future regression that special-cases 0 in the save path would go undetected. Suggested addition: open modal with METRON_CANDIDATE_ALL_AGES, copy age_rating, save, assert capturedBody.age_rating === 0 (number, not string, not absent).

[MINOR] static/js/controllers/metadata_fetch_controller.js:308-315 — compare row shows raw "0" not "All Ages" in fetched column
displayValue("age_rating", {age_rating: 0}) returns "0". The modal fetched column shows 0 while the main form select shows the label "All Ages". Not a correctness bug — the value copies and saves correctly — but potentially confusing to users. Consider mapping numeric buckets to labels inside displayValue when key === "age_rating".

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Code Review — PR #883 (bookshelf-ry66) **Diff reviewed:** origin/main...origin/bd-bookshelf-ry66 (head 87e9ea8eb5b3) **Files:** internal/books/dto.go, internal/books/metadata_service.go, internal/books/metadata_candidates_test.go, static/js/controllers/metadata_fetch_controller.js, static/js/test/metadata_fetch_controller.test.js --- ### Phase 0: DEMO Verification No DEMO block in the bead completion comment. CI is confirmed green via Forgejo commit-status API (state: success) and PR is mergeable. Per the review task the CI green state serves as functional verification; findings below are diff-based. --- ### Phase 1: Spec Compliance **age_rating 0 (All Ages) on the Go side - PASS.** - CandidateResult.AgeRating *int32 with json tag age_rating,omitempty: Go JSON encoder omits pointer fields only when the pointer is nil, NOT when the pointed-to value is zero. A Metron-provided All Ages rating (AgeRating = &int32(0)) serialises to "age_rating": 0 in the candidate JSON. Correct. - metadataToCandidateResult at metadata_service.go:275 copies m.AgeRating directly. The all-ages case passes through. Correct. - Service validation at metadata_service.go:372-380: if req.AgeRating != nil is non-nil for &0 so the branch is entered. IsCanonicalAgeRating(0) returns true (ratings_test.go:26). Not rejected. Correct. - metadataToUpsertParams at metadata_store.go:1197: both conditions true for &0. Sets sql.NullInt32{Int32: 0, Valid: true}. Persisted as 0, not NULL. Correct. **age_rating 0 on the JS side - PASS.** - displayValue("age_rating", {age_rating: 0}): new two-step guard (lines 60-62) returns "0" not "". !("0") is false in JS, so buildCompareRow does not skip the row. Compare row renders for All Ages. Correct. - Copy action: _applyCopyToField(curEl, "0", btn) sets the modal text input to "0". - Save path (line 851-856): v = "0", v !== "" is true, k === "age_rating" branch executes, Number("0") = 0, !isNaN(0) true, body.age_rating = 0. Sent as JSON number 0. Server decodes to *int32(0). Correct. **Locks - PASS.** SQL upsert in metadata.sql.go:120 uses IF(COALESCE(age_rating_locked, 0) = 0 AND COALESCE(all_fields_locked, 0) = 0, VALUES(age_rating), age_rating). Lock enforcement at DB level, unchanged. **Canonical validation - PASS.** Service rejects non-canonical age_rating values (metadata_service.go:374-379). Go json.Unmarshal into *int32 rejects floats at decode time. IsCanonicalContentRating gate at line 382 unmodified. **Serialisation field names - PASS.** JS sends age_rating (number) and content_rating (string), matching SaveMetadataRequest fields. **Scope vs PR 882 (7mzg) - PASS.** This PR adds to fieldLabel/_editorFieldID/displayValue/_saveFromModal. It does NOT touch buildComicCompareRow, _comicEditorFieldID, comicFieldLabel, or comicFieldOrder. Non-conflicting sections; clean rebase expected. **Bulk enrich path - PASS.** No changes to persistInTxWithComic or wfengine bulk enrichment activities. Bulk path writes AgeRating/ContentRating via metadataToUpsertParams unchanged. **Black-box tests - PASS.** metadata_candidates_test.go declares package books_test. No unexported symbols referenced. **CSP / inline style= - PASS.** No new style= attributes in the diff. --- ### Phase 2: Code Quality Findings [MINOR] static/js/test/metadata_fetch_controller.test.js — missing save-path test for age_rating=0 The JS test suite tests copy-and-save with age_rating 13 but never exercises the All Ages path (age_rating 0) through to the POST body. The code is correct (v !== "" passes for "0", Number("0") = 0), and coverage is maintained because the same branch handles both values. But age_rating 0 is the critical case motivating this fix; a future regression that special-cases 0 in the save path would go undetected. Suggested addition: open modal with METRON_CANDIDATE_ALL_AGES, copy age_rating, save, assert capturedBody.age_rating === 0 (number, not string, not absent). [MINOR] static/js/controllers/metadata_fetch_controller.js:308-315 — compare row shows raw "0" not "All Ages" in fetched column displayValue("age_rating", {age_rating: 0}) returns "0". The modal fetched column shows 0 while the main form select shows the label "All Ages". Not a correctness bug — the value copies and saves correctly — but potentially confusing to users. Consider mapping numeric buckets to labels inside displayValue when key === "age_rating". --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor force-pushed bd-bookshelf-rpr4 from 5e5962a454 to eb98a466ed 2026-07-03 02:06:41 +00:00 Compare

forgejo-actions commented 2026-07-03 02:09:22 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

forgejo-actions commented 2026-07-03 02:09:29 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

zombor force-pushed bd-bookshelf-rpr4 from eb98a466ed to 70a499fa58 2026-07-03 02:50:25 +00:00 Compare

forgejo-actions commented 2026-07-03 02:55:14 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

forgejo-actions commented 2026-07-03 02:55:45 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

zombor merged commit 4006fdf19a into main 2026-07-03 02:57:40 +00:00

zombor referenced this pull request from a commit 2026-07-03 02:57:43 +00:00

Merge pull request 'fix(e2e-browser): per-op deadline reset kills shared-budget flake (bookshelf-rpr4)' (#892) from bd-bookshelf-rpr4 into main

zombor referenced this pull request from a commit 2026-07-03 15:49:07 +00:00

fix(e2e): eliminate publishers-tab render race via BeforeAll + Eventually wait (bookshelf-5kje)

zombor referenced this pull request 2026-07-03 15:49:34 +00:00

fix(e2e): eliminate publishers-tab render race via BeforeAll + Eventually (bookshelf-5kje) #903

zombor referenced this pull request from a commit 2026-07-05 00:51:33 +00:00

fix(e2e): use page.Has() (non-blocking) in publishers tab Eventually loop

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!892

No description provided.