Repository navigation
Conversation
Closes anomalyco#54267 Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Remove the port marker after the listener has closed and verify marker-owned services before termination. Replace lifecycle sleeps with polling.\n\nCo-authored-by: Copilot <[email protected]>
Require a live marker PID and occupied endpoint before terminating an unresponsive owner, and synchronize the marker regression test with the fixture.\n\nCo-authored-by: Copilot <[email protected]>
|
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.
With the bot's reproduction for #54267 (registration deleted, old service paused with SIGSTOP), the base fails after about 21 s with "Managed service port … already in use". With this PR, service start kills the unresponsive owner it finds through the .port marker and comes up on the same port in about 15 s. Client tests (26) pass, and typecheck passes in packages/client and packages/cli. The new client test fails on the base, so it does guard against the bug.
Public API: no exported signatures change. The only external addition is a new state file, <registration>.port (for example service.json.port), written and removed by the service. Nothing reads it except the client. Backwards compatible.
Worth fixing:
- The new CLI lifecycle test passes unchanged on the base, so it does not cover the fix (see inline).
- The extra registration read added on every poll in
ensureis redundant (see inline).
| signal(original.pid, "SIGSTOP") | ||
| await fs.rm(home.registration) | ||
| const starting = cli(home, "service", "start") | ||
| await until(async () => { |
There was a problem hiding this comment.
This test also passes on the base branch; I ran it there unchanged. until returns as soon as servers(home) is [original.pid], and that is already true before the client has done anything, so the wait checks nothing. After SIGCONT, the old service sees its registration is gone and shuts down by itself, and the base contender's 15 s bind retry then succeeds. Could you either make this fail without the fix (e.g. leave the old service paused and expect it to be replaced, which is the case from #54267), or drop it and rely on the client test, which does fail on the base?
| const found = yield* Effect.gen(function* () { | ||
| const registration = yield* registered(options.file, timing.requestTimeout) | ||
| const registrationFile = options.file ?? fallback() | ||
| const registrationPresent = (yield* read(registrationFile)) !== undefined |
There was a problem hiding this comment.
This adds a second read of the registration file on every poll (every 25 ms), and the result doesn't change anything: registered() only returns info when the registration or the marker was read, so registrationPresent || registration.markerOnly is always true when info !== undefined. The same applies in promise/service.ts (line 55). Dropping the extra read and the condition would keep the behaviour and make the loop simpler.
Issue for this PR
Closes #54267
Type of change
What does this PR do?
A managed service can lose its registration while its process still owns the configured port. The port marker remains available for that recovery case, while marker-based termination now requires the recorded PID to be live and the recorded endpoint to still own a listening port. The marker regression test synchronizes with the fixture instead of relying on a fixed time window.
How did you verify your code works?
packages/client:bun run typecheckpackages/cli:bun run typecheckgit diff --checkScreenshots / recordings
Not applicable.
Checklist
Added by the opencode bot (AI-generated).
Related: #54181, #50672