Repository navigation
fix(server): avoid booting cold locations for pending prompt reads - #53925
mlimarenko wants to merge 4 commits into
Conversation
|
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 |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
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.
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 |
|
Rebased onto Passed the cold/warm prompt regression, 2 affected fetch cases, 6 SDK instance/lifecycle tests, the server instance regression, and native pre-push Upstream CI is not green: |
a50a888 to
a6dc9e3
Compare
a357974 to
11b96de
Compare
There was a problem hiding this comment.
I checked the rebased head the same way as before. After a restart, GET /api/session/:id/permission and /form each start the session's location once on the base; with this PR they return {"data":[]} and nothing starts. The changed server, SDK and core tests and the core/server/sdk/protocol type checks all pass. Since my last review, the PR was rebased (only an import path changed), the deleted-folder behaviour is now documented, and the test is shortened as discussed. I have no further concerns.
Public API: (1) HTTP: the form and permission list endpoints now return 200 {"data":[]} instead of LocationNotFoundError when a session's folder has been deleted, and session/global/form does the same for a missing directory. Unknown sessions still get a 404. The error types session.form.list declares stay the same; they now sit on the endpoint instead of coming from the middleware. (2) Core: Instance.Interface gains a required provideCached, and Instance.cached is exported. Any custom selector implementation has to add it, as the PR does for the host, the SDK and the test fixtures. The SDK's method signatures are unchanged.
There was a problem hiding this comment.
The only new commit merges the base branch into this one; the PR's own diff is unchanged since my last review, and no earlier points are open. I re-checked it on the new base. After a restart, GET /api/session/:id/permission and /form still start the session's location once on the base; with this PR they return {"data":[]} without starting it. The changed server, SDK and core tests pass. The core/server/sdk/protocol type checks pass, so nothing brought in by the merge implements the instance selector without the new provideCached. Public API impact is the same as before: the form and permission list endpoints return 200 [] instead of LocationNotFoundError for a session whose folder was deleted, and Instance.Interface gains a required provideCached. No new concerns.
6db387b to
6879161
Compare
There was a problem hiding this comment.
This push rebases the branch onto the latest base; the PR's own diff is unchanged since my last review, and no earlier points are open. I re-checked it on the new base. After a restart, GET /api/session/:id/permission and /form still start the session's location once on the base; with this PR they return {"data":[]} without starting it. The changed server, SDK and core tests pass, and tsc passes for core, server, sdk and protocol. Public API impact is unchanged: the form and permission list endpoints return 200 [] instead of LocationNotFoundError for a session whose folder was deleted, and Instance.Interface gains a required provideCached. No new concerns.
6879161 to
c5eb3d3
Compare
There was a problem hiding this comment.
Re-checked on the rebased head. #59083 has since landed on the base: form listing for a real session already returns [] without starting its location. On the base, GET /api/session/:id/permission after a restart still starts the location; with this PR it doesn't. The global form route also no longer starts one. The changed server, SDK and core tests (including upstream's session-form.test.ts) and tsc for core/server/sdk/protocol pass.
The main thing to settle now is the overlap with #59083: there are two mechanisms for the same thing (see inline). Public API impact is unchanged from before: the form and permission list endpoints return 200 [] instead of LocationNotFoundError for a session whose folder was deleted, and Instance.Interface gains a required provideCached alongside the optional live.
packages/core/src/instance/service.ts:17: After the rebase, the selector has two ways to ask "is this location already running?":live?from #59083 and this new requiredprovideCached. This PR also takessession.form.listoffFormLocationMiddleware, so thelivebranch #59083 added there (options.endpoint.identifier === "session.form.list") no longer runs for that endpoint. Please pick one. Either reuseinstances.livefor the permission list (and theglobalform route), keeping this change small and in line with what just landed. Or keepprovideCached, which holds the entry while reading so it can't race an eviction, and then removeliveand the middleware branch so they don't linger as unused code.
c5eb3d3 to
ae31a1a
Compare
|
Consolidated on The focused server tests pass, including upstream's |
There was a problem hiding this comment.
The overlap with #59083 is resolved: ae31a1ae6 removes live and the now-unused form-middleware branch, so provideCached is the only mechanism, and nothing else referenced live. Re-checked from source. On the base, GET /api/session/:id/permission after a restart still starts the session's location; with this PR neither it nor /form does. The changed server, SDK and core tests pass, including #59083's session-form.test.ts, and tsc passes for core/server/sdk/protocol.
Public API: the form and permission list endpoints return 200 [] instead of LocationNotFoundError for a session whose folder was deleted, and the global form route also skips starting a cold location. In core, Instance.Interface gains a required provideCached and drops the optional live from #59083; custom selector implementations must add provideCached. No new concerns.
ae31a1a to
5fa55db
Compare
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
Added by the opencode bot (AI-generated).
Related: #51003, #52034
Public API: form/permission list endpoints return 200 [] instead of LocationNotFoundError for a deleted folder; Instance selector gains a required provideCached and drops the optional live (custom implementations must update)