Repository navigation
fix(server): avoid booting unloaded locations on pending form and permission reads - #57019
kitlangton wants to merge 1 commit into
Conversation
c07b9b2 to
e67f713
Compare
There was a problem hiding this comment.
Looks sound. I ran the PR's server test routes pending requests by Session without loading an instance against the base checkout. It fails there: listing pending permissions or forms loads the session's Location. With this PR it passes, and loaded Locations still return their real pending forms. The server, core, sdk and app tests closest to the change pass, and tsc -b passes for core, server, sdk, protocol and app. provideIfLoaded is built on RcMap.getOption, so it waits for a Location that is still starting and never starts a new one. That's the behaviour the description promises.
Public API (HTTP/OpenAPI):
GET /api/session/{sessionID}/permission: errors go fromSessionNotFoundError | LocationNotFoundErrortoSessionNotFoundError.GET /api/session/{sessionID}/form: theformLocationMiddlewareis removed, soInvalidRequestErrorandLocationNotFoundErrorare no longer returned.SessionNotFoundErrorstays.- When the Location isn't loaded, both routes now return
{ data: [] }instead of loading it. Success shapes are unchanged.
This is backwards compatible. Clients that matched LocationNotFoundError on these routes just won't see it anymore. Internally, Instance.Interface gains a required provideIfLoaded, so any custom Instance.Service implementation needs it. The PR updates all of them in this repo.
One optional note on the new core test, inline.
| }) | ||
|
|
||
| describe("Instance.provideIfLoaded", () => { | ||
| it.live("skips absent instances and scopes loaded borrows without replacing the caller's Scope", () => |
There was a problem hiding this comment.
This packs several separate behaviours into one fairly dense case: absent → None without loading, loaded → Some, failure passthrough, the caller's Scope being kept, and invalidation during a borrow. It also lives in session-remove.test.ts, which isn't about provideIfLoaded. The server test in fetch.test.ts already covers the behaviour that matters (no Location is loaded, and loaded ones return real data). Optional: cut this down to the absent/loaded cases, or move it to an instance-focused test file, so it's easier to follow.
What it does
Opening a project or session in the app used to boot that session's Location just to list pending permissions and forms. A booted Location costs about 35 MB, so browsing the sidebar left them piling up: one server had 133 Locations loaded on Oct 19, and Studio's server had 33 on Oct 23. Now these two list reads answer from a Location that's already loaded, or return an empty list without booting one.
Dev already unloads idle Locations after 60 minutes (
location-activity.ts). This PR stops browsing from loading them again and doesn't add any eviction.How it works
Instance.ServicegetsprovideIfLoaded(session). It runs the effect with the session's instance only if that instance is already loaded (or still booting, in which case it waits), and returnsNoneotherwise.GET /api/session/:sessionID/permissionandGET /api/session/:sessionID/formuse it and return{ data: [] }when nothing is loaded. Theglobalform list does the same throughInstance.provideContextEffectOption. Pending permissions and forms only exist in a loaded Location's memory, so an unloaded one has nothing to report.SessionNotFoundError. These routes can no longer produceLocationNotFoundError, so the OpenAPI 404 type narrows to justSessionNotFoundError.session/requests/model.tsandproject-avatar-state.tsare scoped by session family, so a child session's pending request still shows on its parent and siblings.provideIfLoadedwas added in #46442 and then removed in #46639 as part of the plugin/config decoupling refactor, which moved these handlers back toinstances.provide. I couldn't find a stated reason for removing it. This use is narrower: two read-only list routes whose answer only lives in a loaded Location's memory, so booting one to answer them is pure cost. @thdxr, was there a reason to dropprovideIfLoadedin #46639 that should rule it out here?Risk
LocationNotFoundErroron these two routes would never see it now. The generated clients don't change.Proof
No media; this is server behavior.
bun turbo typecheckfor core, server, sdk, protocol and app: passbun test test/session-remove.test.ts test/instance-vanilla.test.ts test/session-move.test.ts: 22 passbun test test/fetch.test.ts test/session-instances.test.ts: 13 pass. Includes checks that the permission and form lists don't load an instance.bun test test/instances-lifecycle.test.ts test/instances.test.ts: 6 passbun test src/session/requests/session-request-tree.test.ts: 14 passbun script/lint-changed.ts origin/dev: 0 problemsbun --cwd services/www script/generate-openapi.ts --check: passAdded by the opencode bot (AI-generated).
Fixes #53918
Related: #51340
Public API: narrows the error types of GET /api/session/{id}/permission and /form; unloaded Locations now return an empty list (backwards compatible)