Skip to content

test(browser): wait for the app's own quota cooldown before retrying - #284

Merged
wesbillman merged 2 commits into
mainfrom
larry/cooldown-browser-clock
Sep 26, 2026
Merged

wesbillman merged 2 commits into
mainfrom
larry/cooldown-browser-clock

Conversation

@loganj

@loganj loganj commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

🤖

Summary

  • Fixes an intermittent browser-test failure. After a rate-limit ("quota") refusal, the test clicked Retry live updates at the time the cooldown should have ended, but the button sometimes stayed on screen.
  • Cause: the test measured the cooldown on the test machine's real clock, starting when the fake relay refused. The app starts its own cooldown later, when the browser processes the refusal. That happens after the broker hop and whenever the busy renderer gets to it. If the test's single recovery click arrived first, the app ignored it, and the test failed.
  • Now the two live retry tests use Playwright's page clock. After the error appears and the ignored clicks are checked, the test advances the browser clock by the full advertised delay. The app sets its deadline before it shows the error, so this always passes that deadline, however late the browser processed the refusal.

Details

  • The broker (the local server between the app and the relay) also pauses its own request queue for the advertised delay, on the real clock. It sets that pause before it finishes sending the refusal. The fixture records the finish time, and the tests wait until exactly that time plus the delay, with no extra margin. This keeps the broker from refusing the retry.
  • The older-page retry in history-loading.spec.mjs has no cooldown in the browser. Only the broker holds it, so that test waits for the broker bound only.
  • The existing before- and after-retry request-count, route, and reading-position assertions are unchanged. No test cases or engines are removed. No product code changes.

Evidence

All local runs are on Chromium unless noted, using playwright test --config tests/browser/playwright.config.mjs --no-deps on the quota retry tests in live.spec.mjs and history-loading.spec.mjs (--repeat-each 3).

Injected delay (test-only patch) main Server timestamp + 250 ms (first version of this PR) This PR
400 ms between relay refusal and broker response 6 of 9 fail 9 of 9 pass 9 of 9 pass
400 ms between browser fetch receiving the 429 and the app reading it — 3 of 9 fail (live.spec.mjs:32, Retry button still visible) 9 of 9 pass
  • Without injection, both full files pass 40 of 40 on Chromium and 40 of 40 on WebKit (--repeat-each 5).
  • Timing, Chromium, one worker, three runs each, compared with the first version of this PR: live.spec.mjs:32 8.7–8.9 s → 7.8–7.9 s, the roster retry 6.3–6.5 s → 5.4–6.4 s, and the older-page retry 5.5–5.6 s → 4.4–4.6 s. The page clock replaces up to 3 s of real waiting.
  • Not checked: the browser-side delay injection was not run on WebKit.

@loganj
loganj force-pushed the larry/cooldown-browser-clock branch 3 times, most recently from 5f4eee4 to 28665d6 Compare September 25, 2026 19:31
@loganj
loganj marked this pull request as ready for review September 25, 2026 19:45
@loganj
loganj requested review from a team, comp615 and wesbillman as code owners September 25, 2026 19:45

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Star Lord’s automated source review via Wes’s account — non-blocking COMMENT, not approval or merge authorization.

Reviewed head 28665d6ec98f4b79b082ce1b5b642c64d6c3fd3b against base/merge-base 4012979b1d200c3793714d7d7438e6dcd5231668.

One actionable finding (P2), inline: the new cooldown predicate still uses a server-side timestamp plus a scheduling margin rather than the browser-owned retry boundary. Moving the timestamp past broker work improves the reported upstream-delay case, but does not remove the remaining race.

The four-file change retains the existing recovery, request-count, healthy-route and reading-position assertions; no browser cases or engines are removed. I traced the three callers, fixture response lifecycle, quota normalization, and session/store retry ownership. No additional actionable findings in that scope.

Validation/evidence: source-only; I did not run PR code, tests, builds or the app. One read-only CI snapshot shows all automatic lanes successful in run 36180219561. Its reports identify merge snapshot b6ffa8c872003133abe8a4152df2a340e4ad4faf (this head/base), Ubuntu, Playwright 1.63.0, two workers and zero retries. Both affected files passed in Chromium/WebKit shard 2: each shard reports 98/98 passing; wrapper elapsed time 271.73s/374.42s and summed test execution 521.53s/724.44s respectively. Slowest file in each was navigation-sidebar.spec.mjs (72.74s/98.52s). These are current-run observations, not a matched before/after cost comparison or proof against the remaining ordering. The PR’s injected-delay/repeat results and estimated added wait were not independently reproduced. Native/manual acceptance and a controlled browser-consumption-delay check remain unverified.

Comment thread tests/browser/policy-relay.mjs Outdated
Larry added 2 commits September 25, 2026 19:50
Quota retry tests waited until the fake relay's cooldown ended plus
50 ms. The app starts its cooldown only when the refusal reaches the
browser, after the broker hop, so under CI load the retry click could
land while the app still refused and the Retry button stayed visible.

Record when the broker finishes relaying each quota refusal and wait for
that time plus the advertised retry delay plus 250 ms.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
The app starts its retry deadline when it consumes the refusal, which a
busy renderer can do well after the broker finishes the response. A
server timestamp plus a margin could still let the only recovery click
land early.

Install the page clock in the two live retry tests and advance it by the
full advertised delay after the ignored clicks. The app's deadline is
set before the error is shown, so this always crosses it. The broker's
own admission pause runs in real time and is set before the response
finishes, so wait for exactly that bound, with no margin. Older-page
retries have no browser-side deadline and use the broker bound only.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
@loganj
loganj force-pushed the larry/cooldown-browser-clock branch from 28665d6 to 9492d3b Compare September 25, 2026 23:50

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Star Lord automated source follow-up

Published through Wes’s GitHub account (wesbillman); this is a non-blocking COMMENT, not Wes’s manual approval or merge authorization.

The prior P2 cooldown-ordering finding is addressed; no new actionable source defects found in this follow-up.

Reviewed head 9492d3b1d45651a0a9e9c6e5fdd8df65c7fa2678 against base/merge-base 734949a62229c56b2e3368701a4b4d5413ac555a, following review of 28665d6ec98f4b79b082ce1b5b642c64d6c3fd3b.

  • tests/browser/live.spec.mjs:22–32, 95–106, 174–202: the browser clock is installed before navigation, and the full advertised delay is advanced after the visible quota error and ignored retry clicks. The actual catch-up and roster owners set their performance.now() deadlines before publishing that error (session.ts:1783–1804, store.ts:881–919). This removes the old assumption that server response finish plus 250 ms bounds browser consumption.
  • fixture.mjs:1382–1391 and policy-relay.mjs:104–112: response finish remains a conservative bound for the separate real-time broker pause. admittedApiRequest sets that pause before returning the refusal; the older-page retry has no additional browser cooldown and uses this broker-only bound.
  • The four-file change is test-only (35 additions, 10 deletions). No browser cases or engines are added/removed; the eight cases across the affected spec files remain, including request-count, healthy-route, recovery and reading-position assertions.

Validation limits: source analysis only; no tests, builds or app workflows were executed. Pinned source extracts were verified against Git blobs and the diff whitespace check passed. The PR reports delayed-consumption controls and repeated Chromium/WebKit file passes, but I did not reproduce those results or establish their exact checked snapshots. The one current-head CI snapshot showed the JavaScript, Rust and browser jobs still running; DCO and security checks had passed. Completed job-summary counts, wall/summed execution times and slowest-file evidence were therefore unavailable at that snapshot; no independently verified performance comparison or final CI/human acceptance is claimed. The author explicitly leaves the browser-consumption-delay injection on WebKit untested. No CI polling was performed.

@wesbillman
wesbillman merged commit 401fb8d into main Sep 26, 2026
14 checks passed
@wesbillman
wesbillman deleted the larry/cooldown-browser-clock branch September 26, 2026 13:49
zrmarley added a commit that referenced this pull request Sep 28, 2026
…ad-on-send

* origin/main: (58 commits)
  Keep profile avatar cutouts transparent and align the header gutter (#319)
  Restore sidebar status icons beside names (#316)
  docs(mentions): specify portable mention rules (#343)
  fix(agents): wait for native host operations (#331)
  Simplify channel templates and report setup failures accurately (#318)
  feat(agents): Harnesses Goose install (slice 3/5) (#279)
  feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277)
  Fix timer operation ownership and stabilize timing regressions (#317)
  Restore cached workspace before relay startup (#311)
  test(browser): wait for the app's own quota cooldown before retrying (#284)
  docs: define Harnesses setup and global agent defaults (#272)
  Make mention choices consistent and stable (#258)
  Discover saved relay agents without changing the page (#224)
  feat: add persistent dev log levels and relay traffic summaries (#306)
  Polish inline message reactions and previews (#213)
  feat(identity): add native macOS import, creation and backup (#308)
  fix(status): reopen a Today status as Today near 16:00 (#275)
  test: use current navigation for GIF send roundtrip (#309)
  Fix composer focus when selecting channels and DMs (#307)
  fix: retire mention searches after chips and refuted prose (#303)
  ...

# Conflicts:
#	src/features/messages/MessageComposer.test.tsx
#	src/features/messages/MessageComposer.tsx
johnmatthewtennant pushed a commit that referenced this pull request Sep 28, 2026
* origin/main: (45 commits)
  Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322)
  perf(messages): index the emoji catalog for reaction lookups (#333)
  Polish search palette and add conversation search (#340)
  Use step-ten avatar colors with contrasting outlines (#320)
  Keep profile avatar cutouts transparent and align the header gutter (#319)
  Restore sidebar status icons beside names (#316)
  docs(mentions): specify portable mention rules (#343)
  fix(agents): wait for native host operations (#331)
  Simplify channel templates and report setup failures accurately (#318)
  feat(agents): Harnesses Goose install (slice 3/5) (#279)
  feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277)
  Fix timer operation ownership and stabilize timing regressions (#317)
  Restore cached workspace before relay startup (#311)
  test(browser): wait for the app's own quota cooldown before retrying (#284)
  docs: define Harnesses setup and global agent defaults (#272)
  Make mention choices consistent and stable (#258)
  Discover saved relay agents without changing the page (#224)
  feat: add persistent dev log levels and relay traffic summaries (#306)
  Polish inline message reactions and previews (#213)
  feat(identity): add native macOS import, creation and backup (#308)
  ...

Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>

# Conflicts:
#	src/bundled/agents/AgentCard.tsx
#	src/bundled/agents/AgentsPage.tsx
johnmatthewtennant pushed a commit that referenced this pull request Sep 28, 2026
* origin/main: (36 commits)
  Delay message timestamp tooltips by 500 ms (#321)
  Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322)
  perf(messages): index the emoji catalog for reaction lookups (#333)
  Polish search palette and add conversation search (#340)
  Use step-ten avatar colors with contrasting outlines (#320)
  Keep profile avatar cutouts transparent and align the header gutter (#319)
  Restore sidebar status icons beside names (#316)
  docs(mentions): specify portable mention rules (#343)
  fix(agents): wait for native host operations (#331)
  Simplify channel templates and report setup failures accurately (#318)
  feat(agents): Harnesses Goose install (slice 3/5) (#279)
  feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277)
  Fix timer operation ownership and stabilize timing regressions (#317)
  Restore cached workspace before relay startup (#311)
  test(browser): wait for the app's own quota cooldown before retrying (#284)
  docs: define Harnesses setup and global agent defaults (#272)
  Make mention choices consistent and stable (#258)
  Discover saved relay agents without changing the page (#224)
  feat: add persistent dev log levels and relay traffic summaries (#306)
  Polish inline message reactions and previews (#213)
  ...

Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>

# Conflicts:
#	src/bundled/agents/AgentEditor.tsx
#	src/bundled/profiles/ProfileAgentIdentity.test.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants