fix(process-compose): reset user-stopped services in startService#5814
fix(process-compose): reset user-stopped services in startService#5814martijnwalraven wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8018c45b56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const state = yield* SubscriptionRef.get(svc.state); | ||
| if (state.status === "Stopped" || state.status === "Failed") { | ||
| yield* FiberMap.remove(fibers, d.name); | ||
| yield* resetService(d.name); |
There was a problem hiding this comment.
When startService is called for a dependent while its existing fiber is still pending on this service's old readiness signal (for example, api waits for db to become healthy, db fails before healthy, then /services/api/start is requested before api times out), this reset replaces db's healthy/started Deferreds but the later onlyIfMissing run skips the already-pending api fiber. That fiber is still awaiting the old Deferred and will not observe the restarted dependency becoming ready, so waitReady(api) can hang or fail on dependency timeout even though the dependency recovered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f0fe642: the start-path reset (resetForStart) now replaces a signal Deferred only when it is already resolved, so a waiting dependent stays attached to the same healthy signal the restarted dependency resolves. Regression test: "startService of a failed dependency unblocks a waiting dependent".
| const state = yield* SubscriptionRef.get(svc.state); | ||
| if (state.status === "Stopped" || state.status === "Failed") { | ||
| yield* FiberMap.remove(fibers, d.name); | ||
| yield* resetService(d.name); |
There was a problem hiding this comment.
If startService races with stop(), a dependent may have already reached Stopped and resolved its stopped Deferred while one of its dependencies has not yet looked up that signal. Resetting the terminal dependent here recreates stopped; stop() then awaits the fresh unresolved Deferred even though that dependent's stop path already finished, so dependency-ordered shutdown can hang until the global shutdown timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — and with the resolved-only gating above this can still replace an already-resolved stopped signal during a concurrent stop(), turning the pre-existing start-during-stop wart (an unreaped respawn) into a wait until the shutdown timeout. Start-during-stop needs lifecycle-verb serialization to be right under any semantics; scope proposal in the PR comment (per-service locks shared by start/stop/restart — here or as a follow-up issue).
| if (svc !== undefined) { | ||
| const state = yield* SubscriptionRef.get(svc.state); | ||
| if (state.status === "Stopped" || state.status === "Failed") { | ||
| yield* FiberMap.remove(fibers, d.name); |
There was a problem hiding this comment.
When two startService calls overlap for the same stopped/failed service (or for two targets that share that dependency), both can read the old terminal state before either reset completes. The later caller then reaches this removal after the first caller has already reset and inserted a fresh run, so it interrupts the newly-started process and resets its signals again, causing duplicate spawns or a spurious kill from concurrent start requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f0fe642: each service's start step now serializes on a startGate semaphore, so a second concurrent caller observes the reset state and coalesces via onlyIfMissing instead of removing the first's fresh fiber. Regression test: "concurrent startService calls coalesce on one fresh run".
stopService marks a service stoppedByUser and drives its state machine
to Stopped — a state the transition table only exits via
RestartTriggered. startService re-ran the fiber (the process starts
and serves) but never reset, so every subsequent event was dropped by
applyEvent: the service stays Stopped for getState/state streams
forever, the unless-stopped restart policy stays disarmed, and the
stack README's documented stop→start cycle ("Restart it (blocks until
ready)") never worked through startService.
The fix mirrors restartService's remove-reset-run sequence
(stopForRestart): the fiber-map entry is removed before the reset — a
terminal fiber not yet pruned would make the onlyIfMissing run a no-op
and strand the freshly reset service in Pending — then the state
machine resets, then the run proceeds. Gated on terminal state
(Stopped/Failed) only: a service still Stopping under an in-flight
stopService is not reset out from under that stop.
Hardened against the PR review's concurrency findings: the start-path
reset (resetForStart) replaces a signal Deferred only when already
resolved — an unresolved signal can have live waiters (a dependent's
dependency await, a waitReady caller) that replacement would orphan,
while a resolved one has no waiters by definition and must be replaced
so the fresh life can re-attest it. And each service's start step
serializes on a startGate semaphore, so concurrent startService calls
coalesce on one fresh run instead of the second removing the first's
fiber. Regression tests: the stop→start cycle, the failed→start heal,
a probed dependency that failed before ever becoming healthy
unblocking its waiting dependent through the preserved signal, and
concurrent starts spawning exactly once.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
martijnwalraven
force-pushed
the
callboard/start-service-reset
branch
from
8018c45 to
f0fe642
Compare
|
Updated the branch with fixes for the review findings — and apologies for the force-push; further changes to this PR will be append-only commits.
|
1 participant
Problem
stopServicemarks a servicestoppedByUserand drives its state machine toStopped— a state the transition table only exits viaRestartTriggered.startServicere-runs the fiber (the process starts and serves) but never resets the state machine, so every subsequent event is dropped byapplyEvent: the service staysStoppedforgetState/state streams forever, and theunless-stoppedrestart policy stays disarmed.This breaks the stop→start cycle
packages/stack/README.mddocuments on the public handle (await stack.stopService("auth"); await stack.startService("auth"); // Restart it (blocks until ready)). No CLI command callsstartServicetoday, so this surfaces only for programmatic embedders of the stack/orchestrator.Fix
Mirror
restartService's own remove-reset-run sequence (stopForRestart): remove the fiber-map entry (a terminal fiber not yet pruned would make theonlyIfMissingrun a no-op and strand the freshly reset service inPending), reset the state machine, then run. The reset is gated on terminal state (Stopped/Failed) only — a service stillStoppingunder an in-flightstopServiceis not reset out from under that stop; the stop completes toStoppedand a subsequent start heals it.Tests
startServiceafterstopServiceresets the user-stopped state machine (stop→start reachingHealthyon a second spawn)startServiceafter terminal failure resets and re-runs (failed→start heal)🤖 Generated with Claude Code