mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
Devin FoleyandPaperclip
46fb933b01
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Native runs can execute in a remote sandbox. When a run fails, Paperclip stops the sandbox and records the provider's stop receipt on the environment lease. > - System workspace recovery can restart that stopped sandbox to save its files. Before the restart, it removed the stop receipt from the lease. > - If the restart failed and the provider could not stop the sandbox after that, the lease had no receipt. Continuation admission requires a stop receipt on every remote lease of the failed run. So every Retry and every comment waited for an environment that never stopped. > - This pull request keeps the original receipt until the provider confirms the restart. It restores the receipt when the restart fails and the sandbox cannot stop. A restart that fails because the sandbox disk is full now ends recovery with a fresh-workspace outcome. > - The benefit is that a failed file-preserving restart no longer blocks the task permanently. Saved input and Retry continue in a fresh isolated workspace, and the original sandbox stays retained. ## Linked Issues or Issue Description No public issue exists. This is a bug report in the format of `bug_report.yml`. **What happened?** 1. A native run failed because its Daytona sandbox filled its 10 GB disk. 2. Paperclip stopped the sandbox normally and recorded the stop receipt on the environment lease. 3. Some minutes later, system workspace recovery tried to restart the sandbox to preserve its files. Before the restart, `prepareStoppedSandboxForRecovery` deleted the stop receipt. 4. The restart failed on the full disk. The sandbox went into the provider's `error` state. 5. The stop-only teardown after the failed restart also failed, because the provider cannot stop a sandbox in that state. The lease stayed in `pending_cleanup` with no receipt. The cleanup sweep retried the same failing stop with backoff. 6. `admitExplicitNativeContinuation` requires a provider stop receipt on every remote lease of the failed run. Every Retry and every comment was deferred with "Waiting for the previous environment to stop. Your message will start automatically." The message never started. Also, after recovery was exhausted, a Retry replaced the exhausted generation with a new one. That generation repeated the same restart, and the Retry still reported success. **Expected behavior** A lease must not lose its stop receipt while the real sandbox state is unknown or not running. When the file-preserving restart cannot succeed, recovery must end in a state that lets the task continue, for example in a fresh isolated workspace. **Steps to reproduce** 1. Use a native run with a retained remote sandbox lease that has a `stopped` receipt and a pending workspace recovery. 2. Make the provider resume fail (for example with `no space left on device`), and make the provider stop fail. 3. Run `recoverRetainedWorkspace` until recovery is exhausted. 4. Save a user comment on the task and run the deferred-comment admission. 5. Before this change, the lease is `pending_cleanup` without a receipt. The comment stays deferred with reason `remote_cleanup`, and no successor run starts. **Paperclip version or commit** Observed on a hosted staging build. Reproduced on `master` at `6452e964a36` with the new tests in this PR. **Deployment mode** Hosted, with the Daytona sandbox provider plugin. The defect is in core server recovery code, so it is not specific to one adapter. ## What Changed - `server/src/services/sandbox-stop-and-retain.ts` - `prepareStoppedSandboxForRecovery` still writes the stop-only intent and moves the lease to `pending_cleanup` before the provider call. It now keeps the prior `remoteExecutionTermination` (and `sandboxStopAndRetainReceipt`, if present) as `priorStop` inside the `sandboxStopAndRetain` intent. - New `confirmSandboxRecoveryRestart` removes `priorStop` when the provider resume returns. From that point, the sandbox may be running, and only a fresh stop can settle the lease. - New `restoreSandboxStopAfterFailedRestart` restores the prior receipts and the `released` / `success` lease state. It works only while `priorStop` is present, so only when no restart was confirmed. It checks the restored receipt with `hasRemoteTerminationReceipt` first. It uses the same attempt fence as `settleStopOnlyCleanup`, and it writes through `writeEnvironmentCleanup` so dispatch and cleanup wake up. - `settleStopOnlyCleanup` now shares that attempt fence (`stopOnlyAttempt`). Its behavior does not change. - `server/src/services/workspace-recovery.ts` - After `resumeWorkspaceRecoveryLease` returns, recovery confirms the restart before any other check. - If the restart is not confirmed, recovery still tries the stop-only teardown first. A successful stop records a fresh receipt, as before. If the stop also fails, recovery restores the prior receipt instead of leaving the lease without one. - A later attempt that finds a pending stop-only lease (for example after a process restart during the reopen) uses the same rule when its stop fails. - A resume error that reports a full sandbox disk (`No space left on device` or `Disk quota exceeded`) becomes `source_unavailable` / `use_alternative_strategy`. The generation is exhausted at once, and the existing fresh-workspace continuation applies. Other resume errors keep the existing bounded retry policy (`provider_unavailable`, at most six attempts in 30 minutes). Because the receipt stays, that policy also ends in a state that lets the task continue. - `server/src/services/task-retry.ts`: Retry binds to an exhausted generation with a `use_alternative_strategy` failure and no fresh attempt, the same way it binds to a complete or running generation. It does not create a new generation that repeats a step the policy already rejected. The Retry continues in a fresh isolated workspace. An explicit workspace recovery request (`requestWorkspaceRecovery`) still starts a new generation. - `doc/task-recovery.md`: describes the receipt handling around the reopen and the Retry binding. - `server/src/services/workspace-recovery.test.ts`: four new tests (five cases) with the real database and mocked provider calls. ### Design choice I kept the existing write-ahead fence. The lease still leaves `released` and shows no receipt while the provider reopen is in progress. So no admission, reuse or destructive cleanup can act in that window, and a crash still leaves a stop-only obligation. The original receipt is not deleted. It stays in the stop-only intent and is discarded only when the provider confirms the restart. This makes one simple rule: the suspended receipt exists only while no restart has been confirmed. I did not use "restore only when the provider confirms that the sandbox is not running". The Daytona plugin reports a sandbox in the `error` state as `active` in `inspectLease`, and its `stopLease` cannot stop that state. So with the current plugin, no provider confirmation exists for this exact failure, and the hold would stay. A separate change to the Daytona plugin's `stopLease` can recover an errored sandbox before it stops it. This PR does not depend on that change. If that change lands and its recovery succeeds, the existing stop attempt records a fresh receipt, and the restore path is not used. I also did not change continuation admission to accept a lease in `pending_cleanup`. Many gates treat pending cleanup as unfinished work (`hasPendingRunCleanup`, the remote stop check and others). A change there would be wider and more sensitive. The restored receipt is the provider's original receipt, bound to the same company, run, lease, provider, allocation and plugin. No process ID or run status takes its place. ## Verification All commands ran from `server/` in a clean worktree of `master` on macOS (Apple Silicon), with the embedded PostgreSQL test database. Vitest ran from the real path. - Baseline on `master` (`6452e964a36`), before any change: `npx vitest run src/services/workspace-recovery.test.ts` → exit 0, 331 passed. - The new tests fail on `master`. I reverted only the three source files and kept the new tests: - `npx vitest run src/services/workspace-recovery.test.ts -t "file-preserving restart fails|without repeating a restart|keeps the stop obligation once|interrupted restart that the provider"` → exit 1, 4 failed, 1 passed. - In the comment cases, no successor run starts (`expected [] to have a length of 1`). A temporary log (not committed) showed the lease in `pending_cleanup` in both cases. In the `unknown_failure` case, the comment was deferred with `remote_cleanup` / "Waiting for the previous environment to stop. Your message will start automatically." This is the same hold as in the incident. - The guard test "keeps the stop obligation once the provider has confirmed a restart" passes on `master`, as expected. It makes sure that the fix does not restore a receipt after a confirmed restart. - With this change, the same command → exit 0, 5 passed. - `npx vitest run src/services/workspace-recovery.test.ts` on the committed head → exit 0, 336 passed (331 existing and 5 new). - Neighboring suites (`task-retry`, `explicit-native-continuation`, `heartbeat/recovery`, `environment-runtime`, and the other server test files that import the changed modules: `live-recovery-work`, `retained-native-session-cleanup-work`, `execution-control-work`, `legacy-workspace-restore-recovery`, `deferred-execution-receipt-work`, `issue-agent-mutation-ownership-routes`, `native-workspace-export-retry`, `native-cancelled-workspace-preservation`, `native-workspace-warm-reuse`, `native-workspace-reconstruction`, `native-failed-workspace-recovery`, `heartbeat/workspace-preparation`) → exit 0, 16 files, 1456 passed. - `npx vitest run src/__tests__/chat-channels.integration.test.ts src/__tests__/slack-conversation-lifecycle.test.ts src/services/native-runtime/preserved-native-response-wait.integration.test.ts` (other importers of `task-retry`) → exit 0, 3 files, 2074 passed. - `pnpm --filter @paperclipai/server typecheck` → exit 0. - Not run: the full `pnpm -r typecheck`, `pnpm test:run` and `pnpm build` repo checks. CI runs them. No live Daytona probe ran. New tests: - `lets saved input continue after a file-preserving restart fails on a sandbox that cannot stop` (`disk_full`, `unknown_failure`): resume fails and stop fails. The lease keeps its original receipt and is `released` / `success`. Recovery is exhausted (after one attempt for a full disk, and after six attempts for an unknown failure). The saved comment starts one successor run with `fresh_isolated_workspace` readiness. The source files stay in the retained sandbox. - `continues a Retry in a fresh workspace without repeating a restart that cannot succeed`: after a full-disk exhaustion, Retry keeps the same generation and binds its intent. The restart is not called again, and the Retry starts one run in a fresh isolated workspace. - `keeps the stop obligation once the provider has confirmed a restart`: resume returns, a later check fails, and the stop fails. The lease stays `pending_cleanup` without a receipt, and the intent has no `priorStop`. - `restores the stop receipt after an interrupted restart that the provider never confirmed`: the lease starts in `pending_cleanup` with a suspended receipt, as after a process restart during the reopen. The stop fails, the receipt is restored, and the attempt continues to the normal reopen. Assumption and limits (runtime dependency rule): the provider calls are mocked, and the database is real. The tests do not exercise the real Daytona error text. The full-disk check matches the patterns that Paperclip already uses for sandbox disk-full errors. If the provider error text does not match, recovery uses the bounded retry path, which now also ends in a fresh-workspace continuation. The change adds one database write after each confirmed reopen and one after a failed reopen and stop. It adds no polling. ## Risks - A resume can fail after the provider started the sandbox, for example when the reply is lost. If the stop-only teardown then also fails, recovery restores a `stopped` receipt for a sandbox that can be running. The old run's processes do not survive the earlier stop, so the receipt is still correct as proof that the old execution ended. But the retained sandbox can run until the provider stops it or a later recovery or cleanup stops it. Before this change, the same case caused a permanent hold. - A full-disk restart failure now exhausts recovery at once. The task continues in a fresh workspace without the failed run's files. The files stay in the retained sandbox. An operator can still request workspace recovery explicitly after the disk is freed. - Retry no longer renews an exhausted generation that has a `use_alternative_strategy` failure and no fresh attempt. For destroyed or missing sources, the result is the same as before but without the extra attempt. - No schema or migration change. A suspended receipt is one extra field in the existing `sandboxStopAndRetain` metadata. Older servers ignore the field. The intent equality checks compare the stored value, so a mixed-version deploy keeps working. - #18820 also changes `server/src/services/workspace-recovery.test.ts`. The two PRs touch different tests. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Anthropic Claude Opus 5.5 (model ID `claude-opus-5-5`), running as a Claude Code agent with tool use (shell, file reads and edits, local test and typecheck runs) and extended reasoning. The runtime does not expose the context window size to the agent. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass (affected suites; full repo runs are left to CI, as noted above) - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing>