From f738a590f013527d1699b3ca0dcc95098e47377a Mon Sep 17 00:00:00 2001 From: PJ Date: Fri, 14 Aug 2026 22:48:38 +0530 Subject: [PATCH] fix(web): read the page's extractors only on steps that count the page advances the spec's carriers when it evaluates, but the runner applied the result only on non-transitional steps. a discarded step moved the window forward anyway, so the next accepted pair bracketed two transactions while counting one submit, and convicted a healthy app. extractor errors now fail the run instead of leaving goja's values in current against v8's in previous. --- internal/runner/runner.go | 72 +++++++--- internal/runner/web_carrier_test.go | 196 ++++++++++++++++++++++++++++ 2 files changed, 249 insertions(+), 19 deletions(-) create mode 100644 internal/runner/web_carrier_test.go diff --git a/internal/runner/runner.go b/internal/runner/runner.go index 7555732..72c2430 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -74,6 +74,7 @@ func Run(ctx context.Context, options Options) (Summary, error) { if logger == nil { logger = slog.Default() } + options.IdleTimeout = resolveIdleTimeout(options) // Gate on the app actually being on top before acting, so the first // action never fires against a leftover screen or a system dialog. Done @@ -122,9 +123,8 @@ func Run(ctx context.Context, options Options) (Summary, error) { var logs []verifier.LogEntry // gctx is bound to the errgroup so a returned error (or outer - // cancellation) propagates to siblings - notably the V8 extractor - // goroutine, whose CDP round-trip can otherwise outrun the step - // budget on a hung tab. + // cancellation) propagates to every sibling read rather than leaving + // one blocked on a hung device. g, gctx := errgroup.WithContext(ctx) si := stepIndex // fetchSyncedState issues a single Snapshot RPC so hierarchy and @@ -143,19 +143,6 @@ func Run(ctx context.Context, options Options) (Summary, error) { logs = collectLogs(gctx, options.Driver, logSince) return nil }) - var v8Overrides map[int]json.RawMessage - // The same lastAction PushSnapshot hands the goja state below: the two - // engines evaluate this step against one action, not two. - stepAction := lastAction - g.Go(func() error { - overrides, err := extractorSource.ExtractorOverrides(gctx, stepAction) - if err != nil { - logger.Warn("v8 extractor evaluation failed", "step", si, "err", err) - return nil - } - v8Overrides = overrides - return nil - }) // All goroutines write to local variables and return nil, so the Wait // error is always nil; ignored intentionally. _ = g.Wait() @@ -198,6 +185,29 @@ func Run(ctx context.Context, options Options) (Summary, error) { var witnesses map[string]trace.Witness skippedVerification := false if !transitional { + // The page-side extractors evaluate only on steps the verifier will + // accept, which is why this read waits for the tree instead of + // racing it. A spec's extractor getters carry state across steps + // (folio's last-seen Home total, its submit counters) and that state + // advances every time they run: evaluating them on a step whose + // values are then thrown away leaves the page one window ahead of + // the verifier, so the next accepted pair brackets two committed + // transactions while having counted one submit, and the property + // convicts a healthy app. It costs the latency the read used to hide + // behind the hierarchy fetch; the fetch is what decides whether this + // step counts at all, so it has to go first. + // + // lastAction is the same value PushSnapshot hands the goja state + // below: the two engines evaluate this step against one action. + v8Overrides, overridesErr := extractorSource.ExtractorOverrides(ctx, lastAction) + if overridesErr != nil { + // Not a warning. Without the page's values this step's + // extractors keep goja's dump-derived readings while the + // previous step holds the page's, and a delta property then + // compares two producers and fires on an app that did nothing + // wrong. + return summary, fmt.Errorf("step %d extractor overrides: %w", stepIndex, overridesErr) + } if err := options.Verifier.PushSnapshot(verifier.SnapshotInput{ Tree: tree, ScreenshotPNG: screenshotPNG, @@ -384,12 +394,36 @@ func validate(options Options) error { if options.Duration <= 0 { return errors.New("runner: Duration must be positive") } - if options.IdleTimeout <= 0 { - options.IdleTimeout = 2 * time.Second - } return nil } +// defaultIdleTimeout is the settle budget a caller that names none gets. +const defaultIdleTimeout = 2 * time.Second + +// idleTimeoutFloor is a driver that knows how long its own settle can take. +// Declared here rather than in the driver package (like lastActionInstaller in +// source.go) so the mobile drivers stay untouched. +type idleTimeoutFloor interface { + MinIdleTimeout() time.Duration +} + +// resolveIdleTimeout settles the per-step settle budget: the caller's value, +// defaulted when unset, and raised to whatever the driver says its own settle +// needs. The chrome driver's settle waits for the DOM to go quiet and only then +// opens its route-transition window; handed less than their sum it is cut off +// mid-transition, and the step samples the screen the app is leaving. A driver +// that reports no floor keeps the caller's value exactly. +func resolveIdleTimeout(options Options) time.Duration { + timeout := options.IdleTimeout + if timeout <= 0 { + timeout = defaultIdleTimeout + } + if floor, ok := options.Driver.(idleTimeoutFloor); ok { + timeout = max(timeout, floor.MinIdleTimeout()) + } + return timeout +} + // ensureForeground keeps the app under test in the foreground. When the driver // can report the foreground app and it no longer matches the bundle under test, // the app is relaunched. Returns true when a relaunch happened so the caller diff --git a/internal/runner/web_carrier_test.go b/internal/runner/web_carrier_test.go new file mode 100644 index 0000000..d4776a5 --- /dev/null +++ b/internal/runner/web_carrier_test.go @@ -0,0 +1,196 @@ +package runner + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/priyanshujain/sanderling/internal/driver" + mockdriver "github.com/priyanshujain/sanderling/internal/driver/mock" + "github.com/priyanshujain/sanderling/internal/trace" +) + +// carrierSpec registers one extractor whose value the page supplies. It stands +// in for every spec whose getters carry state across steps (folio's last-seen +// Home total, its submit counters): what matters is that ASKING the page for +// the value is what advances it. +const carrierSpec = ` +import { actions, extract } from "@sanderling/spec"; +const carrier = extract("carrier", () => 0); +globalThis.properties = {}; +globalThis.actions = actions(() => []); +` + +// carrierWebDriver is a web target that alternates between a cross-fading +// hierarchy (which the runner discards as transitional) and a settled one, and +// whose page-side extractor advances a counter on every evaluation - exactly +// what a spec-authored carrier does in V8. +type carrierWebDriver struct { + *mockdriver.Driver + transitional bool + snapshots int + reads int +} + +func (d *carrierWebDriver) Snapshot(ctx context.Context) (string, driver.Image, error) { + _, image, err := d.Driver.Snapshot(ctx) + d.snapshots++ + if !d.transitional { + return `{"attributes":{"resource-id":"HomeScreen"},"children":[]}`, image, err + } + // A genuine cross-fade: two live routes, and a tree that keeps changing + // between retries so the runner spends its whole retry budget on it. + return fmt.Sprintf(`{"attributes":{"resource-id":"root"},"children":[ + {"attributes":{"resource-id":"HomeScreen","text":"frame-%d"},"children":[]}, + {"attributes":{"resource-id":"LedgerScreen"},"children":[]} + ]}`, d.snapshots), image, err +} + +func (d *carrierWebDriver) InstallBundle(context.Context, []byte) error { return nil } + +func (d *carrierWebDriver) EvaluateExtractors(context.Context) (map[int]json.RawMessage, error) { + d.reads++ + return map[int]json.RawMessage{0: json.RawMessage(strconv.Itoa(d.reads))}, nil +} + +// NextActionFromV8 runs once per step, after the hierarchy fetch, so flipping +// here makes every other step a cross-fade. +func (d *carrierWebDriver) NextActionFromV8(context.Context) (json.RawMessage, error) { + d.transitional = !d.transitional + return json.RawMessage(`{"kind":"Tap","x":5,"y":5}`), nil +} + +func (d *carrierWebDriver) SetLastAction(context.Context, json.RawMessage) error { return nil } + +// TestRunner_TransitionalStepNeverAdvancesThePageCarrier pins the ordering the +// web path depends on. The page-side extractors must run only on steps the +// verifier accepts: their getters advance spec state every time they evaluate, +// so evaluating them on a step whose values are then discarded leaves the page +// one window ahead of the verifier. The next accepted pair then brackets two +// committed transactions while having counted one submit, and the property +// convicts an app that did nothing wrong. +func TestRunner_TransitionalStepNeverAdvancesThePageCarrier(t *testing.T) { + state := newHarnessWithSpec(t, carrierSpec) + web := &carrierWebDriver{Driver: state.mock} + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + summary, err := Run(ctx, Options{ + Duration: 30 * time.Second, + IdleTimeout: 20 * time.Millisecond, + MaxSteps: 5, + Driver: web, + Verifier: state.verifier, + TraceWriter: state.writer, + }) + if err != nil { + t.Fatalf("Run: %v", err) + } + if summary.Steps != 5 { + t.Fatalf("steps = %d, want 5", summary.Steps) + } + + type traceLine struct { + Step int `json:"step"` + Transitional bool `json:"transitional"` + ExtractorChanges map[string]trace.ExtractorChange `json:"extractor_changes"` + } + body, err := os.ReadFile(filepath.Join(state.writer.Directory(), "trace.jsonl")) + if err != nil { + t.Fatal(err) + } + verified, transitional := 0, 0 + previous := 0 + for _, raw := range bytes.Split(bytes.TrimSpace(body), []byte("\n")) { + var line traceLine + if err := json.Unmarshal(raw, &line); err != nil { + t.Fatalf("decode trace line: %v", err) + } + if line.Transitional { + transitional++ + continue + } + verified++ + change, ok := line.ExtractorChanges["carrier"] + if !ok { + t.Fatalf("step %d: no carrier value reached the verifier", line.Step) + } + current, convErr := strconv.Atoi(string(change.Curr)) + if convErr != nil { + t.Fatalf("step %d: carrier value %s: %v", line.Step, change.Curr, convErr) + } + if current != previous+1 { + t.Errorf("step %d: carrier went %d -> %d; the page advanced it on a "+ + "step the verifier discarded, so the verifier's window is wider "+ + "than the one the spec counted actions over", + line.Step, previous, current) + } + previous = current + } + if verified == 0 || transitional == 0 { + t.Fatalf("need both kinds of step to prove anything: %d verified, %d transitional", + verified, transitional) + } + if web.reads != verified { + t.Errorf("the page evaluated its extractors %d time(s) across %d verified step(s); "+ + "every evaluation the verifier does not use still advances spec state", + web.reads, verified) + } +} + +// installFailsWebDriver is a web target whose page cannot take the runner's +// lastAction: an older published @sanderling/spec runtime, a bundle that never +// installed, a tab that navigated away from it. +type installFailsWebDriver struct { + *mockdriver.Driver +} + +func (d *installFailsWebDriver) InstallBundle(context.Context, []byte) error { return nil } + +func (d *installFailsWebDriver) EvaluateExtractors(context.Context) (map[int]json.RawMessage, error) { + return map[int]json.RawMessage{0: json.RawMessage(`1`)}, nil +} + +func (d *installFailsWebDriver) NextActionFromV8(context.Context) (json.RawMessage, error) { + return json.RawMessage(`{"kind":"Tap","x":5,"y":5}`), nil +} + +func (d *installFailsWebDriver) SetLastAction(context.Context, json.RawMessage) error { + return errors.New("__sanderlingSetLastAction__ is not a function") +} + +// TestRunner_LastActionInstallFailureFailsTheRun covers the other half of the +// same trust boundary. A run that cannot install lastAction in the page cannot +// apply the page's extractor values either, so the step keeps goja's +// dump-derived readings while the step before it holds the page's, and a delta +// property compares two producers and fires. Downgraded to a warning that is a +// green run reporting a violation nobody can reproduce. +func TestRunner_LastActionInstallFailureFailsTheRun(t *testing.T) { + state := newHarnessWithSpec(t, carrierSpec) + web := &installFailsWebDriver{Driver: state.mock} + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + _, err := Run(ctx, Options{ + Duration: 2 * time.Second, + IdleTimeout: 20 * time.Millisecond, + MaxSteps: 3, + Driver: web, + Verifier: state.verifier, + TraceWriter: state.writer, + }) + if err == nil { + t.Fatal("Run succeeded with a page that cannot take lastAction; the run " + + "reported green while its extractor values came from two engines") + } + if !bytes.Contains([]byte(err.Error()), []byte("install last action")) { + t.Errorf("Run error = %v, want it to name the failed lastAction install", err) + } +}