3 Commits

Include renames

Author	SHA1	Message	Date
3 people	ccc77c5b5e	fix(server): record legacy controller lease-loss timings (#16359) ... ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The heartbeat runner executes legacy agent runs, and a controller lease watchdog keeps durable ownership of each run > - The watchdog renews the lease every 10 seconds and stops the run when a renewal does not finish within the 60-second lease > - A staging run failed in setup with `LegacyControllerLeaseLostError` and the `renewal_deadline` cause, after about eight minutes of preparation and with no server restart > - The setup failure path did not record the cause, and no evidence showed why the renewal did not finish: a stalled database query or a blocked event loop > - This pull request records bounded watchdog timings with each lease loss, on the setup failure path and on the cancellation path > - The benefit is that the next event shows which of the two causes stops renewal, which is the missing fact to fix the root cause ## Linked Issues or Issue Description Refs #16084 (added the closed lease-loss cause for cancelled runs) Refs #16355 (retries a run that a restart stopped before dispatch; a different path) **What happened?** A legacy `paperclip_runner` run on staging failed with `setup_failed` and the message "Legacy controller lease lost". The stack shows the watchdog deadline timer (`renewal_deadline`). The run was in `executionStage: "preparing"` for about 467 seconds. The server started about 11 minutes before the run, so this was not a restart. The run record and the Sentry event do not show the lease-loss cause, because the setup failure path does not read it. Nothing shows if a renewal query was in flight at the deadline, or if the deadline callback ran late. **Expected behavior** When the watchdog loses the lease, the run records the closed cause and the watchdog timings. This applies when the loss fails setup and when it stops the adapter. The failure report contains only the closed cause and bounded numbers. **Steps to reproduce** 1. Start a legacy run with a controller lease. 2. During setup, make a lease renewal query hang for more than 60 seconds, or block the event loop past the deadline. 3. Let the watchdog abort the run. 4. Read `resultJson` on the failed run. Before this change, it has no lease-loss evidence. ## What Changed - `legacy-controller-lease.ts`: The watchdog tracks the last successful renewal, the renewals in flight, the scheduled deadline, and the renewal count. `LegacyControllerLeaseLostError` now carries `leaseLossTiming`: `sinceRenewalMs`, `renewalPendingMs` (or `null`), `deadlineLateMs` (only for `renewal_deadline`), and `renewalCount`. Ownership behavior does not change. - `run-cancellation.ts`: Add the `LegacyControllerLeaseLossTiming` type and `readControllerLeaseLoss()`. The reader accepts only a closed cause and non-negative finite numbers, and clamps each value to 24 hours. - `heartbeat/run-completion.ts`: Persist `resultJson.controllerLeaseLoss` with the cause and timings. This applies to a lease loss that stops the adapter and to a lease loss that fails setup (the thrown abort reason). - `run-failure-diagnostics.ts`: Report `controllerLeaseLossCause` and the four bounded timing fields from `controllerLeaseLoss`. A setup failure now gets the cause label too. - `doc/run-log-events.md`: Document the new record and how to read the timings. - Tests: watchdog timings for a stalled query, a blocked event loop, and a failing query; completion records the evidence for an adapter stop and for a setup failure; the failure report clamps values, drops unknown keys, and drops a record without a closed cause. ## Verification - `pnpm --filter @paperclipai/server exec vitest run src/services/legacy-controller-lease.test.ts src/services/heartbeat/run-completion.test.ts src/services/__tests__/run-failure-report.test.ts src/services/run-cancellation.test.ts` - I could not run these tests locally. The sandbox has a 4 GB memory limit, and `pnpm install` was stopped by the out-of-memory killer during lockfile resolution. CI is the test run for this change. - After deployment, a new lease loss shows `controllerLeaseRenewalPendingMs` and `controllerLeaseDeadlineLateMs` in the Sentry execution context. A pending time near 50 seconds or more points at the database. A large deadline lateness points at the event loop. ## Risks - Low risk. This change adds diagnostics. It does not change when the watchdog stops a run, and it does not prevent the lease loss. - The new `resultJson.controllerLeaseLoss` field is server-written. Readers ignore unknown values. Old rows do not have it. - The failure report gets five closed or numeric fields. It gets no reason text, IDs, or user content. - The event-loop test blocks the loop for about 80 ms to make the deadline late. It uses real timers, so a very slow CI host makes the lateness larger, not smaller. > 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 (`claude-opus-5-5`) in Claude Code, with tool use (shell, file edits, Sentry and GitHub connectors). The exact context-window size is 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 - [ ] I have run tests locally and they pass (not possible in this sandbox; see Verification) - [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: Devin Foley <139239+devinfoley@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-10-14 19:52:23 -07:00
4 people	a64969b288	fix(server): attribute legacy runs stopped by controller lease loss (#16084) ... ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The heartbeat runner executes legacy agent runs. A controller lease watchdog keeps durable ownership of each legacy run. > - When the watchdog loses the lease, it aborts the adapter. The abort reason was a plain `Error`, and nothing recorded why the run stopped. > - Completion then saved the cancellation as source `unknown`. Sentry got an unexpected cancel that it could not tell apart from other unrecorded stops. > - We saw this on a sandboxed Grok run. The system cancelled it after about 22 minutes with `cancellationSource: unknown`. Workspace copy-back then failed. Every `cancelRun` path records a stop request, so an unrecorded stop this late points to the lease watchdog. > - This pull request records a lease loss as its own cancellation source, with a closed cause label. > - The benefit is that the next event shows whether the lease loss caused the stop, and which renewal failure caused it. That is the missing fact to fix the root cause. ## Linked Issues or Issue Description Refs #15790 Refs #15920 **What happened?** A legacy sandbox run was stopped by the system while it was working. The run record and the Sentry diagnostics show `cancellationSource: unknown`, with no initiator. The controller lease watchdog aborts the adapter with a plain `Error("Legacy controller lease lost")`. Completion does not read the abort reason, so it records the stop as `unknown`. **Expected behavior** When the host stops its own adapter because it lost the controller lease, the run records that cause. Diagnostics name the cause with a closed label. A stop that a user or the control plane requested earlier keeps its recorded source. **Steps to reproduce** 1. Start a legacy run that has a controller lease. 2. Make lease renewal fail: the renewal query errors, the update matches no row, or the lease deadline passes. 3. Let the adapter return its cancelled result. 4. Read `resultJson.cancellation` on the run. Before this change, `source` is `unknown`. **Agent adapter(s) involved** Not adapter-specific (core bug). We saw it with `grok_local` in a sandbox. **Additional context** This change adds diagnostics. It does not prevent the lease loss. #15920 retries a transient copy-back failure. It does not help here, because the stopped sandbox rejects every retry. ## What Changed - `legacy-controller-lease.ts`: The watchdog aborts with a new `LegacyControllerLeaseLostError`. The error carries a closed cause: `renewal_rejected`, `renewal_deadline`, or `renewal_error`. - `run-cancellation.ts`: Add the `controller_lease` cancellation source. Add `controllerLeaseLossCancellation()` and `readControllerLeaseLossCause()`. The reader accepts only the server-written reason text. - `heartbeat/run-completion.ts`: When the outcome is `cancelled` and the abort reason is a lease loss, record a `controller_lease` cancellation (`expected: false`). This covers both an adapter that returns a cancelled result (`completeRun`) and one that throws after the abort, for example through `signal.throwIfAborted()` (`failRun`). A stop request recorded earlier still takes precedence. - `run-failure-diagnostics.ts`: Add `controllerLeaseLossCause` to the execution diagnostics. Only the closed label goes to Sentry, never the reason text. - `heartbeat/run-state.ts`: preserve `controller_lease` in the bounded SQL projection when saved results exceed 64 KB. A database regression verifies the default run-detail read. - `canContinueCancelledRun` stays unchanged. It does not accept `controller_lease`, so a lease loss does not authorize automatic continuation. - `doc/run-log-events.md`: Document the new source and cause labels. - Tests: unit tests for the cause on each watchdog path, completion tests that record the source when the adapter returns and when it throws the abort reason (both fail without the change), tests that a recorded stop request takes precedence, and a test that the Sentry report contains the closed label and not the reason text. ## Verification - `cd server && npx vitest run src/services/run-cancellation.test.ts src/services/legacy-controller-lease.test.ts src/services/__tests__/run-failure-report.test.ts src/services/heartbeat/run-completion.test.ts`: 238 passed, including the embedded Postgres suites. - I reverted only `run-completion.ts` and ran the new completion test. It failed with `source: "unknown"`. With the change, it records `controller_lease`. - I removed only the `failRun` attribution and ran the new thrown-error test. It failed without the `controller_lease` record, and passes with it. - Final full repository `pnpm -r typecheck` and `pnpm build` pass. - Final local regression verification: all 247 tests pass across run-state, cancellation, watchdog, failure reporting, and completion suites. The large-result test fails before the SQL allowlist change. All latest-head CI checks pass (54 successes and two intentional skips). Fresh Apex is 5/5 on `708856c568d07d7b446a4a4af2edc6d9eec7d155` with no unresolved findings. ## Risks - Low risk. The change adds a new source value to server-owned JSON. Old rows keep their values. The readers accept the new value, and no UI code switches on this field. - A lease-loss cancel stays `expected: false`, so it is still reported as unexpected, as before. Now it has a more specific source. - The Sentry payload gets one new closed label (`controllerLeaseLossCause`). It does not include reason text or IDs. > 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 (`claude-opus-5-5`) in Claude Code, with tool use (shell, file edits, test runs). OpenAI GPT-6 (`gpt-6`) through Codex added the large-result regression and SQL projection fix, with shell, file-edit, and test tools. The exact context-window size is 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Paperclip <noreply@paperclip.ing>	2026-10-13 14:50:40 -07:00
DottaandPaperclip	7a52dcdc74	fix: repair MCP validation and cancelled execution recovery (#14951) ... ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The tool gateway gives agents access to connected services. Recovery controls what happens when a run stops. > - Generated tool names can exceed the provider limit after the MCP client adds its prefix. > - The same invalid definition can fail each automatic retry. A cancelled run can also hold saved messages without showing its cause. > - This pull request bounds tool names, stops configuration retries, and retains cancellation evidence. > - It shows the stopped run and admits saved input only after the existing safety checks pass. > - The benefit is a clear recovery path that preserves operator Stop and prevents duplicate message delivery. ## Linked Issues or Issue Description **What happened?** A long connected MCP tool name makes the provider reject the entire request. Automatic recovery repeats the invalid request. Separately, unexpected legacy cancellations can leave saved input behind a recovery hold. The notice does not identify the stopped run or its cause. **Expected behavior** Complete MCP names fit the provider limit. Tool-definition errors require configuration repair. Cancelled runs retain their source and reason. The recovery notice shows the cause and saved-message count. Verified unexpected cancellations can start a fresh turn through the existing admission checks. **Steps to reproduce** 1. Assign an App gallery connection with a long application key and tool name to a Claude agent. 2. Start a run. The provider rejects a name over 128 characters, including its MCP prefix. 3. For cancellation recovery, stop a legacy provider turn without an operator Stop request and send a user message while the recovery hold is active. 4. Inspect the recovery notice and the deferred message queue. **Paperclip version or commit** Rebased onto master at `cf8ad63c806685bfd7c48e3ed4a919d61a7c55f1`. **Deployment mode** Hosted or self-hosted server with legacy Claude or Codex execution. Related public work: - Refs #14017. That PR caps name segments. This PR preserves existing short names and uses stable hash aliases for long complete names. It also covers classification and recovery. - Refs #4510. That PR adds a cancellation-source column. This PR records bounded evidence in the existing run result, without a migration. - Refs #12552 and #4506. Those PRs suppress recovery after operator cancellation. This PR preserves operator intent and uses the existing continuation gates. ## What Changed - Bound gateway names with the full provider prefix in the 128-character budget. Retain the original upstream tool name for dispatch and permissions. - Classify invalid tool definitions as configuration failures before diagnostic redaction. Stop automatic retries and continuation attempts for that error code. - Persist cancellation source, expectedness, initiator, reason, and time. Preserve recorded Stop intent when adapter results arrive. Report unexpected started cancellations with closed diagnostic labels. - Show the run cause, saved-message count, and Inspect run link. Offer Continue for eligible unexpected cancellations. Require verified provider stop, empty tool inventory, ownership, and the existing pause, budget, approval, and dependency gates. Use the existing queue for single delivery. - Add regression coverage and update the execution, MCP gateway, and run-log documentation. ## Verification - `pnpm -r typecheck` and `pnpm build` passed. - `pnpm check:token-gates` passed. - Ran `pnpm test:run` and completed its workspace and serialized groups. Initial resource and timing failures passed on isolated reruns. All 149 serialized route suites passed. - Reran the changed server, adapter, and UI suites after the rebase. Coverage includes long-name upstream dispatch, configuration retry suppression, cancellation evidence retention, privacy labels, oversized run projection, and concurrent saved-message delivery. - `pnpm test:e2e tests/e2e/legacy-failure-continuation.spec.ts` passed all six browser scenarios. The recovery notice shows the run cause and inspection link, and each recovery entry point reaches one new response. - Added database-backed checks for active, removed, paused, unavailable, and disabled chat connections. The final continuation and recovery-notice suites passed 167 tests. Externally bound chats hide board Continue and show a usable next action. - All 55 GitHub checks passed on `42afbf1371dcaeb72646e3d8f65c19ff7cddf8de`. Two unrelated Storybook jobs were skipped by their normal conditions. Greptile reviewed that commit at 5/5 with no findings and no open review threads. ## Risks - Long tool names change to aliases. Existing short names stay compatible. The original connection and upstream name remain the dispatch authority. - Invalid tool definitions no longer get automatic retries. An operator must repair the configuration before a new attempt. - Continuation changes apply only to positively identified unexpected legacy cancellations with complete empty tool inventory. Operator Stop, unknown historical cancellations, outstanding tools, and unverified provider termination keep their holds. - No database migration. The added projection fields are optional. Cancellation reason and initiator IDs remain local run evidence; Sentry receives only closed source and initiator-type labels and expectedness. ## Model Used - OpenAI GPT-6 through Codex, with reasoning, repository editing, shell execution, and GitHub tool use. The runtime does not expose the exact model variant or context-window size. ## 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>	2026-10-02 13:47:59 -05:00