paritytech / paritytech/dotli-community

Protocol request timeout starts after the frame-ready wait, not at the call

Open
#166 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
TypeScript
Stars
4
Forks
3
Avg merge
1d 8h
Merged PRs (30d)
40

Description

Problem Statement

postRequest applies a per-method timeout, 30 seconds by default, and callers reasonably read that as the bound on the call. The timer is only created after it awaits the shared protocol iframe becoming ready, and that wait carries its own 240-second budget. So the first request after a cold or wedged protocol frame can block for roughly four and a half minutes while reporting a 30-second contract, and the eventual rejection does not say whether the time went into booting the frame or waiting for a reply.

Goal

A protocol request rejects within its own documented per-method budget measured from the moment it is called, including any time spent waiting for the host or protocol frame to become ready.

Evidence: the await precedes the timer

packages/protocol/src/client.ts:497-521

async function postRequest<M extends ProtocolRequestMethod>(
  method: M,
  payload: ProtocolRequestMap[M],
  onProgress?: (message: string) => void,
  needsProtocolReady = !isSharedAuthRequestMethod(method) &&
    !isSharedModeRequestMethod(method),
): Promise<unknown> {
  await (needsProtocolReady ? ensureProtocolFrame() : ensureHostFrame());
  // ...
  const timeoutMs = UNTIMED_METHODS.has(method)
    ? null
    : (METHOD_TIMEOUTS[method] ?? DEFAULT_TIMEOUT_MS);

The setTimeout that enforces timeoutMs is created at :528, inside the new Promise at :524 — after the await on line 504 has already returned.

Evidence: the budgets that stack

packages/protocol/src/client.ts:317, :321, :481, :487-488

const IFRAME_LOAD_TIMEOUT_MS = 30_000;
const IFRAME_READY_TIMEOUT_MS = 240_000;   // must exceed TIMEOUTS.SHARED_WORKER_READY
const DEFAULT_TIMEOUT_MS = 30_000;
const UNTIMED_METHODS: ReadonlySet<ProtocolRequestMethod> =
  new Set<ProtocolRequestMethod>(["warmup"]);

Worst case for a needsProtocolReady method is the 240s ready wait plus the 30s request budget. warmup is the only method exempted from a request timeout, so no account, session, or auth method is exempt by design.

Orientation

  • packages/protocol/src/client.tspostRequest, ensureProtocolFrame, ensureHostFrame, the ready-waiter list pendingReadyResolvers (:440), and the load/ready rejections at :352-353 and :437-438.
  • There is already a fast-fail path that rejects ready-waiters immediately when the chain is known dead rather than letting them run the full budget (:144-145, :242-246) — the same idea, applied to one cause.
  • Shared-auth and shared-mode methods pass needsProtocolReady=false and await only ensureHostFrame(), so they are exposed to the 30s load bound rather than the 240s one. Both paths need accounting.
  • METHOD_TIMEOUTS (:489) already carries per-method overrides, e.g. chainConnect: 30_000.

Non-Counting Outcomes

  • Shrinking IFRAME_READY_TIMEOUT_MS instead of making the request budget cover the wait. The comment at :318-321 says that budget must exceed TIMEOUTS.SHARED_WORKER_READY, so cutting it breaks a legitimate cold boot to make a metric look better.
  • Starting the timer before the await but leaving the ensure path to reject with its own unrelated error, so a caller still cannot attribute the elapsed time.
  • Fixing only the needsProtocolReady=true branch and leaving shared-auth and shared-mode requests unaccounted, since those are exactly the session and preference reads that run at boot.
  • A test using fake timers that asserts rejection after the budget without ever driving a frame that is slow to signal ready — it verifies arithmetic, not the ordering defect.
  • Making warmup timed to simplify the change; :482-486 gives a stated reason it is exempt.

Acceptance Criteria

  • (gatekeeper) A test drives a protocol frame that never signals ready and asserts a non-warmup request rejects within its per-method budget measured from call time; it fails when the change is reverted. Name the test file in packages/protocol.
  • pnpm test in packages/protocol exits 0.
  • The rejection distinguishes "budget spent waiting for the frame to become ready" from "budget spent waiting for a reply".
  • Both branches of needsProtocolReady are covered by tests.
  • warmup remains exempt from a request timeout.
  • No existing timeout constant is reduced to satisfy the bound.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in packages/protocol/src/client.ts at postRequest, ensureProtocolFrame, ensureHostFrame, and the ready-waiter handling. Trace both needsProtocolReady branches and the existing timeout and rejection paths, then add tests in packages/protocol covering a frame that never becomes ready, reply waiting, and warmup; run pnpm test in packages/protocol. Done means both branches enforce the method budget from call time and distinguish readiness from reply timeout without changing existing timeout constants.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
networking, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.