mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-10 12:07:09 +02:00
DottaandPaperclip
04de3132c2
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The heartbeat service coordinates agent runs and their recovery. > - The earlier refactors separated workspaces, run preparation, run state, and retries. > - Restart recovery and lease cleanup still occupy more than 2,000 lines in the main service. > - These operations share process ownership and database claims that must stay intact. > - This pull request moves that group into one recovery module and adds boundary tests. > - Reviewers can now inspect recovery separately from queue admission and run execution. ## Linked Issues or Issue Description **What existing behavior does this improve?** The structure and testability of heartbeat restart recovery and lease cleanup. **Subsystem affected** server/ — orchestration services. **Current behavior** Hot-restart adoption, native restart recovery, shutdown draining, orphan reaping, and lease cleanup live inside `heartbeat.ts`. **Proposed behavior** Keep the same recovery behavior in `server/src/services/heartbeat/recovery.ts`. Bind the database and lifecycle callbacks with `createHeartbeatRecovery`. **Reason and benefit** This removes 2,181 lines from the main service. The extracted module is about 2,400 lines. It keeps related recovery operations together in one file. **Breaking changes** None. The public service methods, database claims, cleanup fences, and ownership checks stay the same. **Additional context** Continues the merged heartbeat extractions in #15568, #15573, #15578, #15591, #15601, and #15617. A search of open and closed PRs found no duplicate recovery extraction. This does not add a roadmap feature. ## What Changed - Move hot-restart snapshots and adoption, native restart recovery, shutdown draining, orphan reaping, and lease sweeps into `heartbeat/recovery.ts`. - Pass lifecycle callbacks and the existing shared execution sets into the factory. Preserve module-wide cleanup single-flight state. - Set the service's existing shutdown flag through a callback at the same point in shutdown preparation. - Add 14 focused tests for construction, shutdown ordering, empty selective drains, ownership races, shared state, retry timers, and background execution tracking. - Wait for the existing execution-drain barrier in the Stop recovery test instead of polling for one second. - Document the module boundary in `doc/DEVELOPING.md`. ## Verification - New module and additional cleanup coverage: **57 tests passed** across five files. ```sh pnpm exec vitest run server/src/services/heartbeat/recovery.test.ts server/src/__tests__/heartbeat-native-cleanup-admission.test.ts server/src/__tests__/heartbeat-run-terminalize-before-release.test.ts server/src/__tests__/heartbeat-run-lease-release-terminalization.test.ts server/src/services/hot-restart.test.ts ``` - Final recovery coverage on the rebased branch: **514 tests passed across 11 files**, including all 14 new module tests. ```sh pnpm exec vitest run server/src/services/heartbeat/recovery.test.ts server/src/__tests__/heartbeat-process-recovery.test.ts server/src/__tests__/heartbeat-pending-cleanup-sweep.test.ts server/src/__tests__/heartbeat-orphaned-active-lease-sweep.test.ts server/src/__tests__/native-session-resumption.test.ts server/src/__tests__/heartbeat-task-drain-admission-release.test.ts server/src/shutdown.test.ts server/src/__tests__/heartbeat-native-cleanup-admission.test.ts server/src/__tests__/heartbeat-run-terminalize-before-release.test.ts server/src/__tests__/heartbeat-run-lease-release-terminalization.test.ts server/src/services/hot-restart.test.ts ``` - Full `pnpm -r typecheck` and `pnpm build` passed, including after rebasing onto master. - Full local `pnpm test:run` was started again after the rebase. It was stopped after the full CI suite passed. It did not finish and is not counted as a full local pass. An earlier local AI-connection run hit an HTTP-test timeout; all corresponding CI coverage passed. - Structural comparison confirms 28 moved functions and 15 moved declarations. The only function-body adaptation is the shutdown flag callback. Remaining service functions and all 140 public exports match master. - All **54 current-head checks passed**; two unrelated Storybook checks were skipped. This includes the full general and serialized test matrix, typechecks, build checks, Runner verification, and canary dry run. [CI run](https://github.com/paperclipai/paperclip/actions/runs/37843023706). - Greptile completed on head `01fe39818d483e05f940b9fe842162a08e9b9e8e` with **5/5 and no actionable findings**. There are no review threads or merge conflicts. ## Risks The main risk is breaking shared execution ownership or changing recovery order while moving closures. The factory receives the original execution sets and lifecycle callbacks. It does no work during construction. Database transactions, compare-and-set predicates, cleanup deadlines, and native ownership gates stay intact. This PR has no schema or API changes. ## Model Used OpenAI GPT-6 via Codex. The exact serving model ID and context window were not exposed in this session. Used reasoning, repository inspection, shell tools, 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 - [x] All Paperclip CI gates are green - [x] 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>