feat: add depends_on condition: completed for one-off programs - #26
Merged
Conversation
started waits for the process and healthy waits for readiness. Neither expresses "wait for the work to finish", which is what a migration, an init task or a seed job is depended on for. Naming one in depends_on today fails two ways: with condition: started the dependent runs alongside the task rather than after it, and once the task has exited a dependent can never start at all, so one that crashes an hour later cannot come back. A program latches as completed when a run it was not asked to stop exits with status 0. The latch is set before the EXITED state change, since that change is what wakes the dependents, and it survives the program sitting in EXITED. Running the program again clears it, so sctl restart re-runs the work and latches again on success; reload replaces the process outright, so a redefined task runs on its own account. A task that cannot complete drives its dependents to the existing "will never come up" path rather than an indefinite wait: FATAL was already covered, and an unsuccessful exit under autorestart: never is now covered too. autorestart: always on a program something waits to complete is rejected at startup, naming both programs. Two waits had to be fixed for the re-run flow to work at all, both reachable today with any one-off: - stopping a program that had already exited waited for STOPPED specifically, which the reconciler never moves it to, so the request timed out after 30s; - starting a program that finished its work inside startsecs waited for RUNNING, which it never reaches, so a successful run was reported as a failure. probes/completed-latch.sh reproduces both failures against condition: started and shows the latch holding across a dependent restart, which is the part a unit test shows least convincingly. Closes #25
Stopping a program that had already exited reported success without releasing it. EXITED is not running, but it is not settled either: the monitor may be sitting in a restart backoff that only Stop() cancels. The reconciler skipped every stopped state, so a stopped program spawned a fresh run one backoff after the stop was reported as done, and a reload that replaced it discarded the Process object while that run was still queued, leaving a child nothing tracked, nothing could stop and nothing would reap. The reconciler now releases anything that is not already STOPPED when its desired state is STOPPED, which cancels the pending restart. That is the root of what the previous commit worked around by accepting EXITED and FATAL as "stopped", so those predicates go back to waiting for STOPPED specifically, and now get it. A stop no longer reports that a program cannot start. awaitState's "a dependency of this will never come up" shortcut applied to stop requests too, so stopping a program whose dependency happened to be stopped failed with "process api cannot start: dependency db is stopped and is not set to start" while the stop was proceeding normally, and sctl restart propagated it instead of starting anything. Dependencies decide what may start, never what may stop. Also widen the window TestReconcile_WaitsForAOneOffToComplete asserts RUNNING in, keep the probe's closing diagnostics from aborting under set -e when the daemon is the thing that died, and note in the README that a task with autostart: false holds its dependents back across a daemon restart until it is started by hand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #25.
startedwaits for the process,healthywaits for readiness. Neither expresses "wait for the work to finish", which is what a migration, an init task or a seed job is depended on for. This adds the third condition.Semantics
As proposed in the issue:
autorestart: unexpecteddraws.EXITED, which is what the current behaviour gets wrong.sctl restart migratere-runs the work and latches again on success. Dependents already up are left alone.One decision the issue did not cover:
sctl stopon a completed task does not clear the latch. Stopping a program that has already exited does not undo the work it did.Implementation
The latch lives on
Process(internal/process/process.go), set in the monitor when a run nobody asked to stop exits 0, cleared inStart(). It is set before theEXITEDstate change, because that change is what wakes the dependents — setting it afterwards would have them look and find nothing. Reload replaces theProcess, so the latch clears there without any extra bookkeeping.blockedByDependency(internal/server/reconcile.go) became condition-aware, which it could not be while it walked the dependency graph: the graph carries ordering, the config carries conditions. It now also treats an unsuccessful exit underautorestart: neveras final, which is the second way a task fails to complete —FATALwas already covered.Validation rejects
autorestart: alwayson a program something waits to complete, naming both programs.autorestart: unexpectedis the natural pairing and is accepted.Two pre-existing waits this had to fix
Both are reachable today with any one-off, and
sctl restart migrate— which the issue requires to re-run and re-latch — hits both:StopProcesswaited forSTOPPEDspecifically. The reconciler deliberately leavesEXITEDalone, so stopping a program that had already exited timed out after 30s.StartProcesswaited forRUNNING. A task that finishes insidestartsecsnever reaches it, so a successful run was reported as a failure after 30s.StopProcessnow accepts any settled state, andStartProcessaccepts a completed run. Both are covered by tests that measure the elapsed time, so a regression shows up as a timeout rather than as a slow pass.Tests
Unit tests cover the four cases the issue lists, plus the latch's placement relative to the state change, a failed exit not latching, and the two waits above.
probes/completed-latch.shruns the whole shape in a container.--startedis the control, and reproduces both of today's failures:against the same configuration with
condition: completed:Documentation
README.mdgains a Waiting for a one-off to finish subsection under Dependency Management, and theBest practicessection from #10 is rewritten: it told people not to do this, and now points at the supported way of doing it.