Skip to content

fix(*): retire every generation's skill watcher before the process exits - #490

Merged
0xKT merged 5 commits into
mainfrom
fix/gateway_sigterm_segfault
Sep 19, 2026
Merged

0xKT merged 5 commits into
mainfrom
fix/gateway_sigterm_segfault

Conversation

@LivXue

@LivXue LivXue commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

Summary

raven gateway exited with SIGSEGV (-11, or 139 in a shell) on every SIGTERM,
so a systemd or docker stop recorded a crash instead of the clean stop it was.
raven tui and raven agent had the same defect.

Root cause: LocalSkillCatalog auto-starts SkillFileWatcher, a daemon thread
that parks inside the Rust watch() of watchfiles. Daemon status does not make
process exit safe while the thread sits in native code -- CPython runs
Py_FinalizeEx under that call. Every Python shutdown step succeeds first, so
the crash lands after the graceful chain is complete and the real exit code is
masked. With faulthandler armed all three faults report <no Python frame>,
i.e. no Python code is on the stack.

Every owner now retires it:

  • the gateway's shutdown chain, for the generation still bound at exit;
  • the TUI's teardown;
  • the one-shot agent's teardown;
  • RavenRuntime.dispose, for a generation retired at a swap. build_runtime
    mints a fresh AgentLoop per generation, so each swap starts a new watcher
    and abandons the previous one; without this a reloaded gateway still
    segfaults on stop. The generation-organ roster grows the same entry, so the
    two halves of that contract stay in step;
  • the gateway's shutdown again, for a candidate staged by a reload that the
    serving loop never consumed. That generation is not the bound agent, so
    nothing else reaches it;
  • the gateway's shutdown once more, for a candidate take() handed out whose
    binding a cancelled unbind never finished. _serve_generations holds that
    candidate in a local across await _unbind_generation(), so a shutdown
    cancelling the await unwinds the coroutine and drops the local, leaving the
    generation staged nowhere and bound nowhere. SwapCoordinator now keeps it:
    take() records the candidate it hands out and release() clears it, which
    spans exactly handover to the moment the caller's own binding points at the
    new loop. The serving loop needed no new line, because it already calls both
    ends of that span.

The replay runner already stopped the same watcher for the same reason; these
were the other owners. The stop sits in the generation contract and in each
command's teardown rather than in AgentLoop.stop, whose other caller is the
generation swap -- that path keeps the process running and must keep skill
auto-refresh with it.

The gateway's sweep now lives in a module-level _retire_generation_watchers
rather than inline in the serve closure. run() is a 550-line closure with no
import seam, so the inline form could only be pinned by reading its source text
-- which never executes it, and so earned no coverage on the lines that matter.
The helper drains all three seats and keeps going when one stop raises, since a
sweep that stops early still leaves a watcher parked in native code.

Two comments describing a removed guard go with it: one in agent_commands
pointing at an exit chokepoint in raven.cli.commands.run, and the test-side
scaffolding that patched os._exit for an exit the command no longer makes.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

Process-level, faulthandler armed, throwaway home and a dummy key nothing calls:

                                      before   after
raven gateway   SIGTERM                 -11      0     (-11 on 5 of 5 runs)
raven gateway   SIGHUP then SIGTERM     -11      0     (2 reloads after)
raven tui       PTY, then signal        -11      0
raven agent     ordinary one-shot exit  139      1     (1 is the turn's own
                                                        auth failure, which
                                                        139 had been masking)

Controlled pair isolating the cause, on the loop factory the TUI uses -- build
the loop, let the interpreter finalize:

watcher left running   exit 139
stop_file_watcher()    exit 0

Suites, from the branch tree:

uv run pytest tests/integration/test_gateway_sigterm_smoke.py -q -n 0 -m integration
  1 passed          (this is the guard test that was failing)

pytest tests/test_cli_agent_loop_parity.py tests/test_cli_gateway_commands.py \
  tests/test_cli_gateway_health.py tests/test_cli_tui_commands.py \
  tests/test_cli_agent_commands.py tests/test_gateway_spine.py \
  tests/test_generation_swap.py tests/test_plugin_services_lifecycle.py \
  tests/test_rpc_control.py tests/test_rpc_system.py \
  tests/test_skill_watcher_cleanup.py tests/test_trajectory_replay.py \
  tests/test_utils_asyncio_runner.py -q
  307 passed, exit 0

ruff check / ruff format --check over the changed files   clean
ty check over the changed files                           no diagnostics on the changed lines
scripts/check_large_files.py origin/main..HEAD            exit 0
scripts/check_source_language.py origin/main..HEAD        exit 0
commitlint --from origin/main                             exit 0

Second round, after review found the in-transition window; rebased onto
origin/main at 6d8c363:

pytest tests/test_cli_gateway_commands.py tests/test_core_runtime_swap.py \
  tests/test_generation_swap.py tests/test_cli_tui_commands.py \
  tests/test_cli_agent_commands.py -q
  171 passed, exit 0

every test file importing raven.core.runtime          131 passed, exit 0
real gateway: boot, SIGTERM, exit code                0 on 3 of 3, no fault text
ruff check / ruff format --check                      clean
make check-source-language / make check-large-files   exit 0
coverage_gate.py diff --threshold 90                  95.83% (23/24), passed

Revert check on that round: removing the _in_transition record, the
in-transition seat in the sweep, or the sweep's try/except each fails its own
test and nothing else. The cancellation window itself is reproduced against real
SkillFileWatcher threads, counted by thread name, and is red without the fix.

Diff coverage on changed lines was 50% (4 of 8) before that round, because the
gateway-side tests read the chain's source instead of running it. The one line
still uncovered is the call site inside run(), which cannot execute without
booting a real gateway; the wiring to the seam is pinned by source inspection.

Each of the four regression tests was confirmed red before its fix. The TUI and
agent stops were additionally re-confirmed afterwards by removing both again and
watching those two tests go red, then restoring them.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Risk

User-visible change: these three commands now exit with their real status
instead of a segfault. Anything reading the exit code sees a different value --
0 where it used to see 139, and for a failed one-shot the turn's own non-zero
code rather than 139.

Rollback: revert the commits. The watcher is a cache-invalidation helper, so
removing the stops restores the previous behaviour exactly, crash included.

Checked and deliberately not changed:

  • The stops are not wrapped in try/except. ContextBuilder.__init__ assigns
    skills unconditionally, stop_file_watcher is documented safe when no
    watcher was started, and the watcher's own contract is that neither start
    nor stop raises. This matches the existing production caller in the replay
    runner, which is also unwrapped.
  • raven acp reaches finalization the same way but calls os._exit(0) after a
    real session, so the fault is masked there rather than fixed. Left as is;
    making it consistent is a separate change.
  • No central gate was reinstated. A future command that builds a loop and exits
    normally would reintroduce this, and the source-inspection tests here only
    guard the retirement seats that exist today.
  • The watcher starts in ContextBuilder.__init__, which is construction-time
    work, and RavenRuntime.discard is contract-bound to stay call-free because
    a call there would mean an organ was started before FREEZE. So a staged or
    in-transition candidate is retired by the gateway's sweep rather than by
    discard, and the docstring records why. Moving the watcher out of
    construction would let discard own it; that is a separate change.
  • The shutdown-before-consumption window was not reproduced by timing: three
    runs sending SIGTERM 0.2s, 1.0s and 2.0s after a reload all completed the
    swap first and exited 0. The staged-candidate retirement is driven by
    forcing the state in a test rather than by racing for it.

Known gap in the evidence: the two raven tui runs were ended with SIGTERM
after Ctrl+C went unanswered, because the TUI never finished loading against a
throwaway home. The ordinary-exit case is evidenced by the agent one-shot,
which exits by itself with no signal involved.

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Related Issues

N/A

@LivXue
LivXue requested review from 0xKT and gloryfromca September 18, 2026 05:55

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: retire the watcher owned by a staged reload candidate when shutdown wins the swap race.

I found one concrete blocker, marked inline. The direct agent, TUI, gateway-shutdown, and normal generation-unbind stops are otherwise consistent with the watcher's idempotent contract.

I covered the repository rules in AGENTS.md/CLAUDE.md and the Runtime Generation/Assembly Root terms routed by CONTEXT-MAP.md; the complete diff; the LocalSkillCatalog, AgentLoop, RavenRuntime, SwapCoordinator, reload, and shutdown callers; relevant watcher/finalization history; backward compatibility; and whether the tests were weakened. The source-inspection tests follow an existing pattern, but they do not exercise the staged-candidate shutdown window.

Verification:

  • uv run pytest tests/test_cli_agent_commands.py tests/test_cli_gateway_commands.py tests/test_cli_tui_commands.py tests/test_generation_swap.py tests/test_core_runtime_swap.py tests/test_skill_watcher_cleanup.py -q: 165 passed in 14.74s.
  • uv run pytest tests/integration/test_gateway_sigterm_smoke.py -q -n 0 -m integration: 1 passed in 14.44s, with the suite's non-strict idle-ceiling warning.
  • git diff --check github/main...HEAD: clean.

Comment thread raven/cli/gateway_commands.py Resolved

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: keep the taken reload candidate reachable through cancellable unbinding so shutdown can retire its watcher.

The revision correctly moves normal served-generation watcher retirement into RavenRuntime.dispose() and handles candidates that remain staged in SwapCoordinator. The existing thread remains open because cancellation after take() but before rebinding removes the candidate from both cleanup paths; I replied there with the concrete state reproduction. I found no other blockers.

I covered AGENTS.md/CLAUDE.md, the Runtime Generation and Assembly Root contracts routed by CONTEXT-MAP.md, the full github/main...HEAD diff and revision delta, watcher/runtime/coordinator/gateway callers and history, backward compatibility, and whether tests were weakened. The new dispose contract test strengthens the normal path, but the staged-candidate source test does not cover the post-take() transition.

Verification:

  • Focused state reproduction: after stage -> take -> drop the in-transition candidate -> stop the current catalog, 1 SkillFileWatcher remained alive.
  • uv run pytest tests/test_cli_agent_commands.py tests/test_cli_gateway_commands.py tests/test_cli_tui_commands.py tests/test_generation_swap.py tests/test_core_runtime_swap.py tests/test_skill_watcher_cleanup.py tests/test_trajectory_replay.py -q: 202 passed in 10.02s.
  • uv run pytest tests/integration/test_gateway_sigterm_smoke.py -q -n 0 -m integration: 1 passed in 13.90s, with the existing non-strict idle-ceiling warning.
  • git diff --check github/main...HEAD: clean.

LivXue and others added 5 commits September 18, 2026 07:13
LocalSkillCatalog auto-starts SkillFileWatcher, a daemon thread that parks
inside the Rust watch() of watchfiles. Daemon status does not make process
exit safe while the thread sits in native code: CPython runs Py_FinalizeEx
under that call, and the process dies of SIGSEGV after every Python shutdown
step has already succeeded. The crash also masks the command's real exit code.

Measured on this tree with faulthandler armed. Every fault reports
"<no Python frame>", so no Python code is on the stack:

  raven gateway  SIGTERM      exit -11, 5 of 5 runs
  raven tui      PTY signal   exit -11
  raven agent    normal exit  exit 139

A controlled pair on the loop factory the TUI uses isolates the cause: build
the loop and let the interpreter finalize, and the process exits 139; call
stop_file_watcher() first and it exits 0.

All three teardown chains now stop the watcher. The replay runner already
did this for the same reason; these were the other long-lived owners.

The call sits in each command's teardown rather than in AgentLoop.stop, whose
other caller is the gateway generation swap: that path keeps the process
running and must keep skill auto-refresh with it.

Also drops a comment in agent_commands that pointed at the exit chokepoint in
raven.cli.commands.run, a gate removed in #238.

Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
build_runtime mints a fresh AgentLoop per generation, so every swap starts a
new SkillFileWatcher and abandons the previous one. Stopping only the live
generation's watcher in the teardown chain is not enough: a watcher left by an
earlier generation is still parked in the Rust watch() of watchfiles when
Py_FinalizeEx runs, and the process dies of SIGSEGV anyway.

Measured on this tree, boot then SIGHUP then SIGTERM:

  before  1 reload   exit -11
  after   2 reloads  exit 0

_unbind_generation is the generation slice of the shutdown path, and the
outgoing loop is still bound to `agent` there -- the caller rebinds it after
the unbind returns -- so the watcher goes with the generation that owned it.

Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
Three tests monkeypatched os._exit into a SystemExit so a hard exit could not
take the pytest process down, and the comment above the first said the -m path
hard-exits via os._exit(0). That exit was removed when the CLI gained a central
finalization gate, and the gate itself was removed later, so the patches were
inert and the comment described a guard that no longer exists.

The three local `import os as _os` statements existed only for those patches
and go with them.

Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
A reload builds the next generation before the serving loop stops, and
building is what starts that generation's skill watcher. Two of its exits
left the thread running:

- a served generation retired through RavenRuntime.dispose(), which stopped
  plugin services, sub-agents, MCP, the loop and the backend but not the
  watcher;
- a candidate the serving loop never consumed, because a shutdown reached
  the process between stage() and take(). That generation is not the bound
  agent, so the teardown chain's own stop does not reach it, and no unbind
  ever runs for it.

dispose() now stops it, and the generation-organ roster grows the same entry
so the two halves stay in step. The gateway's own unbind no longer stops the
watcher itself: dispose() runs a few lines later and owns it now.

The gateway's shutdown retires a staged candidate directly rather than
through discard(). discard() is contract-bound to stay call-free, because a
call there would mean an organ was started before FREEZE -- which this one
is, the context builder starting the watcher in __init__. Taking the watcher
out of construction is the change that would let discard() own this, and it
is not this one; discard()'s docstring now records that instead of claiming
a never-served generation has nothing to stop.

Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
_serve_generations binds take()'s result to a local and only then awaits
the outgoing generation's unbind. A shutdown cancelling that await unwinds
the coroutine and drops the local: the candidate is then staged nowhere and
bound nowhere, so neither the teardown chain's own stop nor its staged sweep
reaches it. It was fully built, so its skill watcher is still parked in
watchfiles' Rust watch() when Py_FinalizeEx runs, and the process exits -11
once every Python shutdown step has already succeeded.

SwapCoordinator now keeps the candidate it hands out: take() records it and
release() clears it, which spans exactly the window from handover to the
moment the caller's own binding points at the new loop. The serving loop
needs no new line for this -- it already tells the coordinator both ends of
that span.

The teardown chain's watcher sweep moves out of the serve closure into a
module-level _retire_generation_watchers, so tests can execute it instead of
only source-inspecting it. It drains three seats (bound, staged, in
transition) and keeps going when one stop raises, because a sweep that stops
early still leaves a watcher parked in native code.

Verified: a real gateway boots, takes SIGTERM and exits 0, 3/3, with no fault
text in its log. A new test reproduces the cancellation window against real
SkillFileWatcher threads and fails without the fix. Diff coverage on changed
lines rises from 50% to 95.83%.

Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
@LivXue
LivXue force-pushed the fix/gateway_sigterm_segfault branch from b97ed77 to f7eeb67 Compare September 18, 2026 07:14
@LivXue LivXue changed the title fix(cli): stop the skill file watcher before the process exits fix(*): retire every generation's skill watcher before the process exits Sep 18, 2026

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

The cancellation-window fix holds: coordinator ownership spans take() through the next generation's release(), and shutdown sweeps the bound, staged, and in-transition generations without abandoning later seats if one stop fails. The real-thread regression reproduces the previously missed ownership state. I replied to and resolved the thread I opened.

I covered the complete github/main...HEAD diff and the revision delta; AGENTS.md/CLAUDE.md; the Runtime Generation and Assembly Root contracts routed by CONTEXT-MAP.md; the gateway, coordinator, runtime, catalog, and watcher callers; relevant history; backward compatibility; and whether tests were weakened. I found no remaining plain error or blocker.

Verification:

  • uv run pytest tests/test_cli_agent_commands.py tests/test_cli_gateway_commands.py tests/test_cli_tui_commands.py tests/test_generation_swap.py tests/test_core_runtime_swap.py tests/test_skill_watcher_cleanup.py tests/test_trajectory_replay.py -q: 210 passed in 9.89s.
  • uv run pytest tests/integration/test_gateway_sigterm_smoke.py -q -n 0 -m integration: 1 passed in 15.64s, with the existing non-strict idle-ceiling warning.
  • git diff --check github/main...HEAD: clean.

@0xKT

0xKT commented Sep 18, 2026

Copy link
Copy Markdown
Member

Not a blocker -- nothing this PR fixes regresses, and the gap below predates it. But the title
says "every generation", and after this merges one entrance is still uncovered.

RpcStack.teardown (raven/rpc/bootstrap.py:386-458) is the fifth teardown of this shape. It owns
the loop it built (owns_loop = agent_loop is None at bootstrap.py:224, then
agent_loop = build_agent_loop(channel=channel) at :235) and retires the same organs in the same
order -- stop_plugin_services, subagents.cancel_all, the backend drain and stop, close_mcp --
and never calls stop_file_watcher.

That teardown is what raven serve runs: raven/cli/serve_commands.py:326 builds the stack with no
agent_loop=, and :378 awaits stack.teardown(). There is no hard exit on that path -- the only
os._exit in the tree is raven/cli/acp_commands.py:147, on the ACP served branch -- so raven serve
returns into ordinary interpreter finalization with the thread still parked in watchfiles' Rust
watch(), which is precisely the state this PR's own docstring (raven/cli/gateway_commands.py:213-226)
describes as the SIGSEGV.

Measured here, calling only the two expressions raven serve calls:

before build_rpc_stack      : []
after  build_rpc_stack      : ['SkillFileWatcher']
after  stack.teardown()     : ['SkillFileWatcher']      <- still parked
after  one stop_file_watcher(): []                       <- control

The control is the point: the property is measurable and this PR's own one-line shape closes it, so
the first three lines are the code's behaviour and not my probe's.

After this PR the rule is decided in five places -- raven/core/runtime.py:100,
raven/cli/gateway_commands.py:247, raven/cli/tui_commands.py:606, raven/cli/agent_commands.py:365,
raven/trajectory/replay.py:1118 -- and raven/rpc/bootstrap.py is the one dissenter. Either the same
line in RpcStack.teardown, or a single owner that every teardown already goes through, ends that.

Worth knowing while deciding: raven web supervises raven serve (raven/cli/serve_commands.py:694),
and its supervisor treats a zero exit as "the gateway exited cleanly; not restarting it"
(serve_commands.py:805-806). A crash at finalization is not zero, so the supervisor restarts an
engine it was told to leave down and spends its crash-loop budget. That behaviour exists today; this
PR is the natural place to notice it.

Things I checked and found sound, for the record:

  • The three-seat claim holds for the gateway: SwapCoordinator has no fourth holder of a
    candidate at shutdown, in_transition is set in take() and cleared in release(), and both
    halves are pinned (tests/test_core_runtime_swap.py:568 and :571).
  • The sweep's own ordering claim is tested: with the bound seat raising,
    tests/test_cli_gateway_commands.py:1101-1106 asserts the staged and in-transition seats are
    still stopped, so a raising seat does not strand the ones behind it.
  • stop_file_watcher is safe on a seat whose watcher is already gone -- the catalogue documents
    the start side as idempotent, and I called the stop twice against a real catalogue without an
    exception.

@0xKT
0xKT merged commit 242489d into main Sep 19, 2026
31 of 32 checks passed
@0xKT
0xKT deleted the fix/gateway_sigterm_segfault branch September 19, 2026 12:23
LivXue added a commit that referenced this pull request Oct 12, 2026
…its (#490)

## Summary

`raven gateway` exited with SIGSEGV (-11, or 139 in a shell) on every
SIGTERM,
so a systemd or docker stop recorded a crash instead of the clean stop
it was.
`raven tui` and `raven agent` had the same defect.

Root cause: `LocalSkillCatalog` auto-starts `SkillFileWatcher`, a daemon
thread
that parks inside the Rust `watch()` of watchfiles. Daemon status does
not make
process exit safe while the thread sits in native code -- CPython runs
`Py_FinalizeEx` under that call. Every Python shutdown step succeeds
first, so
the crash lands after the graceful chain is complete and the real exit
code is
masked. With faulthandler armed all three faults report `<no Python
frame>`,
i.e. no Python code is on the stack.

Every owner now retires it:

- the gateway's shutdown chain, for the generation still bound at exit;
- the TUI's teardown;
- the one-shot agent's teardown;
- `RavenRuntime.dispose`, for a generation retired at a swap.
`build_runtime`
mints a fresh `AgentLoop` per generation, so each swap starts a new
watcher
  and abandons the previous one; without this a reloaded gateway still
segfaults on stop. The generation-organ roster grows the same entry, so
the
  two halves of that contract stay in step;
- the gateway's shutdown again, for a candidate staged by a reload that
the
serving loop never consumed. That generation is not the bound `agent`,
so
  nothing else reaches it;
- the gateway's shutdown once more, for a candidate `take()` handed out
whose
binding a cancelled unbind never finished. `_serve_generations` holds
that
candidate in a local across `await _unbind_generation()`, so a shutdown
cancelling the await unwinds the coroutine and drops the local, leaving
the
generation staged nowhere and bound nowhere. `SwapCoordinator` now keeps
it:
`take()` records the candidate it hands out and `release()` clears it,
which
spans exactly handover to the moment the caller's own binding points at
the
new loop. The serving loop needed no new line, because it already calls
both
  ends of that span.

The replay runner already stopped the same watcher for the same reason;
these
were the other owners. The stop sits in the generation contract and in
each
command's teardown rather than in `AgentLoop.stop`, whose other caller
is the
generation swap -- that path keeps the process running and must keep
skill
auto-refresh with it.

The gateway's sweep now lives in a module-level
`_retire_generation_watchers`
rather than inline in the serve closure. `run()` is a 550-line closure
with no
import seam, so the inline form could only be pinned by reading its
source text
-- which never executes it, and so earned no coverage on the lines that
matter.
The helper drains all three seats and keeps going when one stop raises,
since a
sweep that stops early still leaves a watcher parked in native code.

Two comments describing a removed guard go with it: one in
`agent_commands`
pointing at an exit chokepoint in `raven.cli.commands.run`, and the
test-side
scaffolding that patched `os._exit` for an exit the command no longer
makes.

## Type

- [x] Fix
- [ ] Feature
- [ ] Docs
- [ ] CI / tooling
- [ ] Refactor
- [ ] Other

## Verification

Process-level, faulthandler armed, throwaway home and a dummy key
nothing calls:

```
                                      before   after
raven gateway   SIGTERM                 -11      0     (-11 on 5 of 5 runs)
raven gateway   SIGHUP then SIGTERM     -11      0     (2 reloads after)
raven tui       PTY, then signal        -11      0
raven agent     ordinary one-shot exit  139      1     (1 is the turn's own
                                                        auth failure, which
                                                        139 had been masking)
```

Controlled pair isolating the cause, on the loop factory the TUI uses --
build
the loop, let the interpreter finalize:

```
watcher left running   exit 139
stop_file_watcher()    exit 0
```

Suites, from the branch tree:

```
uv run pytest tests/integration/test_gateway_sigterm_smoke.py -q -n 0 -m integration
  1 passed          (this is the guard test that was failing)

pytest tests/test_cli_agent_loop_parity.py tests/test_cli_gateway_commands.py \
  tests/test_cli_gateway_health.py tests/test_cli_tui_commands.py \
  tests/test_cli_agent_commands.py tests/test_gateway_spine.py \
  tests/test_generation_swap.py tests/test_plugin_services_lifecycle.py \
  tests/test_rpc_control.py tests/test_rpc_system.py \
  tests/test_skill_watcher_cleanup.py tests/test_trajectory_replay.py \
  tests/test_utils_asyncio_runner.py -q
  307 passed, exit 0

ruff check / ruff format --check over the changed files   clean
ty check over the changed files                           no diagnostics on the changed lines
scripts/check_large_files.py origin/main..HEAD            exit 0
scripts/check_source_language.py origin/main..HEAD        exit 0
commitlint --from origin/main                             exit 0
```

Second round, after review found the in-transition window; rebased onto
`origin/main` at 6d8c363:

```
pytest tests/test_cli_gateway_commands.py tests/test_core_runtime_swap.py \
  tests/test_generation_swap.py tests/test_cli_tui_commands.py \
  tests/test_cli_agent_commands.py -q
  171 passed, exit 0

every test file importing raven.core.runtime          131 passed, exit 0
real gateway: boot, SIGTERM, exit code                0 on 3 of 3, no fault text
ruff check / ruff format --check                      clean
make check-source-language / make check-large-files   exit 0
coverage_gate.py diff --threshold 90                  95.83% (23/24), passed
```

Revert check on that round: removing the `_in_transition` record, the
in-transition seat in the sweep, or the sweep's `try/except` each fails
its own
test and nothing else. The cancellation window itself is reproduced
against real
`SkillFileWatcher` threads, counted by thread name, and is red without
the fix.

Diff coverage on changed lines was 50% (4 of 8) before that round,
because the
gateway-side tests read the chain's source instead of running it. The
one line
still uncovered is the call site inside `run()`, which cannot execute
without
booting a real gateway; the wiring to the seam is pinned by source
inspection.

Each of the four regression tests was confirmed red before its fix. The
TUI and
agent stops were additionally re-confirmed afterwards by removing both
again and
watching those two tests go red, then restoring them.

- [x] Relevant tests pass locally
- [x] Relevant lint / type checks pass locally
- [ ] User-facing docs or screenshots are updated when needed

## Risk

User-visible change: these three commands now exit with their real
status
instead of a segfault. Anything reading the exit code sees a different
value --
0 where it used to see 139, and for a failed one-shot the turn's own
non-zero
code rather than 139.

Rollback: revert the commits. The watcher is a cache-invalidation
helper, so
removing the stops restores the previous behaviour exactly, crash
included.

Checked and deliberately not changed:

- The stops are not wrapped in try/except. `ContextBuilder.__init__`
assigns
`skills` unconditionally, `stop_file_watcher` is documented safe when no
watcher was started, and the watcher's own contract is that neither
`start`
nor `stop` raises. This matches the existing production caller in the
replay
  runner, which is also unwrapped.
- `raven acp` reaches finalization the same way but calls `os._exit(0)`
after a
real session, so the fault is masked there rather than fixed. Left as
is;
  making it consistent is a separate change.
- No central gate was reinstated. A future command that builds a loop
and exits
normally would reintroduce this, and the source-inspection tests here
only
  guard the retirement seats that exist today.
- The watcher starts in `ContextBuilder.__init__`, which is
construction-time
work, and `RavenRuntime.discard` is contract-bound to stay call-free
because
a call there would mean an organ was started before FREEZE. So a staged
or
in-transition candidate is retired by the gateway's sweep rather than by
  `discard`, and the docstring records why. Moving the watcher out of
  construction would let `discard` own it; that is a separate change.
- The shutdown-before-consumption window was not reproduced by timing:
three
runs sending SIGTERM 0.2s, 1.0s and 2.0s after a reload all completed
the
  swap first and exited 0. The staged-candidate retirement is driven by
  forcing the state in a test rather than by racing for it.

Known gap in the evidence: the two `raven tui` runs were ended with
SIGTERM
after Ctrl+C went unanswered, because the TUI never finished loading
against a
throwaway home. The ordinary-exit case is evidenced by the agent
one-shot,
which exits by itself with no signal involved.

- [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>
0xKT pushed a commit that referenced this pull request Oct 12, 2026
## Summary

An RPC stack that built its own engine left the skill watcher running
after teardown. Stop that watcher before draining the owned stores, and
continue the remaining cleanup if stopping it raises.

A stack mounted on a host-owned engine leaves the host's watcher
running. The regression uses a real local watcher and checks repeated
teardown and failure isolation.

## Type

- [x] Fix
- [ ] Feature
- [ ] Docs
- [ ] CI / tooling
- [ ] Refactor
- [ ] Other

## Verification

- [ ] Relevant tests pass locally
- [x] Relevant lint / type checks pass locally
- [ ] User-facing docs or screenshots are updated when needed

Native Windows, Python 3.12. PYTHONUTF8=1; the existing environment was
selected through UV_PROJECT_ENVIRONMENT, and PYTHONPATH pointed at this
checkout.

```text
uv run --no-sync pytest -o addopts= tests/test_rpc_bootstrap.py -k "skill_watcher or failed_watcher" -q --tb=short --show-capture=no
```

Result: 3 passed in the targeted selection. The broader RPC/serve
selection previously produced 136 passed, 2 failed and 26 skipped on
Windows. Both failures reproduced on the unmodified base: owner-only
POSIX mode bits and the supervisor SIGTERM cleanup test. The
relevant-tests box is left unchecked because that broader selection is
not all green.

Whole-branch gates:

```powershell
$changed = @(git diff --name-only origin/main...HEAD -- '*.py')
$production = @($changed | Where-Object { $_ -like 'raven/*' })
uv run --no-sync ruff check @Changed
uv run --no-sync ruff format --check @Changed
uv run --no-sync ty check @production
uv run --no-sync lint-imports
uv run --no-sync pre-commit run --from-ref origin/main --to-ref HEAD
uv run --no-sync python scripts/check_commit_messages.py origin/main..HEAD
uv run --no-sync python scripts/check_source_language.py origin/main...HEAD
uv run --no-sync python scripts/check_large_files.py origin/main...HEAD
git diff --check origin/main...HEAD
```

All listed gates passed. The whole-branch review covered callers,
ownership, guard boundaries, failure paths, regression strength,
architecture and compatibility. No screenshot change applies; directly
affected code documentation is updated where needed. The full repository
suite and Linux/macOS execution were not run locally; the local Docker
Linux engine was unavailable.

## Risk

- [x] Security impact considered
- [x] Backward compatibility considered
- [x] Rollback path is clear for risky changes

Cleanup applies only to the engine owned by this RPC stack. Mounted
stacks retain host ownership. Stop failures are logged without
preventing the remaining resources from closing. Revert the squash
commit to roll back.

## Related Issues

#490
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants