Repository navigation
feat(daemon): the host keeps its own record of old-client restores it refuses - #27496
Conversation
0c98466 to
11a1d7f
Compare
f481c99 to
4307225
Compare
4307225 to
21ec56b
Compare
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
📥 Commits
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to Expired refused-restore records can stay listed in a daemon that runs for more than 14 days. This is a minor bound violation, not a failure of the feature, and can be fixed before or shortly after merge. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 12 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
-
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description is complete and aligned with the template. It explains the user impact, implementation, rationale, linked work, testing, security, cross-platform considerations, and checklist status. … Title check The title clearly and concisely describes the main change: the daemon keeps records of refused restores from older clients.
-
-
✨ Finishing Touches 💡 1
-
-
-
-
📝 Generate docstrings 💡
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
62885918-a513-4028-90ff-10c46d6937f7
📥 Commits
Reviewing files that changed from the base of the PR and between 6e9dc09 and 21ec56b99e460be84e78bdce961daf317d488cfa.
📒 Files selected for processing (12)
src/cli/handlers/serve-restore.test.tssrc/cli/handlers/serve-restore.tssrc/cli/specs/serve.tssrc/main/daemon/daemon-automatic-work-guard.tssrc/main/daemon/legacy-restore-admission.tssrc/main/daemon/legacy-restore-records.test.tssrc/main/daemon/legacy-restore-records.tssrc/main/daemon/terminal-host-session-create.tssrc/main/daemon/terminal-spawn-gate-legacy-restores.test.tssrc/main/daemon/terminal-spawn-gate.tssrc/main/daemon/terminal-spawn-waiters.tssrc/shared/host-spawn-admission.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| private load(): LegacyRestoreRecord[] { | ||
| if (this.records) { | ||
| return this.records | ||
| } | ||
| let parsed: unknown = null | ||
| try { | ||
| parsed = this.path ? JSON.parse(readFileSync(this.path, 'utf8')) : null | ||
| } catch { | ||
| parsed = null | ||
| } | ||
| const raw = | ||
| typeof parsed === 'object' && parsed !== null && 'records' in parsed ? parsed.records : null | ||
| const now = this.now() | ||
| this.records = (Array.isArray(raw) ? raw : []) | ||
| .map(parseRecord) | ||
| .filter((record): record is LegacyRestoreRecord => record !== null) | ||
| .filter((record) => now - record.refusedAt <= MAX_AGE_MS) | ||
| .slice(0, MAX_RECORDS) | ||
| return this.records | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply the 14-day age limit on every read, not only on the first load.
load() applies MAX_AGE_MS only while this.records is still null. After the first read, list(), record(), and clearStarted() all reuse the cached array. If a daemon runs longer than 14 days, status() keeps returning expired records. save() also writes those expired records back to the file. The records disappear only when the daemon restarts. This breaks the 14-day bound that the PR states.
Fix: filter the cached array by age in load() before it returns.
🐛 Proposed fix
--- "a/src/main/daemon/legacy-restore-records.ts"
+++ "b/src/main/daemon/legacy-restore-records.ts"
@@ -94,10 +94,12 @@
}
}
private load(): LegacyRestoreRecord[] {
if (this.records) {
- return this.records
+ const now = this.now()
+ this.records = this.records.filter((record) => now - record.refusedAt <= MAX_AGE_MS)
+ return this.records
}
let parsed: unknown = null
try {
parsed = this.path ? JSON.parse(readFileSync(this.path, 'utf8')) : null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private load(): LegacyRestoreRecord[] { | |
| if (this.records) { | |
| return this.records | |
| } | |
| let parsed: unknown = null | |
| try { | |
| parsed = this.path ? JSON.parse(readFileSync(this.path, 'utf8')) : null | |
| } catch { | |
| parsed = null | |
| } | |
| const raw = | |
| typeof parsed === 'object' && parsed !== null && 'records' in parsed ? parsed.records : null | |
| const now = this.now() | |
| this.records = (Array.isArray(raw) ? raw : []) | |
| .map(parseRecord) | |
| .filter((record): record is LegacyRestoreRecord => record !== null) | |
| .filter((record) => now - record.refusedAt <= MAX_AGE_MS) | |
| .slice(0, MAX_RECORDS) | |
| return this.records | |
| } | |
| private load(): LegacyRestoreRecord[] { | |
| if (this.records) { | |
| const now = this.now() | |
| this.records = this.records.filter((record) => now - record.refusedAt <= MAX_AGE_MS) | |
| return this.records | |
| } | |
| let parsed: unknown = null | |
| try { | |
| parsed = this.path ? JSON.parse(readFileSync(this.path, 'utf8')) : null | |
| } catch { | |
| parsed = null | |
| } | |
| const raw = | |
| typeof parsed === 'object' && parsed !== null && 'records' in parsed ? parsed.records : null | |
| const now = this.now() | |
| this.records = (Array.isArray(raw) ? raw : []) | |
| .map(parseRecord) | |
| .filter((record): record is LegacyRestoreRecord => record !== null) | |
| .filter((record) => now - record.refusedAt <= MAX_AGE_MS) | |
| .slice(0, MAX_RECORDS) | |
| return this.records | |
| } |
21ec56b to
e2344f8
Compare
e2344f8 to
ff54f5f
Compare
ELI5
An older Orca client (an older desktop or a phone build from before #27452) deletes its own "resume this agent" note before it asks the host to restart the agent. If the host refuses that restart, because the machine is overloaded or at its terminal limit (#27488), the note is gone and the user cannot resume that agent. Now the host keeps its own copy of every restore like that.
orca serve restore resumelists them with the exact command that resumes each one, and a restore of the same agent from any client removes the copy.What Changed
sleeping-agent-session-launch.ts:109), and the host remembered nothing.daemon-v<N>.legacy-restores.json(mode 0600, the 32 newest, 14 days). A record holds the session id, the reason (pressureorhost_cap), when it happened, the agent, the working directory, the worktree, the resume command line and the claim digest. It never holds the environment, which can carry credentials, or the scrollback seed.terminal_spawn_refused) or at the host terminal cap. A client that declared its intent is never recorded, because its pane already stays asleep with its handle. A plain shell is never recorded.AutomaticWorkStatusgains an optionallegacyRestoreslist, which flows through the existing daemon → runtime →runtime.automaticWork.status/resume→ CLI chain.orca serve restore resumeprints the kept restores after its usual message, for example:- claude in /repo, refused 2026-10-11T12:00:00.000Z (the host was under heavy process load). Resume it with: claude --resume 6f1c.COMPAT(legacy-restore-intent): added 2026-10-11 (v1.4.214) for clients that predate spawnIntent (#27452). Remove after 2027-04-11 with the gate's undeclared-caller branches.terminal-spawn-waiters.ts, and the recording wrapper lives inlegacy-restore-admission.ts. Gate behavior is unchanged.Why
Decision Q9: old clients are paced and refused only at critical pressure, and "the host records every legacy restore it defers or refuses, keyed by session identity, so the user can resume it later from any client". The daemon is the only place every refusal passes through: local IPC, paired desktops and phones all reach the same gate, so recording there covers every client path with one hook.
Why not rebuild the client's sleeping record instead. That record needs the provider session key and id. The host only sees the command and an HMAC claim digest, which cannot be reversed, so it cannot rebuild the record faithfully. The command is exactly what the old client was about to run, so it is the faithful thing to keep.
Wire safety:
legacyRestoresis an optional field on an existing result. Older clients ignore it, and older daemons omit it, which reads as "none". No wire message or error string changes; the refusal text an old client sees is identical.Linked Issue
Part of the headless-serve robustness work for #10493 (D1 step 4, decision Q9). #27492 is merged; this PR is based on main.
Visual Proof
N/A. CLI text and host state only; no UI change. The CLI output is pinned by a test.
Testing
Store: survives a daemon restart with a private file mode; one record per restore; dropped once the same session starts; bounded to 32 and aged out after 14 days; an unreadable or unwritable file never throws into a spawn; path sits beside the run marker.
Gate, through a real
TerminalHost: an old client's restore refused under critical pressure is recorded with agent, cwd, worktree, command and claim digest, and the env token is absent; a refusal at the host cap is recorded ashost_cap; a plain shell and a declared restore are never recorded; a lateruser-resumeof the same session clears the record.CLI: lists kept restores with their resume command, says when a command was not recorded, and prints nothing extra when there are none.
pnpm tc,pnpm lint,oxlintandpnpm run check:code-quality:changedpass. The gate, shared-budget, guard, spawn-intent, async-cancellation, runtimeautomatic-work, CLIserve restoreand renderer resume suites pass.I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Agent skill upstream boundary
Notes
SSH: the record lives on the execution host that refused the restore, next to its daemon, and the CLI and RPC read it from that host. Security: the file is 0600 in the daemon's private runtime dir, and the environment is never stored. The command line can include the user's own agent arguments, which paired clients with workspace permission can already see in the terminal itself. Windows: the same file logic, written with the existing durable-write helper.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)