Skip to content

fix(server): avoid booting cold locations for pending prompt reads - #53925

Open
mlimarenko wants to merge 4 commits into
anomalyco:devfrom
mlimarenko:location-recovery
Open

mlimarenko wants to merge 4 commits into
anomalyco:devfrom
mlimarenko:location-recovery

Conversation

@mlimarenko

@mlimarenko mlimarenko commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #53918

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

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. LocationNotFoundError can 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:

  • HTTP prompt/private-instance regressions: 2 tests, 84 assertions passed; 2 affected fetch cases also passed.
  • SDK instance/lifecycle regressions: 9 tests passed.
  • Related Core session/instance/supervisor tests: 56 passed; 16 affected tests passed after the final fixture adjustment.
  • Package typechecks, changed-file formatter/linter checks, and client generation passed; no generated client diff.
  • Native pre-push bun run check: all 36 tasks passed on d7a21e83407624db469fd84f3b9053eb891e1592.

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

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

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)

@github-actions github-actions Bot added needs:compliance This means the issue will auto-close after 2 hours. needs:issue labels Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@mlimarenko

Copy link
Copy Markdown
Contributor Author

The native pre-push bun run check passed all 36 tasks on head d7a21e83407624db469fd84f3b9053eb891e1592. GitHub reports the PR as conflict-free. The test, check, and nix-eval workflows currently have action_required; could a maintainer approve these fork workflow runs? Full upstream CI is not claimed green.

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

@mlimarenko

Copy link
Copy Markdown
Contributor Author

Updated the description to the repository PR template, with Closes #53918 in the required issue section and the verification/checklist sections filled in. Please recheck compliance and the linked issue.

@github-actions github-actions Bot removed the needs:compliance This means the issue will auto-close after 2 hours. label Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Thanks for updating your PR! It now meets our contributing guidelines. 👍

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/server/src/handlers/session.ts Resolved
@mlimarenko

mlimarenko commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased onto current v2 at 7d179e83f42a9bb6377b6f34d8d4fcbf04707621 (package version 2.0.26). New head: 800f6b1d4ebcd798d0855f9b6a2044af231c3922.

Addressed both review points:

  • Yes, a pending-prompt read for an existing session whose folder was deleted intentionally returns 200 {"data":[]} when the Location is cold. Global form reads for missing cold directories do the same. The description now states this explicitly; session-ID validation and active mutations remain unchanged. LocationNotFoundError remains possible when an already-starting cached Location fails.
  • Removed the invalidation/lifetime half of session-prompts.test.ts and its finalizer wiring/imports. The focused HTTP test retains the cold no-lookup case, unknown-session errors, and warm pending form/permission/global-form reads. No production semantics changed; the original implementation commit is unchanged according to git range-diff.

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 git diff --check passed. The native pre-push bun run check passed all 36 tasks, and the lease-protected push completed successfully.

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 Closes #53918.

Base clarification: release commit 9b4ec5714d481559990db0a816d5dec19541a814 is a sibling of the current development line, not its ancestor; these PRs target and are rebased onto upstream/v2 as requested.

@thdxr
thdxr changed the base branch from v2 to dev October 10, 2026 19:36
@mlimarenko

mlimarenko commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased onto v2 at 4617210822bdbb31e5749751afbfb4584ccedac8; head: 6caa8bc194f9931ca43929467382963f78d59063. Rebase adjustments only follow upstream's Effect HTTP import-path rename; cached-read behavior is unchanged.

Passed the cold/warm prompt regression, 2 affected fetch cases, 6 SDK instance/lifecycle tests, the server instance regression, and native pre-push bun run check (36 typecheck tasks; lint warnings remain). As documented in the description, cold pending-prompt reads intentionally return an empty list even for a deleted folder. The PR's current dev target is unchanged.

Upstream CI is not green: check, test, and nix-eval on this exact head are action_required and need maintainer approval. PR standards passed.

@mlimarenko
mlimarenko force-pushed the location-recovery branch 5 times, most recently from a50a888 to a6dc9e3 Compare October 17, 2026 07:36
@mlimarenko
mlimarenko force-pushed the location-recovery branch 3 times, most recently from a357974 to 11b96de Compare October 22, 2026 13:48

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 required provideCached. This PR also takes session.form.list off FormLocationMiddleware, so the live branch #59083 added there (options.endpoint.identifier === "session.form.list") no longer runs for that endpoint. Please pick one. Either reuse instances.live for the permission list (and the global form route), keeping this change small and in line with what just landed. Or keep provideCached, which holds the entry while reading so it can't race an eviction, and then remove live and the middleware branch so they don't linger as unused code.

@mlimarenko

Copy link
Copy Markdown
Contributor Author

Consolidated on provideCached in ae31a1a. It borrows the existing graph for the read, including an in-flight build, rather than checking presence and then doing a separate lookup. Removed Instance.live, both production implementations, and the now-unreachable session.form.list middleware branch; form mutations still use FormLocationMiddleware.

The focused server tests pass, including upstream's session-form.test.ts, cold/warm permission and form reads, the global form route, and missing-session errors. The SDK instance/lifecycle tests also pass. Client generation produced no diff. The missing-folder behavior remains intentional: pending in-memory prompts return 200 [] without booting that location; model/integration reads still report LocationNotFoundError. #57617's client changes remain untouched.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

V2: pending form and permission reads bootstrap idle session locations

2 participants