Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fixes the problem. I ran the #55107 reproduction from source on both checkouts. On the base, a service whose boot failed kept its port, so every later start failed until the process was killed by hand. With this change, the failing start still reports the error, the failed process is stopped, and the next start succeeds once the database is fixed. The client tests and the client type check pass.
Public API: no signature changes. service.ensure in @opencode/client (effect and promise versions) now also stops a registered service in the failed state before throwing the same error.
Concerns:
packages/cli/test/service-lifecycle.test.ts› "a server that fails to boot reports the failure without respawning" still expects the failed server to stay registered and alive. With this change it times out at line 164 waiting for that registration, while on the base it gets past that point. Please update it to the new behaviour: the start fails, then no server and no registration are left.- If the stop itself fails, its error replaces the original startup error (inline).
| const decision = decide(service, options) | ||
| if (decision._tag === "fail") return yield* Effect.fail(decision.error) | ||
| if (decision._tag === "fail") { | ||
| if (service.state === "failed") yield* terminate(service.info, options, timing) |
There was a problem hiding this comment.
If terminate fails, for example with Server process <pid> is still running after the SIGKILL retries, that error replaces decision.error. The caller then never sees "Background service failed to start". The promise version at packages/client/src/promise/service.ts:76 has the same problem. Making the stop best-effort would keep the startup failure as the reported error: here terminate(...).pipe(Effect.ignore), and in the promise version .catch(() => undefined), as replace already does with stop.
There was a problem hiding this comment.
Updated both clients: failed-service termination is now best-effort (Effect.ignore / .catch(() => undefined)), so a cleanup failure cannot replace the original startup error. The existing lifecycle suites passed all 45 tests, and client typecheck passed.
There was a problem hiding this comment.
Thanks, I checked afe7a525f. Both clients now stop the failed service as best-effort, so the original "Background service failed to start" error is kept. That resolves this thread.
One point from the review is still open. The CLI test packages/cli/test/service-lifecycle.test.ts › "a server that fails to boot reports the failure without respawning" still expects the failed server to stay registered and alive. With this change it times out at line 164 waiting for that registration. The 45 tests you mention are probably the packages/client suites, but this test is in packages/cli. Please update it to the new behaviour: the start fails, then no server and no registration are left.
There was a problem hiding this comment.
Updated the CLI regression to expect failed-start cleanup: no server or registration remains, and the port can be rebound, without calling stop to perform cleanup. The focused CLI case passes (1 test, 2 assertions), and CLI typecheck passes.
The earlier 45-test result was the client suites. The full CLI lifecycle run was interrupted after an unrelated concurrency case failed because process discovery returned no PIDs in this sandbox despite a responding service. Lint and formatting pass; full workspace checks still fail on Astro preferences permissions.
There was a problem hiding this comment.
Thanks, I checked 582a7a6fc. The CLI case now expects the new behaviour: the start fails, then no server process and no registration are left, and the port can be bound again, all without calling stop. That covers the last open point from my review. The other lifecycle failures you saw also fail on the base in my sandbox, so they don't look related to this change.
opencode service stop is run
#62326
|
Tested this on a Windows checkout, and the behaviour is right. Two things worth fixing before merge. The branch is behind The 2 failures are the new assertion, not the cleanup. Both Waiting for the exit first makes both pass: await process.exited
expect(process.exitCode).not.toBe(null)With that change the two tests are green ( Two smaller notes:
After a rebase plus the |
Issue for this PR
Fixes #55107
Type of change
What does this PR do?
When a registered service reports failed boot, both clients now attempt to stop that owner before returning the startup error. Cleanup is best-effort so termination failure cannot mask the original boot error. This releases its port and registration so a later launch can recover after the underlying cause is corrected. Cleanup uses the existing registration identity checks and bounded stop escalation.
The server’s failed-health response stays available until a client consumes the failure. This does not change slow-start recovery or database initialization.
How did you verify your code works?
URL/globalThistype errors).Screenshots / recordings
No UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #54071, #54810