Repository navigation
fix(cli): wait out each stopped process on its own timeout - #681
Conversation
`raven web --stop` computed one `time.monotonic` deadline and passed the same stamp to both waits. They run in sequence -- the supervisor has to be gone before the gateway is signalled, or it restarts it -- so the gateway was charged for however long the supervisor took, down to no wait at all, while the failure it printed still named the whole 20s budget. Observed against a gateway that had been up for hours: `--stop` reported the gateway still running after 20s, and that process was gone by the next poll. A fresh instance stops in 1.8s total, which is why the budget looked correctly sized. `_await_exit` now takes a duration and derives the deadline itself, so two waits cannot share one; the two sibling wait helpers in this file already took a duration for the same reason. The number the message reports is true by construction rather than restated, and the worst case is two budgets instead of one, which the constant now documents. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the PR's two-file diff and the surrounding stop/supervisor, gateway-state, relaunch, and installer paths. Giving each sequentially signalled process its own duration preserves the required supervisor-first ordering and makes the reported 20-second allowance true for each named process; the helper is private and has no other callers, so the parameter semantic change has no compatibility exposure. The regression tests exercise both the formerly false gateway timeout and the full two-process failure case with a virtual monotonic clock, without weakening existing coverage.
I also checked the branch history and base, AGENTS.md and CONTEXT-MAP.md constraints, naming and architecture boundaries, diff hygiene, and whether the change introduces a backward-compatibility issue. I found no plain error. Local verification: uv run pytest tests/test_cli_serve_commands.py -x -> 95 passed. At review time, lint and non-unit CI checks shown by GitHub were passing; four unit shards were still in progress.
Handsome-wzw
left a comment
There was a problem hiding this comment.
Approved. The fault is real and the shape of the fix is right: a duration, with the deadline derived where the wait happens, is what makes two sequential waits independent. Sharing one stamp charged the gateway for however long the supervisor took.
The second test is the one I would keep if I could only keep one. unresponsive is printed as one list under one number, which makes that number a claim about every name in it -- and under a shared deadline the second name was a claim about a wait that had already been spent. Asserting the elapsed clock is at least two budgets, not just that both names appear, is what pins the honesty of the message rather than its wording. The fake clock keeps that instant instead of forty seconds, and bounding the overshoot at one poll stops it passing for the wrong reason.
One thing worth saying so nobody reads more into this than it fixes. On this machine the supervisor exits in well under a second and the gateway takes about forty, so the gateway was already getting nearly the whole twenty. Measured today across several stops: 40s, 45s, 47s from signal to both gone. So a reader who sees
error: still running after 20s: gateway (pid NNNNN)
will still see it after this change -- the gateway now gets a full 20s of its own instead of 19.5s, and it needs about twice that. What this change does fix is the case the tests describe, where the supervisor is slow and the gateway is charged for it, and it makes the printed number true of every process named. Both are worth having on their own; neither is the ceiling being too low.
That ceiling is a separate decision, and the docstring is right that raising it would paper over a process ignoring SIGTERM. Whatever is decided there, it wants the measurement above rather than another guess.
CI 20 green, 0 failing, 0 unresolved threads.
## Summary `raven web --stop` computed one `time.monotonic` deadline and passed the same stamp to both of its waits. Those waits run in sequence, and they have to: the supervisor restarts the gateway, so it must be confirmed gone before the gateway is signalled. A shared stamp therefore charges the gateway for however long the supervisor took, down to no wait at all, while the failure it prints still names the whole 20s budget. The result is a failure reported on a successful stop. Observed against a gateway that had been up for hours with live sessions: `--stop` printed `error: still running after 20s: gateway (pid ...)`, and that process was gone by the next poll. It does not reproduce on a fresh instance, which stops in 1.8s in total, and `--supervise` exists precisely to keep one resident. The fix is in the parameter rather than in a reset. `_await_exit` now takes a duration and derives its own deadline, so there is no stamp to share and the defect is unreachable rather than corrected. The two sibling wait helpers in the same file, `_bound_port_of` and the attach wait, already took a duration and derived the deadline internally; this one was the only deviation, and the only site where the sharing could happen. The number the message reports is now true by construction, because every process it names was given that long, and `_STOP_WAIT_S` documents the per-process contract and the resulting worst case of two budgets. Checked and deliberately not changed: the installers run `web --stop` and continue into `web --foreground` whether or not the stop succeeded, so the refusal this code raises is swallowed. That was verified rather than assumed. `_run_foreground` starts `_gateway_argv`, which returns `serve` while the old flock is held, and `serve` builds no ChannelManager. It is narrower than it looks, because `web` attaches to a gateway that still answers before it reaches that path, so reaching it needs a process wedged enough not to answer and alive enough to hold the lock. Making the installers stop on a failed stop reverses the deliberate decision in #476 that they end on a running page, which is a product call rather than part of this fix. With the timeout corrected that refusal stops being mostly false positives, so it is worth deciding on its own. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Both regressions were written first and watched fail against the unfixed code. One assertion was then corrected, because the poll loop overshoots its deadline by up to one poll, so the corrected pair was re-run against the unfixed code as well to confirm the looser bound is still load-bearing. With only `raven/cli/serve_commands.py` reverted to `main`: `test_the_gateway_gets_a_wait_of_its_own` raises `typer.Exit` printing `error: still running after 20s: gateway (pid 222)`, the same message the bug produces, and `test_both_processes_it_names_waited_the_time_it_reports` measures a 20.0s total wait against the 40.0s the message claims for the two processes it names. Both pass with the fix. The tests drive a virtual clock that advances only when the code under test sleeps, so two 20s budgets are measured in no real time. Commands and results: - `uv run --frozen --python 3.12 --all-extras pytest tests/test_cli_serve_commands.py -q` gives 95 passed. - `uv run --frozen --python 3.12 --all-extras pytest -q` gives 1 failed, 25181 passed, 113 skipped in 420s. The failure is `tests/test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for`, a local node-directory probe. With both changed files reverted to `main` the same id fails identically, so 0 failures are introduced. - `make lint-python` gives ruff check clean and ruff format clean over 2046 files. - `make lint-imports` gives 10 contracts kept, 0 broken. - `make lint-types` gives all checks passed. - `scripts/check_commit_messages.py`, `scripts/check_large_files.py`, `scripts/check_source_language.py` and `npx commitlint` all exit 0. `make lint` does not complete as a whole because `lint-ui` needs a `node_modules` install that was not available. This change is Python only and touches no UI file. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed The docs box is left unchecked because nothing needed updating: no page under `docs/` or `docs-site/` states a stop budget, and the message wording is unchanged. ## Risk A `raven web --stop` that has to wait out both processes can now take up to two budgets, 40s, instead of one, and it refuses in fewer cases, because a gateway that used to be reported unresponsive after a short remainder now gets a full 20s of its own. Nothing that stopped cleanly before behaves differently: a fresh instance still stops in about 1.8s, and the exit code, the message wording and the order the two signals are sent in are all unchanged. Rollback is a revert of the single commit; the change is confined to one private helper and its only caller. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
Summary
raven web --stopcomputed onetime.monotonicdeadline and passed the samestamp to both of its waits. Those waits run in sequence, and they have to: the
supervisor restarts the gateway, so it must be confirmed gone before the gateway
is signalled. A shared stamp therefore charges the gateway for however long the
supervisor took, down to no wait at all, while the failure it prints still names
the whole 20s budget.
The result is a failure reported on a successful stop. Observed against a gateway
that had been up for hours with live sessions:
--stopprintederror: still running after 20s: gateway (pid ...), and that process was gone by the next poll.It does not reproduce on a fresh instance, which stops in 1.8s in total, and
--superviseexists precisely to keep one resident.The fix is in the parameter rather than in a reset.
_await_exitnow takes aduration and derives its own deadline, so there is no stamp to share and the
defect is unreachable rather than corrected. The two sibling wait helpers in the
same file,
_bound_port_ofand the attach wait, already took a duration andderived the deadline internally; this one was the only deviation, and the only
site where the sharing could happen. The number the message reports is now true
by construction, because every process it names was given that long, and
_STOP_WAIT_Sdocuments the per-process contract and the resulting worst caseof two budgets.
Checked and deliberately not changed: the installers run
web --stopandcontinue into
web --foregroundwhether or not the stop succeeded, so therefusal this code raises is swallowed. That was verified rather than assumed.
_run_foregroundstarts_gateway_argv, which returnsservewhile the oldflock is held, and
servebuilds no ChannelManager. It is narrower than itlooks, because
webattaches to a gateway that still answers before it reachesthat path, so reaching it needs a process wedged enough not to answer and alive
enough to hold the lock. Making the installers stop on a failed stop reverses the
deliberate decision in #476 that they end on a running page, which is a product
call rather than part of this fix. With the timeout corrected that refusal stops
being mostly false positives, so it is worth deciding on its own.
Type
Verification
Both regressions were written first and watched fail against the unfixed code.
One assertion was then corrected, because the poll loop overshoots its deadline
by up to one poll, so the corrected pair was re-run against the unfixed code as
well to confirm the looser bound is still load-bearing.
With only
raven/cli/serve_commands.pyreverted tomain:test_the_gateway_gets_a_wait_of_its_ownraisestyper.Exitprintingerror: still running after 20s: gateway (pid 222), the same message the bug produces,and
test_both_processes_it_names_waited_the_time_it_reportsmeasures a 20.0stotal wait against the 40.0s the message claims for the two processes it names.
Both pass with the fix. The tests drive a virtual clock that advances only when
the code under test sleeps, so two 20s budgets are measured in no real time.
Commands and results:
uv run --frozen --python 3.12 --all-extras pytest tests/test_cli_serve_commands.py -qgives 95 passed.
uv run --frozen --python 3.12 --all-extras pytest -qgives 1 failed, 25181passed, 113 skipped in 420s. The failure is
tests/test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for,a local node-directory probe. With both changed files reverted to
mainthesame id fails identically, so 0 failures are introduced.
make lint-pythongives ruff check clean and ruff format clean over 2046 files.make lint-importsgives 10 contracts kept, 0 broken.make lint-typesgives all checks passed.scripts/check_commit_messages.py,scripts/check_large_files.py,scripts/check_source_language.pyandnpx commitlintall exit 0.make lintdoes not complete as a whole becauselint-uineeds anode_modulesinstall that was not available. This change is Python only and touches no UI file.
The docs box is left unchecked because nothing needed updating: no page under
docs/ordocs-site/states a stop budget, and the message wording is unchanged.Risk
A
raven web --stopthat has to wait out both processes can now take up to twobudgets, 40s, instead of one, and it refuses in fewer cases, because a gateway
that used to be reported unresponsive after a short remainder now gets a full
20s of its own. Nothing that stopped cleanly before behaves differently: a fresh
instance still stops in about 1.8s, and the exit code, the message wording and
the order the two signals are sent in are all unchanged.
Rollback is a revert of the single commit; the change is confined to one private
helper and its only caller.
Related Issues
N/A