Repository navigation
fix: never report a launching process as Disabled - #548
shivakumargn wants to merge 3 commits into
Conversation
|
| // to skip this process. A manual start must use an enabled runtime copy, | ||
| // however, or its new state remains Disabled while it waits for | ||
| // dependencies and the TUI falsely reports it as not started. | ||
| processConfig.Disabled = false |
There was a problem hiding this comment.
Side effect worth calling out: watchAdd(&processConfig) further down gets this same copy. With Disabled cleared, isWatchEligible now passes, so a disabled (or up <proc> deselected) process gets a file watch when started by hand. Before this change that call was always a no-op for them.
The comment down there says that's the intent, so I'm not against it, but it is a behavior change and it's not in the description or the test. A few things follow from it:
- once a one-shot started this way exits, the armed watch keeps the project from completing
- the first save restarts it through
launchUnlessStopped, which runs into the Disabled issue above - it only kicks in if some other process had a watch at startup, because
startWatcherdoesn't create the watcher otherwise
If you go with the newIncarnationState change, this goes back to the old behavior and we can handle the watch question separately. If you'd rather keep it, please mention it in the description and add a test for it.
There was a problem hiding this comment.
Fixed. StartProcess no longer touches the config, so watchAdd gets the original disabled config and behaves as before. The watch question can be handled separately.
| // however, or its new state remains Disabled while it waits for | ||
| // dependencies and the TUI falsely reports it as not started. | ||
| processConfig.Disabled = false | ||
| p.runProcess(&processConfig) |
There was a problem hiding this comment.
This fixes the start path, but runProcess is also called from launchUnlessStopped, and that one still reads the stored config with Disabled: true. So a restart brings the old state right back.
Repro: disabled manual depends on dep (process_healthy). Start manual and wait for Running, make dep not ready, then RestartProcess("manual"). While it waits, manual shows Disabled again and IsReadyReason() returns true, "process is disabled", so is-ready and the TUI treat it as ready even though it hasn't launched. You get the same result from Ctrl+R in the TUI, process restart, the MCP restart tool, or a watch trigger.
I'd move the fix one level down. newIncarnationState is only called from runProcess, and anything that gets there is being launched, so it should never come out as Disabled:
state := types.NewProcessState(config)
// Whatever reaches runProcess is being launched, so it is never Disabled,
// even if the stored config still says so (manual start, restart).
if state.Status == types.ProcessStateDisabled {
state.Status = types.ProcessStatePending
}That covers start and restart in one place, and StartProcess doesn't have to touch the config at all. Could you also add a restart case to the test? Start it, make the dependency not ready, restart, and expect Pending.
There was a problem hiding this comment.
Done as suggested. The fix is now in newIncarnationState, so start and restart both get Pending. I added a restart case to the test. One difference from the suggested repro: I restart while manual is still Pending on an unready dependency, rather than after it reached Running. Once a dependency has been ready, that is latched, so if it later fails, a restart ends up Skipped instead of waiting. Restarting during Pending goes through the same launchUnlessStopped path with the stored Disabled: true config, and the assertion fails without the fix.
A disabled process that is started or restarted manually read the stored config in runProcess, so its new state came out as Disabled while it waited for dependencies. The TUI showed it as not started and IsReadyReason reported it as ready. Normalize the status in newIncarnationState instead of mutating the config in StartProcess. Anything reaching runProcess is being launched, so it is Pending. This covers start and restart (TUI, CLI, MCP, watch) and leaves watch registration as it was.
|
shivakumargn
changed the title
Summary
Fix manual start and restart of processes configured with
disabled: true.A disabled process that was started or restarted by hand read the stored
config in
runProcess, so its new state came out asDisabledwhile it waswaiting for dependencies. The TUI showed it as not started, and
IsReadyReason()reported it as ready ("process is disabled").newIncarnationStatenow turns a would-beDisabledstatus intoPending.Anything that reaches
runProcessis being launched, so it is neverDisabled. This covers manual start and every restart path (TUI Ctrl+R,process restart, the MCP restart tool, watch triggers) in one place.The stored project config stays
disabled: true, so initial startup stillskips the process.
StartProcessno longer modifies the config, so file-watchregistration behaves as before.
Changes since the first revision
Per review feedback:
StartProcessintonewIncarnationState, so restartsare covered too.
StartProcess, which also removes thewatchAddside effect.Tests
TestStartDisabledProcessIsPendingUntilDependenciesAreHealthycovers:Disabled -> Pending -> Runningon manual startPendingon an unready dependency: state isPendingandIsReadyReason()reports not readydisabled: trueThe restart assertion fails without the fix (
want Pending, got Disabled).make lintandmake testracepass on Linux (Go 1.27).