mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 14:19:00 +01:00
fix(workspaces): a busy-workspace holder whose controller lease is held is live, however silent (#20023)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Two runs must never write one workspace at once. A run that finds a workspace held (a remote workspace's writer run, or a running holder of a shared local tree) waits behind it > - The holder counted as live only while it produced output: after 60 minutes of silence it was "stale" > - For the shared local tree, that meant the waiter went ahead while the silent holder might still be working, which is a second writer. > - For a remote workspace, the waiter failed instead of waiting. > - Silence is not proof a run stopped. A long tool call can be silent and alive > - The durable ownership plan proves ownership by identity or by a held fence, never by age > - This pull request treats a holder whose controller still renews its lease (the legacy controller lease or the native coordinator lease) as live, however silent. The 60-minute rule applies only once that fence lapses > - The benefit is that a silent but live holder is never overtaken. A dead holder is still settled by the run reapers, which wake the waiters ## Linked Issues or Issue Description Refs #19831 (plan, Phase 2 sections A and C, and Phase 3 E), #19838 (inventory: "Remote workspace writer lane", and the "Released only by TTL/age" list: "Remote writer lane: 60 min"). **What happened?** `WORKSPACE_BUSY_HOLDER_STALE_AFTER_MS` (60 min of output silence) decided whether a running holder still held a workspace. This applied in four places: - the remote reservation deferral; - the parked busy-workspace wait; - `findSharedWorkspaceHolder`, which stopped counting the holder at all; - `findExecutionWorkspaceHolder`, which flagged it stale. **Expected behavior** A running holder whose controller lease is held is live by fence. Only a holder with no held fence is judged by the silence cutoff, which keeps the visible failure instead of parking forever. The check is database-only (`runOwnerFenceHeldSql`): no process probe on the workspace-preparation path before a turn starts. ## What Changed - `remote-workspace-writer-hold.ts`: new `runOwnerFenceHeldSql`. It is true while `heartbeat_runs.controller_lease_expires_at > now()` or the run's native coordinator lease is held. `RemoteWorkspaceWriter.ownerFenceHeld` is reported for a running writer. - `heartbeat/workspace-preparation.ts`: the reservation keeps deferring behind a silent writer whose fence is held. - `run-dispatch/adapters/workspace-busy-wait.ts`: a parked wait behind a running holder with a held fence stays parked and re-checks at the parked cadence. - `heartbeat/retries.ts`: `findSharedWorkspaceHolder` counts a silent holder whose fence is held. `findExecutionWorkspaceHolder` flags a holder stale only when it is silent and its fence lapsed. ## Verification - `heartbeat/workspace-preparation.test.ts`: 138 pass. The extended "waits for the live writer of an inherited remote … at reservation" (both modes) fails on `master` and passes here: a silent writer with a held lease defers, and once the lease lapses it is the 409 as before. - `heartbeat-workspace-busy.test.ts`: 41 pass. The new "still defers behind a silent holder whose controller renews its lease" fails on `master` and passes here. The existing "does not defer when the only holder has been silent past the staleness threshold" (no lease) still passes. - The server package typecheck passes. ## Risks - A silent holder whose controller is alive now blocks waiters for as long as it holds its lease. That is the safe direction. A hung but live run is the active-run silence watchdog's to handle, and a dead controller's lease lapses within a minute. - The extra predicate is an indexed lookup of the run's own row and its coordinator row. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, 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>
This commit is contained in:
1 parent
46f32ed33f
commit
7c56147bb3
6 files changed
+46
-8
No files matched your search
@@ -3,7 +3,7 @@ import { executionWorkspaces, heartbeatRuns, issues, type Db } from "@paperclipa
|
||||
import { readRemoteWorkspaceAuthority } from "@paperclipai/shared";
|
||||
import { notifyScheduledRetryDispatch } from "../../../services/run-dispatch-notifications.js";
|
||||
import { notifyScheduledRetryCancellation } from "../../../services/scheduled-retry-cancellation-notifications.js";
|
||||
import { readRemoteWorkspaceWriterHold } from "../../../services/remote-workspace-writer-hold.js";
|
||||
import { readRemoteWorkspaceWriterHold, runOwnerFenceHeldSql } from "../../../services/remote-workspace-writer-hold.js";
|
||||
import {
|
||||
WORKSPACE_BUSY_HOLDER_STALE_AFTER_MS,
|
||||
WORKSPACE_BUSY_PARKED_RECHECK_MS,
|
||||
@@ -42,6 +42,7 @@ export async function parkWorkspaceBusyRetryInTx(tx: Db, run: HeartbeatRun, now:
|
||||
.select({
|
||||
status: heartbeatRuns.status,
|
||||
lastActivityAt: sql<Date>`coalesce(${heartbeatRuns.lastOutputAt}, ${heartbeatRuns.startedAt}, ${heartbeatRuns.createdAt})`,
|
||||
ownerFenceHeld: runOwnerFenceHeldSql(),
|
||||
})
|
||||
.from(heartbeatRuns)
|
||||
.where(and(eq(heartbeatRuns.companyId, run.companyId), eq(heartbeatRuns.id, wait.holderRunId)));
|
||||
@@ -50,7 +51,8 @@ export async function parkWorkspaceBusyRetryInTx(tx: Db, run: HeartbeatRun, now:
|
||||
// Reservation defers on that hold, so the wait stays parked through it too.
|
||||
const settlingSince = holder && holder.status !== "running" ? await settlingWriterActivityAt(tx, run, wait) : null;
|
||||
const holderStaleAt = holder?.status === "running"
|
||||
? new Date(holder.lastActivityAt).getTime() + WORKSPACE_BUSY_HOLDER_STALE_AFTER_MS
|
||||
? Math.max(new Date(holder.lastActivityAt).getTime() + WORKSPACE_BUSY_HOLDER_STALE_AFTER_MS,
|
||||
holder.ownerFenceHeld === true ? now.getTime() + WORKSPACE_BUSY_PARKED_RECHECK_MS : 0)
|
||||
: settlingSince ? settlingSince.getTime() + WORKSPACE_BUSY_HOLDER_STALE_AFTER_MS : null;
|
||||
const holderLive = holderStaleAt !== null && holderStaleAt > now.getTime();
|
||||
|
||||
|
||||
Reference in new issue
Block a user