Skip to content

Report canceled inference as operation-cancelled - #1108

Open
bmehta001 wants to merge 17 commits into
mainfrom
bmehta001/cancellation-outcome
Open

bmehta001 wants to merge 17 commits into
mainfrom
bmehta001/cancellation-outcome

Conversation

@bmehta001

@bmehta001 bmehta001 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • arbitrate request cancellation against terminal publication with an atomic reusable lifecycle
  • surface canceled Generator and Engine turns as FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED instead of successful finishReason="none"
  • stage responses and claim completion before transcript commit/final streaming output so canceled turns remain uncommitted
  • make direct JavaScript cancellation reject iteration and stream.response, while preserving AbortError mapping for AbortSignal
  • clear the Node streaming callback on success and error so a canceled stream cannot leave a stale callback on the session

SSE transport disconnect/backpressure is intentionally out of scope for this focused outcome-semantics change.

Validation

  • Windows VS 2026: canonical C++ build
  • Windows: lifecycle, callback, C API, Generator chat, Engine chat, and streaming-audio cancellation tests
  • WSL/Linux: canonical C++ build and lifecycle/cancellation tests
  • Windows and WSL/Linux: Node addon build and focused cancellation streaming tests
  • Windows: all 11 JavaScript streaming tests
  • TypeScript typecheck and Biome checks for touched TypeScript files

Real-model cancellation stress

Using cached qwen2.5-0.5b-instruct-generic-cpu:4, every iteration started real inference, waited for the first streamed item, canceled, reused the same process/session, and ended with a successful recovery inference.

API Windows WSL/Linux
Direct C ABI Request_Cancel 25/25 canceled 25/25 canceled
C++ Request::Cancel 25/25 canceled 25/25 canceled
C# Request.Cancel 25/25 canceled Not run: no .NET SDK in WSL
C# CancellationToken 25/25 canceled Not run: no .NET SDK in WSL
JavaScript Request.cancel 25/25 canceled 25/25 canceled
JavaScript inference AbortSignal 25/25 canceled 25/25 canceled
Python Request.cancel 25/25 canceled 25/25 canceled

Make request completion and cancellation race-safe so abandoned work reports OperationCancelled instead of a successful finishReason of none. Preserve transactional chat recovery and clear native streaming callbacks on every terminal path.

Files changed:
- sdk_v2/cpp/src/inferencing/session and c_api.cc: add atomic request lifecycle arbitration and error propagation
- sdk_v2/cpp/src/inferencing/generative and manager.cc: propagate backend cancellation without committing output
- sdk_v2/cpp/test: cover lifecycle, chat, Engine, C API, and audio cancellation
- sdk_v2/js/native/src/session.cc: clear callbacks after both success and failure
- sdk_v2/js/src/request.ts and sdk_v2/js/test: document and verify cancellation rejection semantics

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1ec443fe-00d0-4f49-abcd-3970c5ea102e
Copilot AI balanced review requested due to automatic review settings September 15, 2026 06:39
@vercel

vercel Bot commented Sep 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
foundry-local Ready Ready Preview Sep 19, 2026 1:51am UTC

Request Review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Engine stop handling, callback synchronization, and a timing-dependent JavaScript assertion can produce incorrect or flaky behavior.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Introduces atomic request lifecycle arbitration so canceled inference surfaces consistently without publishing partial responses.

Changes:

  • Adds reusable request lifecycle state and staged response publication.
  • Propagates cancellation through C++, C API, and JavaScript streaming.
  • Expands cancellation and session-reuse tests.
File summaries
File Description
sdk_v2/js/test/streaming.test.ts Updates streaming cancellation expectations.
sdk_v2/js/test/items.test.ts Renames pre-cancellation test.
sdk_v2/js/src/request.ts Documents remembered cancellation.
sdk_v2/js/native/src/session.cc Clears streaming callbacks after all outcomes.
sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc Expects cancellation errors.
sdk_v2/cpp/test/sdk_api/cpp_api_test.cc Updates pre-cancellation semantics.
sdk_v2/cpp/test/internal_api/session_manager_test.cc Tests lifecycle arbitration.
sdk_v2/cpp/test/internal_api/item_test.cc Tests request state transitions.
sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc Verifies canceled Engine turns remain uncommitted.
sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc Updates chat cancellation assertions.
sdk_v2/cpp/test/internal_api/callback_handler_test.cc Uses lifecycle cancellation checks.
sdk_v2/cpp/test/internal_api/c_api_test.cc Updates C API cancellation test.
sdk_v2/cpp/src/manager.cc Updates shutdown documentation.
sdk_v2/cpp/src/inferencing/session/session.h Documents lifecycle-based cancellation.
sdk_v2/cpp/src/inferencing/session/session.cc Arbitrates completion and cancellation.
sdk_v2/cpp/src/inferencing/session/request.h Implements atomic request states.
sdk_v2/cpp/src/inferencing/session/callback_handler.h Uses lifecycle cancellation APIs.
sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc Maps Engine cancellation to an error.
sdk_v2/cpp/src/inferencing/generative/chat/chat_session.h Removes cancellation from finish-reason resolution.
sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc Stages and claims chat completion.
sdk_v2/cpp/src/inferencing/generative/audio/audio_session.cc Migrates audio cancellation checks.
sdk_v2/cpp/src/c_api.cc Routes cancellation through the lifecycle.
Review details
  • Files reviewed: 22/22 changed files
  • Comments generated: 4
  • Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread sdk_v2/js/native/src/session.cc
Comment thread sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc Outdated
Comment thread sdk_v2/js/test/streaming.test.ts Outdated
Comment thread sdk_v2/cpp/src/c_api.cc
@bmehta001 bmehta001 self-assigned this Sep 15, 2026
Treat OGA cancellation as a backend terminal fact and let the request lifecycle decide whether the public operation was cancelled. This avoids coupling usage retrieval to API error policy and preserves internal cancellation mechanisms.

Files changed:
- sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc: map backend cancellation without throwing
- sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc: cover backend and request cancellation boundaries

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1ec443fe-00d0-4f49-abcd-3970c5ea102e
bmehta001 and others added 2 commits September 17, 2026 17:54
Distinguish caller, callback, callback-exception, and shutdown cancellation so each public surface reports an actionable outcome. Keep accepted JavaScript workers alive safely through disposal and document the completion-versus-cancellation race instead of asserting a timing-dependent result.

Files changed:
- sdk_v2/cpp/include and src/inferencing/session: preserve cancellation provenance and bounded callback draining
- sdk_v2/cpp/test/internal_api: cover lifecycle causes, callback failures, and shutdown cancellation
- sdk_v2/js/native and src: serialize callbacks with lifetime-safe session ownership and align streaming semantics
- sdk_v2/cs, sdk_v2/python, and sdk_v2/js request wrappers: document remembered pre-start cancellation

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9290243a-cd23-4b0d-bb8c-2ce78ebe946d
Bring #1108 onto the latest mainline while preserving request cancellation arbitration across the new malformed-tool recovery and raw-envelope paths.

Files changed:
- sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc: combine tool recovery with staged cancellation-aware publication
- sdk_v2/cpp/src/inferencing/session/request.h: retain lifecycle provenance alongside upstream request fields
- sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc: preserve both recovery and cancellation coverage
- remaining files: merge upstream commits from origin/main unchanged

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9290243a-cd23-4b0d-bb8c-2ce78ebe946d

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Reused requests can bypass a shutdown latch, while Node request gating introduces concurrency and worker-pool regressions.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 29/29 changed files
  • Comments generated: 4
  • Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

Comment thread sdk_v2/cpp/src/inferencing/session/session.cc Outdated
Comment thread sdk_v2/js/native/src/session.cc Outdated
Comment thread sdk_v2/js/native/src/session.cc Outdated
Comment thread sdk_v2/js/native/src/session.cc Outdated
Queue chat and audio operations before assigning worker threads so overlapping requests cannot exhaust the shared libuv pool. Preserve accepted work across disposal, keep embeddings concurrent, and latch shutdown only after reusable requests re-enter the running state.

Files changed:
- sdk_v2/cpp/src/inferencing/session/session.cc: apply shutdown cancellation after request admission
- sdk_v2/cpp/test/internal_api/session_manager_test.cc: cover reuse after session shutdown
- sdk_v2/js/native/src/session.cc and session.h: add lifetime-safe FIFO scheduling and bypass it for embeddings
- sdk_v2/js/test/streaming.test.ts: cover overlapping callbacks and disposal with queued work

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9290243a-cd23-4b0d-bb8c-2ce78ebe946d

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Dropping a large callback backlog can leave DrainPending() blocked indefinitely because no condition-variable notification follows the drop.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 29/29 changed files
  • Comments generated: 2
  • Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

Comment thread sdk_v2/cpp/src/inferencing/session/callback_handler.h
Comment thread sdk_v2/js/src/session.ts Outdated
Notify DrainPending after cancellation discards an oversized callback backlog so waiters cannot sleep after the queue becomes empty. Keep the JavaScript streaming contract within the repository line limit.

Files changed:
- sdk_v2/cpp/src/inferencing/session/callback_handler.h: publish backlog-drop completion
- sdk_v2/cpp/test/internal_api/callback_handler_test.cc: exercise DrainPending on the drop path
- sdk_v2/js/src/session.ts: wrap cancellation documentation at 120 columns

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9290243a-cd23-4b0d-bb8c-2ce78ebe946d

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The cross-language concurrency and terminal-publication changes warrant final human review despite no identified actionable defects.

Review details
  • Files reviewed: 29/29 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

Keep idle and completed requests reusable by making Cancel a no-op outside an admitted invocation. Deregister terminal operations before publishing reusable completion so canceled, failed, and successful requests cannot race a later reuse.

Files changed:
- sdk_v2/cpp/src/inferencing/session: enforce in-flight-only cancellation and safe active-registration teardown
- sdk_v2/cpp/test: cover idle cancellation, canceled reuse, callback diagnostics, and concurrent admission
- sdk_v2/cpp/include and language bindings: document idle and completed cancellation as no-ops
- sdk_v2/js docs and tests: align FIFO-waiting and active-operation cancellation semantics

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9290243a-cd23-4b0d-bb8c-2ce78ebe946d

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Atomic lifecycle arbitration and cross-thread Node scheduling are broad concurrency changes requiring final human validation.

Review details
  • Files reviewed: 30/30 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Keep #1108 focused on active inference cancellation by removing the independent JavaScript and Python model-download work. Also consolidate overlapping real-model streaming coverage to avoid a redundant model invocation.

Files changed:
- sdk_v2/js model-download implementation, types, and tests
- sdk_v2/js/test/streaming.test.ts
- sdk_v2/python model-download implementation, documentation, and tests

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2d2c76d2-0c86-4672-b91b-3fcb54ed7906

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Streaming finalization can strand the per-session FIFO if response conversion or promise settlement throws.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread sdk_v2/js/native/src/session.cc Outdated
Copilot round 2 found that ResponseToJs or promise settlement could throw before scheduler completion. Add RAII finalization so every exit advances the per-session FIFO and releases StreamCtx, while conversion failures reject the promise.

Files changed: sdk_v2/js/native/src/session.cc

Verified at sdk_v2/js/native/src/session.cc:300 and with Windows/Linux native addon builds.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 2d2c76d2-0c86-4672-b91b-3fcb54ed7906

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Non-standard backend exceptions currently bypass cancellation-versus-completion arbitration.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread sdk_v2/cpp/src/inferencing/session/session.cc
Copilot round 3 found that catch-all backend failures skipped the cancellation-versus-completion decision. Apply the same terminal arbitration used for standard exceptions and cover the cancellation race directly.

Files changed: sdk_v2/cpp/src/inferencing/session/session.cc, sdk_v2/cpp/test/internal_api/session_manager_test.cc

Verified with filtered lifecycle tests and canonical RelWithDebInfo builds on Windows and Linux.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 2d2c76d2-0c86-4672-b91b-3fcb54ed7906

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Atomic lifecycle arbitration and cross-thread Node callback scheduling warrant final human concurrency review.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Remove the unused manager include and correct stale comments so the generic worker is documented only for its remaining catalog and model lifecycle consumers.

Files changed:
- sdk_v2/js/native/src/manager.cc
- sdk_v2/js/native/src/promise_worker.h
- sdk_v2/js/native/src/session.h

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2d2c76d2-0c86-4672-b91b-3fcb54ed7906

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The streaming-audio timeout path can wait indefinitely without canceling the stalled inference.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc
Align comments and JS documentation with the atomic request lifecycle, addon-owned session queue, per-request streaming worker, and current processStreamingRequest API.

Files changed:
- sdk_v2/cpp/src/inferencing/session/session.h
- sdk_v2/cpp/test/internal_api/callback_handler_test.cc
- sdk_v2/cs/src/StreamingResponse.cs
- sdk_v2/js/docs/PortJsToSdkV2.md
- sdk_v2/js/src/request.ts
- sdk_v2/js/src/session.ts

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2d2c76d2-0c86-4672-b91b-3fcb54ed7906

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The cross-language lifecycle, concurrency, and callback-ordering changes warrant final human validation despite extensive focused tests.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Ensure the cancellation test's admission-timeout cleanup requests cancellation before joining the worker, preventing a failed test path from hanging indefinitely.

Files changed:
- sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2d2c76d2-0c86-4672-b91b-3fcb54ed7906

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The cross-thread lifecycle and scheduler changes span core native inference and multiple language bindings, warranting final human review.

Review effort: Balanced
Findings: None

Resolved since last review (1)

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