mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 06:10:44 +01:00
fa86162790cb17aba8a08eb2c38c213a7a632877
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 DottaandClaude Opus 5.5
|
fa86162790 |
refactor(recovery): one process boot id and a cached controller identity (#19851)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Restart recovery decides when a record owned by a dead server process may be released > - One server process minted three different boot UUIDs, one per module (`controller-process-identity.ts:13`, `native-restart-recovery.ts:284`, `legacy-controller-lease.ts:8`) > - The controller identity also ran `ps -o lstart` on macOS at every native claim, preparation check and lease renewal, which is on the path before `turn.start` > - The durable ownership plan (Phase 1, step 1) needs one process identity before it can add one owner stamp and one prover > - This pull request makes the three modules stamp one boot id and reads the process start time from the OS once > - The benefit is one meaning of "owned by this process" across tables, and no `ps` spawn on the turn path ## Linked Issues or Issue Description Refs #19831 (plan: `doc/plans/2026-11-07-durable-ownership-and-reconciliation.md`, Phase 1). Refs #19846 (Phase 0 inventory, finding 1, and the TTFT baseline row "Controller identity"). **Problem** One process has three boot ids, so the same fact ("this record is owned by this process") has three encodings. The uncached controller identity spawns `ps` on macOS on every claim and renewal. **Expected behavior** One boot id per process. The OS start-time read runs once per process. **Impact** Phase 1 needs one identity to build `OwnerStamp` and `proveOwnerGone` on. The turn path loses one subprocess spawn per claim and renewal. ## What Changed - `controller-process-identity.ts` owns the single process boot id. It adds `currentProcessStartedAtMs()`, which caches a successful OS start-time read for the life of the process and retries a failed read. It also exports `currentProcessSpaceHash()` for the coming prover. - `native-restart-recovery.ts`: `currentNativeControllerIdentity()` and `isCurrentNativeControllerProcess()` use the shared boot id and the cached start time. Its own `randomUUID()` boot id is removed. The fallback to the server-info start time is kept. - `legacy-controller-lease.ts`: `legacyControllerBootId` is now the shared boot id. - Tests in `native-controller-identity.test.ts`: one boot id across native claims, legacy leases and creation intents; one OS read across many claims and renewals; a failed read is retried, not cached. ## Verification - `npx vitest run src/services/native-runtime/native-controller-identity.test.ts src/services/legacy-controller-lease.test.ts src/__tests__/heartbeat-pending-cleanup-sweep.test.ts src/__tests__/sandbox-creation-operations.test.ts src/services/heartbeat/upgrade-readiness-post-result.test.ts src/services/heartbeat/recovery.test.ts` (in `server/`): 6 files pass. - `npx tsc --noEmit -p server`: no errors. - I checked every reader of the three ids. Each one compares a stored column only with the id of the module that writes that column (`heartbeat_runs.controller_boot_id` with the legacy id, `native_run_finalizations.controller_boot_id` with the native id, metadata `bootId` with the creation id). No reader compares the ids of two modules, so making them equal changes no decision inside one process. ## Risks - Low risk. No schema change and no persisted shape change. - Previous-release story (`rules/upgrade-safe-run-state.mdc`): rows saved by an older release carry that release's random per-module ids. They never equal the new process's id, exactly as a row from any earlier boot never did. They carry forward with no migration. - The start-time cache is correct because a process's own start time never changes. A failed read is not cached, so a transient `ps` failure does not pin the fallback value. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, in Claude Code with tool use and code execution. ## 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 - [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 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
DottaandClaude Opus 5.5
|
630411af53 |
fix(recovery): require a 1 s start-time gap and treat EPERM as alive in owner proofs (#19848)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - After a restart, recovery code decides whether the process that owns an in-progress run is gone > - Two proofs compare process start times to the millisecond, but `ps -o lstart` on macOS and BSD reports whole seconds > - So a live owner can look like a reused PID, and recovery can release it while it still runs > - Signal-0 handling also disagrees: one check treats EPERM as dead, while all others treat it as alive > - This pull request puts one start-time rule and one EPERM rule in `controller-process-identity.ts` and uses them at every site the Phase 0 ownership inventory named > - The benefit is that no proof releases a live owner because of clock precision or a permission error ## Linked Issues or Issue Description **Bug description** The Phase 0 ownership inventory (finding 1) found inconsistent "owner gone" proofs. `native-failed-run-owner-release.ts` (`recordedProcessMayBeLive`, `runProcessMayBeLive`) and `native-session-executor.ts` (`verifiedRecoveryProcessIsAlive`) treat any start-time difference of 1 ms or more as a reused PID. `native-restart-recovery.ts` and `controllerProcessExited` require a gap of at least 1 s. `local-service-supervisor.ts` (`isPidAlive`, `isProcessGroupAlive`) treats EPERM as dead. **Steps to reproduce** 1. On macOS, record a live process start time with millisecond precision. 2. Run the abandoned-owner release pass, or the recovered-runner liveness check, against that live PID. 3. The pass releases the owner, or the check reports the runner as gone. **Expected behavior** A sub-second start-time gap is ambiguous and keeps the owner. EPERM means the process exists. Refs #19846 ## What Changed - `controller-process-identity.ts`: export `processExists` (EPERM = alive) and `processStartsDefinitelyDiffer` (gap >= 1 s; a missing time proves nothing). `controllerProcessExited` uses the new helper. - `native-failed-run-owner-release.ts`: the controller and run-process checks use the shared helpers instead of exact millisecond equality. - `native-session-executor.ts`: `verifiedRecoveryProcessIsAlive` accepts a start time within 1 s as the same runner. Its signal path uses the same check. - `native-restart-recovery.ts`: `definitivelyDifferentProcessStart` delegates to the shared helper. No behavior change. - `local-service-supervisor.ts`: `isPidAlive` and `isProcessGroupAlive` treat EPERM as alive. ## Verification - New `controller-process-identity.test.ts`: the 1 s boundary, EPERM and ESRCH for every signal-0 check, and a live spawned runner whose recorded start is 500 ms off. - `native-failed-run-owner-release.test.ts`: two new cases (controller and run process) with a live child and a recorded start 500 ms off. The owner stays `observed`. - Red before the fix: with the old call sites, all 4 new cases fail (`releaseAbandonedNativeOwners` returns 1 and releases the live owner; EPERM returns false; the live runner reads as gone). Green after the fix. - `npx vitest run src/services/controller-process-identity.test.ts src/services/native-runtime/native-failed-run-owner-release.test.ts src/services/native-runtime/native-restart-recovery.test.ts src/__tests__/local-service-supervisor.test.ts`: 59 passed. ## Risks - Low risk. The change only makes proofs stricter: a sub-second gap or EPERM now keeps an owner that the old code released. Such a row waits for the existing lease, TTL, or real process exit. - A recycled PID whose new process started within 1 s of the recorded one now stays held until it exits. That is the same rule the takeover path already used. - Previous-release story: no persisted shape changes. Rows saved by the previous release are evaluated with the stricter rule. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used with tool use and code execution in Claude Code. ## 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 - [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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
DottaandClaude Opus 5.5
|
a5fe5deebe |
fix(environments): reclaim sandboxes left by a restart during acquire or teardown (#19696)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Remote agent runs execute in provider sandboxes (for example Daytona) that Paperclip creates and must clean up > - Before each provider create, Paperclip saves a durable creation intent (`sandbox_creation_operations`). Recovery uses it to find and destroy a sandbox whose lease was never committed > - Recovery waited 30 minutes after creation before it checked an intent, because a live server could still be waiting for the provider > - After a restart, the creating server is gone, but the intent still waited the full 30 minutes. A short-lived or redeployed instance never reclaimed those sandboxes > - The teardown that follows has the same problem: a teardown claim fences other sweeps for 15 minutes, and a restart during the claim kept the sandbox alive until the fence expired > - This pull request saves the server process identity with each intent and each teardown claim, and ends the wait when same-host proof shows that process exited > - The benefit is that a restart during sandbox acquire or teardown no longer leaves paid provider sandboxes behind ## Linked Issues or Issue Description **What happened?** A control-plane restart during Daytona sandbox acquire left the created sandbox without an `environment_leases` row. In one restart chaos wave, 6 creation intents stayed `pending` with `next_attempt_at` = creation + 30 minutes. The wave ended (and its environment was deleted) before that time, so 4 sandboxes stayed `stopped` in Daytona. Only Daytona's own auto-delete would remove them. Two of the runs did not exist in `heartbeat_runs` because they were non-run acquires; their intents still had the run id. **Expected behavior** After a restart, each sandbox that this instance and company created, with no committed lease and no live creator, is found by its exact creation name and labels and destroyed through the plugin, after the existing ownership checks. Sandboxes from another instance, another host, or a live server stay untouched. Nothing acts org-wide. **Steps to reproduce** 1. Start a Daytona-backed run. 2. Kill the server (SIGKILL) after the creation intent commits and before the lease commits. 3. Restart the server. On master, the intent stays `pending` for 30 minutes and the sandbox stays in Daytona. 4. The regression tests in `server/src/__tests__/sandbox-creation-operations.test.ts` reproduce this with the controlled provider fixture. Refs #18357 (added the durable creation intent this change completes). **Paperclip version or commit** master |