From c1f0daa2fbc201ea57d12278622b0da2b16c53b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 00:04:25 +0000 Subject: [PATCH 1/7] fix(steward): harden canary, cancellation, and placement invariant Four hardening fixes from the #829 review triage (#830): - A canary probe that emits partial output and then stalls past the deadline now counts as a failed probe: the cancelled deadline scope fails the probe regardless of text seen, so the partial-output wedge can reach the three-failure teardown threshold. - The steward turn's advertised command id now honors the generic cancel-by-id contract: live turns register their outer id, and POST /v1/cancel/{id} routes to the harness, which cancels the active inner generation and latches the investigation loop closed. - The steward placement invariant reconciles preference-list edits: a placed steward whose model was removed from steward_models is torn down and re-placed from the current list on the next tick. Reordering alone never replaces a working steward. - Steward candidates are filtered on servability before placement: a non-text or tool-less card named through an operator override is skipped with a warning instead of placing a steward that fails every turn. The vllm tools-rejected message no longer mentions a family fallback that does not exist. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BU73qqVrPobiDcwcDpzNt4 --- src/skulk/api/main.py | 48 ++++++++++++ src/skulk/api/steward.py | 33 +++++++- src/skulk/api/tests/test_cancel_command.py | 1 + src/skulk/api/tests/test_steward_canary.py | 76 ++++++++++++++++++- .../api/tests/test_steward_chunk_stream.py | 65 ++++++++++++++++ src/skulk/master/main.py | 53 ++++++++++++- .../master/tests/test_steward_placement.py | 34 +++++++++ src/skulk/worker/runner/vllm/runner.py | 7 +- 8 files changed, 310 insertions(+), 7 deletions(-) diff --git a/src/skulk/api/main.py b/src/skulk/api/main.py index 97e4a0c3f..6d38faaee 100644 --- a/src/skulk/api/main.py +++ b/src/skulk/api/main.py @@ -1567,6 +1567,11 @@ def __init__( # this node), so an outstanding failed probe surfaces as `degraded` # instead of hiding until the third failure tears the steward down. self._steward_canary = StewardCanaryState() + # Live steward turns keyed by their advertised outer command id, so + # the generic cancel-by-id endpoint can stop a turn whose inner + # generation ids the caller never sees. Entries live exactly as long + # as their turn's chunk stream. + self._steward_turn_harnesses: dict[CommandId, StewardHarness] = {} self._event_log = DiskEventLog(_API_EVENT_LOG_DIR) if enable_event_log else None self._event_log_appends_since_retention_check = 0 self._system_id = SystemId() @@ -3371,6 +3376,12 @@ async def _steward_chat_completions( chunk_stream = self._extensions.tap_chat_stream( self._extension_context, task_params, chunk_stream ) + # The advertised command id must honor the generic cancel-by-id + # contract (POST /v1/cancel/{command_id}). The harness's inner + # generation ids are never shown to the caller, so the outer id is + # registered for the turn's lifetime and cancel_command routes it to + # the harness. + chunk_stream = self._register_steward_turn(command_id, harness, chunk_stream) if payload.stream: return StreamingResponse( with_sse_keepalive( @@ -3388,6 +3399,33 @@ async def _steward_chat_completions( media_type="application/json", ) + async def _register_steward_turn( + self, + command_id: CommandId, + harness: StewardHarness, + chunk_stream: "AsyncGenerator[TokenChunk | ErrorChunk | ToolCallChunk | PrefillProgressChunk, None]", + ) -> "AsyncGenerator[TokenChunk | ErrorChunk | ToolCallChunk | PrefillProgressChunk, None]": + """Expose a steward turn to cancel-by-id for the stream's lifetime. + + Registration and removal bracket the passthrough stream itself, so + the mapping can never outlive its turn: normal completion, client + disconnect, and cancellation all pass through the ``finally``. + + Args: + command_id: The outer command id advertised to the caller. + harness: The harness serving this turn. + chunk_stream: The turn's chunk stream to pass through. + + Yields: + The wrapped stream's chunks, unchanged. + """ + self._steward_turn_harnesses[command_id] = harness + try: + async for chunk in chunk_stream: + yield chunk + finally: + self._steward_turn_harnesses.pop(command_id, None) + async def _steward_extension_transform( self, history: list[StewardChatMessage], *, stream: bool ) -> tuple[list[StewardChatMessage], str, TextGenerationTaskParams]: @@ -3665,6 +3703,16 @@ async def cancel_command(self, command_id: CommandId) -> CancelCommandResponse: or self._audio_transcription_queues.get(command_id) ) if sender is None: + # A steward turn advertises one outer command id while its inner + # generations run under private ids; route the outer id to the + # harness so the generic cancel contract holds there too. + steward_turn = self._steward_turn_harnesses.get(command_id) + if steward_turn is not None: + await steward_turn.cancel_turn() + return CancelCommandResponse( + message="Steward turn cancelled.", + command_id=command_id, + ) raise HTTPException( status_code=404, detail="Command not found or already completed", diff --git a/src/skulk/api/steward.py b/src/skulk/api/steward.py index daaf53482..59286722a 100644 --- a/src/skulk/api/steward.py +++ b/src/skulk/api/steward.py @@ -642,6 +642,10 @@ def __init__(self, api: "API") -> None: # abandoned stream (client disconnect, cancel button) can stop the # runner instead of leaving it generating for nobody. self._active_command_id: CommandId | None = None + # Latched by cancel_turn: the investigation loop checks it before + # every inner dispatch, so cancelling the advertised outer command + # stops the whole turn rather than just its current generation. + self._turn_cancelled = False # Aggregated across the turn's inner generations so the terminal # chunk reports real usage instead of null, and the model's actual # terminal reason (e.g. length) is preserved. @@ -713,7 +717,7 @@ async def canary_probe( # stream iteration, and the chunk stream's own cancellation handling # already sends TaskCancelled and finalizes; a second cancel here # would just duplicate it for an already-finalized command. - with anyio.move_on_after(CANARY_PROBE_TIMEOUT_SECONDS): + with anyio.move_on_after(CANARY_PROBE_TIMEOUT_SECONDS) as deadline_scope: async for chunk in chunk_stream: if isinstance(chunk, ErrorChunk): return False @@ -725,6 +729,12 @@ async def canary_probe( got_text = True if isinstance(chunk, TokenChunk) and chunk.finish_reason is not None: break + # A cancelled deadline is a failed probe no matter what arrived + # first: partial output followed by a stall is exactly the wedge + # this canary exists to catch, and counting it as success would + # reset the failure run the teardown threshold depends on. + if deadline_scope.cancelled_caught: + return False return got_text def steward_instance(self) -> tuple[InstanceId, str] | None: @@ -952,6 +962,25 @@ async def run_turn_chunks( with anyio.move_on_after(2, shield=True): await self._api.send_task_cancellation(abandoned) + async def cancel_turn(self) -> None: + """Cancel the running turn on behalf of the advertised command id. + + The chat surface advertises one outer command id for the whole turn + while the harness dispatches per-step inner generations under ids the + client never sees. Cancelling the outer id must therefore stop the + inner generation currently in flight AND latch the turn closed so + the investigation loop does not simply dispatch its next step. + + Side effects: + Sends a task cancellation for the active inner command, if any. + """ + self._turn_cancelled = True + active = self._active_command_id + if active is None: + return + self._active_command_id = None + await self._api.send_task_cancellation(active) + async def _run_investigation( self, messages: list[ChatCompletionMessage], @@ -962,6 +991,8 @@ async def _run_investigation( wrap it with abandonment cleanup.""" reply = "" for step_index in range(MAX_STEPS_PER_TURN): + if self._turn_cancelled: + return if step_index == MAX_STEPS_PER_TURN - 1: messages.append( ChatCompletionMessage( diff --git a/src/skulk/api/tests/test_cancel_command.py b/src/skulk/api/tests/test_cancel_command.py index 112861c09..77bb1a17e 100644 --- a/src/skulk/api/tests/test_cancel_command.py +++ b/src/skulk/api/tests/test_cancel_command.py @@ -42,6 +42,7 @@ def _make_api() -> Any: api._vision_media_failures = {} # pyright: ignore[reportPrivateUsage] api._vision_media_packet_sender = None # pyright: ignore[reportPrivateUsage] api._cancelled_command_ids = set() # pyright: ignore[reportPrivateUsage] + api._steward_turn_harnesses = {} # pyright: ignore[reportPrivateUsage] api._chunk_reorder = {} # pyright: ignore[reportPrivateUsage] api._data_dedup_cursor = {} # pyright: ignore[reportPrivateUsage] api._data_plane_observer = DataPlaneObserver( # pyright: ignore[reportPrivateUsage] diff --git a/src/skulk/api/tests/test_steward_canary.py b/src/skulk/api/tests/test_steward_canary.py index 16596e67f..981da28cf 100644 --- a/src/skulk/api/tests/test_steward_canary.py +++ b/src/skulk/api/tests/test_steward_canary.py @@ -1,9 +1,18 @@ -"""Canary target selection: pure decision logic for the liveness probe.""" +"""Canary behavior: probe target selection and probe outcome semantics.""" -from skulk.api.steward import canary_probe_target +from collections.abc import AsyncGenerator +from types import SimpleNamespace +from typing import TYPE_CHECKING, cast + +import anyio +import pytest + +import skulk.api.steward as steward_module +from skulk.api.steward import StewardHarness, canary_probe_target from skulk.master.placement import place_instance from skulk.master.tests.test_placement import fully_connected_three_nodes from skulk.shared.models.model_cards import ModelCard, ModelId, ModelTask +from skulk.shared.types.chunks import TokenChunk from skulk.shared.types.commands import PlaceInstance from skulk.shared.types.common import CommandId from skulk.shared.types.memory import Memory @@ -13,6 +22,9 @@ from skulk.shared.types.worker.runners import RunnerLoading, RunnerReady, RunnerRunning from skulk.shared.types.worker.shards import Sharding +if TYPE_CHECKING: + from skulk.api.main import API + def _steward_placement() -> dict[InstanceId, Instance]: topology, node_memory, node_network, _node_ids = fully_connected_three_nodes( @@ -179,3 +191,63 @@ def test_terminal_lifecycle_tasks_do_not_mute_the_canary() -> None: assert ( canary_probe_target(placed, runners, tasks, host) == instance.instance_id ) + + +class _WedgedStreamApi: + """Stub API whose generation emits text and then stalls forever.""" + + def __init__(self, model_id: str) -> None: + self._model_id = model_id + + async def running_model_card(self, model_id: ModelId) -> ModelCard: + return ModelCard( + model_id=ModelId(self._model_id), + storage_size=Memory.from_gb(3), + n_layers=12, + hidden_size=30, + supports_tensor=True, + tasks=[ModelTask.TextGeneration], + ) + + async def dispatch_text_generation( + self, + task_params: TextGenerationTaskParams, + target_instance_id: InstanceId | None = None, + ) -> object: + return SimpleNamespace(command_id=CommandId()) + + def text_generation_chunk_stream( + self, + command: object, + task_params: TextGenerationTaskParams, + *, + extension_tap: bool = True, + ) -> "AsyncGenerator[TokenChunk, None]": + async def _stream() -> "AsyncGenerator[TokenChunk, None]": + yield TokenChunk( + model=ModelId(self._model_id), + text="O", + token_id=-1, + usage=None, + finish_reason=None, + ) + # The partial-output wedge: text arrived, the terminal never does. + await anyio.Event().wait() + + return _stream() + + +async def test_partial_output_then_stall_is_a_failed_probe( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A cancelled deadline must fail the probe even after text arrived. + + Counting the partial output as success would clear the consecutive + failure run, so the exact wedge the canary exists to catch could never + reach the three-failure teardown threshold (#830). + """ + monkeypatch.setattr(steward_module, "CANARY_PROBE_TIMEOUT_SECONDS", 0.05) + api = _WedgedStreamApi("canary-brain") + harness = StewardHarness(cast("API", cast(object, api))) + + assert await harness.canary_probe(InstanceId(), "canary-brain") is False diff --git a/src/skulk/api/tests/test_steward_chunk_stream.py b/src/skulk/api/tests/test_steward_chunk_stream.py index b32cb681d..9dafe6de4 100644 --- a/src/skulk/api/tests/test_steward_chunk_stream.py +++ b/src/skulk/api/tests/test_steward_chunk_stream.py @@ -206,3 +206,68 @@ async def test_complete_literal_example_survives_in_final_answer() -> None: token_chunks = [c for c in chunks if isinstance(c, TokenChunk)] content = "".join(c.text for c in token_chunks if not c.is_thinking) assert content == answer + + +async def test_cancel_turn_stops_the_generation_and_the_loop() -> None: + """Cancelling the advertised outer id ends the whole turn (#830). + + The latch matters as much as the cancellation: without it the + investigation loop would treat the cancelled step as complete and + dispatch the next one, leaving the turn running behind the caller's + back. + """ + cancelled: list[object] = [] + + class _Api: + async def send_task_cancellation(self, command_id: object) -> None: + cancelled.append(command_id) + + # A script that would otherwise tool-loop for the full step budget. + harness = _ScriptedHarness(turns=[("", [_call("get_cluster_state")])]) + harness._api = cast("API", cast(object, _Api())) # pyright: ignore[reportPrivateUsage] + harness._active_command_id = cast(Any, "cmd-inner-1") # pyright: ignore[reportPrivateUsage] + + stream = harness.run_turn_chunks( + [StewardChatMessage(role="user", content="hi")] + ) + first = await stream.__anext__() + assert isinstance(first, TokenChunk) + generations_before_cancel = len(harness.system_prompts) + + await harness.cancel_turn() + assert cancelled == ["cmd-inner-1"] + + async for _chunk in stream: + pass + # The loop stopped at the latch instead of dispatching further steps. + assert len(harness.system_prompts) == generations_before_cancel + + +async def test_cancel_command_routes_a_registered_steward_turn() -> None: + """POST /v1/cancel/{outer_id} must reach the turn's harness, not 404.""" + from skulk.api.main import API + from skulk.shared.types.common import CommandId + + cancelled: list[object] = [] + + class _Api: + async def send_task_cancellation(self, command_id: object) -> None: + cancelled.append(command_id) + + harness = _ScriptedHarness(turns=[("irrelevant", [])]) + harness._api = cast("API", cast(object, _Api())) # pyright: ignore[reportPrivateUsage] + harness._active_command_id = cast(Any, "cmd-inner-9") # pyright: ignore[reportPrivateUsage] + + outer_id = CommandId() + api = API.__new__(API) + api._text_generation_queues = {} # pyright: ignore[reportPrivateUsage] + api._image_generation_queues = {} # pyright: ignore[reportPrivateUsage] + api._embedding_queues = {} # pyright: ignore[reportPrivateUsage] + api._audio_speech_queues = {} # pyright: ignore[reportPrivateUsage] + api._audio_transcription_queues = {} # pyright: ignore[reportPrivateUsage] + api._steward_turn_harnesses = {outer_id: harness} # pyright: ignore[reportPrivateUsage] + + response = await api.cancel_command(outer_id) + + assert response.command_id == outer_id + assert cancelled == ["cmd-inner-9"] diff --git a/src/skulk/master/main.py b/src/skulk/master/main.py index 3994fcef0..313f16ae0 100644 --- a/src/skulk/master/main.py +++ b/src/skulk/master/main.py @@ -30,11 +30,18 @@ from skulk.shared.apply import apply from skulk.shared.constants import SKULK_EVENT_LOG_DIR, SKULK_TRACING_ENABLED from skulk.shared.log_summaries import summarize_command_for_log +from skulk.shared.models.capabilities import resolve_model_capability_profile from skulk.shared.models.memory_estimate import ( estimate_shard_footprint, shard_fraction_of_model, ) -from skulk.shared.models.model_cards import ModelId, get_card, get_model_cards +from skulk.shared.models.model_cards import ( + ModelCard, + ModelId, + ModelTask, + get_card, + get_model_cards, +) from skulk.shared.types.commands import ( AddCustomModelCard, AudioTranscription, @@ -235,6 +242,29 @@ def observe(self, *, now: float, idle: bool) -> float | None: NODE_HEARTBEAT_GAP_WARNING = timedelta(seconds=10) +def steward_candidate_is_servable(card: "ModelCard") -> bool: + """Whether a configured steward candidate can actually serve steward turns. + + The steward harness always dispatches ``TextGeneration`` with server-side + tools, so a candidate must be a text-generation card whose resolved + capability profile supports tool calling. Anything else (an embedding, + image, speech, or tool-less card named through an operator override of + ``steward_models``) would place a steward that fails every turn instead + of falling through to the next candidate. The bundled defaults are + additionally CI-locked to pass this check. + + Args: + card: The candidate's model card. + + Returns: + True when the card can serve tool-driven steward text generation. + """ + if ModelTask.TextGeneration not in card.tasks: + return False + profile = resolve_model_capability_profile(card.model_id, model_card=card) + return profile.supports_tool_calling + + def _aware_timestamp(when: datetime) -> datetime: """Return a timestamp that is safe to compare with UTC receipt times.""" return when if when.tzinfo is not None else when.replace(tzinfo=timezone.utc) @@ -1994,6 +2024,21 @@ async def _maintain_steward_placement(self) -> None: await self._teardown_steward_instances(extras) return if stewards: + # Reconcile a preference-list edit: a steward whose model was + # removed from ``steward_models`` is deliberately deselected and + # must not keep serving indefinitely. Teardown here is enough — + # this same invariant re-places from the current list on the + # next tick. Reordering the list alone never replaces a placed + # steward (upgrade churn is worse than a working older brain). + placed = self.state.instances.get(stewards[0]) + if placed is not None: + placed_model = str(placed.shard_assignments.model_id) + if placed_model not in fabric.steward_models: + logger.info( + f"Steward model {placed_model} was removed from " + "steward_models; replacing the placement" + ) + await self._teardown_steward_instances(stewards) return now = time.monotonic() @@ -2011,6 +2056,12 @@ async def _maintain_steward_placement(self) -> None: f"Steward model {model_ref} has no model card; skipping" ) continue + if not steward_candidate_is_servable(card): + logger.warning( + f"Steward model {model_ref} is not a tool-calling text " + "card; skipping" + ) + continue command = PlaceInstance( model_card=card, sharding=Sharding.Pipeline, diff --git a/src/skulk/master/tests/test_steward_placement.py b/src/skulk/master/tests/test_steward_placement.py index 256a0bdb3..bcce0846c 100644 --- a/src/skulk/master/tests/test_steward_placement.py +++ b/src/skulk/master/tests/test_steward_placement.py @@ -166,3 +166,37 @@ def test_mlx_ring_meta_is_benign_for_a_single_node_gguf_steward() -> None: assert isinstance(instance, MlxRingInstance) assert instance.system_role == "steward" assert len(instance.shard_assignments.node_to_runner) == 1 + + +def test_steward_candidates_require_text_and_tool_capability() -> None: + """Only tool-calling text cards may serve as steward brains (#830). + + The harness always dispatches ``TextGeneration`` with server tools, so + an operator override naming any other card shape must be skipped, not + placed. + """ + from skulk.master.main import steward_candidate_is_servable + from skulk.shared.models.model_cards import ToolingCardConfig + + def _card(tasks: list[ModelTask], *, tools: bool) -> ModelCard: + return ModelCard( + model_id=ModelId("candidate-model"), + storage_size=Memory.from_gb(3), + n_layers=12, + hidden_size=30, + supports_tensor=True, + tasks=tasks, + tooling=ToolingCardConfig(supports_tool_calling=True) if tools else None, + ) + + assert steward_candidate_is_servable( + _card([ModelTask.TextGeneration], tools=True) + ) + # A speech card cannot serve steward turns no matter what it declares. + assert not steward_candidate_is_servable( + _card([ModelTask.TextToSpeech], tools=True) + ) + # A text card without tool calling can never investigate. + assert not steward_candidate_is_servable( + _card([ModelTask.TextGeneration], tools=False) + ) diff --git a/src/skulk/worker/runner/vllm/runner.py b/src/skulk/worker/runner/vllm/runner.py index 4125cd389..280df28e3 100644 --- a/src/skulk/worker/runner/vllm/runner.py +++ b/src/skulk/worker/runner/vllm/runner.py @@ -834,9 +834,10 @@ def _generate(self, task: Task) -> None: if task.task_params.tools and self._tool_call_parser is None: raise RuntimeError( "This model's card declares no vLLM tool-call parser " - "(runtime.vllm_tool_call_parser or a family default), so " - "the server was launched without tool support. Retry " - "without tools or serve a tool-capable card." + "(runtime.vllm_tool_call_parser; there is no family " + "fallback), so the server was launched without tool " + "support. Retry without tools or serve a card that pins " + "a parser." ) if wants_logprobs( task.task_params.logprobs, task.task_params.top_logprobs From a3d01d7977989af9426c43dec359139970449fcb Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 00:14:40 +0000 Subject: [PATCH 2/7] fix(steward): close cancellation races, gate vllm-only brains, document cancel Review round on #833: register the steward turn's outer id before the response advertises it (long-prefill cancel could otherwise race a lazy registration and 404); recheck the cancellation latch after inner dispatch so a cancel that lands mid-dispatch stops the fresh command instead of streaming one more generation; extend the servability gate with backend truth (a vllm-only candidate without a pinned tool-call parser would place and then fail every tools-bearing turn); document steward cancel-by-id in the API guide. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BU73qqVrPobiDcwcDpzNt4 --- src/skulk/api/main.py | 28 +++++---- src/skulk/api/steward.py | 10 +++ .../api/tests/test_steward_chunk_stream.py | 62 +++++++++++++++++++ src/skulk/master/main.py | 21 ++++++- .../master/tests/test_steward_placement.py | 37 +++++++++++ website/docs/api-guide.md | 16 +++-- 6 files changed, 154 insertions(+), 20 deletions(-) diff --git a/src/skulk/api/main.py b/src/skulk/api/main.py index 6d38faaee..eec2d0c99 100644 --- a/src/skulk/api/main.py +++ b/src/skulk/api/main.py @@ -3378,10 +3378,14 @@ async def _steward_chat_completions( ) # The advertised command id must honor the generic cancel-by-id # contract (POST /v1/cancel/{command_id}). The harness's inner - # generation ids are never shown to the caller, so the outer id is - # registered for the turn's lifetime and cancel_command routes it to - # the harness. - chunk_stream = self._register_steward_turn(command_id, harness, chunk_stream) + # generation ids are never shown to the caller, so the outer id maps + # to the harness for the turn's lifetime and cancel_command routes + # it there. Registration happens HERE, before the response exists: + # generate_chat_stream advertises the id in its SSE comment before + # it pulls the first chunk, so registering lazily inside the stream + # would leave a cancel-by-id 404 window during long prefill. + self._steward_turn_harnesses[command_id] = harness + chunk_stream = self._release_steward_turn_after(command_id, chunk_stream) if payload.stream: return StreamingResponse( with_sse_keepalive( @@ -3399,27 +3403,25 @@ async def _steward_chat_completions( media_type="application/json", ) - async def _register_steward_turn( + async def _release_steward_turn_after( self, command_id: CommandId, - harness: StewardHarness, chunk_stream: "AsyncGenerator[TokenChunk | ErrorChunk | ToolCallChunk | PrefillProgressChunk, None]", ) -> "AsyncGenerator[TokenChunk | ErrorChunk | ToolCallChunk | PrefillProgressChunk, None]": - """Expose a steward turn to cancel-by-id for the stream's lifetime. + """Deregister a steward turn from cancel-by-id when its stream ends. - Registration and removal bracket the passthrough stream itself, so - the mapping can never outlive its turn: normal completion, client - disconnect, and cancellation all pass through the ``finally``. + The caller registers the turn BEFORE constructing the response (the + id is advertised to the client before the first chunk is pulled); + this wrapper owns only the removal, so normal completion, client + disconnect, and cancellation all clean up through one ``finally``. Args: - command_id: The outer command id advertised to the caller. - harness: The harness serving this turn. + command_id: The outer command id the caller registered. chunk_stream: The turn's chunk stream to pass through. Yields: The wrapped stream's chunks, unchanged. """ - self._steward_turn_harnesses[command_id] = harness try: async for chunk in chunk_stream: yield chunk diff --git a/src/skulk/api/steward.py b/src/skulk/api/steward.py index 59286722a..c6df1b058 100644 --- a/src/skulk/api/steward.py +++ b/src/skulk/api/steward.py @@ -1159,6 +1159,16 @@ async def _generate_events( task_params, target_instance_id=instance_id ) self._active_command_id = command.command_id + if self._turn_cancelled: + # cancel_turn ran while this dispatch was in flight: the latch + # was set before this inner id existed, so cancel_turn had + # nothing to stop. Honor it here rather than letting an + # accepted cancellation stream one more full generation. The + # step ends without a result event, which the loop reads as an + # empty final answer. + self._active_command_id = None + await api.send_task_cancellation(command.command_id) + return # No extension tap: this is one investigation step, not the turn. # The turn's single tap is applied by the caller of # run_turn_chunks (API._steward_chat_completions), so observers see diff --git a/src/skulk/api/tests/test_steward_chunk_stream.py b/src/skulk/api/tests/test_steward_chunk_stream.py index 9dafe6de4..9c42ce485 100644 --- a/src/skulk/api/tests/test_steward_chunk_stream.py +++ b/src/skulk/api/tests/test_steward_chunk_stream.py @@ -271,3 +271,65 @@ async def send_task_cancellation(self, command_id: object) -> None: assert response.command_id == outer_id assert cancelled == ["cmd-inner-9"] + + +async def test_cancellation_racing_dispatch_still_cancels_the_step() -> None: + """A latch set while dispatch is in flight cancels the fresh command. + + cancel_turn can run between step start and dispatch completion, when no + inner id exists yet to cancel. The step must then cancel the command it + just obtained and end the turn instead of streaming one more full + generation behind an accepted cancellation (#833). + """ + from types import SimpleNamespace + + from skulk.shared.models.model_cards import ModelCard, ModelTask + from skulk.shared.types.memory import Memory + + cancelled: list[object] = [] + stream_requests: list[object] = [] + harness_holder: list[StewardHarness] = [] + + class _RacingApi: + async def running_model_card(self, model_id: ModelId) -> ModelCard: + return ModelCard( + model_id=ModelId("steward-brain"), + storage_size=Memory.from_gb(3), + n_layers=12, + hidden_size=30, + supports_tensor=True, + tasks=[ModelTask.TextGeneration], + ) + + async def dispatch_text_generation( + self, task_params: object, target_instance_id: object = None + ) -> object: + # The race: cancellation lands while dispatch is in flight. + await harness_holder[0].cancel_turn() + return SimpleNamespace(command_id="cmd-fresh-inner") + + def text_generation_chunk_stream( + self, command: object, task_params: object, *, extension_tap: bool = True + ) -> object: + stream_requests.append(command) + raise AssertionError("a cancelled step must not open a stream") + + async def send_task_cancellation(self, command_id: object) -> None: + cancelled.append(command_id) + + harness = StewardHarness(cast("API", cast(object, _RacingApi()))) + harness_holder.append(harness) + harness.steward_instance = lambda: (InstanceId(), "steward-brain") + + chunks = [ + chunk + async for chunk in harness.run_turn_chunks( + [StewardChatMessage(role="user", content="hi")] + ) + ] + + assert cancelled == ["cmd-fresh-inner"] + assert stream_requests == [] + final = chunks[-1] + assert isinstance(final, TokenChunk) + assert final.finish_reason == "stop" diff --git a/src/skulk/master/main.py b/src/skulk/master/main.py index 313f16ae0..dd291c2f1 100644 --- a/src/skulk/master/main.py +++ b/src/skulk/master/main.py @@ -28,6 +28,7 @@ usable_vram_by_node, ) from skulk.shared.apply import apply +from skulk.shared.backends import engine_of, platform_compatible_backends from skulk.shared.constants import SKULK_EVENT_LOG_DIR, SKULK_TRACING_ENABLED from skulk.shared.log_summaries import summarize_command_for_log from skulk.shared.models.capabilities import resolve_model_capability_profile @@ -262,7 +263,25 @@ def steward_candidate_is_servable(card: "ModelCard") -> bool: if ModelTask.TextGeneration not in card.tasks: return False profile = resolve_model_capability_profile(card.model_id, model_card=card) - return profile.supports_tool_calling + if not profile.supports_tool_calling: + return False + # Backend truth, not just model truth: a card whose only platform- + # servable engine is vllm needs an explicit tool-call parser pin, or + # the launched server rejects every tools-bearing request and the + # steward would place but fail every turn. + servable_engines = { + engine + for tag in platform_compatible_backends( + card.placement.compatible_backends, + card_serves_vision=card.vision is not None, + card_serves_speech=False, + ) + if (engine := engine_of(tag)) is not None + } + return not ( + servable_engines == {"vllm"} + and (card.runtime is None or card.runtime.vllm_tool_call_parser is None) + ) def _aware_timestamp(when: datetime) -> datetime: diff --git a/src/skulk/master/tests/test_steward_placement.py b/src/skulk/master/tests/test_steward_placement.py index bcce0846c..41dc06048 100644 --- a/src/skulk/master/tests/test_steward_placement.py +++ b/src/skulk/master/tests/test_steward_placement.py @@ -200,3 +200,40 @@ def _card(tasks: list[ModelTask], *, tools: bool) -> ModelCard: assert not steward_candidate_is_servable( _card([ModelTask.TextGeneration], tools=False) ) + + +def test_vllm_only_steward_candidates_require_a_pinned_parser() -> None: + """Backend truth joins model truth in the servability gate (#833). + + A tool-declaring card whose only platform-servable engine is vllm still + fails every steward turn unless the card pins + ``runtime.vllm_tool_call_parser``: the launched server rejects + tools-bearing requests loudly, so such a candidate must be skipped. + """ + from skulk.master.main import steward_candidate_is_servable + from skulk.shared.models.model_cards import ( + RuntimeCapabilityCardConfig, + ToolingCardConfig, + ) + + def _vllm_card(parser: str | None) -> ModelCard: + return ModelCard( + model_id=ModelId("vllm-only-model"), + storage_size=Memory.from_gb(3), + n_layers=12, + hidden_size=30, + supports_tensor=True, + tasks=[ModelTask.TextGeneration], + tooling=ToolingCardConfig(supports_tool_calling=True), + runtime=( + RuntimeCapabilityCardConfig(vllm_tool_call_parser=parser) + if parser is not None + else None + ), + placement=PlacementCardConfig( + compatible_backends=frozenset({"vllm-cuda"}), + ), + ) + + assert not steward_candidate_is_servable(_vllm_card(None)) + assert steward_candidate_is_servable(_vllm_card("hermes")) diff --git a/website/docs/api-guide.md b/website/docs/api-guide.md index de2f9622e..e43100eb1 100644 --- a/website/docs/api-guide.md +++ b/website/docs/api-guide.md @@ -1317,7 +1317,10 @@ Requests cancellation of one in-flight generation command by its command ID. It covers text generation, image generation, embeddings, and speech synthesis or transcription commands owned by the API node you call: Skulk closes the local response stream and sends a task cancellation so the serving runner -stops instead of generating into the void. +stops instead of generating into the void. Steward turns (`skulk/steward`) +are covered too: the ID advertised on a steward response cancels the whole +turn — the in-flight generation stops and the investigation loop ends +instead of proceeding to its next step. Finding the command ID: @@ -1334,11 +1337,12 @@ curl -X POST http://localhost:52415/v1/cancel/ ``` A cancelled command returns -`{"message": "Command cancelled.", "command_id": "..."}`. An unknown or -already-completed command returns **404 Command not found or already -completed**. Command streams are node-local, so call the same API node that -accepted the original request. Simply disconnecting from a streaming response -triggers the same cancellation path implicitly. +`{"message": "Command cancelled.", "command_id": "..."}`; a cancelled steward +turn returns `{"message": "Steward turn cancelled.", "command_id": "..."}`. +An unknown or already-completed command returns **404 Command not found or +already completed**. Command streams are node-local, so call the same API +node that accepted the original request. Simply disconnecting from a +streaming response triggers the same cancellation path implicitly. ## Model Discovery From c8e5daa5b4fc86425904680aa5b32dcd1e1de915 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 00:24:01 +0000 Subject: [PATCH 3/7] fix(steward): gate the stamped backend, not just the compatibility set Review round two on #833: a multi-engine card passes the card-level servability gate but can still resolve to vllm on the fleet at hand; placement_resolves_parserless_vllm inspects the backends place_instance actually stamped, and the candidate walk skips the brain instead of committing a steward whose server rejects every tools-bearing request. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BU73qqVrPobiDcwcDpzNt4 --- src/skulk/master/main.py | 36 ++++++++++ .../master/tests/test_steward_placement.py | 72 +++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/src/skulk/master/main.py b/src/skulk/master/main.py index dd291c2f1..487aa2cc3 100644 --- a/src/skulk/master/main.py +++ b/src/skulk/master/main.py @@ -284,6 +284,36 @@ def steward_candidate_is_servable(card: "ModelCard") -> bool: ) +def placement_resolves_parserless_vllm( + card: "ModelCard", placements: "Mapping[InstanceId, Instance]" +) -> bool: + """Whether a minted placement stamped vllm for a card with no parser pin. + + The pre-placement servability gate can only reject a card whose EVERY + servable engine needs the parser; a multi-engine card passes it and may + still resolve to vllm on the fleet at hand (backend preference, hardware + mix). This checks the backends placement actually stamped, so the walk + can skip the candidate instead of committing a steward whose server + rejects every tools-bearing request. + + Args: + card: The candidate's model card. + placements: The instances ``place_instance`` minted for it. + + Returns: + True when any stamped shard resolved to the vllm engine while the + card pins no ``runtime.vllm_tool_call_parser``. + """ + if card.runtime is not None and card.runtime.vllm_tool_call_parser is not None: + return False + for instance in placements.values(): + for shard in instance.shard_assignments.runner_to_shard.values(): + backend: str | None = getattr(shard, "resolved_backend", None) + if backend is not None and engine_of(backend) == "vllm": + return True + return False + + def _aware_timestamp(when: datetime) -> datetime: """Return a timestamp that is safe to compare with UTC receipt times.""" return when if when.tzinfo is not None else when.replace(tzinfo=timezone.utc) @@ -2113,6 +2143,12 @@ async def _maintain_steward_placement(self) -> None: f"Steward placement with {model_ref} not possible yet: {err}" ) continue + if placement_resolves_parserless_vllm(card, final_placement): + logger.warning( + f"Steward model {model_ref} resolved to the vllm engine " + "without a pinned tool-call parser; skipping" + ) + continue logger.info( f"Establishing steward placement with {model_ref} " "(intelligent fabric)" diff --git a/src/skulk/master/tests/test_steward_placement.py b/src/skulk/master/tests/test_steward_placement.py index 41dc06048..15c2a5bd1 100644 --- a/src/skulk/master/tests/test_steward_placement.py +++ b/src/skulk/master/tests/test_steward_placement.py @@ -237,3 +237,75 @@ def _vllm_card(parser: str | None) -> ModelCard: assert not steward_candidate_is_servable(_vllm_card(None)) assert steward_candidate_is_servable(_vllm_card("hermes")) + + +def test_stamped_vllm_placement_without_parser_is_rejected() -> None: + """The post-placement gate catches multi-engine cards that resolve to vllm. + + A card listing vllm alongside another engine passes the card-level + servability gate, but a fleet whose hardware makes placement stamp + vllm still needs the parser pin; the walk must skip the candidate when + the minted shards resolved to vllm without one (#833). + """ + from skulk.master.main import placement_resolves_parserless_vllm + from skulk.shared.models.model_cards import ( + RuntimeCapabilityCardConfig, + ToolingCardConfig, + ) + from skulk.shared.types.worker.instances import Instance, InstanceId + + def _card(parser: str | None) -> ModelCard: + return ModelCard( + model_id=ModelId("multi-engine-model"), + storage_size=Memory.from_gb(3), + n_layers=12, + hidden_size=30, + supports_tensor=True, + tasks=[ModelTask.TextGeneration], + tooling=ToolingCardConfig(supports_tool_calling=True), + runtime=( + RuntimeCapabilityCardConfig(vllm_tool_call_parser=parser) + if parser is not None + else None + ), + placement=PlacementCardConfig( + compatible_backends=frozenset({"vllm-cuda", "llama_server-cuda"}), + backend_preference=("vllm-cuda", "llama_server-cuda"), + ), + ) + + topology, node_memory, node_network, node_ids = fully_connected_three_nodes( + (10.0, 10.0, 10.0) + ) + + def _place(card: ModelCard) -> "dict[InstanceId, Instance]": + command = PlaceInstance( + model_card=card, + sharding=Sharding.Pipeline, + instance_meta=InstanceMeta.MlxRing, + min_nodes=1, + system_role="steward", + ) + return place_instance( + command, + topology, + {}, + node_memory, + node_network, + node_resources={ + node_id: NodeResources( + backends=frozenset({"vllm-cuda", "llama_server-cuda"}) + ) + for node_id in node_ids + }, + ) + + parserless = _place(_card(None)) + stamped = { + shard.resolved_backend + for instance in parserless.values() + for shard in instance.shard_assignments.runner_to_shard.values() + } + assert stamped == {"vllm-cuda"}, "test premise: placement resolves vllm" + assert placement_resolves_parserless_vllm(_card(None), parserless) + assert not placement_resolves_parserless_vllm(_card("hermes"), _place(_card("hermes"))) From 135c23ff6969b37404eada5038904cafb532759c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 00:31:55 +0000 Subject: [PATCH 4/7] fix(steward): close the inner stream on cancel; treat unstamped backends as vllm-selectable Review round three on #833: turn cancellation now goes through one shared local-cancellation path (cancel_local_command) that closes the inner command's local queue immediately - a served engine mid-generation may only observe worker-side cancellation at completion, and the accepted cancel must end the response now. The parserless-vllm gate treats an unstamped resolved_backend as still-selectable while vllm is among the card's servable engines: during telemetry warm-up the worker's local fallback is free to pick vllm, so the invariant waits for a later tick instead of committing a possibly tools-rejecting steward. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BU73qqVrPobiDcwcDpzNt4 --- src/skulk/api/main.py | 63 +++++++++++++------ src/skulk/api/steward.py | 12 +++- .../api/tests/test_steward_chunk_stream.py | 15 ++++- src/skulk/master/main.py | 41 ++++++++---- .../master/tests/test_steward_placement.py | 29 ++++++++- 5 files changed, 122 insertions(+), 38 deletions(-) diff --git a/src/skulk/api/main.py b/src/skulk/api/main.py index eec2d0c99..86595a1a6 100644 --- a/src/skulk/api/main.py +++ b/src/skulk/api/main.py @@ -3695,8 +3695,27 @@ async def delete_instance(self, instance_id: InstanceId) -> DeleteInstanceRespon instance_id=instance_id, ) - async def cancel_command(self, command_id: CommandId) -> CancelCommandResponse: - """Cancel an active command by closing its stream and notifying workers.""" + async def cancel_local_command(self, command_id: CommandId) -> bool: + """Cancel one locally registered command stream, if it exists. + + The single cancellation implementation shared by the public + cancel-by-id endpoint and the steward harness's turn cancellation: + closes the local response queue immediately (so the caller's stream + ends now, not when worker-side cancellation lands — a served engine + mid-generation may only observe it at completion) and notifies + workers through ``TaskCancelled``. + + Args: + command_id: The command whose local stream should be cancelled. + + Returns: + True when a local queue existed and was cancelled; False when the + command is unknown here or already completed. + + Side effects: + Sends ``TaskCancelled``, records the id so local stream cleanup + suppresses its ``TaskFinished``, and closes the queue. + """ sender = ( self._text_generation_queues.get(command_id) or self._image_generation_queues.get(command_id) @@ -3705,31 +3724,35 @@ async def cancel_command(self, command_id: CommandId) -> CancelCommandResponse: or self._audio_transcription_queues.get(command_id) ) if sender is None: - # A steward turn advertises one outer command id while its inner - # generations run under private ids; route the outer id to the - # harness so the generic cancel contract holds there too. - steward_turn = self._steward_turn_harnesses.get(command_id) - if steward_turn is not None: - await steward_turn.cancel_turn() - return CancelCommandResponse( - message="Steward turn cancelled.", - command_id=command_id, - ) - raise HTTPException( - status_code=404, - detail="Command not found or already completed", - ) - + return False await self._send(TaskCancelled(cancelled_command_id=command_id)) # Suppress the final TaskFinished emitted by local stream cleanup so the # worker can observe the Cancelled task and deliver runner-local cancel # before event-sourced task deletion happens. self._cancelled_command_ids.add(command_id) sender.close() + return True - return CancelCommandResponse( - message="Command cancelled.", - command_id=command_id, + async def cancel_command(self, command_id: CommandId) -> CancelCommandResponse: + """Cancel an active command by closing its stream and notifying workers.""" + if await self.cancel_local_command(command_id): + return CancelCommandResponse( + message="Command cancelled.", + command_id=command_id, + ) + # A steward turn advertises one outer command id while its inner + # generations run under private ids; route the outer id to the + # harness so the generic cancel contract holds there too. + steward_turn = self._steward_turn_harnesses.get(command_id) + if steward_turn is not None: + await steward_turn.cancel_turn() + return CancelCommandResponse( + message="Steward turn cancelled.", + command_id=command_id, + ) + raise HTTPException( + status_code=404, + detail="Command not found or already completed", ) def _command_task_is_terminal(self, command_id: CommandId) -> bool: diff --git a/src/skulk/api/steward.py b/src/skulk/api/steward.py index c6df1b058..ba200ae54 100644 --- a/src/skulk/api/steward.py +++ b/src/skulk/api/steward.py @@ -972,14 +972,22 @@ async def cancel_turn(self) -> None: the investigation loop does not simply dispatch its next step. Side effects: - Sends a task cancellation for the active inner command, if any. + Cancels the active inner command through the API's shared local + cancellation path (closing its local queue so this turn's stream + ends immediately), falling back to a bare worker notification + when the queue is already gone. """ self._turn_cancelled = True active = self._active_command_id if active is None: return self._active_command_id = None - await self._api.send_task_cancellation(active) + # Close the inner command's LOCAL queue, not just notify workers: a + # served engine mid-generation may observe worker-side cancellation + # only at completion, and the accepted cancel must end this response + # now, not then. + if not await self._api.cancel_local_command(active): + await self._api.send_task_cancellation(active) async def _run_investigation( self, diff --git a/src/skulk/api/tests/test_steward_chunk_stream.py b/src/skulk/api/tests/test_steward_chunk_stream.py index 9c42ce485..e6d5ec34b 100644 --- a/src/skulk/api/tests/test_steward_chunk_stream.py +++ b/src/skulk/api/tests/test_steward_chunk_stream.py @@ -219,8 +219,16 @@ async def test_cancel_turn_stops_the_generation_and_the_loop() -> None: cancelled: list[object] = [] class _Api: - async def send_task_cancellation(self, command_id: object) -> None: + async def cancel_local_command(self, command_id: object) -> bool: + # The shared path closes the local queue; recording it here is + # the test's proof the turn cancelled through that path. cancelled.append(command_id) + return True + + async def send_task_cancellation(self, command_id: object) -> None: + raise AssertionError( + "the fallback must not fire when the local cancel succeeded" + ) # A script that would otherwise tool-loop for the full step budget. harness = _ScriptedHarness(turns=[("", [_call("get_cluster_state")])]) @@ -251,6 +259,11 @@ async def test_cancel_command_routes_a_registered_steward_turn() -> None: cancelled: list[object] = [] class _Api: + async def cancel_local_command(self, command_id: object) -> bool: + # The inner queue is already gone in this scenario; the harness + # must fall back to the bare worker notification. + return False + async def send_task_cancellation(self, command_id: object) -> None: cancelled.append(command_id) diff --git a/src/skulk/master/main.py b/src/skulk/master/main.py index 487aa2cc3..e845a2cea 100644 --- a/src/skulk/master/main.py +++ b/src/skulk/master/main.py @@ -284,32 +284,48 @@ def steward_candidate_is_servable(card: "ModelCard") -> bool: ) -def placement_resolves_parserless_vllm( +def placement_may_select_parserless_vllm( card: "ModelCard", placements: "Mapping[InstanceId, Instance]" ) -> bool: - """Whether a minted placement stamped vllm for a card with no parser pin. + """Whether a minted placement could serve vllm for a card with no parser. The pre-placement servability gate can only reject a card whose EVERY servable engine needs the parser; a multi-engine card passes it and may - still resolve to vllm on the fleet at hand (backend preference, hardware - mix). This checks the backends placement actually stamped, so the walk - can skip the candidate instead of committing a steward whose server - rejects every tools-bearing request. + still end up on vllm on the fleet at hand. This checks the backends + placement actually stamped, so the walk can skip the candidate instead + of committing a steward whose server rejects every tools-bearing + request. An UNSTAMPED shard (``resolved_backend=None``, the telemetry + warm-up window) counts as selectable too: the worker then falls back to + its local probe, which is free to pick vllm, so the only safe answer + while vllm is among the card's servable engines is "not yet" — the + invariant simply retries on a later tick once resources have arrived. Args: card: The candidate's model card. placements: The instances ``place_instance`` minted for it. Returns: - True when any stamped shard resolved to the vllm engine while the - card pins no ``runtime.vllm_tool_call_parser``. + True when the card pins no ``runtime.vllm_tool_call_parser``, vllm + is among its platform-servable engines, and any shard either + resolved to vllm or carries no stamped backend. """ if card.runtime is not None and card.runtime.vllm_tool_call_parser is not None: return False + servable_engines = { + engine + for tag in platform_compatible_backends( + card.placement.compatible_backends, + card_serves_vision=card.vision is not None, + card_serves_speech=False, + ) + if (engine := engine_of(tag)) is not None + } + if "vllm" not in servable_engines: + return False for instance in placements.values(): for shard in instance.shard_assignments.runner_to_shard.values(): backend: str | None = getattr(shard, "resolved_backend", None) - if backend is not None and engine_of(backend) == "vllm": + if backend is None or engine_of(backend) == "vllm": return True return False @@ -2143,10 +2159,11 @@ async def _maintain_steward_placement(self) -> None: f"Steward placement with {model_ref} not possible yet: {err}" ) continue - if placement_resolves_parserless_vllm(card, final_placement): + if placement_may_select_parserless_vllm(card, final_placement): logger.warning( - f"Steward model {model_ref} resolved to the vllm engine " - "without a pinned tool-call parser; skipping" + f"Steward model {model_ref} resolved (or may fall back) to " + "the vllm engine without a pinned tool-call parser; " + "skipping" ) continue logger.info( diff --git a/src/skulk/master/tests/test_steward_placement.py b/src/skulk/master/tests/test_steward_placement.py index 15c2a5bd1..d2b04ac53 100644 --- a/src/skulk/master/tests/test_steward_placement.py +++ b/src/skulk/master/tests/test_steward_placement.py @@ -247,7 +247,7 @@ def test_stamped_vllm_placement_without_parser_is_rejected() -> None: vllm still needs the parser pin; the walk must skip the candidate when the minted shards resolved to vllm without one (#833). """ - from skulk.master.main import placement_resolves_parserless_vllm + from skulk.master.main import placement_may_select_parserless_vllm from skulk.shared.models.model_cards import ( RuntimeCapabilityCardConfig, ToolingCardConfig, @@ -307,5 +307,28 @@ def _place(card: ModelCard) -> "dict[InstanceId, Instance]": for shard in instance.shard_assignments.runner_to_shard.values() } assert stamped == {"vllm-cuda"}, "test premise: placement resolves vllm" - assert placement_resolves_parserless_vllm(_card(None), parserless) - assert not placement_resolves_parserless_vllm(_card("hermes"), _place(_card("hermes"))) + assert placement_may_select_parserless_vllm(_card(None), parserless) + assert not placement_may_select_parserless_vllm( + _card("hermes"), _place(_card("hermes")) + ) + + # Telemetry warm-up: no NodeResources means placement stamps no + # backend, and the worker's local fallback could still pick vllm, so + # an unstamped parserless placement must read as unsafe too. + unstamped_command = PlaceInstance( + model_card=_card(None), + sharding=Sharding.Pipeline, + instance_meta=InstanceMeta.MlxRing, + min_nodes=1, + system_role="steward", + ) + unstamped = place_instance( + unstamped_command, topology, {}, node_memory, node_network + ) + stamped_backends = { + shard.resolved_backend + for instance in unstamped.values() + for shard in instance.shard_assignments.runner_to_shard.values() + } + assert stamped_backends == {None}, "test premise: warm-up leaves no stamp" + assert placement_may_select_parserless_vllm(_card(None), unstamped) From be0e5a03ecb69ff748bda753c4445dcd509d0e95 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 00:41:40 +0000 Subject: [PATCH 5/7] fix(steward): routing-truth servability, minted-only gate, marker hygiene, pacing reset Review round four on #833: the servability gate now rejects multi-task cards whose image/embedding/speech tasks would route them to a specialized runner before text-engine dispatch; the parserless-vllm check inspects only the instances the placement actually minted (place_instance returns existing state plus the new instance, so an unrelated shard could falsely condemn a candidate); cancellations that never open a chunk stream no longer retain the TaskFinished-suppression marker (nothing would ever discard it); and the reconciliation teardown opens the pacing window so replacement really happens on the next tick. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BU73qqVrPobiDcwcDpzNt4 --- src/skulk/api/main.py | 20 ++++++++++--- src/skulk/api/steward.py | 15 +++++++--- src/skulk/api/tests/test_cancel_command.py | 19 ++++++++++++ .../api/tests/test_steward_chunk_stream.py | 14 +++++++-- src/skulk/master/main.py | 29 +++++++++++++++++-- .../master/tests/test_steward_placement.py | 12 ++++++++ 6 files changed, 97 insertions(+), 12 deletions(-) diff --git a/src/skulk/api/main.py b/src/skulk/api/main.py index 86595a1a6..a3c232cb0 100644 --- a/src/skulk/api/main.py +++ b/src/skulk/api/main.py @@ -4719,15 +4719,27 @@ async def _expire_stale_vision_media(self, now: float) -> None: "Vision media timed out waiting for worker verification", ) - async def send_task_cancellation(self, command_id: CommandId) -> None: + async def send_task_cancellation( + self, command_id: CommandId, *, suppress_local_finish: bool = True + ) -> None: """Public cancellation seam for internal callers (the steward harness). Sends the same TaskCancelled command the HTTP cancel endpoint sends, - suppressing the final TaskFinished so the worker can observe the - cancelled task and stop the runner promptly. + by default suppressing the final TaskFinished so the worker can + observe the cancelled task and stop the runner promptly. + + Args: + command_id: The command to cancel on the workers. + suppress_local_finish: Record the id so local stream cleanup + skips its TaskFinished. Pass False when NO local stream will + ever open for this command (e.g. a cancellation accepted + before its chunk stream was created): the marker is only + discarded by stream finalization, so retaining it with no + stream would leak one set entry per cancellation. """ await self._send(TaskCancelled(cancelled_command_id=command_id)) - self._cancelled_command_ids.add(command_id) + if suppress_local_finish: + self._cancelled_command_ids.add(command_id) async def dispatch_text_generation( self, diff --git a/src/skulk/api/steward.py b/src/skulk/api/steward.py index ba200ae54..0e9fd4ed9 100644 --- a/src/skulk/api/steward.py +++ b/src/skulk/api/steward.py @@ -985,9 +985,13 @@ async def cancel_turn(self) -> None: # Close the inner command's LOCAL queue, not just notify workers: a # served engine mid-generation may observe worker-side cancellation # only at completion, and the accepted cancel must end this response - # now, not then. + # now, not then. When the queue is already gone there is no stream + # left to finalize, so the fallback must not retain the local-finish + # marker (nothing would ever discard it). if not await self._api.cancel_local_command(active): - await self._api.send_task_cancellation(active) + await self._api.send_task_cancellation( + active, suppress_local_finish=False + ) async def _run_investigation( self, @@ -1173,9 +1177,12 @@ async def _generate_events( # nothing to stop. Honor it here rather than letting an # accepted cancellation stream one more full generation. The # step ends without a result event, which the loop reads as an - # empty final answer. + # empty final answer. No chunk stream ever opens for this + # command, so the local-finish marker must not be retained. self._active_command_id = None - await api.send_task_cancellation(command.command_id) + await api.send_task_cancellation( + command.command_id, suppress_local_finish=False + ) return # No extension tap: this is one investigation step, not the turn. # The turn's single tap is applied by the caller of diff --git a/src/skulk/api/tests/test_cancel_command.py b/src/skulk/api/tests/test_cancel_command.py index 77bb1a17e..c213907da 100644 --- a/src/skulk/api/tests/test_cancel_command.py +++ b/src/skulk/api/tests/test_cancel_command.py @@ -189,3 +189,22 @@ async def test_finalize_command_stream_reports_natural_completion() -> None: task_finished = api._send.call_args[0][0] assert task_finished.finished_command_id == cid assert cid not in queue + + +@pytest.mark.asyncio +async def test_send_task_cancellation_without_stream_retains_no_marker() -> None: + """A no-stream cancellation must not leak a local-finish marker. + + The marker is discarded only by stream finalization; a cancellation for + a command whose chunk stream never opens (the steward dispatch race) + would otherwise grow `_cancelled_command_ids` forever (#833). + """ + api = _make_api() + cid = CommandId("no-stream-cmd") + + await api.send_task_cancellation(cid, suppress_local_finish=False) + assert cid not in api._cancelled_command_ids + api._send.assert_called_once() + + await api.send_task_cancellation(cid) + assert cid in api._cancelled_command_ids diff --git a/src/skulk/api/tests/test_steward_chunk_stream.py b/src/skulk/api/tests/test_steward_chunk_stream.py index e6d5ec34b..82627f42d 100644 --- a/src/skulk/api/tests/test_steward_chunk_stream.py +++ b/src/skulk/api/tests/test_steward_chunk_stream.py @@ -264,7 +264,12 @@ async def cancel_local_command(self, command_id: object) -> bool: # must fall back to the bare worker notification. return False - async def send_task_cancellation(self, command_id: object) -> None: + async def send_task_cancellation( + self, command_id: object, *, suppress_local_finish: bool = True + ) -> None: + # The queue is already gone, so nothing would ever discard a + # retained local-finish marker (#833). + assert suppress_local_finish is False cancelled.append(command_id) harness = _ScriptedHarness(turns=[("irrelevant", [])]) @@ -327,7 +332,12 @@ def text_generation_chunk_stream( stream_requests.append(command) raise AssertionError("a cancelled step must not open a stream") - async def send_task_cancellation(self, command_id: object) -> None: + async def send_task_cancellation( + self, command_id: object, *, suppress_local_finish: bool = True + ) -> None: + # The race path must not retain a local-finish marker: no chunk + # stream ever opens for this command (#833). + assert suppress_local_finish is False cancelled.append(command_id) harness = StewardHarness(cast("API", cast(object, _RacingApi()))) diff --git a/src/skulk/master/main.py b/src/skulk/master/main.py index e845a2cea..e2339c000 100644 --- a/src/skulk/master/main.py +++ b/src/skulk/master/main.py @@ -40,6 +40,7 @@ ModelCard, ModelId, ModelTask, + card_serves_speech, get_card, get_model_cards, ) @@ -262,6 +263,17 @@ def steward_candidate_is_servable(card: "ModelCard") -> bool: """ if ModelTask.TextGeneration not in card.tasks: return False + # Routing truth: worker bootstrap dispatches image, embedding, and speech + # cards to their specialized runners BEFORE text-engine selection, so a + # multi-task card carrying any of those never reaches a text runner no + # matter what else it declares. + if ( + ModelTask.TextToImage in card.tasks + or ModelTask.ImageToImage in card.tasks + or ModelTask.TextEmbedding in card.tasks + or card_serves_speech(card) + ): + return False profile = resolve_model_capability_profile(card.model_id, model_card=card) if not profile.supports_tool_calling: return False @@ -302,7 +314,10 @@ def placement_may_select_parserless_vllm( Args: card: The candidate's model card. - placements: The instances ``place_instance`` minted for it. + placements: ONLY the instances minted by this placement. The caller + must filter out pre-existing state (``place_instance`` returns + existing instances plus the new one), or an unrelated vllm or + unstamped shard would falsely condemn the candidate. Returns: True when the card pins no ``runtime.vllm_tool_call_parser``, vllm @@ -2104,6 +2119,11 @@ async def _maintain_steward_placement(self) -> None: "steward_models; replacing the placement" ) await self._teardown_steward_instances(stewards) + # This teardown is an intentional replacement: open the + # pacing window so the invariant really can re-place on + # the next tick, as promised above, instead of waiting + # out a window started by the previous placement. + self._steward_last_attempt_monotonic = time.monotonic() - 60.0 return now = time.monotonic() @@ -2159,7 +2179,12 @@ async def _maintain_steward_placement(self) -> None: f"Steward placement with {model_ref} not possible yet: {err}" ) continue - if placement_may_select_parserless_vllm(card, final_placement): + minted_instances = { + instance_id: instance + for instance_id, instance in final_placement.items() + if instance_id not in self.state.instances + } + if placement_may_select_parserless_vllm(card, minted_instances): logger.warning( f"Steward model {model_ref} resolved (or may fall back) to " "the vllm engine without a pinned tool-call parser; " diff --git a/src/skulk/master/tests/test_steward_placement.py b/src/skulk/master/tests/test_steward_placement.py index d2b04ac53..443364a2d 100644 --- a/src/skulk/master/tests/test_steward_placement.py +++ b/src/skulk/master/tests/test_steward_placement.py @@ -200,6 +200,18 @@ def _card(tasks: list[ModelTask], *, tools: bool) -> ModelCard: assert not steward_candidate_is_servable( _card([ModelTask.TextGeneration], tools=False) ) + # Routing truth: bootstrap sends image/embedding/speech cards to their + # specialized runners before text-engine dispatch, so a multi-task card + # carrying any of those tasks never reaches a text runner. + assert not steward_candidate_is_servable( + _card([ModelTask.TextGeneration, ModelTask.TextToSpeech], tools=True) + ) + assert not steward_candidate_is_servable( + _card([ModelTask.TextGeneration, ModelTask.TextEmbedding], tools=True) + ) + assert not steward_candidate_is_servable( + _card([ModelTask.TextGeneration, ModelTask.TextToImage], tools=True) + ) def test_vllm_only_steward_candidates_require_a_pinned_parser() -> None: From 0efb428d1240cf3c8f7957d7b9487de4c39f121c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 00:48:40 +0000 Subject: [PATCH 6/7] fix(steward): deregister turns from the outermost response iterator Review round five on #833: the keepalive wrapper emits its first byte before pulling its source, so a client disconnecting right after that write abandons every inner generator unstarted - and an unstarted generator's finally never runs, leaking the turn registry entry and its harness. The release wrapper now wraps the outermost response iterator, the one Starlette itself drives, which is guaranteed to start and therefore to clean up on completion, disconnect, and cancellation alike. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BU73qqVrPobiDcwcDpzNt4 --- src/skulk/api/main.py | 40 +++++++++++++------ .../api/tests/test_steward_chunk_stream.py | 40 +++++++++++++++++++ 2 files changed, 67 insertions(+), 13 deletions(-) diff --git a/src/skulk/api/main.py b/src/skulk/api/main.py index a3c232cb0..255a8ed69 100644 --- a/src/skulk/api/main.py +++ b/src/skulk/api/main.py @@ -3384,12 +3384,19 @@ async def _steward_chat_completions( # generate_chat_stream advertises the id in its SSE comment before # it pulls the first chunk, so registering lazily inside the stream # would leave a cancel-by-id 404 window during long prefill. + # Deregistration wraps the OUTERMOST response iterator: an inner + # layer (the keepalive wrapper emits bytes before pulling its + # source) can be abandoned before ever starting, and an unstarted + # generator's finally never runs — only the iterator Starlette + # itself drives is guaranteed to start and therefore to clean up. self._steward_turn_harnesses[command_id] = harness - chunk_stream = self._release_steward_turn_after(command_id, chunk_stream) if payload.stream: return StreamingResponse( - with_sse_keepalive( - generate_chat_stream(command_id, chunk_stream), + self._release_steward_turn_after( + command_id, + with_sse_keepalive( + generate_chat_stream(command_id, chunk_stream), + ), ), media_type="text/event-stream", headers={ @@ -3399,32 +3406,39 @@ async def _steward_chat_completions( }, ) return StreamingResponse( - collect_chat_response(command_id, chunk_stream), + self._release_steward_turn_after( + command_id, + collect_chat_response(command_id, chunk_stream), + ), media_type="application/json", ) async def _release_steward_turn_after( self, command_id: CommandId, - chunk_stream: "AsyncGenerator[TokenChunk | ErrorChunk | ToolCallChunk | PrefillProgressChunk, None]", - ) -> "AsyncGenerator[TokenChunk | ErrorChunk | ToolCallChunk | PrefillProgressChunk, None]": - """Deregister a steward turn from cancel-by-id when its stream ends. + response_stream: "AsyncIterator[str]", + ) -> "AsyncGenerator[str, None]": + """Deregister a steward turn from cancel-by-id when its response ends. The caller registers the turn BEFORE constructing the response (the id is advertised to the client before the first chunk is pulled); - this wrapper owns only the removal, so normal completion, client - disconnect, and cancellation all clean up through one ``finally``. + this wrapper owns only the removal. It must wrap the OUTERMOST + response iterator — the one Starlette drives — because that is the + only generator guaranteed to be started (and thus finalized) even + when the client disconnects after the first keepalive byte; an + abandoned inner generator that never started never runs its + ``finally``. Args: command_id: The outer command id the caller registered. - chunk_stream: The turn's chunk stream to pass through. + response_stream: The fully assembled response body iterator. Yields: - The wrapped stream's chunks, unchanged. + The wrapped response's items, unchanged. """ try: - async for chunk in chunk_stream: - yield chunk + async for item in response_stream: + yield item finally: self._steward_turn_harnesses.pop(command_id, None) diff --git a/src/skulk/api/tests/test_steward_chunk_stream.py b/src/skulk/api/tests/test_steward_chunk_stream.py index 82627f42d..9abb03b8b 100644 --- a/src/skulk/api/tests/test_steward_chunk_stream.py +++ b/src/skulk/api/tests/test_steward_chunk_stream.py @@ -356,3 +356,43 @@ async def send_task_cancellation( final = chunks[-1] assert isinstance(final, TokenChunk) assert final.finish_reason == "stop" + + +async def test_release_wrapper_deregisters_on_early_disconnect() -> None: + """An early disconnect must still deregister the turn (#833). + + The keepalive layer emits its first byte before pulling its source, so + a client can vanish while every inner generator is still unstarted; an + unstarted generator's ``finally`` never runs. The release wrapper sits + outermost — the iterator Starlette itself drives — so it is always + started and its cleanup always fires, even when the wrapped source + never ran at all. + """ + from skulk.api.main import API + from skulk.shared.types.common import CommandId + + api = API.__new__(API) + outer_id = CommandId() + harness = _ScriptedHarness(turns=[("irrelevant", [])]) + api._steward_turn_harnesses = {outer_id: harness} # pyright: ignore[reportPrivateUsage] + + source_started: list[bool] = [] + + async def _source(): + source_started.append(True) + yield "never reached" + + async def _keepalive_like(source): + yield ": keep-alive\n\n" + async for item in source: + yield item + + stream = api._release_steward_turn_after( # pyright: ignore[reportPrivateUsage] + outer_id, _keepalive_like(_source()) + ) + first = await stream.__anext__() + assert first == ": keep-alive\n\n" + await stream.aclose() + + assert outer_id not in api._steward_turn_harnesses # pyright: ignore[reportPrivateUsage] + assert source_started == [], "the inner source never started, by design" From 3e2a13a8dacae8d48e892c028abe5ed52611107d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 00:51:06 +0000 Subject: [PATCH 7/7] test: type the early-disconnect fixtures Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BU73qqVrPobiDcwcDpzNt4 --- src/skulk/api/tests/test_steward_chunk_stream.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/skulk/api/tests/test_steward_chunk_stream.py b/src/skulk/api/tests/test_steward_chunk_stream.py index 9abb03b8b..68093ff79 100644 --- a/src/skulk/api/tests/test_steward_chunk_stream.py +++ b/src/skulk/api/tests/test_steward_chunk_stream.py @@ -4,6 +4,7 @@ tool collaborators is the loop's unit-test seam. """ +from collections.abc import AsyncGenerator from typing import TYPE_CHECKING, Any, cast if TYPE_CHECKING: @@ -378,11 +379,13 @@ async def test_release_wrapper_deregisters_on_early_disconnect() -> None: source_started: list[bool] = [] - async def _source(): + async def _source() -> "AsyncGenerator[str, None]": source_started.append(True) yield "never reached" - async def _keepalive_like(source): + async def _keepalive_like( + source: "AsyncGenerator[str, None]", + ) -> "AsyncGenerator[str, None]": yield ": keep-alive\n\n" async for item in source: yield item