mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-27 19:35:13 +01:00
DottaandPaperclip
2a9d527c68
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The native runtime keeps a durable runner alive across controller restarts. > - A restart can begin while a provider session is still opening. > - The old controller fenced read-only run admission before it could publish that prepared session. > - Admission failure then closed the runner that shutdown needed to detach. > - This pull request lets only the existing startup finish admission within the bounded shutdown wait. > - The controller detaches that exact session before it can submit a turn. New work and state changes stay fenced. ## Linked Issues or Issue Description **What happened?** A controller restart during native provider startup could close the prepared runner with `native control-plane run admission failed`. Recovery then found no running process and placed the task in a reconciliation hold. The same admission path follows both fresh startup and replacement after a failed provider resume. **Expected behavior** Shutdown must detach the existing prepared session so the next controller can recover it through the normal identity checks. Shutdown must not admit a new executor, submit a turn, or override an explicit Stop. **Steps to reproduce** 1. Delay a native backend while it opens its provider session. 2. Request controller restart detachment for that run. 3. Let backend startup finish before the 30-second deadline. 4. Observe that read-only admission fails and shutdown reports the run as inactive instead of detached. **Additional context** Related work: #17073 handles remote process-monitor classification; #17459 handles macOS process fingerprints; #17097 covers broader hot upgrade recovery. This change fixes the startup admission fence. It does not relax process identity or provider launch-profile checks. ## What Changed - Separate the read-only admission guard from the guard on control-plane state changes. - Let shutdown await only the startup already registered for the run. Close its admission window when the wait ends. - Reject a new execution after its run is fenced for shutdown. - Detach the prepared session and exit before runtime-question attachment or provider turn submission. - If Stop is pending before or after publication, retain its exact cancellation acknowledgment and cleanup owner across authority reads and runtime-question attachment. - Share one 30-second admission/Stop-settlement deadline across the restart sweep. Report an unsettled run as pending while healthy runs continue hot-restart adoption. - Cover the real runtime and admission path, deadline expiry, new-start rejection, persisted binding checks, and write fences. - Document the bounded startup handoff in the restart ownership contract. ## Verification - Red: the new actual-runtime/admission regression failed on the previous code. Shutdown reported the run as inactive and did not detach it. - Red: review regressions reproduced detach before a post-publication Stop acknowledgment and an actual hot-restart preparation error when startup Stop cleanup timed out. - Green on `e10d0cb238`: 18 startup/restart tests passed, including Stop during authority reads and runtime-question attachment, the real recovery preparation and shutdown coordinator with a stalled Stop plus healthy peer, and the shared sweep deadline. The earlier 36-case Stop/restart selection also passed before this follow-up. - Both complete changed test files previously passed on head `8e9c2610fe`: 857 tests. They used real PostgreSQL 18 with a fresh database per fixture. An earlier local harness attempt reused one database and hit duplicate fixture keys; that harness was corrected before this complete pass. - Real PostgreSQL: 7 admission and revocation tests passed. These cover wrong runner identity, persisted binding mismatch, checkpoint/result rejection, and in-flight write revocation. The local test harness used a fresh Docker PostgreSQL 18 instance because macOS embedded Postgres exhausted the host semaphore limit. Only database provisioning was replaced; assertions and production port code were unchanged. - Transport safety acceptance: the current source transport controlled a real Linux ARM runner and a deterministic Codex test provider in a disposable Debian container. After startup, detach and generic cleanup preserved the process. A new controller authenticated the same PID and provider session. The provider log had exactly one session start and no turn start. Duplicate launch was forbidden, the retained workspace was unchanged, and no stop or suspend command was issued. - The Linux acceptance check tests the shared native transport safety contract. It is not a live Claude response, a hosted Daytona run, or recovery of the original task. Those remain unverified. No original task, sandbox, credentials, or permissions were changed. - `pnpm -r typecheck` and `pnpm build` passed before the review follow-up. Server typecheck passed on current head `e10d0cb238`; current-head CI also passed the full typecheck and build gates. - The full local `pnpm test:run` command was attempted, then stopped after repeated embedded PostgreSQL bootstrap setup failures. That broad local run is incomplete. The current-head four-file Docker PostgreSQL run passed 879 tests, with one existing warm-copy `claim-uncertain` case timing out and recovery failing to import because the temporary provisioning mock omitted a support export. The harness is corrected and affected cases are being rechecked. The timeout is unexplained; it is not being labeled an infrastructure failure. No assertions are skipped by the database provisioning replacement. - All 55 clean-host CI checks passed on current head `e10d0cb238` (2 conditional skips), including all general test shards and native runner verification. Greptile reviewed that exact head at 5/5 with no findings. The previous Paperclip review findings are addressed in [the review response](https://github.com/paperclipai/paperclip/pull/17511#issuecomment-6391520242); its new advisory review remains queued. ## Risks - Admission must stay read-only. All writes retain the original synchronous controller fence, and both admission checks still validate the exact persisted binding. - Startup that exceeds the 30-second shutdown wait still fails closed. It cannot claim a successful detach or start a turn. - No schema migration or provider configuration change is included. ## Model Used OpenAI Codex, based on GPT-6, with reasoning, repository tools, code execution, and test execution. The exact runtime model ID and context window are not exposed in this session. ## 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>