minifront: retry status stream #2080

turbocrime · 2025-02-25T01:28:23Z

Description of Changes

Retry the status stream in minifront, when it fails due to #2074

zquery doesn't really have a stream-retry this feature, so state management in the slice and in the zquery stream callbacks will collect the stream status for interested components, and schedule re-fetch attempts.

Checklist Before Requesting Review

I have ensured that any relevant minifront changes do not cause the existing extension to break.

changeset-bot · 2025-02-25T01:28:28Z

🦋 Changeset detected

Latest commit: 2da7a35

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
minifront	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

turbocrime · 2025-02-25T19:30:09Z

apps/minifront/src/fetchers/status.ts

initial status is now called independently of the status stream, to disambiguate initial (local) status returns and remote status returns.

turbocrime · 2025-02-25T19:30:25Z

apps/minifront/src/fetchers/status.ts

+): AsyncGenerator<PlainMessage<StatusStreamResponse>> {
+  for await (const item of penumbra
+    .service(ViewService)
+    .statusStream({}, { timeoutMs: 15_000, ...opt })) {


a timeout of 15 seconds is applied, unless the caller overrides with a different value.

suggestion: can we add a comment here to the effect of "the timeout specifies how long the client will wait for a response before considering the streaming request failed"?

apps/minifront/src/state/status.ts

turbocrime · 2025-02-27T04:02:14Z

apps/minifront/src/components/syncing-dialog/index.tsx

dialog is significantly modified

comment: the dialogue feels too cramped – can we revert to the previous dialogue (with the sync percentage) while keeping the message "Streaming new blocks to update private state"?

before

after

thoughts? cc @smmhrdmn

sure, no aesthetic preference here, i was primarily seeking to have the displayed status reflect the new state information

TalDerei

noticeable zquery improvements! I’ve left some initial observations

TalDerei · 2025-02-27T01:03:28Z

apps/minifront/src/components/header/header.tsx

  const [chainId, setChainId] = useState<string | undefined>();
+  const initialStatus = useInitialStatus();
+  const status = useStatus();
+  const { error: streamError } = useStore(statusStreamStateSelector);


suggestion: we should abstract these stream stalled and signal timeout errors from surfacing in the modal.

Untitled.mov

it's important to display some kind of failure to the user, when the request has failed.

if you want to conceal the failure message, can you suggest a replacement?

should different failures use different replacements? this quickly gets complex.

maybe a more general "connection issue detected" message, rather than anything specific to streams?

TalDerei · 2025-02-27T01:14:04Z

apps/minifront/src/components/syncing-dialog/index.tsx

+    if (streamError) {
+      setDialogText('Retrying...');
+    } else if (!initialStatus.data) {
+      setDialogText('Querying local block height...');
+    } else if (!status.data) {
+      setDialogText('Fetching remote block height...');
+    } else if (!isSynced) {
+      setDialogText('Streaming new blocks to update private state...');


comment: It switches quickly between "querying local block height" and "fetching remote block height", so I think we should synthesize the messaging for these states rather than distinguishing between them.

Screen.Recording.2025-02-25.at.5.53.11.PM.mov

these are distinct states.

the cases you've tested involve a quick transition, and it is often a quick transition, but the lack of distinction between these states is at the root of the issue in #2081.

specifically, the 'Querying local block height' state is the 'initial status' unary request that will pend and possibly time out if the extension is connected, but taking a long time to respond to RPC queries.

i don't think making these states ambiguous is a good idea. there is no downside to making these states distinct. the upside is that users are more informed of information that is highly useful for issue reports and investigation.

i'm disinclined to make this change. please suggest a different dialog message if you think it could be better

TalDerei · 2025-02-27T01:19:30Z

apps/minifront/src/components/syncing-dialog/index.tsx

+            {dialogText}
+            {!!streamError && (
+              <Text technical color={theme => theme.caution.main} as='div'>
+                {streamError instanceof Error ? streamError.message : String(streamError)}


comment: clearing the cache surfaces some interesting errors

Screen.Recording.2025-02-26.at.6.18.15.PM.mov

clearing the cache through this menu will restart the extension service worker. once the extension service worker is restarted, the page content scripts lose context and it's not possible to recover from that without a reload.

ideally this loss of context would be transmitted to the page, but the data clone error you see is the #2070 issue resolved by #2071 so unless your prax build in this review included those changes, that is expected to be present as a data clone error.

eventually this should be addressed with a more comprehensive attempt at addressing context loss.

i would like to consider this case out of scope for this PR, because no part of minifront handles this case well, and it's a significant effort to address, with many considerations.

TalDerei · 2025-02-27T01:23:32Z

apps/minifront/src/state/status.ts

-  fullSyncHeight?: bigint;
-  latestKnownBlockHeight?: bigint;
+// Time in milliseconds to wait before attempting to reconnect the status stream
+const RECONNECT_TIMEOUT = 5_000;


comment: do we wants a fixed connection timeout rather than an exponential backoff? I think the former may be more preferable here as implemented.

attempting an exponential backoff implementation here would become significantly more complex, due to the fact that the query is not a single awaited promise over which we have direct control, but a zquery stream.

i agree backoff would be better, but this PR is essentially a spike to land a single case of stream re-request without getting deep into zquery. any more than this becomes significant zquery work.

i would like to consider new zquery features to be out of scope for this PR.

these are low-cost queries, so i don't think it's a big deal.

TalDerei · 2025-02-27T17:03:11Z

apps/minifront/src/fetchers/status.ts

+): AsyncGenerator<PlainMessage<StatusStreamResponse>> {
+  for await (const item of penumbra
+    .service(ViewService)
+    .statusStream({}, { timeoutMs: 15_000, ...opt })) {


suggestion: can we add a comment here to the effect of "the timeout specifies how long the client will wait for a response before considering the streaming request failed"?

apps/minifront/src/state/status.ts

TalDerei · 2025-02-27T18:23:51Z

apps/minifront/src/state/status.ts

+        });
+      },
+
+      scheduleRefetch: () => {


comment: potential race condition here? scheduleRefetch checks if streamState.running is false before attempting reconnection, however what if the stream status changes between when the timeout is set and when it executes?

if the stream is running when the timeout executes, a reconnect is not necessary.

apps/minifront/src/state/status.ts

TalDerei · 2025-02-27T18:28:31Z

apps/minifront/src/state/status.ts

+        ...item,
+      };
+    },
+    onError: (prevData, error) => {


comment: what's the difference here between onError versus onEnd in terms of reconnections?

according to zquery documentation, onEnd executes on every stream end. onError executes before onEnd, if there is an error from the fetch.

there is another method, onAbort, which i investigated but it does not seem to be involved in the cases we care about here.

apps/minifront/src/state/status.ts

turbocrime requested a review from TalDerei February 25, 2025 01:28

turbocrime force-pushed the minifront-retry-status-stream branch from ff631c6 to 6205c07 Compare February 25, 2025 18:03

turbocrime marked this pull request as ready for review February 25, 2025 18:03

turbocrime force-pushed the minifront-retry-status-stream branch 3 times, most recently from a218fb9 to 0e136ec Compare February 25, 2025 20:00

turbocrime changed the title ~~minifront retry status stream~~ minifront: retry status stream Feb 25, 2025

turbocrime force-pushed the minifront-retry-status-stream branch from 41b4184 to b46e852 Compare February 25, 2025 20:20

turbocrime force-pushed the transport-dom-stream-timeout branch from 155418d to fa86717 Compare February 25, 2025 22:07

turbocrime force-pushed the minifront-retry-status-stream branch from b46e852 to daecbfa Compare February 25, 2025 22:09

turbocrime force-pushed the transport-dom-stream-timeout branch from fa86717 to d68e5fe Compare February 26, 2025 02:33

turbocrime mentioned this pull request Feb 26, 2025

time out stalled streams, emit stream abort events #2074

Merged

2 tasks

turbocrime force-pushed the transport-dom-stream-timeout branch 2 times, most recently from 98cdcea to a024bb1 Compare February 26, 2025 03:27

turbocrime force-pushed the minifront-retry-status-stream branch from daecbfa to f95bbbe Compare February 26, 2025 03:27

turbocrime force-pushed the transport-dom-stream-timeout branch from a024bb1 to 4fd6ac3 Compare February 26, 2025 03:58

turbocrime force-pushed the minifront-retry-status-stream branch from f95bbbe to 6e16b5a Compare February 26, 2025 03:58

turbocrime force-pushed the transport-dom-stream-timeout branch from 4fd6ac3 to de77c7c Compare February 26, 2025 04:19

turbocrime force-pushed the minifront-retry-status-stream branch from 6e16b5a to f7fff02 Compare February 26, 2025 04:22

turbocrime force-pushed the transport-dom-stream-timeout branch from de77c7c to 1952085 Compare February 26, 2025 05:14

turbocrime force-pushed the minifront-retry-status-stream branch from f7fff02 to f358734 Compare February 26, 2025 05:16

Base automatically changed from transport-dom-stream-timeout to main February 26, 2025 05:26

turbocrime added 5 commits February 25, 2025 21:29

minifront retry status stream

33bf4f3

changeset

15cdbf1

test status slice

177d6ec

simplify

1df2117

change strings

2da7a35

turbocrime force-pushed the minifront-retry-status-stream branch from f358734 to 2da7a35 Compare February 26, 2025 05:29

turbocrime commented Feb 27, 2025

View reviewed changes

TalDerei reviewed Feb 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minifront: retry status stream #2080

minifront: retry status stream #2080

turbocrime commented Feb 25, 2025 •

edited

Loading

changeset-bot bot commented Feb 25, 2025 •

edited

Loading

turbocrime Feb 25, 2025

turbocrime Feb 25, 2025

TalDerei Feb 27, 2025

turbocrime Feb 27, 2025

TalDerei Feb 27, 2025

TalDerei Feb 27, 2025

turbocrime Feb 27, 2025

TalDerei left a comment

TalDerei Feb 27, 2025

turbocrime Feb 27, 2025

TalDerei Feb 27, 2025

TalDerei Feb 27, 2025

turbocrime Feb 27, 2025

turbocrime Feb 27, 2025

TalDerei Feb 27, 2025

turbocrime Feb 27, 2025 •

edited

Loading

turbocrime Feb 27, 2025

TalDerei Feb 27, 2025

turbocrime Feb 27, 2025

TalDerei Feb 27, 2025

TalDerei Feb 27, 2025

turbocrime Feb 27, 2025

TalDerei Feb 27, 2025

turbocrime Feb 27, 2025

turbocrime Feb 27, 2025

minifront: retry status stream #2080

Are you sure you want to change the base?

minifront: retry status stream #2080

Conversation

turbocrime commented Feb 25, 2025 • edited Loading

Description of Changes

Checklist Before Requesting Review

changeset-bot bot commented Feb 25, 2025 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbocrime Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbocrime commented Feb 25, 2025 •

edited

Loading

changeset-bot bot commented Feb 25, 2025 •

edited

Loading

turbocrime Feb 27, 2025 •

edited

Loading