Repository navigation
fix(client): keep a live service that stalls across a suspend - #58130
AlexeyShirshov wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
The approach looks right to me, with three small fixes before merge (inline). I ran a real opencode serve --service and froze it with SIGSTOP while a second opencode service start ran. On the base, a 12 s freeze makes the client replace the service after about 6 s. With this PR the same process is kept and reused. A service frozen for 45 s is still replaced, after about 47 s instead of about 6 s. I couldn't reproduce the WSL2 sleep and resume itself, or the stale pooled socket theory, since Linux loopback connections survive a pause. packages/client tests (232) and its type check pass.
Public API: unchanged. unresponsiveAfter is a new field on the internal EnsureTiming type, and probeResult now also returns stalled.
Concerns (details inline):
- In the Effect client, the new
alivecheck failsensure()instead of returningfalse. - The 30 s window is no longer tied to one registration.
- The window uses the wall clock, which the issue says jumps on resume.
A minor point: on the replacement path, PtyHandoff.prepare asks a service that isn't answering for the handoff (2 s timeout), then asks it to shut down (another 2 s). In practice it publishes a null handoff, so terminals are not actually preserved when the service is really hung. That is fine, but it adds about 4 s, and the description's claim that persistent terminals survive only holds when the service answers again.
|
|
||
| // Whether a registered owner still exists. Signals are not delivered for an unresponsive check, so | ||
| // a live process that merely stopped answering keeps its registration. | ||
| const alive = (pid: number) => |
There was a problem hiding this comment.
Effect.try({ try, catch: () => false }) puts false in the error channel; it doesn't return false. When process.kill(pid, 0) throws (ESRCH, or EPERM for a process owned by another user), yield* alive(info.pid) fails the whole ensure() with a bare false. The Promise client's !stopped(pid) treats that case as "not alive". Adding Effect.orElseSucceed(() => false), like stopped below, would make the two clients behave the same.
| // A stalled sample means the probe spanned a suspend, so it says nothing about the service: | ||
| // restart the window instead of counting evidence against a process that was not running. | ||
| if (!registration.stalled && registration.timedOut && info !== undefined && (yield* alive(info.pid))) { | ||
| unresponsiveSince ??= Date.now() |
There was a problem hiding this comment.
The old counter was reset when the registration changed (same(timeouts.info, info)). unresponsiveSince is now carried over to a different registration. If service A times out for about 29 s and a newly registered service B times out once, B is replaced right away. Could you keep the window tied to the Info it started on, here and in promise/service.ts?
Also, the window uses Date.now(). The issue describes the clock jumping on WSL resume, so a forward jump after a single timed-out probe would close the window at once and kill the service, which is the false positive this PR targets. A monotonic clock (performance.now()) for the window would avoid that. The stall check in probeResult can stay as it is.
There was a problem hiding this comment.
The new commit fixes the Effect alive failure, and the recovery window now restarts when the registered service changes. The window still uses Date.now(), so that part of the earlier thread stays open. I re-ran the 12 s SIGSTOP check with a real opencode serve --service: the same service is still kept. In packages/client, all 240 tests pass and the type check is clean. Public API: unchanged.
One new regression should block merge (inline): a client now waits forever on a stale registration whose PID belongs to an unrelated process.
| shouldRecruit(owner: "absent" | "alive" | "dead") { | ||
| if (owner === "absent") return due() | ||
| if (lastSpawn === 0) lastSpawn = Date.now() | ||
| if (owner === "alive") return false |
There was a problem hiding this comment.
When the registered process is alive, this never starts a replacement, and the recovery window never starts when the request fails outright instead of timing out. One common case: service.json survives a reboot or crash, and its PID now belongs to an unrelated process. The probe gets "connection refused" (timedOut: false) and the PID exists, so the policy resets on every poll and no replacement is ever started.
I reproduced it with a registration pointing at the PID of a running sleep and an unused URL. On the base, opencode service start spawned a new service and exited 0 after 8 s. With this PR it was still waiting when my 60 s timeout killed it. The Promise client would only give up at promiseTimeout.
Could an owner that refuses connections (or answers with a different PID) still get one spawn delay and then a replacement, as before? Only a process that times out while alive needs to be held back.
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
Addressed in f88c036:
|
There was a problem hiding this comment.
The latest commit addresses my remaining points. The recovery window now uses performance.now(), a clock that a wall-clock jump on resume can't move. A registration whose process refuses the connection, answers as a different PID, or no longer exists is replaced again after one spawn delay. I re-ran both cases with opencode service start from source. A leftover registration naming an unrelated live PID now gets a new service after 6 s instead of hanging. A service frozen with SIGSTOP for 12 s is kept, as before. In packages/client, all 243 tests pass and tsc --noEmit is clean. I still couldn't reproduce the actual WSL2 sleep and resume.
Public API: unchanged. ownerState, recoveryPolicy and EnsureTiming.unresponsiveAfter are internal to packages/client.
No further concerns.
There was a problem hiding this comment.
The only new commit merges dev. In the files this PR touches it brings in only dev's own small refactors (the Info type alias and the shared headers() helper in pty-handoff.ts), and the recovery logic is unchanged since my last review. I re-ran the two checks on the new head. A leftover registration naming an unrelated live PID gets a new service after 6 s, and a service frozen with SIGSTOP for 12 s is kept. In packages/client, all 247 tests pass and tsc --noEmit is clean. Public API: unchanged.
No further concerns.
3c7d3a3 to
de5a00e
Compare
|
Rebased onto current Re-verified after the rebase: |
The unresponsive recovery path counted three 2s health-probe timeouts and then SIGTERM/SIGKILLed the registered service. After a resume from sleep the probe can sit on a pooled socket the server already dropped, so three timeouts arrive while the process is healthy: the client kills it, discards the PTY handoff, and spawns a replacement. The caller gets a fresh service and exits 0, so nothing surfaces to the user beyond a lost turn and lost persistent terminals. - probe /api/info over a fresh connection so a stalled socket cannot masquerade as an unresponsive service - discard samples whose deadline passed by more than a second: they spanned a suspend, so they are not evidence about the process - require a live process and a continuous unresponsiveAfter window (30s) before replacing it, and attempt a PTY handoff instead of clearing it - keep the same policy in the Promise client
The unresponsive window could not close on its own: a contender recruited while the owner was still alive cannot bind its port, and its exit failure is reported as the ensure() error long before the window ends, so a hung but live service would never be replaced. Recruitment now depends on the owner's state: a live owner is never contested, a gone one still gets its one spawn delay. Move the window into a shared recovery policy so the Effect and Promise clients cannot drift, and fix alive() in the Effect client: Effect.try's catch is the error channel, so a dead pid failed the whole ensure() with `false` instead of answering false. - service-contender: shouldRecruit(owner: "absent" | "alive" | "dead") - service-recovery: recoveryPolicy(timing).observe(sample) - tests: contender gate, stale-owner replacement without the window, and a real command in the "answers again" case so the gate is exercised
…ervice A registration can outlive the process it names: after a reboot its PID can belong to an unrelated process, and its URL can answer as a different one. The previous commit held back any live owner, so such a registration was never replaced and ensure() waited until promiseTimeout. Recruitment now depends on what the poll learned. Only an owner that did not answer, either by timing out or across a suspend, holds its port for the recovery window. A dead or refusing owner gets one spawn delay and then a replacement, as before. Measure the unresponsive window on the monotonic clock, so a wall-clock jump on resume cannot close it with a single timed-out probe. - service-contender: ownerState(info, probe, alive), shouldRecruit(OwnerState) - both clients: pass performance.now() to the recovery policy - tests: owner classification, the contender gate per state, and a registration whose PID belongs to another process (times out without this change)
de5a00e to
b016ffd
Compare
|
Rebased onto
|
Issue for this PR
Closes #58111
Type of change
What does this PR do?
A client no longer kills a healthy background service because its health probes did not answer, and it no longer waits forever on a registration whose process is gone.
ensure()treated three consecutive 2 s/api/infotimeouts as proof that the registered service was dead: it cleared the PTY handoff, sentSIGTERM(thenSIGKILL) and spawned a replacement. After a resume from sleep the probes can be sent on a pooled connection the server already dropped, so the timeouts arrive while the process is healthy and answering. Reproduced on 2.1.2 against a sandboxed service frozen withSIGSTOP:Background service is unresponsive, the frozen process killed, a replacement registered, and the calling client exiting0— the user only loses the running turn and the persistent terminals.What changed:
service-probe.tsprobes/api/infowithconnection: close, which disables pooled sockets for the Desktop client (Node'sfetch/undici honours it). Bun'sfetch, used by the TUI, already sendsConnection: closeand opens a fresh connection per request, so the header is a no-op there: what protects the TUI is the liveness check, the window and the stall classification below. A probe whose deadline passed by more than a second is classifiedstalledinstead oftimedOut.service-recovery.tsholds the replacement decision in one place, shared by the Effect and Promise clients so they cannot drift. A live but silent owner is replaced only after a continuousunresponsiveAfter(30 s) window measured on the monotonic clock, so a wall-clock jump on resume cannot close it with a single timed-out probe. An answered probe, a stalled sample, or a different registration restarts the window; a process that is gone never opens one.service-contender.tsclassifies the registered owner (absent,unanswered,gone,refusing) instead of only asking whether a PID exists. An unanswered owner holds its port and may still be recovering from a suspend, so only the recovery window may replace it. A gone owner leaves the port free, and one that refuses the connection or answers as a different PID is no longer the service the registration describes; both get one spawn delay and then an attempt, as before.alive()reported throughEffect.try's error channel, so a dead PID failed the wholeensure()with a barefalseinstead of answering "not alive", unlike the Promise client. It now returnsfalse.PtyHandoff.prepareinstead of clearing the handoff. That is best-effort: a service that cannot answer publishes no handoff, so terminals survive only when it can still answer.The 30 s window also bounds the respawn storm from #57038 more tightly than before, since a recovered service answers long before the window closes.
How did you verify your code works?
bun testinpackages/client(243 tests) and thepackages/cliservice tests (32),bun typecheckandoxlintonpackages/client, and prettier are clean.opencode serve --service: frozen withSIGSTOPfor 12 s, the same process is kept and reused; frozen past the window, it is signalled and replaced.connection: close; Bun 1.4.2 already sendsConnection: closeand opens a new connection per request, whether or not the header is set.Screenshots / recordings
Not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #51216, #57864, #57038, #52049, #54810