Conversation
A surface view mints its token once and keeps it for the life of its
mount, including across `BlitSurfaceCanvas.setConnectionId` — which
re-points a canvas at another server without re-minting. But the token
came from a per-connection counter with no connection prefix, so it was
only unique within the connection that issued it. A canvas carrying `s3`
from connection A onto connection B collided with B's own `s3`, and
`SurfaceSub.views` is keyed on that string alone: two views, one entry,
last writer wins.
Canvases do change connection in place. The foreground pane lives in a
non-keyed `<Show when={focusedSurfaceId()}>`, BSP leaves take
`connectionId`/`surfaceId` as props, and dock cards use an index-keyed
`<Index>`, so a card's canvas is handed a different surface whenever
`offScreenSurfaces()` shifts — any focus change or surface close.
Two ways that hurt, both of which a user reads as "this surface went
laggy out of the blue and reloading fixed it":
- The pane's subscribe writes `{target: null, maxFps: 0}` at the shared
key, overwriting the card's `{512x256, 15fps}`. Nothing looks wrong
until the card's box moves and its `refreshScaledTarget()` puts the
thumbnail request back — now speaking for the pane. The live pane
decodes a 512x256, 15fps stream and cannot take it back:
`serverSubscribe` early-returns once `_subscribedSurface` is set, and
`refreshScaledTarget` only runs off the pane's own box or a
`_displaySize` null boundary, neither of which a *surface* resize
touches. The trigger is cheap, because a card's height is derived
from the surface's aspect, so any server-side resize moves its box.
- The card scrolls out of the dock and its `sendSurfaceUnsubscribe`
deletes the *pane's* registration, dropping `views` to empty and
taking the pane's stream with it. The pane freezes on its last frame
and never re-registers.
The same token also keys `surfaceViewSizes`, so a collision corrupted
size mediation the same way.
Prefix the token with the connection id. It never reaches the wire — it
only keys those two maps — so this costs nothing but the string, and
session ids are already built this way. Tests cover the mint invariant
and the pane-keeps-its-request behaviour; both fail without the prefix,
the second with exactly the reported symptom (`expected 15 to be +0`).
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
🔗 Preview: https://blit-p5jsuu3dz-indent.vercel.app |
Coverage
|
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.
A surface view mints its subscription token once and keeps it for the life of its mount — including across
BlitSurfaceCanvas.setConnectionId, which re-points a canvas at another server without re-minting. But the token came from a per-connection counter with no connection prefix:so it was only unique within the connection that issued it. A canvas carrying
s3from connection A onto connection B collided with B's owns3, andSurfaceSub.viewsis keyed on that string alone: two views, one entry, last writer wins.Needs two or more connections (local + a remote) to bite. With a single connection the target/fps derivation converges on every transition.
Why a canvas changes connection in place
<Show when={focusedSurfaceId()}>, andfocusSurfaceByIdwrites non-null → non-null, so the branch isn't re-created — the canvas just getssetSurfaceId+setConnectionId.connectionId=/surfaceId=as props.<Index>, so a card's canvas is handed a different surface wheneveroffScreenSurfaces()shifts — any focus change or surface close.Two ways it hurts
Both read to a user as "this surface went laggy out of the blue, and reloading fixed it".
A live pane pinned to 15 fps at a thumbnail size. The pane's subscribe writes
{target: null, maxFps: 0}at the shared key, overwriting the card's{512x256, 15fps}— nothing looks wrong yet. Then the card's box moves and itsrefreshScaledTarget()puts the thumbnail request back, now speaking for the pane.effectiveSurfaceTarget/effectiveSurfaceMaxFpssee only that, so the pane decodes a 512x256, 15 fps stream and cannot take it back:serverSubscribeearly-returns once_subscribedSurfaceis set, andrefreshScaledTargetonly runs off the pane's own box or a_displaySizenull boundary — neither of which a surface resize touches.The trigger is cheap: a dock card's height is derived from the surface's aspect (
aspect-ratio: surface.width / surface.height; height: auto), so any server-side surface resize moves its box. That style is deliberate — it's what broke the encoder feedback loop that used to segfault libnvcuvid — so it isn't the bug, it's what makes the collision fire often.Or the pane's stream is deleted. The card scrolls out of the dock (
overflow-y: auto) and itssendSurfaceUnsubscribedoessub.views.delete(token)— deleting the pane's registration.views.sizedrops to 0, the deferred wire UNSUBSCRIBE fires, and the pane freezes on its last frame with no subscription, never re-registering.The same token also keys
surfaceViewSizes, so a collision corrupted size mediation the same way — reintroducing the defect the comment aboveofferSurfaceViewSizeexists to prevent.The fix
Prefix the token with the connection id. It never reaches the wire — it only keys
surfaceSubs'viewsandsurfaceViewSizes'views— so this costs nothing but the string, and session ids are already built this way.Prefixing rather than re-minting in
setConnectionId, becauseserverUnsubscribe()needs the old token against the old connection first; making the token globally unique avoids that ordering question entirely and fixes thesurfaceViewSizeskeying in the same stroke.Tests
Two, both of which fail without the prefix:
mints view tokens no other connection can collide with— the invariant. Fails withexpected 's1' not to be 's1'.keeps a pane's request when a view that arrived from another connection shares the surface— the behaviour: a foreign-token card sharing the surface must not speak for the pane, through its initial subscribe, a latersetSurfaceViewTarget(its ResizeObserver firing), and its unsubscribe. Fails withexpected 15 to be +0— the reported symptom exactly.Full JS suites pass (core 1208, ui 425, solid 23, react 12);
tsc --noEmitclean across all packages.Not in this PR
Two independent contributors found in the same investigation, both left alone because they're separate changes with more risk:
+max(q/8,12)per 250 ms vs-6per 1000 ms), down to q=200 — worse than any user-selectable preset — and two of its three congestion triggers are wrong:decoder_pressure_depth > 4when the same file says 5–6 is a healthy decoder's standing depth, and a connection-widewrite_blocked_usthat counts terminal frames and fs/git/LSP replies (the same class of input surface: stop a busy terminal from stranding video quality at the floor #262 removed from this exact function).blitFromStore→applyLayout→syncImeTargetforces a synchronous layout per decoded frame once a guest text field is focused, andhandleWheelforces two more per wheel event. AlsoonChangere-blits every mounted view on a title change.