Repository navigation
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 72 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
There was a problem hiding this comment.
Requesting changes. The owned-contender part works, but the shared startedAt window blocks recovery from a service that crashes soon after it registers.
I checked this with the client and CLI service test suites (all pass) and tsc in both packages. I also ran a scratch test where the registration has a fresh startedAt but its process is dead. On the base, ensure() spawned a replacement in ~240 ms. With this PR, it never spawns and fails with "Timed out waiting for the background service to start" at the deadline. With the real timing, every client would wait 120 s and then error. register() runs as soon as the server starts listening, before the slow startup work described in #54810, so a crash in that phase is exactly the case this window catches. See the inline comment.
The updated "prior failure" tests also show a behaviour change: a hung contender this client spawned is no longer evicted. ensure() now waits out the whole deadline, reports the other contender's failure, and leaves the hung process registered. If that's intended, please call it out in the description.
The owned-contender guard is the same as the one in the open #54883 (same starting(pid) helper and the same two guards), so maintainers will likely want to choose between the two.
The reformatting of the rename/retry chain in service-registration.ts is unrelated and can be dropped.
Public API: adds an optional startedAt (epoch ms) to the client Info type, the Effect Info schema and the service.json registration file. This is backwards compatible: older registrations without it still decode, and the field is optional for readers.
Related: #54810
| } | ||
|
|
||
| /** Keep a registered service's shared startup window open for every concurrent client. */ | ||
| export function withinStartupWindow(info: Info, timing: EnsureTiming) { |
There was a problem hiding this comment.
This window only looks at the clock, so it also covers registrations whose process is already dead. When the probe gets connection refused (not a timeout), starting is still true, so !starting && pool.shouldRecruit(...) never spawns, and every client fails with a timeout at the deadline. On the base, a replacement is spawned after one spawn delay. I reproduced this with a registration that has a fresh startedAt and an exited pid: the base recovered in ~240 ms, while this PR timed out. Since register() runs as soon as the server is listening, a crash during the slow startup phase hits this. Could the shared window apply only while the registered pid is alive (e.g. process.kill(pid, 0)), and only to the eviction-after-timeouts path rather than to recruitment?
| if (Date.now() >= deadline) | ||
| throw pool.failure() ?? new Error("Timed out waiting for the background service to start") | ||
| const registration = await registered(options.file, timing.requestTimeout) | ||
| const starting = |
There was a problem hiding this comment.
starting now also stops recruitment for any registration inside its window, including one owned by another client. Before, an unanswered registration got one spawnDelay before a lock-probe contender competed with it. So the PR now changes more than eviction: a registration that never answers (dead, or bound to a port it no longer serves) blocks every client for up to promiseTimeout. The same logic is in effect/service.ts.
Issue
Related to #54810, #54883, and #59043.
What does this PR do?
ensure()alive through the existing startup deadline instead of evicting it after three short health-probe timeouts.startedAtmetadata in the managed-service registration so concurrent clients can make the same startup decision.This is scoped to the slow-starting managed-service retry loop. The separate live-but-busy incumbent case is handled by #59043.
How did you verify your code works?
bun testinpackages/client: 235 passed, 4 skipped.bun test test/service.test.ts test/service-lifecycle.test.tsinpackages/cli: 26 passed, 10 skipped.bun run checkpassed with Bun 1.4.4; lint reported existing warnings but no errors, and all 35 typecheck tasks passed.Visual
Added by the opencode bot (AI-generated).
Related: #54810
Public API: adds optional
startedAtto the clientInfotype/schema and the service registration file (backwards compatible)