LLM gateway: ct audit + mandatory ct on ILlmClient (phase 4 of #352)#359
Merged
rockfordlhotka merged 2 commits intomainfrom May 8, 2026
Merged
LLM gateway: ct audit + mandatory ct on ILlmClient (phase 4 of #352)#359rockfordlhotka merged 2 commits intomainfrom
rockfordlhotka merged 2 commits intomainfrom
Conversation
Final phase: makes the cancellation contract enforceable at compile time and fixes the call sites that were silently dropping ct. Audit results across the codebase: 16 ILlmClient.GetResponseAsync call sites. 13 already passed ct correctly. 3 violations found: - src/RockBot.Memory/MemoryTools.cs:352 (background SaveMemory worker via Task.Run — no caller-supplied ct) - src/RockBot.Skills/SkillTools.cs:193 (background skill summary generation via Task.Run — no caller-supplied ct) - src/RockBot.Host/SessionSummaryService.cs:152 (timer-driven evaluation — no caller-supplied ct) All three are detached background work. They now pass CancellationToken.None explicitly with comments documenting the design choice and a note that future work could plumb IHostApplicationLifetime.ApplicationStopping for graceful shutdown of in-flight calls. The audit forces this to be intentional rather than accidental. Compile-time enforcement: - ILlmClient.GetResponseAsync (both overloads) drop the "= default" on cancellationToken. Callers must now pass it explicitly; the compiler catches future violations. - LlmClient implementation matched (defaults removed for consistency). - Two existing call sites (AgentCardSummarizer, McpBridgeService) used the named-arg pattern `cancellationToken: ct` which silently picked up the default for `options`; these are made explicit with `options: null`. Tests: - Adds ExecuteAsync_CancellationWhileInFlight_AbortsAndReleasesSlot, closing the in-flight cancellation gap the design called out for this phase. Verifies that cancelling ct mid-operation aborts, releases the slot, and that follow-up calls succeed (no leaked slots). - 13 LlmGatewayTests pass; 629 Host tests pass; full solution test run clean. This completes #352. Phase 1 (concurrency cap), phase 3 (bounded queue), and phase 4 (ct audit + mandatory ct) all landed; phase 2 (gateway retry) was dropped in favor of keeping SDK retry enabled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Final phase of
design/llm-gateway.md. Closes the cancellation contract: audits every existingILlmClientcall site and makescta mandatory parameter so future violations cannot be introduced silently.Builds on phases 1 (#355) and 3 (#358).
Audit results
16
ILlmClient.GetResponseAsynccall sites across the codebase. 13 already passedctcorrectly. 3 violations found and fixed:RockBot.Memory/MemoryTools.cs:352Task.Run— no caller-supplied ctRockBot.Skills/SkillTools.cs:193Task.Run— no caller-supplied ctRockBot.Host/SessionSummaryService.cs:152All three are detached background work with no natural ct. They now pass
CancellationToken.Noneexplicitly, with comments documenting the design choice and noting that a future refactor could plumbIHostApplicationLifetime.ApplicationStoppingfor graceful shutdown of in-flight LLM calls. The audit forces this to be intentional rather than accidental.Compile-time enforcement
ILlmClient.GetResponseAsync(both overloads) drop= defaultoncancellationToken. The compiler now catches any future call site that tries to omit it.LlmClientimplementation matched (defaults removed for consistency).AgentCardSummarizer,McpBridgeService) used the named-arg patterncancellationToken: ctwhich silently picked up the default foroptions. These are made explicit withoptions: nullin the same change.Tests
ExecuteAsync_CancellationWhileInFlight_AbortsAndReleasesSlotto close the in-flight cancellation gap the design called for in this phase. Verifies cancellation mid-operation aborts, releases the slot, and follow-up calls succeed.LlmGatewayTestspass.Test plan
dotnet build RockBot.slnx— cleandotnet test --filter "FullyQualifiedName~LlmGatewayTests"— 13/13 passCloses #352
Phase 1 (concurrency cap), phase 3 (bounded queue), and phase 4 (ct audit + mandatory ct) all landed. Phase 2 (gateway retry) was dropped in favor of keeping SDK retry enabled.
🤖 Generated with Claude Code