Split initialize() into the half that must run once and the half that retries - #828
Open
cmdcolin wants to merge 2 commits into
Open
Split initialize() into the half that must run once and the half that retries#828cmdcolin wants to merge 2 commits into
cmdcolin wants to merge 2 commits into
Conversation
… half that retries
initialize() did both, and the reaction that called it could only give one of
them what it needed. Its dispose() sat behind both awaits, so the reaction
stayed armed across two round trips with everything once-only already done —
and what it tracks is self.role, which those very requests write: a 403 from
either takes getFetcher through removeToken()/setRole(), clearing the role and
setting it again when a new token arrives. An expired token at startup, which
is the ordinary case, therefore installed everything a second time: a second
"Admin" submenu, and a second set of socket handlers, so every COMMON message
reached the change manager twice.
install() is now the once-only half and loadInitialState() the retryable one,
with a reaction each. The install reaction is synchronous, which is what makes
"at most once" structural rather than a flag — dispose() runs in the same tick,
so nothing can invalidate it in between. addSocketListeners() moves to the top
of install() because it is the only step that can throw ('No Token found',
before it registers anything), so a failed run leaves nothing installed and the
reaction simply stays armed.
Two consequences worth naming. roleNotificationSent is gone: it existed only to
stop the no-access notification firing twice, which the synchronous reaction now
prevents structurally. And the socket handlers are registered earlier than
before — ahead of both requests rather than between them — so there is less of a
window in which a 'connect' can arrive with nothing listening for it.
This was referenced Aug 12, 2026
… from Registering the socket handlers ahead of both startup requests is the point of moving addSocketListeners() to the top of install() — the first connect used to arrive while nothing was listening for it. But that is also the connect with no lastChangeSequenceNumber yet, since loadInitialState() is what sets it, and getMissingChanges() throws without one. It is called as a `void`, so on an ordinary startup that throw becomes an unhandled rejection, and under a dev server that listens for those, an error overlay. Skip it instead: before the baseline exists there is by definition nothing to catch up on. The guard is falsiness rather than `=== undefined` so that it matches getMissingChanges()' own condition exactly — which also covers a server whose sequence is still 0, where the same throw was reachable on every reconnect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SnSmtZPwNZR3764AJ16ZUL
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
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.
Supersedes #826 and #827 — take this one or those two, not both. They guard the two once-only effects with flags; this removes the need for the flags by separating the once-only work from the retryable work, which is the actual defect.
initialize()did both kinds of work, and the single reaction calling it could only serve one: itsdispose()sat behind both awaits, so the reaction stayed armed across two round trips with everything once-only already done — and what it tracks isself.role, which those very requests write, since a 403 from either takesgetFetcherthroughremoveToken()/setRole(). An expired token at startup, the ordinary case, therefore installed everything twice: a second "Admin" submenu (a menu contribution is appended to a log the root model replays on every open) and a second set of socket handlers (each is a fresh arrow function, so socket.io cannot recognize the repeat), which means everyCOMMONmessage reaching the change manager twice.install()is the once-only half andloadInitialState()the retryable one, with a reaction each. The install reaction is synchronous, which is what makes "at most once" structural rather than a flag —dispose()runs in the same tick, so nothing can invalidate it in between — andaddSocketListeners()moves to the top ofinstall()because it is the only step that can throw, before it has registered anything, so a failed run leaves nothing installed and the reaction just stays armed.Two consequences worth naming.
roleNotificationSentis gone: it existed only to stop the no-access notification firing twice, which the synchronous reaction now prevents structurally.And the socket handlers are registered ahead of both requests rather than between them, which needed one guard rather than none. Registering earlier means the first
connectnow lands in a handler instead of arriving while nothing is listening — but that is exactly the connect with nolastChangeSequenceNumberyet, becauseloadInitialState()is what sets it, andgetMissingChanges()throws without one. Called as avoid, that throw is an unhandled rejection on an ordinary startup. So the connect handler now returns early when there is no baseline, since before one exists there is by definition nothing to catch up on. The guard tests falsiness, matchinggetMissingChanges()' own condition, which also covers a server still at sequence 0 — where the same throw was reachable on every reconnect, before this PR and after it.Not covered by tests either way — this path has none, and nothing in the repo instantiates an
ApolloInternetAccountunder test — so it wants a look from someone who owns the account model.Verified with the repo's own toolchain:
prettierreturns the file unchanged,eslint --max-warnings 0is clean, andtsc -p packages/jbrowse-plugin-apolloproduces an error set identical to the branch point (none of them in this file). Also checked againstjbrowse_5: this merges into it cleanly and typechecks there too.🤖 Generated with Claude Code
https://claude.ai/code/session_01SnSmtZPwNZR3764AJ16ZUL