From 973ca16a41e1e4cfa742783a3c0bfdbb8dbb0f11 Mon Sep 17 00:00:00 2001 From: PJ Date: Fri, 14 Aug 2026 22:48:38 +0530 Subject: [PATCH] fix(chrome): anchor the transition deadline when the dom goes quiet it was anchored at script start, so a page that churned past the window reached the check already expired and returned mid cross-fade. the driver now publishes the idle timeout it needs, since the caller's 1s could never spend the 800ms window. --- internal/driver/chrome/driver.go | 63 ++++++++++++++--- internal/driver/chrome/driver_test.go | 98 ++++++++++++++++++++++++++- 2 files changed, 149 insertions(+), 12 deletions(-) diff --git a/internal/driver/chrome/driver.go b/internal/driver/chrome/driver.go index d131141..a819351 100644 --- a/internal/driver/chrome/driver.go +++ b/internal/driver/chrome/driver.go @@ -557,12 +557,35 @@ const domQuietPeriod = 150 * time.Millisecond // shows two *Screen ids at rest costs this much per step and no more. const transitionSettlePeriod = 800 * time.Millisecond +// settleReturnMargin is what WaitForIdle holds back from the caller's timeout, +// so returning late by our own doing surfaces as a settled page rather than a +// context cancellation. +const settleReturnMargin = 100 * time.Millisecond + +// settleScanMargin covers the in-page work the two waits do not themselves +// account for: liveScreens() walks the document and every shadow root on each +// 16 ms poll, and the whole script costs one CDP round trip. +const settleScanMargin = 250 * time.Millisecond + +// MinIdleTimeout is the shortest timeout WaitForIdle can be handed and still +// spend the waits it is built from: the DOM quiet period, the route-transition +// window that only opens once that quiet period has elapsed, and the second +// quiet period the transition's own closing mutation starts. A caller that +// passes less caps the settle below its own budget, and the step then samples a +// page that is still mid-transition - which is the exact failure the transition +// wait exists to prevent. internal/runner raises a shorter caller timeout to +// this value. +func (d *Driver) MinIdleTimeout() time.Duration { + return 2*domQuietPeriod + transitionSettlePeriod + + settleScanMargin + settleReturnMargin +} + func (d *Driver) WaitForIdle(ctx context.Context, timeout time.Duration) error { runCtx, cancel := d.runCtx(ctx) defer cancel() // Leave the caller's deadline some room: returning late by our own doing // would surface as a context cancellation instead of a settled page. - budget := max(timeout-100*time.Millisecond, domQuietPeriod) + budget := max(timeout-settleReturnMargin, domQuietPeriod) script := fmt.Sprintf(settleScript, domQuietPeriod.Milliseconds(), budget.Milliseconds(), @@ -598,11 +621,18 @@ const liveScreensFunction = ` // itself gives up after %d ms. Shadow roots get their own observer: a canvas // app keeps its whole accessibility tree inside one, and mutations there do not // reach an observer on the document. +// +// The transition window opens when the quiet period ends, not when the script +// starts. Anchored at the start it is already spent by the time the check can +// first run on any page that keeps mutating for longer than the window, so the +// wait resolves immediately with both routes still live - the mid-transition +// return this whole wait exists to prevent. Each mutation reopens it, and the +// budget above bounds the total either way. const settleScript = ` new Promise(resolve => { const quietMillis = %d, budgetMillis = %d, transitionMillis = %d; const observers = []; - const transitionDeadline = Date.now() + transitionMillis; + let transitionDeadline = 0; let timer = null; const finish = () => { clearTimeout(timer); @@ -611,6 +641,7 @@ new Promise(resolve => { }; ` + liveScreensFunction + ` const quiet = () => { + if (transitionDeadline === 0) transitionDeadline = Date.now() + transitionMillis; if (liveScreens() > 1 && Date.now() < transitionDeadline) { timer = setTimeout(quiet, 16); return; @@ -619,6 +650,7 @@ new Promise(resolve => { }; const restart = () => { clearTimeout(timer); + transitionDeadline = 0; timer = setTimeout(quiet, quietMillis); }; const watch = (root) => { @@ -770,15 +802,19 @@ func (d *Driver) EvaluateExtractors(ctx context.Context) (map[int]json.RawMessag // action was actually applied. Without this call every web state.lastAction is // null, so a property gated on what the last action did is vacuously true and // reports a green run while checking nothing. +// +// The call is deliberately unguarded. A `setter && setter(...)` form evaluates +// to undefined on a page whose runtime does not define the setter, and chromedp +// reports that as success, so "the page cannot accept lastAction" would be +// indistinguishable from "installed". That page is reachable: a run resolving +// its web runtime from an older published @sanderling/spec would silently no-op +// every step. Unguarded, the missing global throws and the run fails loudly. func (d *Driver) SetLastAction(ctx context.Context, encoded json.RawMessage) error { payload := strings.TrimSpace(string(encoded)) if payload == "" { payload = "null" } - script := fmt.Sprintf( - `window.__sanderlingSetLastAction__ && window.__sanderlingSetLastAction__(%s)`, - payload, - ) + script := fmt.Sprintf(`window.__sanderlingSetLastAction__(%s)`, payload) runCtx, cancel := d.runCtx(ctx) defer cancel() if err := chromedp.Run(runCtx, chromedp.Evaluate(script, nil)); err != nil { @@ -789,16 +825,25 @@ func (d *Driver) SetLastAction(ctx context.Context, encoded json.RawMessage) err // extractorScript resolves the extractor table once the page is not mid route // transition, giving up on that wait after %d ms. +// +// A missing table rejects rather than reporting {}, for the same reason +// SetLastAction no longer guards its call: an empty override map is what a +// spec with no extractors returns, so the guarded form made "this page has no +// sanderling runtime" read as a normal step whose properties then ran on +// goja's dump-derived values instead of the page's. const extractorScript = ` -new Promise(resolve => { +new Promise((resolve, reject) => { const deadline = Date.now() + %d;` + liveScreensFunction + ` const read = () => { if (liveScreens() > 1 && Date.now() < deadline) { setTimeout(read, 16); return; } - resolve(JSON.stringify( - window.__sanderlingExtractors__ ? window.__sanderlingExtractors__() : {})); + if (typeof window.__sanderlingExtractors__ !== "function") { + reject(new Error("__sanderlingExtractors__ is not installed in the page")); + return; + } + resolve(JSON.stringify(window.__sanderlingExtractors__())); }; read(); })` diff --git a/internal/driver/chrome/driver_test.go b/internal/driver/chrome/driver_test.go index 570f512..234223f 100644 --- a/internal/driver/chrome/driver_test.go +++ b/internal/driver/chrome/driver_test.go @@ -672,17 +672,31 @@ func TestWaitForIdle_ReturnsOnABusyPage(t *testing.T) { // app is leaving, which on the folio wasm build recorded a submit that had // landed on Home as still being on the transaction screen: the route gate of an // action-gated property then skipped the very step the action landed on. +// +// The page keeps mutating for 300 ms after the route splice, and the settle +// runs on the timeout production hands it (MinIdleTimeout). Both details are +// load-bearing. A quiet page reaches the transition check immediately, so it +// passes whether the transition window is anchored at the script start or at +// the end of the quiet period; churn is what pushes the check past a +// start-anchored deadline, which then finishes at once with two live screens. +// And a caller timeout below MinIdleTimeout cuts the whole settle off before +// the transition window can be spent, which is the same bug from the other end. func TestWaitForIdle_WaitsOutARouteTransition(t *testing.T) { const page = `
` server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -701,7 +715,7 @@ func TestWaitForIdle_WaitsOutARouteTransition(t *testing.T) { if err := d.Tap(ctx, 40, 40); err != nil { t.Fatalf("Tap: %v", err) } - if err := d.WaitForIdle(ctx, 2*time.Second); err != nil { + if err := d.WaitForIdle(ctx, d.MinIdleTimeout()); err != nil { t.Fatalf("WaitForIdle: %v", err) } @@ -793,3 +807,81 @@ func TestEvaluateExtractors_WaitsOutARouteTransition(t *testing.T) { "mid-transition, so the spec sees the route the app is leaving", got) } } + +// TestSetLastAction_ReportsAPageThatCannotTakeIt covers the install the whole +// web path's action-gated properties hang off. A page without the setter is +// reachable: internal/testrun resolves the web runtime from +// node_modules/@sanderling/spec when no sibling checkout is present, and an +// older published runtime does not define it. Guarded as +// `setter && setter(...)`, that page returns undefined and chromedp reports +// success, so every step silently no-ops and every property gated on the last +// action goes vacuously true - a green run that checked nothing. +func TestSetLastAction_ReportsAPageThatCannotTakeIt(t *testing.T) { + const withSetter = `` + const withoutSetter = `
no sanderling runtime here
` + pages := map[string]string{"/with": withSetter, "/without": withoutSetter} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + _, _ = w.Write([]byte(pages[r.URL.Path])) + })) + defer server.Close() + + d := New() + defer d.Terminate(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + if err := d.Launch(ctx, server.URL+"/with", false, nil); err != nil { + t.Fatalf("Launch: %v", err) + } + action := json.RawMessage(`{"kind":"Tap","on":"id:TxnSubmit"}`) + if err := d.SetLastAction(ctx, action); err != nil { + t.Fatalf("SetLastAction on a page that defines the setter: %v", err) + } + var seen map[string]string + if err := chromedp.Run(d.tabCtx, + chromedp.Evaluate(`window.__lastActionSeen`, &seen)); err != nil { + t.Fatalf("read installed action: %v", err) + } + if seen["on"] != "id:TxnSubmit" { + t.Errorf("the page received %v, want the action the runner applied", seen) + } + + if err := d.Launch(ctx, server.URL+"/without", false, nil); err != nil { + t.Fatalf("Launch: %v", err) + } + if err := d.SetLastAction(ctx, action); err == nil { + t.Error("SetLastAction reported success on a page with no setter; " + + "a runtime that cannot take lastAction is indistinguishable from one that did") + } +} + +// TestEvaluateExtractors_ReportsAMissingTable is the same failure on the other +// sampler. An empty override map is what a spec with no extractors returns, so +// treating a missing table as {} makes "this page has no sanderling runtime" +// read as an ordinary step - and the verifier then judges the run on goja's +// dump-derived values while believing they came from the page. +func TestEvaluateExtractors_ReportsAMissingTable(t *testing.T) { + const page = `
no sanderling runtime here
` + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html") + _, _ = w.Write([]byte(page)) + })) + defer server.Close() + + d := New() + defer d.Terminate(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := d.Launch(ctx, server.URL, false, nil); err != nil { + t.Fatalf("Launch: %v", err) + } + values, err := d.EvaluateExtractors(ctx) + if err == nil { + t.Errorf("EvaluateExtractors returned %v and no error on a page with no "+ + "extractor table; a page that cannot be read must not read as empty", values) + } +}