Repository navigation
fix(server): avoid booting cold locations for pending prompt reads - #53925
mlimarenko wants to merge 2 commits into
Conversation
github-actions
Bot
added
needs:compliance
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The native pre-push |
|
The following comment was made by an LLM, it may be inaccurate: |
|
Updated the description to the repository PR template, with |
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reproduced #53918 from source by restarting the server and then listing a session's permissions or forms. On the base, each call starts the session's location (one location services booted). With this PR, both return {"data":[]} and nothing starts. The changed server, SDK and core tests and the type checks pass. The only failures are three OpenAI OAuth port tests, which also fail on the base.
The approach looks sound. RcMap.getOption only holds an entry that is already running, for the duration of the read. Pending forms and permissions are stored per location, and pending forms are cancelled when it shuts down, so a location that isn't running has nothing to return and [] is correct. Two points, inline: listing for a session whose folder was deleted now returns 200 [] instead of a not-found error, and the new test file is large for what it checks.
| return { data: yield* form.list({ sessionID: ctx.params.sessionID }) } | ||
| const read = Form.Service.use((form) => form.list({ sessionID: ctx.params.sessionID })) | ||
| const forms = | ||
| ctx.params.sessionID === "global" |
There was a problem hiding this comment.
Side effect of skipping the location start: a session whose folder has been deleted now gets 200 {"data":[]} from /form and /permission (and from /api/session/global/form with a missing directory) instead of the LocationNotFoundError added in #52668. fetch.test.ts was changed to expect this. Returning an empty list seems reasonable for a read of pending prompts, but please confirm it's intended and mention it in the description. LocationNotFoundError is now listed for session.form.list, but it can only happen if a start already in progress fails.
| import { createEmbeddedRoutes } from "../src/routes" | ||
| import { cachedLocation } from "../src/location" | ||
|
|
||
| it.live( |
There was a problem hiding this comment.
This test is 140 lines with its own LocationServiceMap/LayerMap wiring, and it also checks how in-progress reads behave when a location is invalidated. The fetch.test.ts change (loaded() now []) and the SDK configured/setups assertions already cover "no start on a cold read". Could you cut this down to the core case: a cold read returns [] without a lookup, and a running location returns its pending form or permission? Or drop the invalidation half if it doesn't guard a specific bug.
d7a21e8 to
800f6b1
Compare
|
Rebased onto current Addressed both review points:
Focused checks passed: 1 prompt-read test, 2 affected fetch tests, 6 SDK instance/lifecycle tests, and 1 private-session-instance test. Changed-file Prettier and A broader fetch-file run had 10 passes and 3 OpenAI OAuth callback failures (timeout/port 1455 in use), matching the unrelated failures already noted in the review. It is not claimed green. Full upstream CI is also not claimed green; fork workflows may require maintainer approval. The description still references Base clarification: release commit |
This branch has not been deployed
2 participants
Issue for this PR
Closes #53918
Type of change
What does this PR do?
Pending form/permission reads currently acquire a session's full Location graph even when no prompts exist. Add a scoped cached-read operation to the Instance selector and use it for these two lists. A missing cached instance returns an empty list; a warm instance returns its existing prompts.
Host and private SDK selectors each use their own existing cache and sharing key. The read retains that entry until completion, including during invalidation. Session-ID validation, global form placement, active acquisition, and mutations remain unchanged. Intentionally, pending form/permission reads for an existing session whose folder was deleted now return
200 {"data":[]}when its Location is cold; global form reads for a missing cold directory do the same. No Location start is needed to establish that no in-memory prompts exist.LocationNotFoundErrorcan still propagate if an already-starting Location fails while the cached read waits for it. No extra registry or fallback is added.How did you verify your code works?
The cold-read HTTP regression failed before the fix because it acquired a Location.
After the fix:
bun run check: all 36 tasks passed ond7a21e83407624db469fd84f3b9053eb891e1592.The optional generated-OpenAPI check exposed existing unrelated schema drift, which is not included. Full upstream CI awaits fork-workflow approval and is not claimed green. This fix does not claim to explain the entire reported memory peak.
Screenshots / recordings
Not applicable: server resource-lifecycle behavior, no UI change.
Checklist