Skip to content

Forward the cancellation token in the synchronous ModifyAssistant - #1272

Open
adityasingh2400 wants to merge 5 commits into
openai:mainfrom
adityasingh2400:fix-modify-assistant-cancellation-token
Open

Forward the cancellation token in the synchronous ModifyAssistant#1272
adityasingh2400 wants to merge 5 commits into
openai:mainfrom
adityasingh2400:fix-modify-assistant-cancellation-token

Conversation

@adityasingh2400

Copy link
Copy Markdown
Contributor

The synchronous AssistantClient.ModifyAssistant overload passed a literal null to the protocol method instead of the caller's cancellation token, so the token was dropped before the request was sent and the call could not be cancelled. Every other convenience method on this client already forwards its token through ToRequestOptions, including ModifyAssistant's own asynchronous twin one method above it, so this was an isolated gap rather than a deliberate difference. The change makes the synchronous overload match its sibling and honor the behavior its own documentation comment describes. Callers who pass a default token see no change at all, because ToRequestOptions returns null for default, which is exactly what was being passed before. The added mock transport regression test exercises both overloads and fails only on the synchronous one without this fix.

The synchronous AssistantClient.ModifyAssistant convenience overload passed a
literal null to the protocol method instead of the caller's cancellation token,
so the token was dropped and the request could not be cancelled. Its own
asynchronous twin and every other convenience method on the client already
forward the token through ToRequestOptions.

Added a mock-transport regression test covering both overloads.
@adityasingh2400
adityasingh2400 force-pushed the fix-modify-assistant-cancellation-token branch from 36abd80 to 9735567 Compare August 11, 2026 00:48
Comment thread tests/Assistants/AssistantsMockTests.cs Outdated
@adityasingh2400

Copy link
Copy Markdown
Contributor Author

Good catch, that was a real gap. ExpectSyncPipeline = !IsAsync set the transport up for both modes, but the body only ever called ModifyAssistantAsync, so the synchronous path this PR fixes was never reached and the sync half of the fixture proved nothing.

Now branched on IsAsync, matching StreamingRunSurfacesErrorEventAsException:

if (IsAsync)
{
    Assert.That(
        async () => await client.ModifyAssistantAsync("asst_abc", new AssistantModificationOptions(), cancellationSource.Token),
        Throws.InstanceOf<OperationCanceledException>());
}
else
{
    Assert.That(
        () => client.ModifyAssistant("asst_abc", new AssistantModificationOptions(), cancellationSource.Token),
        Throws.InstanceOf<OperationCanceledException>());
}

One thing I should be straight about: I could not run this locally. global.json pins SDK 10.0.400 and I only have 9.0.301, so I am relying on CI rather than a local green run. The sync overload takes CancellationToken cancellationToken = default at AssistantClient.cs:201, so the call shape matches, but please treat the pass as unverified by me until CI says otherwise.

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.

2 participants