mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-13 15:55:46 +01:00
Devin FoleyandPaperclip
e9343d8501
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - When a person clicks Retry on a stopped task, Paperclip saves the
request as a held wake and dispatches it right away, unless admission is
held. Admission is held during a task drain, for example while a
deployment restarts the server
> - A Retry held by a drain is released later by the deferred-receipt
worker. That worker shares one queue with every other held wake: comment
waits, execution waits, and interrupted queues
> - On a busy instance that queue holds hundreds of passive waits that
come due every 30 seconds. A Retry is checked in due-time order behind
all of them. Ending a drain does not wake the worker either
> - In a real case, a Retry clicked during a deployment drain was still
held 40 minutes later, after the drain expired and the server restarted.
It ran only when the person clicked Retry again
> - This pull request checks operator Retries ahead of passive waits,
and wakes the worker as soon as a drain stops or expires
> - The benefit is that a Retry held by a drain starts within one worker
pass after the drain ends
## Linked Issues or Issue Description
**What happened?**
A person clicked Retry on a blocked task 14 seconds after a deployment's
task drain started. The Retry was saved as `deferred_issue_execution`,
and the immediate dispatch returned early because of the drain. The
drain had a 20-minute TTL, and the deployment restarted the server about
6 minutes in. The held Retry still did not start, and its `updated_at`
never changed. Forty minutes later the person clicked Retry again, and
the new request started a run and merged with the held one.
That instance had 390 held wakes: 271 execution waits and 116 others,
against 6 task Retries. The execution waits come due every 30 seconds.
Measured on the instance, the deferred-receipt worker took about 7
minutes to get through that backlog once.
**Expected behavior**
A Retry that a drain held starts soon after the drain stops or expires,
or after the server restarts, whatever the size of the passive backlog.
**Steps to reproduce**
1. Start a non-idle task drain (`POST /instance/task-drain`).
2. Click Retry on a stopped task. The receipt is
`deferred_issue_execution`.
3. Stop the drain. On `master`, the deferred-receipt worker only notices
on its next 30-second re-check. It then checks the Retry in due-time
order behind every passive wait that came due before it.
**Paperclip version or commit**
`master` at 8148fc0b1e.
## What Changed
- `server/src/services/scoped-deadline-work.ts`:
- New optional `priority(entry)`. Due entries are sorted by priority
first, then by due time and id as before.
- New `refresh(hint)`. It clears a scope's reconstruction delay and
invalidates it. Failure backoff is kept.
- New optional `onStop` hook, called when the coordinator stops the
worker.
- `server/src/services/heartbeat/deferred-receipt-work.ts`:
- A `task_retry` receipt that a person requested is an operator Retry,
with priority 1. Passive receipts keep priority 0, so their order does
not change.
- Each deferred scan loads operator Retries first, with their own query,
then pages by id as before. A Retry is therefore in memory in the first
pass even beyond the first 100-row page. A receipt loaded twice merges
by id.
- New optional `owner.subscribeRefresh`. The work calls `refresh()` on
all three of its queues when it fires, and releases the subscription
from `onStop`, so the coordinator's own stop removes it.
- `server/src/services/heartbeat.ts`: passes `subscribeRefresh` that
listens to task-drain changes and expiry. Before this change, a deploy
drain left the worker waiting for its 30-second re-check, behind the
scan's reconstruction delay.
- `server/src/__tests__/deferred-execution-receipt-work.test.ts`, four
new tests:
- A real Retry, requested during a deploy drain, starts a run (to
`succeeded`) within 5 seconds after the drain stops.
- An operator Retry whose id sorts after 120 passive execution waits
(more than one load page) is resumed first, in the first worker pass.
- A held Retry inserted with no notification right after a full scan is
resumed as soon as the drain refresh fires.
- Stopping only the coordinator removes the drain listener.
## Verification
- `npx vitest run src/__tests__/deferred-execution-receipt-work.test.ts
-t "held by a deploy drain|ahead of a backlog|ordinary committed task
Retry"`: 3 pass. The third is the existing ordinary Retry dispatch test.
- Red checks, removing one piece at a time:
- Without the drain subscription, the drain test fails: the Retry is
still `deferred_issue_execution` with no run after 5 seconds.
- Without the priority, or without the prioritized load, the backlog
test fails: the first resumed request is the passive `00000000-…` wake.
- With `invalidate()` instead of `refresh()`, the post-scan test fails
after 5 seconds.
- Without the `onStop` release, the listener test fails.
- Full `deferred-execution-receipt-work.test.ts`: 50 pass.
- `chat-channels.integration.test.ts -t "still batches several replies
in one finding thread"`: it timed out once on CI and passes 2 of 2 times
locally with this change.
- `scoped-deadline-work`, `heartbeat-task-drain`,
`heartbeat-task-drain-admission-release`, `cloud-control-task-drain`,
`task-admission-deadline`, `onboarding-first-task-admission` and
`delivery-work-coordinator` suites: 164 pass.
- `pnpm run typecheck` in `server` (layered): clean.
## Risks
- Operator Retries are a small set; there were 6 on the instance above.
They now take the first slots of a worker pass, so passive waits can
move by at most that many places. Passive waits keep their own order.
- A Retry that keeps failing its check still gets the existing failure
backoff, so it cannot take every pass.
- A drain start also wakes the worker. That pass ends at once, because
the worker's run gate still reads the drain.
## Model Used
- Claude Opus 5.5 (`claude-opus-5-5`) via Claude Code, with tool use
(shell, file editing, vitest runs against embedded PostgreSQL, read-only
production log and database analysis).
## 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>