fix(protocol): enforce a call-time request budget with phase attribution - #168
Open
ryanleecode wants to merge 18 commits into
Open
fix(protocol): enforce a call-time request budget with phase attribution#168ryanleecode wants to merge 18 commits into
ryanleecode wants to merge 18 commits into
Conversation
… wait `postRequest` armed its per-method timer only after awaiting the shared protocol frame, and that wait carries its own budget of up to 240 seconds. A first request could therefore block for roughly four and a half minutes while advertising a 30 second contract, and the eventual rejection did not say whether the time went into booting the frame or waiting for a reply. The per-method budget is now resolved before the frame wait and enforced by a single timer armed at call time, raced against the frame wait and then the reply wait. Rejections carry a typed error naming which wait spent the budget: opening the host frame, waiting for the frame to report ready, or waiting for a reply. A more specific frame failure still wins when it settles first, so a torn-down frame or a dead chain reports its own cause. No timeout constant changed. `warmup` stays exempt, so a legitimate cold boot keeps its full window through the untimed warm-up and the direct frame call that drive it.
The budget exposed `enterPhase` and `guard` as separate calls, so a caller had to remember to announce the phase before the wait it belonged to. The phase is now an argument to `guard`, which makes the pairing impossible to get wrong and lets a rejection name only a wait the request was actually in. Also drops the nullable request-id local. The pending entry is dropped in the one place a reply can no longer arrive, so the outer block no longer carries state whose only purpose was a conditional cleanup.
Three defects the review pass found in the first cut, plus the coverage that proves each one. A chain connection awaited the protocol frame outside any budget and only then issued its own request, so a dApp connecting during a cold boot still waited out the frame budgets before the connection's 30 second allowance even started. The request already performs both frame waits, so the outer wait was redundant as well as unbounded. A failed request recorded a duration sample. The budget starts at the call while that timer starts at the reply, so a timeout wrote the leftover budget into a series with no outcome to filter on, and a 90 second failure could land as a 3 second sample. Only a completed roundtrip is recorded now, matching what the fatal and response-error paths already did. The host mapped a protocol timeout to its user-facing copy by matching the message text, which this branch reworded. It now matches on the error type, keeping the text match for the foreign errors that still need it. The suite was blind to two invariants: deleting the timer disarm or the pending-entry cleanup left every test passing. Six mutations are now each caught by a named scenario, including a crash relabelled as a timeout and warmup handed a budget it must not have. Intent moved out of comments and into names and structure: the untimed and budgeted paths are separate functions rather than one branch with a warning, and the request timer says what it records.
The defect generalises past this fix: a per-operation budget armed after an unbounded setup wait advertises a bound it does not hold. Records the two rejected alternatives with their reasons, and the mutation-probe result that matters most - the suite was fully green with the timer disarm deleted and again with the pending-entry cleanup deleted, so a green run was not evidence either invariant was defended. Seeds CONCEPTS.md for the protocol-frame area, including the host-frame versus protocol-frame distinction that names two readiness stages of one iframe rather than two elements.
Contributor
⚡ Performance Report
|
Contributor
Bundle Size ReportChunks over 500 KB:
All files
Commit: 6334e5d |
… DSL Split monolithic client.test.ts into domain-focused suites (timeouts, error precedence, chain provider) using a reusable dual-driver harness (DAppDriver / ProtocolFrame) and Rpc factory. Expand parameterized test coverage and add package AGENTS.md.
ryanleecode
force-pushed
the
fix/protocol-request-call-time-budget
branch
from
August 14, 2026 22:29
07194d7 to
ecaab14
Compare
ryanleecode
marked this pull request as ready for review
August 14, 2026 22:35
ryanleecode
requested review from
BigTava,
Cyr06130,
KarimJedda,
ba1uev,
justraman,
leonardocustodio and
tallesborges
as code owners
August 14, 2026 22:35
Contributor
|
❌ E2E Product suite failed on Failed tests:
Logs: https://github.com/paritytech/dotli-community/actions/runs/31856154758 |
… on frame teardown resetProtocolFrameState now drains pendingRequests so a teardown between request and reply rejects the caller immediately instead of orphaning it until its budget timer fires; the fatal/init-failed handler reuses that drain instead of duplicating the loop. bindMessageListener drops messages whose source is not the mounted frame window, closing the gap where a valid-origin message with no frame mounted passed the old null check.
A ProtocolRequestTimeoutError spent in the load or ready phase means the light client timed out during startup (presync exceeded the request budget), not that the host lost its peers. Map those phases to SW_TIMED_OUT before the generic timeout branch.
createProviderHarness moves to tests/support/broker.ts so the routing property-style suite and example tests share one harness. errors.test.ts mirrored constructor parameters and could not fail on a plausible domain bug; thrown error attributes are already asserted on live rejection paths. AGENTS.md now states the testing doctrine in prose.
ryanleecode
changed the title
… and error mapping
…ad event The source-window guard read `protocolIframe`, which was only assigned in the iframe's load handler — several ticks after the frame was appended to the DOM. A frame that posted `ready` while its document was still parsing therefore had its own handshake discarded as untrusted, so the protocol never became ready, resolution never settled, and the host rendered no error page at all. Eight functional loading scenarios timed out waiting for `.error-page-title`; the ones that survived did so only because their mock retried the post twelve times with backoff. Trust is now established when the frame is attached rather than when it loads, and revoked on the load-timeout and error paths so a dead frame never stays trusted. Post-teardown discarding is unchanged.
…guard The previous test hardcoded `http://host.localhost:5173` as the message origin, but the protocol origin resolves to port 3000. The origin check rejected the event before it ever reached the source-window check, so the test passed under both the strict and the permissive guard and defended nothing — reverting the guard left the whole suite green. It now calls `getProtocolOrigin()` and asserts the exploit the guard exists to stop: an untrusted window forging a shared-auth broadcast into every subscriber after teardown. Reverting the guard fails this test.
ryanleecode
force-pushed
the
fix/protocol-request-call-time-budget
branch
from
August 15, 2026 01:18
8747e8c to
9c611d5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
postRequestpreviously advertised a per-method timeout contract (30s default, 90s for name/owner lookups) but only began counting after the shared protocol iframe finished loading and became ready. Because the frame bootstrap carries its own allowances (IFRAME_LOAD_TIMEOUT_MS = 30_000,IFRAME_READY_TIMEOUT_MS = 240_000), cold-start or stalled frame requests could block callers for up to ~5 minutes while promising a 30s deadline — without reporting whether time was spent booting the iframe or waiting on an RPC reply.A unified request budget (
startRequestBudget) is now armed at the call site and raced across each sequential lifecycle wait (load,ready,reply). Timeouts reject with a typedProtocolRequestTimeoutErrorattributing the exact in-flight phase (load,ready, orreply). In addition, frame teardown now rejects in-flight requests immediately instead of orphaning them, message listener guards discard messages from non-frame sources, the host shell reports startup-phase timeouts accurately, the protocol test harness is refactored into a dual-driver domain DSL (DAppDriverandProtocolFrame), and package testing doctrine is codified inpackages/protocol/AGENTS.md.Fixes #166
What Changed
startRequestBudgetinitializes a single timer before awaiting frame setup.RequestBudget.guard(phase, work)tags the active lifecycle phase and races each step against the shared deadline.ProtocolRequestTimeoutErrorcapturesmethod,timeoutMs, andphase(load,ready, orreply), read directly when the timer fires so callers and telemetry distinguish exactly where time was spent.createRemoteChainProviderinvokespostRequest("chainConnect")directly instead of pre-awaitingensureProtocolFrame(), eliminating redundant 30s + 240s + 30s (~300s) timeout cascades on cold starts while maintaining queueing and error flushes.resetProtocolFrameStatenow drains and rejects allpendingRequestsimmediately with"Protocol frame state reset before reply", eliminating orphaned promises on post-ready frame teardowns.bindMessageListenerdrops messages when!frameWindow || event.source !== frameWindow, ensuring post-teardown or spoofed window messages with valid origins are discarded.protocol.request. Timeouts emit only counter samples tagged by phase, preventing partial-window residuals from skewing p95/p99 latency distributions.apps/host/src/errors.ts:describeErrormatchesProtocolRequestTimeoutErrorby type, and mapsloadandreadyphase timeouts toHOST_ERRORS.SW_TIMED_OUT("The light client timed out during startup.") instead of generic peer-loss copy.packages/protocol/tests/using a clean DSL (tests/support/{dapp,frame,rpc,broker,time,index}.ts), eliminating inline JSON parsing, raw wire inspection, and magic tick yields in test bodies.packages/protocol/tests/errors.test.ts(constructor parameter echoing; thrown error properties are asserted on live rejection paths).packages/protocol/AGENTS.mdspecifying package testing doctrine in clean prose: dual-driver test doubles, domain getter assertions, virtual timer synchronization, and contract defense.Design Decisions and Tradeoffs
setTimeoutraced per phaseDate.now()deadline arithmetic: Rejected because wall-clock shifts (NTP steps, system sleep/resume) distort time boundaries.loadphase reachable.IFRAME_LOAD_TIMEOUT_MS: Rejected because shrinking frame allowances globally breaks independent timeout contracts.spentOninside the timer callback guarantees the rejection accurately attributes the exact wait currently in flight.protocol.requestis an attribute-less latency metric. Because the budget timer begins at the call while the request timer begins at the reply phase, recording a timeout would sample only the residual duration (e.g. 3s left out of 90s), artificially skewing latency downward.@std/asyncrelease(), preserves custom phase attribution, and avoids external dependencies.@std/async(deadline/abortable):deadline()creates a new timer per call (reintroducing the reset bug);abortable()withAbortSignal.timeoutleaks active timers on early resolution.Review Notes: Behavioral Impact
resolveDotNameRemote), the request will fail at 90s instead of waiting up to ~5 minutes for presync to finish. Automatic retries are not performed on this path.Test Plan
packages/protocol/tests/(99 tests across 6 test files, all passing):client-timeouts.test.ts: Parameterized matrix verifying request budgets acrossload,ready, andreplyphases, two-sided timer boundary checks (remainingWaitMs - 1still pending), timer cleanup on success, untimedwarmup, and cross-tab storage change notification relays.client-precedence.test.ts: Parameterized matrix verifying that explicit frame errors (ProtocolFatalError, frame resets before ready, frame resets after ready, iframe load errors) take precedence over budget timeouts when settling first.client-chain-provider.test.ts: Connection lifecycle, request queuing before frame boot, chain response routing,chainSendfailure propagation, cleandisconnect(),chain-halthandling, and post-teardown message discard.auth-storage.test.ts: Origin allowlist edge cases, key validation, and siteId constraints.broker.test.ts: Broker request remapping, subscription token rewriting, early event buffering, statement fanout, and disconnect cleanup.messages.test.ts: Protocol envelope serialization and validation.client-timeouts.test.ts.client-timeouts.test.ts.client-chain-provider.test.ts.client-timeouts.test.ts.client-timeouts.test.ts.client-precedence.test.ts.client-precedence.test.ts.bun run --cwd packages/protocol test(99 passed).bun run --cwd packages/protocol typecheck(clean).bun run --cwd packages/protocol lint(clean).bunx --bun turbo run typecheck lint test(37/37 tasks successful monorepo-wide).