mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-14 00:26:28 +01:00
fix(runner): keep a connected adopted Runner through a failed liveness probe (#20965)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - After a controller restart, the controller adopts the remote Runner
that kept running in the sandbox. A monitor then checks the Runner's
liveness every 250 ms through the environment plugin.
> - #20940 and #20945 made a stalled plugin probe "unknown" for the
launched Runner's monitor, reattachment and launch. The adopted Runner's
monitor still failed the run on the first probe that failed without an
answer.
> - A short plugin stall after adoption therefore failed a healthy run
with `native_adopted_runner_identity_unverifiable`.
> - This pull request uses the evidence that is available: while the
adopted Runner holds its authenticated connection to the controller, a
liveness check that fails without an answer does not fail the run, for
at most the reconnect grace.
> - The benefit is that a healthy adopted Runner survives a short plugin
stall. A check that reports the process gone still fails the run at
once.
## Linked Issues or Issue Description
Refs #20940, #20945
**What happened?**
`#watchAdoptedRunner` in
`packages/paperclip-runner/src/live/runnerd-codex-transport.ts` called
`failTransport("native_adopted_runner_identity_unverifiable: runner
liveness could not be revalidated")` on the first `isAlive()` rejection.
For a remote adopted Runner, `isAlive()` is a plugin
`environmentExecute` RPC. A plugin stall makes it reject, even while the
Runner holds its authenticated connection and works normally.
**Expected behavior**
A rejection is not an answer. While the Runner holds its authenticated
connection, the monitor keeps checking. The bound is the reconnect
grace. A check that returns `false` (the process is gone) still fails
the run at once.
**Steps to reproduce**
1. Adopt a remote Runner after a controller restart.
2. Stall the plugin worker, so that the liveness probe rejects.
3. Within 250 ms the run fails with
`native_adopted_runner_identity_unverifiable`.
**Paperclip version or commit**
master at ef755cecc6.
## What Changed
- `#watchAdoptedRunner`: on an `isAlive()` rejection, the monitor
records when verification was first lost. It does not fail the transport
while the control plane has an authenticated Runner connection and less
than `runnerReconnectGraceMs` has passed. A successful check resets the
clock.
- Unchanged: an `isAlive()` result of `false` fails the run at once. A
rejection with no Runner connection fails it as before. A transport with
no reconnect grace (local adoption) fails it as before. The close path
still uses `isAlive()` directly.
- `doc/execution-semantics.md` describes the rule.
## Verification
- Red on master: `npx vitest run
src/live/runnerd-codex-transport-adoption.test.ts -t "liveness probe
rejects while"` (in `packages/paperclip-runner/`). It failed with
`native_adopted_runner_identity_unverifiable: runner liveness could not
be revalidated`.
- Green: the test passes. The real adopted runnerd keeps its connection,
`isAlive()` rejects for 1.5 seconds (6 monitor ticks), the run still
answers `thread/read`, no signal is sent, and the runner is alive.
- `runnerd-codex-transport-adoption.test.ts` passes (24 tests),
including "retains authenticated adopted-runner ownership when a close
probe rejects". `tsc --noEmit` passes for `packages/paperclip-runner`.
## Risks
- A remote adopted Runner whose liveness cannot be verified, but which
stays connected, keeps running for up to 7 minutes before the run fails
closed. Its own authenticated connection is the evidence during that
time, and no exit is reported.
- The change touches `packages/paperclip-runner/src`, so it changes the
Daytona runner image content ID.
> 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
- Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, through Claude Code
with tool use (shell, file edit, code search). Extended reasoning was
on.
## 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
aaf76a3ca6
commit
d1e4109df1
3 files changed
+43
-1
No files matched your search
@@ -4881,7 +4881,12 @@ transport failures keep the 30-second budget. At the launch of a new remote
|
||||
runner, a launch call that stalls this way is not retried. The controller reads
|
||||
the identity marker for the launch nonce instead, so the one launched runner is
|
||||
adopted or cleaned up by its nonce. The identity read uses the same 3-minute
|
||||
stall budget. Each answer that the sandbox gives keeps its current meaning. Monitoring checks the saved process generation when available.
|
||||
stall budget. Each answer that the sandbox gives keeps its current meaning.
|
||||
|
||||
After the controller adopts a remote runner, it checks the runner's liveness every 250 ms. A check that
|
||||
fails without an answer does not fail the run while the runner holds its authenticated connection. The
|
||||
bound is the remote runner reconnect grace (7 minutes). A check that reports the process gone still fails
|
||||
the run at once. A local adopted runner, which has no reconnect grace, keeps the old rule. Monitoring checks the saved process generation when available.
|
||||
Only a matching ownership marker and confirmed process exit permit automatic
|
||||
replacement. Missing or conflicting identity evidence, failed reads, and an
|
||||
exhausted probe budget retain the existing ownership and recovery safeguards.
|
||||
|
||||
@@ -28,6 +28,7 @@ async function verifyLiveRunnerAdoption(
|
||||
uncertainAtClose = false,
|
||||
semanticRecoveryMode?: "denied-before-cancel" | "cleanup-only",
|
||||
stopExitProofFault?: "missing" | "false",
|
||||
livenessUnverifiedWhileConnected?: "within-grace" | "past-grace" | "reset-by-success",
|
||||
) {
|
||||
const stateDirectory = await mkdtemp(join(tmpdir(), "runnerd-live-adopt-"));
|
||||
const pendingCompletion = toolHandlerOrder?.startsWith("completion-") === true;
|
||||
@@ -214,6 +215,7 @@ async function verifyLiveRunnerAdoption(
|
||||
...options,
|
||||
...(preparedTask && !adoptionAuthorization.startsWith("legacy") ? { authorizeRunnerAdoption } : {}),
|
||||
...(uncertainAtClose ? { closeGraceMs: 2_000 } : {}),
|
||||
...(livenessUnverifiedWhileConnected ? { runnerReconnectGraceMs: livenessUnverifiedWhileConnected === "within-grace" ? 60_000 : 2_000 } : {}),
|
||||
...(semanticRecoveryMode === "cleanup-only" ? { recoveryPurpose: "stop_cleanup" as const } : {}),
|
||||
// Hash different stable bytes without replacing the real runner artifact
|
||||
// used by concurrent tests. Adoption must never execute this path.
|
||||
@@ -311,6 +313,31 @@ async function verifyLiveRunnerAdoption(
|
||||
}),
|
||||
);
|
||||
if (toolHandlerOrder?.endsWith("after")) adopted.transport.setServerRequestHandler(redispatchedTool);
|
||||
if (livenessUnverifiedWhileConnected) {
|
||||
const pause = (ms: number) => new Promise(resolveWait => setTimeout(resolveWait, ms));
|
||||
expect(authority?.activeRunnerConnectionCount()).toBe(1);
|
||||
rejectLiveness = true;
|
||||
if (livenessUnverifiedWhileConnected === "reset-by-success") {
|
||||
await pause(1_400);
|
||||
rejectLiveness = false;
|
||||
await pause(500);
|
||||
rejectLiveness = true;
|
||||
await pause(1_400);
|
||||
} else {
|
||||
await pause(livenessUnverifiedWhileConnected === "within-grace" ? 1_500 : 3_000);
|
||||
}
|
||||
rejectLiveness = false;
|
||||
if (livenessUnverifiedWhileConnected === "past-grace") {
|
||||
await expect(adopted.transport.request("thread/read", {})).rejects.toThrow("native_adopted_runner_identity_unverifiable");
|
||||
} else {
|
||||
await expect(adopted.transport.request("thread/read", {})).resolves.toEqual(
|
||||
expect.objectContaining({ thread: expect.objectContaining({ id: "codex-thread-1" }) }));
|
||||
expect(adopted.evidence().diagnostics.join("\n")).not.toContain("native_adopted_runner_identity_unverifiable");
|
||||
}
|
||||
expect(() => process.kill(runnerPid!, 0)).not.toThrow();
|
||||
expect(signal).not.toHaveBeenCalled();
|
||||
if (livenessUnverifiedWhileConnected === "past-grace") return;
|
||||
}
|
||||
if (semanticRecoveryMode) {
|
||||
const retained = authority as DurablePrpControlPlane | null;
|
||||
expect(retained).not.toBeNull();
|
||||
@@ -520,6 +547,10 @@ it("adopts a prepared runner after losing its controller launch cache without pr
|
||||
it.each(["deny", "legacy", "legacy-missing", "missing-binding"] as const)("preserves adoption authorization with %s",
|
||||
mode => verifyLiveRunnerAdoption(false, false, false, false, false, true, mode), 30_000);
|
||||
|
||||
it.each(["within-grace", "past-grace", "reset-by-success"] as const)(
|
||||
"bounds a rejected liveness probe of a connected adopted runner by the reconnect grace (%s)",
|
||||
mode => verifyLiveRunnerAdoption(false, false, false, false, false, false, "allow", undefined, false, undefined, undefined, mode), 30_000);
|
||||
|
||||
it("retains authenticated adopted-runner ownership when a close probe rejects", () => verifyLiveRunnerAdoption(false, false, false, false, false, false, "allow", undefined, true), 30_000);
|
||||
|
||||
it(
|
||||
|
||||
@@ -8605,11 +8605,13 @@ class DurablePrpCodexTransport implements CodexAppServerTransport {
|
||||
>,
|
||||
): void {
|
||||
let checking = false;
|
||||
let unverifiedSince: number | null = null;
|
||||
this.#adoptedRunnerMonitor = setInterval(() => {
|
||||
if (checking || this.#closed || (this.#releaseEpisode && !this.#releaseEpisode.claimed)) return;
|
||||
checking = true;
|
||||
void Promise.resolve(adoptedRunner.isAlive())
|
||||
.then((alive) => {
|
||||
unverifiedSince = null;
|
||||
if (alive || this.#closed || (this.#releaseEpisode && !this.#releaseEpisode.claimed)) return;
|
||||
this.#evidence.runnerExited = true;
|
||||
this.#publish();
|
||||
@@ -8620,6 +8622,10 @@ class DurablePrpCodexTransport implements CodexAppServerTransport {
|
||||
);
|
||||
})
|
||||
.catch(() => {
|
||||
const now = Date.now();
|
||||
unverifiedSince ??= now;
|
||||
if ((this.#core?.activeRunnerConnectionCount() ?? 0) > 0
|
||||
&& now - unverifiedSince < (this.options.runnerReconnectGraceMs ?? 0)) return;
|
||||
if (!this.#closed && !(this.#releaseEpisode && !this.#releaseEpisode.claimed)) {
|
||||
this.#failTransport(
|
||||
new Error(
|
||||
|
||||
Reference in new issue
Block a user