Repository navigation
Conversation
291c77b to
c9ac180
Compare
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The busy-owner case works. The new test fails on the base (the owner is killed after the third probe timeout) and passes here. bun test in packages/client passes 235/235, and tsc is clean. But the change removes the only automatic recovery for a background service that is stuck for good, so I'd ask for changes before merge.
Public API: unchanged. alive() and contenderPool().owns() are internal helpers, and the Service exports and ensure() options stay the same.
Concerns:
- A stuck service is never recovered (inline). I ran the base's old test, "evicts an unresponsive registered service before starting its replacement", against this branch. It now fails with
Timed out waiting for the background service to start. A service that has deadlocked, been frozen or is stuck in a loop blocks the desktop app, TUI and CLI until the user runsopencode service stop, and the only message they get is a misleading "start" timeout. A busy owner should get more time, but that time needs a limit (for example a recovery window measured from the last answered probe) so a truly stuck one can still be replaced. The old test covering this was replaced rather than adapted, so nothing tests that case anymore. - Reused PIDs (inline). If a crashed service leaves its registration behind and the PID now belongs to another process,
alive()returns true and everyensure()waits until its deadline and fails. Windows reuses PIDs quickly. - Overlap with #58130. That open PR fixes the same root cause in the same files: it doesn't kill an owner that is alive but not answering, but it limits how long it waits and classifies the owner's state. Maintainers will probably want only one of the two approaches, so it's worth coordinating there.
Fixes #51216
Related: #58111, #54810, #52049, #58991, #37795, #54181
| // /api/info unanswered well past the probe timeout, and on Windows SIGTERM ends the | ||
| // process at once with no shutdown. Wait for it, and do not race it with new contenders. | ||
| // Only a contender this call started, or a registration whose process is gone, yields. | ||
| if (!pool.owns(owner.pid) && alive(owner.pid)) { |
There was a problem hiding this comment.
This waits on any registered owner whose PID still exists, with no limit. A service that is stuck for good (deadlocked, frozen, stuck in a loop) is never replaced: each ensure() waits the full deadline and throws Timed out waiting for the background service to start. I confirmed this by running the base's old "evicts an unresponsive registered service" test against this branch. Could the grace period have a limit instead, e.g. tolerate missed probes for a window since the last answered one, then fall back to the old eviction? The same applies to the Effect client (effect/service.ts line 84).
| * Whether the process still exists. Unknown errors count as alive: callers use a negative answer | ||
| * to justify replacing the service, so guessing "gone" is the dangerous direction. | ||
| */ | ||
| export function alive(pid: number) { |
There was a problem hiding this comment.
process.kill(pid, 0) only shows that some process has this PID. After a crash that leaves service.json behind, a reused PID (common on Windows) counts as a live owner. Combined with the wait above, ensure() then fails on every call until the user clears the file by hand. Before, the stale registration was at least eventually cleared and replaced. Please make sure a stale registration with a reused PID can still be recovered, for example through the limited window suggested above, or by also checking the PID's process start time.
There was a problem hiding this comment.
The new commit doesn't address either earlier point, so both threads stay open. A stuck owner is still never replaced: the base's "evicts an unresponsive registered service" test still times out on this head. And a reused PID still counts as a live owner, since the new guard only rejects PIDs that aren't positive integers.
The guard itself is fine: a malformed registration file can no longer make ensure() wait or signal the caller's process group. bun test in packages/client passes (236/236), and tsc is clean.
Public API: unchanged.
There was a problem hiding this comment.
The reused-PID point is fixed: a timed-out registration whose PID belongs to an unrelated process is now dropped after the 30s window and replaced, with no signal sent to that process.
The stuck-owner point isn't fixed in practice yet (inline comment). I tested with a real source serve --service on its default fixed port (49375), froze it with SIGSTOP and called ensure():
- Base: kills the frozen process and returns a working replacement after about 12s.
- This PR: after about 32s it deletes the registration and starts a contender. The contender can't bind the port the frozen process still holds, and
ensure()fails at about 53s withManaged service port 49375 ... is already in use by another process. Configure another port.... The frozen process is left running and the registration is gone.
The new tests pass only because the fixture binds port 0.
A related side effect: deleting the registration makes a busy-but-alive service shut itself down within about 5s, because it checks its registration every 5s. So a request that keeps /api/info blocked for more than 30s still ends the service, as in #59037, just gracefully.
bun test in packages/client passes (241/241), and tsc is clean. Public API: unchanged.
| await terminate(registration.info, options, timing) | ||
| pool.evict(registration.info.pid) | ||
| console.warn("Background service is unresponsive; replacing its registration") | ||
| await abandon(owner, options) |
There was a problem hiding this comment.
This only removes the registration file and leaves a live owner running. The managed service binds a fixed default port, so the replacement contender can't bind while the stuck process still holds it. I reproduced it: with a frozen real serve --service on :49375, this path ran at about 32s, the contender exited with Managed service port 49375 ... is already in use, and ensure() threw at about 53s. The frozen process was still running and the registration was gone. The base kills it and recovers in about 12s.
When the PID still looks alive, it probably needs to be signalled after the window, as before. The stale/reused-PID worry could be handled separately, e.g. by checking that the PID is the one listening on the registered port before signalling. The same applies to the Effect client (effect/service.ts line 102).
There was a problem hiding this comment.
The stuck-owner points are fixed on Linux. I froze a real serve --service on its default port (49375) with SIGSTOP and called ensure(): after about 32s the frozen process is killed, and a working replacement on the same port is returned at about 38s. Reused PIDs still only have their registration removed. bun test in packages/client passes (241/241), and tsc is clean. Public API: unchanged.
One new concern, on the Windows listener check (inline): it depends on netstat's translated LISTENING text, so on non-English Windows it would bring back the "port already in use" failure.
| return result.stdout | ||
| .split("\n") | ||
| .map((line) => line.trim().split(/\s+/)) | ||
| .filter((fields) => fields[0] === "TCP" && fields[3] === "LISTENING" && fields[1]?.endsWith(`:${port}`)) |
There was a problem hiding this comment.
Windows translates netstat's state column (e.g. ABHÖREN in German, ÉCOUTE in French), so fields[3] === "LISTENING" never matches on non-English Windows. ownsListeningPort() then returns false, ensure() only deletes the registration, and the replacement can't bind the managed port the stuck service still holds. That is the "port already in use" failure from the previous round, on the platform this PR targets. The 1s netstat -ano timeout above can also run out on busy machines, with the same result. A check that doesn't depend on that text would avoid both, e.g. Get-NetTCPConnection -State Listen -LocalPort <port> with its OwningProcess field, or connecting to the port.
There was a problem hiding this comment.
Fixed in 7173b1b. windowsListeners now asks Windows for the listening socket's owner instead of parsing netstat:
(Get-NetTCPConnection -State Listen -LocalPort <port> -ErrorAction SilentlyContinue).OwningProcessCmdlet and parameter names are not localized, so the displayed state text no longer takes part in the decision. The query is scoped to one port, so a busy machine cannot overflow a full connection table the way netstat -ano could, and the timeout is 5s because this only runs after the owner has been silent past the 30s recovery window, never on the fast path.
Verified on Windows: the command returns the owning PID for a live listener and empty output when nothing listens. service.test.ts "replaces an unresponsive registered service after the recovery window" takes the terminate branch and passes (the replacement binds the port, which only happens when ownsListeningPort returns true), and bun test in packages/client is 237 pass, 4 skipped, 0 fail; tsc clean.
The Windows listener check parsed netstat and matched the STATE column against "LISTENING". Windows localizes that column (ABHOREN, ECOUTE), so on a non-English install ownsListeningPort never found the owner. Recovery then only deleted the registration instead of terminating the stuck service, and the replacement could not bind the port the stuck service still held. Ask the OS instead: Get-NetTCPConnection -State Listen -LocalPort <port> and read OwningProcess. Cmdlet and parameter names are not localized, the result is structured, and the query is scoped to one port, so a busy machine cannot overflow the output. This runs only after the owner has been silent past the recovery window, so a cold PowerShell start is off any latency-sensitive path; the timeout is 5s to cover it.
There was a problem hiding this comment.
The localized-netstat point is addressed. The Windows port-owner lookup now uses Get-NetTCPConnection ... .OwningProcess, whose cmdlet and property names aren't translated, and allows 5s instead of 1s. If the lookup fails, the client still falls back to removing the registration. I couldn't run the Windows path here (Linux sandbox). bun test in packages/client passes (241/241), and tsc is clean. Public API: unchanged. No new concerns.
|
Closing this to free the external-contributor pull request slot (GitHub limits accounts without write access to 3 open PRs). It overlaps with #58130, which addresses the same suspend/stall liveness problem and should be the canonical fix. The service-liveness branch remains available if it is needed. |
Issue for this PR
Closes #59037
Type of change
What does this PR do?
Service.ensure()treats three consecutive/api/infotimeouts (2 s each) as proof that a registered background service is dead: it clears the PTY handoff and sends SIGTERM, escalating to SIGKILL. On Windows SIGTERM is an immediate, uncatchable kill, so the service disappears with no shutdown line, and the in-flight tool call fails with "Tool execution interrupted".A service that is merely busy ��� for example while running a long MCP tool ��� misses those probes while still owning its port. #59037 is that case: every call to a 229-tool MCP server restarted the service.
This change keeps a registered service that is still alive waiting instead of replacing it. A health-probe timeout now only evicts:
ensure()call spawned, orWhile it waits for a live owner,
ensure()also stops recruiting contenders, so a busy service is not raced by a second process. Explicitstop()and version-mismatch replacement remain destructive on purpose.The same change is applied to both the Promise and Effect clients, which share the probe and contender pool.
terminate()also no longer signals a PID that has already exited, so a stale registration cannot signal a reused PID.How did you verify your code works?
busyservice fixture that blocks/api/infountil released, with regression tests in both clients asserting that a live owner survives three probe timeouts and is reused without recruiting a contender. I confirmed the Promise test fails against the previous behavior: the process is killed and the fourth probe never arrives.bun testinpackages/client(236 tests).bun run checkat the repository root.Screenshots / recordings
No UI change.
Checklist
Added by the opencode bot (AI-generated).
Fixes #51216
Related: #58111, #54810, #52049, #58991, #37795, #54181