From e94674388889e88aff44777fc83342804cde5c8a Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 15 Sep 2026 01:39:25 -0500 Subject: [PATCH 01/16] Report canceled inference as an error 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 --- sdk_v2/cpp/src/c_api.cc | 2 +- .../generative/audio/audio_session.cc | 37 ++++---- .../generative/chat/chat_session.cc | 54 ++++++----- .../generative/chat/chat_session.h | 4 +- .../chat/onnx_engine_chat_stream.cc | 6 +- .../inferencing/session/callback_handler.h | 6 +- sdk_v2/cpp/src/inferencing/session/request.h | 73 +++++++++++++-- sdk_v2/cpp/src/inferencing/session/session.cc | 41 +++++++-- sdk_v2/cpp/src/inferencing/session/session.h | 6 +- sdk_v2/cpp/src/manager.cc | 2 +- sdk_v2/cpp/test/internal_api/c_api_test.cc | 4 +- .../internal_api/callback_handler_test.cc | 10 +-- .../internal_api/chat/chat_session_test.cc | 39 ++++---- .../chat/dynamic_engine_chat_test.cc | 8 +- sdk_v2/cpp/test/internal_api/item_test.cc | 24 ++++- .../test/internal_api/session_manager_test.cc | 82 +++++++++++++---- sdk_v2/cpp/test/sdk_api/cpp_api_test.cc | 4 +- .../cpp/test/sdk_api/streaming_audio_test.cc | 10 ++- sdk_v2/js/native/src/session.cc | 32 +++++-- sdk_v2/js/src/request.ts | 4 +- sdk_v2/js/test/items.test.ts | 2 +- sdk_v2/js/test/streaming.test.ts | 89 +++++++++---------- 22 files changed, 360 insertions(+), 179 deletions(-) diff --git a/sdk_v2/cpp/src/c_api.cc b/sdk_v2/cpp/src/c_api.cc index 92ebc0556..04d41b7a2 100644 --- a/sdk_v2/cpp/src/c_api.cc +++ b/sdk_v2/cpp/src/c_api.cc @@ -1727,7 +1727,7 @@ FL_API_STATUS_IMPL(Request_CancelImpl, flRequest* request) { if (!request) { return MakeStatus(FOUNDRY_LOCAL_ERROR_INVALID_ARGUMENT, "null argument"); } - AsImpl(request)->canceled = true; + AsImpl(request)->Cancel(); return nullptr; API_IMPL_END } diff --git a/sdk_v2/cpp/src/inferencing/generative/audio/audio_session.cc b/sdk_v2/cpp/src/inferencing/generative/audio/audio_session.cc index 896309417..4039cfcdf 100644 --- a/sdk_v2/cpp/src/inferencing/generative/audio/audio_session.cc +++ b/sdk_v2/cpp/src/inferencing/generative/audio/audio_session.cc @@ -240,7 +240,7 @@ void AudioSession::ProcessRequestImpl(const Request& request, Response& response int prompt_tokens = generator->PromptTokenCount(); // Token-by-token generation with optional streaming. - // Check request.canceled each iteration — a streaming callback returning + // Check request cancellation each iteration — a streaming callback returning // non-zero sets this flag asynchronously via CallbackHandler. std::vector token_texts; token_texts.reserve(kInitialTokenCapacity); @@ -248,7 +248,7 @@ void AudioSession::ProcessRequestImpl(const Request& request, Response& response std::vector> segments; segments.reserve(kInitialTokenCapacity); - while (!generator->IsDone() && !request.canceled) { + while (!generator->IsDone() && !request.IsCancellationRequested()) { generator->GenerateNextToken(); std::string token = generator->Decode(); @@ -262,7 +262,7 @@ void AudioSession::ProcessRequestImpl(const Request& request, Response& response token_texts.push_back(std::move(token)); } - if (request.canceled) { + if (request.IsCancellationRequested()) { generator->Cancel(); } } @@ -275,7 +275,7 @@ void AudioSession::ProcessRequestImpl(const Request& request, Response& response response.items.push_back(BuildSpeechResult(std::move(text), std::move(segments))); // Set finish reason - if (request.canceled) { + if (request.IsCancellationRequested()) { response.finish_reason = FOUNDRY_LOCAL_FINISH_NONE; } else { response.finish_reason = FOUNDRY_LOCAL_FINISH_STOP; @@ -358,7 +358,7 @@ void AudioSession::ProcessStreamingAudio(const AudioItem& format_item, ItemQueue } // 4. Read from queue until finished or cancelled - while (!request.canceled) { + while (!request.IsCancellationRequested()) { auto item = queue.WaitAndPop(std::chrono::milliseconds(100)); if (!item) { @@ -383,7 +383,7 @@ void AudioSession::ProcessStreamingAudio(const AudioItem& format_item, ItemQueue } // 5. Flush remaining buffered audio - if (!request.canceled) { + if (!request.IsCancellationRequested()) { auto flush_tensors = processor->Flush(); if (flush_tensors) { @@ -398,7 +398,7 @@ void AudioSession::ProcessStreamingAudio(const AudioItem& format_item, ItemQueue const size_t full_text_size = full_text.size(); response.items.push_back(BuildSpeechResult(std::move(full_text), std::move(segments))); - if (request.canceled) { + if (request.IsCancellationRequested()) { response.finish_reason = FOUNDRY_LOCAL_FINISH_NONE; } else { response.finish_reason = FOUNDRY_LOCAL_FINISH_STOP; @@ -434,7 +434,7 @@ void AudioSession::DecodeTokens(OgaGenerator& generator, OgaTokenizerStream& tok const std::unique_ptr& callback, const Request& request, int& completion_tokens) { - while (!generator.IsDone() && !generator.IsSessionTerminated() && !request.canceled) { + while (!generator.IsDone() && !generator.IsSessionTerminated() && !request.IsCancellationRequested()) { generator.GenerateNextToken(); auto next_tokens = generator.GetNextTokens(); @@ -508,7 +508,7 @@ void AudioSession::ProcessAudioTranscriptionJson(const std::string& request_json // Generate token-by-token std::string text; - while (!generator->IsDone() && !original_request.canceled) { + while (!generator->IsDone() && !original_request.IsCancellationRequested()) { generator->GenerateNextToken(); std::string token = generator->Decode(); @@ -525,7 +525,7 @@ void AudioSession::ProcessAudioTranscriptionJson(const std::string& request_json } } - if (original_request.canceled) { + if (original_request.IsCancellationRequested()) { generator->Cancel(); } } @@ -534,7 +534,7 @@ void AudioSession::ProcessAudioTranscriptionJson(const std::string& request_json int completion_tokens = total_tokens - prompt_tokens; // Set finish reason - if (original_request.canceled) { + if (original_request.IsCancellationRequested()) { response.finish_reason = FOUNDRY_LOCAL_FINISH_NONE; } else { response.finish_reason = FOUNDRY_LOCAL_FINISH_STOP; @@ -589,7 +589,8 @@ void AudioSession::DecodeNemotronTokens(OgaGenerator& generator, OgaTokenizerStr int& completion_tokens) const { const bool is_streaming = (streaming_callback != nullptr); - while (!generator.IsDone() && !generator.IsSessionTerminated() && !original_request.canceled) { + while (!generator.IsDone() && !generator.IsSessionTerminated() && + !original_request.IsCancellationRequested()) { generator.GenerateNextToken(); auto next_tokens = generator.GetNextTokens(); if (next_tokens.empty()) { @@ -624,7 +625,7 @@ void AudioSession::RunNemotronDecodePass(std::unique_ptr tensor const std::unique_ptr& streaming_callback, const std::string& response_id, const Request& original_request, int& completion_tokens) const { - if (!tensors || original_request.canceled) { + if (!tensors || original_request.IsCancellationRequested()) { return; } @@ -636,7 +637,7 @@ void AudioSession::RunNemotronDecodePass(std::unique_ptr tensor void AudioSession::ProcessNemotronFileTranscription(const AudioTranscriptionRequest& req, const Request& original_request, Response& response) { - if (original_request.canceled) { + if (original_request.IsCancellationRequested()) { response.finish_reason = FOUNDRY_LOCAL_FINISH_NONE; return; } @@ -673,18 +674,20 @@ void AudioSession::ProcessNemotronFileTranscription(const AudioTranscriptionRequ int completion_tokens = 0; constexpr size_t kNemotronSamplesPerChunk = 1600; // 100ms at 16kHz - for (size_t offset = 0; offset < samples.size() && !original_request.canceled; + for (size_t offset = 0; + offset < samples.size() && !original_request.IsCancellationRequested(); offset += kNemotronSamplesPerChunk) { size_t count = std::min(kNemotronSamplesPerChunk, samples.size() - offset); RunNemotronDecodePass(processor->Process(samples.data() + offset, count), *generator, *tokenizer_stream, text, streaming_callback, response_id, original_request, completion_tokens); } - if (!original_request.canceled) { + if (!original_request.IsCancellationRequested()) { RunNemotronDecodePass(processor->Flush(), *generator, *tokenizer_stream, text, streaming_callback, response_id, original_request, completion_tokens); } - response.finish_reason = original_request.canceled ? FOUNDRY_LOCAL_FINISH_NONE : FOUNDRY_LOCAL_FINISH_STOP; + response.finish_reason = + original_request.IsCancellationRequested() ? FOUNDRY_LOCAL_FINISH_NONE : FOUNDRY_LOCAL_FINISH_STOP; // Nemotron file-transcription path feeds audio tensors directly and does not expose prompt token accounting. response.usage.prompt_tokens = 0; response.usage.completion_tokens = completion_tokens; diff --git a/sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc b/sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc index d7565aee0..25d6b5bec 100644 --- a/sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc +++ b/sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc @@ -241,17 +241,12 @@ void NormalizeToolOutputBatch(ToolCallStreamAccumulator::Output& output, } } -flFinishReason ResolveGeneratedFinishReason(bool canceled, - bool has_tool_calls, +flFinishReason ResolveGeneratedFinishReason(bool has_tool_calls, bool stop_sequence_matched, bool host_output_limit_reached, std::optional backend_finish_reason, int completion_tokens, std::optional max_output_tokens) { - if (canceled) { - return FOUNDRY_LOCAL_FINISH_NONE; - } - if (has_tool_calls) { return FOUNDRY_LOCAL_FINISH_TOOL_CALLS; } @@ -497,7 +492,6 @@ ToolCallContext ChatSession::BuildToolCallContext(const Request& request, void ChatSession::ProcessGeneratedOutput(std::vector events, const ToolCallContext& tool_ctx, const SearchOptions& effective_options, - bool canceled, bool stop_sequence_matched, bool host_output_limit_reached, Response& response, @@ -550,8 +544,8 @@ void ChatSession::ProcessGeneratedOutput(std::vector event flush_segments(); response.finish_reason = chat_session_internal::ResolveGeneratedFinishReason( - canceled, has_tool_calls, stop_sequence_matched, host_output_limit_reached, backend_finish_reason, - completion_tokens, effective_options.max_output_tokens); + has_tool_calls, stop_sequence_matched, host_output_limit_reached, backend_finish_reason, completion_tokens, + effective_options.max_output_tokens); response.usage.prompt_tokens = prompt_tokens; response.usage.completion_tokens = completion_tokens; @@ -745,7 +739,7 @@ void ChatSession::ProcessRequestImpl(const Request& request, Response& response) } // Generate token-by-token with optional streaming. - // Check request.canceled each iteration — a streaming callback returning + // Check request cancellation each iteration — a streaming callback returning // non-zero sets this flag asynchronously via CallbackHandler. auto streaming_callback = CreateCallbackHandler(request); int output_tokens = 0; @@ -831,7 +825,7 @@ void ChatSession::ProcessRequestImpl(const Request& request, Response& response) auto flush_accumulator = [&]() { emit_tool_output(tool_accumulator.Flush()); }; - while (!cached_generator_->IsDone() && !request.canceled && !turn_guard.TurnEnded()) { + while (!cached_generator_->IsDone() && !request.IsCancellationRequested() && !turn_guard.TurnEnded()) { cached_generator_->GenerateNextToken(); const auto token_id = cached_generator_->CurrentTokenId(); std::string token = cached_generator_->Decode(); @@ -856,7 +850,7 @@ void ChatSession::ProcessRequestImpl(const Request& request, Response& response) chat_session_internal::FlushDecodedStream(active_stop_filter, splitter, emit_segments); flush_accumulator(); - if (request.canceled) { + if (request.IsCancellationRequested()) { cached_generator_->Cancel(); } else if ((stop_sequence_matched || host_output_limit_reached) && !cached_generator_->IsDone()) { cached_generator_->Cancel(); @@ -876,25 +870,21 @@ void ChatSession::ProcessRequestImpl(const Request& request, Response& response) backend_finish_reason = turn_usage->finish_reason; } + if (request.IsCancellationRequested()) { + return; + } + auto assistant_message = MakeAssistantMessage(generated_events, cached_tool_ctx_, logger_); const bool generated_tool_calls = assistant_message.HasToolCalls(); - if (!request.canceled) { - // Reject a generation whose calls cannot be correlated before it reaches the caller — a committed turn must never - // leave the outstanding-call set inconsistent. - transcript_.ValidateGeneratedOutput(assistant_message); - } + // Reject a generation whose calls cannot be correlated before it reaches the caller — a committed turn must never + // leave the outstanding-call set inconsistent. + transcript_.ValidateGeneratedOutput(assistant_message); - ProcessGeneratedOutput(std::move(generated_events), cached_tool_ctx_, effective_options, request.canceled, + ProcessGeneratedOutput(std::move(generated_events), cached_tool_ctx_, effective_options, stop_sequence_matched, host_output_limit_reached, response, prompt_tokens, total_tokens, splitter.ReasoningTokenCount(), backend_finish_reason); - if (request.canceled) { - // Cancel is permanent for classic generators, and Engine cannot rewind. The scope guard therefore discards every - // canceled generator so the next request rebuilds from the committed transcript. - return; - } - // LARK grammar (tool-call-only mode) is a single-shot finite parse. If generation was truncated while grammar was // active, the parser is in an unrecoverable state. Additionally, a completed grammar signals EOS — IsDone() would // return true on the next turn. Invalidate after any grammar-guided generation so the next turn rebuilds. @@ -919,6 +909,10 @@ void ChatSession::ProcessRequestImpl(const Request& request, Response& response) // The reply may only merge into an input message from the last replay segment — this request's own input. Merging // into an earlier hop's assistant message would glue two recorded turns together. + if (!request.TryComplete()) { + return; + } + transcript_.CommitTurn(std::move(inputs), std::move(assistant_message), {pre_turn_token_count, total_tokens}, ingest.last_segment_start); turn_committed = true; @@ -1099,7 +1093,7 @@ void ChatSession::ProcessChatCompletionsJson(const std::string& request_json, co StopStringFilter stop_filter(options.stop_sequences); auto* active_stop_filter = options.stop_sequences.empty() ? nullptr : &stop_filter; bool stop_sequence_matched = false; - while (!generator->IsDone() && !original_request.canceled && !turn_guard.TurnEnded()) { + while (!generator->IsDone() && !original_request.IsCancellationRequested() && !turn_guard.TurnEnded()) { generator->GenerateNextToken(); const auto token_id = generator->CurrentTokenId(); std::string token = generator->Decode(); @@ -1116,7 +1110,7 @@ void ChatSession::ProcessChatCompletionsJson(const std::string& request_json, co chat_session_internal::FlushDecodedStream(active_stop_filter, splitter, process_segments); process_tool_output(tool_accumulator.Flush()); - if (original_request.canceled) { + if (original_request.IsCancellationRequested()) { generator->Cancel(); } else if (stop_sequence_matched && !generator->IsDone()) { generator->Cancel(); @@ -1136,11 +1130,15 @@ void ChatSession::ProcessChatCompletionsJson(const std::string& request_json, co backend_finish_reason = turn_usage->finish_reason; } - ProcessGeneratedOutput(std::move(generated_events), tool_ctx, options, original_request.canceled, + if (original_request.IsCancellationRequested()) { + return; + } + + ProcessGeneratedOutput(std::move(generated_events), tool_ctx, options, stop_sequence_matched, /*host_output_limit_reached=*/false, response, prompt_tokens, total_tokens, splitter.ReasoningTokenCount(), backend_finish_reason); - if (original_request.canceled) { + if (!original_request.TryComplete()) { return; } diff --git a/sdk_v2/cpp/src/inferencing/generative/chat/chat_session.h b/sdk_v2/cpp/src/inferencing/generative/chat/chat_session.h index 896cf7cd1..583ab21f9 100644 --- a/sdk_v2/cpp/src/inferencing/generative/chat/chat_session.h +++ b/sdk_v2/cpp/src/inferencing/generative/chat/chat_session.h @@ -85,8 +85,7 @@ void FlushDecodedStream(StopStringFilter* stop_filter, process_segments(splitter.Flush()); } -flFinishReason ResolveGeneratedFinishReason(bool canceled, - bool has_tool_calls, +flFinishReason ResolveGeneratedFinishReason(bool has_tool_calls, bool stop_sequence_matched, bool host_output_limit_reached, std::optional backend_finish_reason, @@ -175,7 +174,6 @@ class ChatSession : public Session { void ProcessGeneratedOutput(std::vector events, const ToolCallContext& tool_ctx, const SearchOptions& effective_options, - bool canceled, bool stop_sequence_matched, bool host_output_limit_reached, Response& response, diff --git a/sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc b/sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc index 17e9130e7..251659ab2 100644 --- a/sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc +++ b/sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc @@ -23,8 +23,6 @@ std::optional MapFinishReason(OgaFinishReason reason) { case OgaFinishReason_MaxGeneratedTokens: case OgaFinishReason_MaxSessionTokens: return FOUNDRY_LOCAL_FINISH_LENGTH; - case OgaFinishReason_Cancelled: - return FOUNDRY_LOCAL_FINISH_NONE; case OgaFinishReason_Failed: return FOUNDRY_LOCAL_FINISH_ERROR; default: @@ -190,6 +188,10 @@ void OnnxEngineChatStream::ResetTurnDecoder() { std::optional OnnxEngineChatStream::GetTurnUsage() const { const auto result = engine_.GetTurnResult(conversation_); + if (result.finish_reason == OgaFinishReason_Cancelled) { + FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled"); + } + return ChatTurnUsage{ prompt_token_count_, static_cast(result.generated_tokens), diff --git a/sdk_v2/cpp/src/inferencing/session/callback_handler.h b/sdk_v2/cpp/src/inferencing/session/callback_handler.h index 1d73b2237..f78e1baba 100644 --- a/sdk_v2/cpp/src/inferencing/session/callback_handler.h +++ b/sdk_v2/cpp/src/inferencing/session/callback_handler.h @@ -56,7 +56,7 @@ struct CallbackHandler { /// Push an item into the queue and wake the worker. /// Called from the generator thread — returns immediately. void PushItem(std::unique_ptr item) { - if (request_.canceled) { + if (request_.IsCancellationRequested()) { return; } @@ -94,7 +94,7 @@ struct CallbackHandler { try { if (fn_(data_, user_data_) != 0) { - request_.canceled = true; + request_.Cancel(); } } catch (const std::exception& e) { logger_.Log(LogLevel::Warning, @@ -125,7 +125,7 @@ struct CallbackHandler { /// cancelled (so PushItem becomes a no-op and the generator loop stops feeding work) /// and drops any items still queued so the destructor can join cleanly. void DisableAfterException() { - request_.canceled = true; + request_.Cancel(); while (queue_->TryPop()) { } } diff --git a/sdk_v2/cpp/src/inferencing/session/request.h b/sdk_v2/cpp/src/inferencing/session/request.h index ada014610..d822c93a5 100644 --- a/sdk_v2/cpp/src/inferencing/session/request.h +++ b/sdk_v2/cpp/src/inferencing/session/request.h @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -23,6 +24,14 @@ inline constexpr const char* kSystemPromptOption = "system_prompt"; /// Generic inference request — pure input data. /// Items are stored as borrowed pointers. Owned items are kept alive in owned_items. struct Request { + enum class State : uint8_t { + Ready, + Running, + Canceled, + Completing, + Completed, + }; + std::vector items; // all items (borrowed pointers) KeyValuePairs options; @@ -39,26 +48,20 @@ struct Request { /// ascending and consistent with `items`. std::vector item_segment_starts; - /// Cancellation flag — set by the C API or streaming callback handler to cancel - /// an in-flight request. Checked in generation loops. Atomic because it is written - /// by one thread (callback worker or C API) and read by another (generator loop). - /// Uses relaxed ordering since it is a one-way flag and exact timing doesn't matter. - mutable std::atomic canceled{false}; - Request() = default; Request(Request&& other) noexcept : items(std::move(other.items)), options(std::move(other.options)), item_segment_starts(std::move(other.item_segment_starts)), - canceled(other.canceled.load(std::memory_order_relaxed)), + state_(other.state_.load(std::memory_order_relaxed)), owned_items(std::move(other.owned_items)) {} Request& operator=(Request&& other) noexcept { items = std::move(other.items); options = std::move(other.options); item_segment_starts = std::move(other.item_segment_starts); - canceled.store(other.canceled.load(std::memory_order_relaxed), std::memory_order_relaxed); + state_.store(other.state_.load(std::memory_order_relaxed), std::memory_order_relaxed); owned_items = std::move(other.owned_items); return *this; } @@ -83,7 +86,61 @@ struct Request { item_segment_starts.push_back(items.size()); } + /// Atomically wins cancellation against terminal publication. Returns false after completion has won. + bool Cancel() const noexcept { + auto state = state_.load(std::memory_order_acquire); + while (state == State::Ready || state == State::Running) { + if (state_.compare_exchange_weak(state, State::Canceled, + std::memory_order_acq_rel, + std::memory_order_acquire)) { + return true; + } + } + + return state == State::Canceled; + } + + bool IsCancellationRequested() const noexcept { + return state_.load(std::memory_order_acquire) == State::Canceled; + } + + /// Starts first-time processing or reuses a request whose previous operation completed. + bool TryBegin() const noexcept { + auto state = state_.load(std::memory_order_acquire); + while (state == State::Ready || state == State::Completed) { + if (state_.compare_exchange_weak(state, State::Running, + std::memory_order_acq_rel, + std::memory_order_acquire)) { + return true; + } + } + + return false; + } + + /// Atomically claims the normal or error terminal boundary. Returns false when cancellation won first. + bool TryComplete() const noexcept { + auto expected = State::Running; + if (state_.compare_exchange_strong(expected, State::Completing, + std::memory_order_acq_rel, + std::memory_order_acquire)) { + return true; + } + + return expected == State::Completing; + } + + /// Makes a claimed completion reusable after the outer request call has finished publishing its result. + void PublishCompletion() const noexcept { + state_.store(State::Completed, std::memory_order_release); + } + + bool IsCompleted() const noexcept { + return state_.load(std::memory_order_acquire) == State::Completed; + } + private: + mutable std::atomic state_{State::Ready}; std::vector> owned_items; // owned items (lifetime) }; diff --git a/sdk_v2/cpp/src/inferencing/session/session.cc b/sdk_v2/cpp/src/inferencing/session/session.cc index d2f34bb51..2b4ebea4d 100644 --- a/sdk_v2/cpp/src/inferencing/session/session.cc +++ b/sdk_v2/cpp/src/inferencing/session/session.cc @@ -147,13 +147,17 @@ void Session::ProcessRequest(const Request& request, Response& response) { { std::lock_guard active_lock(*active_requests_mutex_); - active_requests_.insert(&request); - // If Cancel() already ran (shutdown began before this request was admitted), stamp it now so the // generation loop exits at its first poll instead of running an uncanceled turn. if (session_canceled_) { - request.canceled.store(true, std::memory_order_relaxed); + request.Cancel(); + } + + if (!request.TryBegin() && !request.IsCancellationRequested()) { + FL_THROW(FOUNDRY_LOCAL_ERROR_INVALID_USAGE, "request is already being processed"); } + + active_requests_.insert(&request); } // RAII: deregister the request even if ProcessRequestImpl throws, so Cancel() never @@ -170,25 +174,46 @@ void Session::ProcessRequest(const Request& request, Response& response) { ActionTracker tracker(Action::kSessionProcessRequest, telemetry_); tracker.SetModelId(CatalogModel().Id()); + Response staged_response; try { + if (request.IsCancellationRequested()) { + FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled"); + } + ValidateRequestItems(request); - ProcessRequestImpl(request, response); + ProcessRequestImpl(request, staged_response); + if (!request.TryComplete()) { + FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled"); + } + + response = std::move(staged_response); + request.PublishCompletion(); tracker.SetStatus(ActionStatus::kSuccess); } catch (const std::exception& ex) { - tracker.RecordException(ex); - throw; + if (request.TryComplete()) { + request.PublishCompletion(); + tracker.RecordException(ex); + throw; + } + + try { + FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled"); + } catch (const std::exception& cancellation) { + tracker.RecordException(cancellation); + throw; + } } } void Session::Cancel() { - // Only flip cancel flags — never block or join — so this is safe to call while the + // Only update request lifecycle state — never block or join — so this is safe to call while the // SessionManager holds its own lock during shutdown. Generation loops poll the flag. std::lock_guard lock(*active_requests_mutex_); session_canceled_ = true; for (const Request* r : active_requests_) { - r->canceled.store(true, std::memory_order_relaxed); + r->Cancel(); } } diff --git a/sdk_v2/cpp/src/inferencing/session/session.h b/sdk_v2/cpp/src/inferencing/session/session.h index f2db93c5c..e46dfac2c 100644 --- a/sdk_v2/cpp/src/inferencing/session/session.h +++ b/sdk_v2/cpp/src/inferencing/session/session.h @@ -54,9 +54,9 @@ class Session { /// in-flight callbacks and ensures the Response is fully populated on return. void ProcessRequest(const Request& request, Response& response); - /// Signal every in-flight request on this session to cancel. Only sets each request's atomic - /// cancel flag — never blocks and never joins — so it is safe to call from a shutdown path while - /// another thread holds a manager lock. Generation loops poll the flag and stop within ~one token. + /// Signal every in-flight request on this session to cancel. Only updates each request's atomic + /// lifecycle — never blocks and never joins — so it is safe to call from a shutdown path while + /// another thread holds a manager lock. Generation loops poll the request between backend scheduling quanta. void Cancel(); /// Add a tool definition to this session. Names are case-sensitive and unique across kinds. diff --git a/sdk_v2/cpp/src/manager.cc b/sdk_v2/cpp/src/manager.cc index af189b641..1a9304717 100644 --- a/sdk_v2/cpp/src/manager.cc +++ b/sdk_v2/cpp/src/manager.cc @@ -542,7 +542,7 @@ void Manager::Shutdown() { // Order matters: // 1. Reject new loads so callers gated on IsShutdownRequested can stop early. // 2. Cancel in-flight generations BEFORE stopping the web service. StopWebService() hard-joins - // streaming threads; a generation grinding in ORT GenAI only stops when request.canceled is + // streaming threads; a generation grinding in ORT GenAI only stops when request cancellation is // set, so cancelling first is what lets JoinAll() return promptly instead of deadlocking // process shutdown. // 3. Stop the web service (JoinAll now unblocks), then drain HTTP-tracked sessions. diff --git a/sdk_v2/cpp/test/internal_api/c_api_test.cc b/sdk_v2/cpp/test/internal_api/c_api_test.cc index 150266bb9..e9223d426 100644 --- a/sdk_v2/cpp/test/internal_api/c_api_test.cc +++ b/sdk_v2/cpp/test/internal_api/c_api_test.cc @@ -1712,7 +1712,7 @@ TEST(CApiTest, ItemQueuePushPopAndFinish) { // Inference API — Request_Cancel // ======================================================================== -TEST(CApiTest, RequestCancelOnIdleRequest) { +TEST(CApiTest, RequestCancelBeforeAttachmentSucceeds) { const flApi* api = GetApi(); const flInferenceApi* inf_api = api->GetInferenceApi(); @@ -1720,7 +1720,7 @@ TEST(CApiTest, RequestCancelOnIdleRequest) { ASSERT_TRUE(IsOk(inf_api->Request_Create(&req))); ASSERT_NE(req, nullptr); - // Cancel on an idle request should succeed (no-op) + // Cancellation is remembered until a session would otherwise attach the request. EXPECT_TRUE(IsOk(inf_api->Request_Cancel(req))); inf_api->Request_Release(req); diff --git a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc index 872cdb45b..be3d0609f 100644 --- a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc +++ b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc @@ -42,7 +42,7 @@ TEST(CallbackHandlerTest, StdExceptionFromCallbackDoesNotTerminate) { } EXPECT_GE(invocations.load(), 1); - EXPECT_TRUE(request.canceled.load()); + EXPECT_TRUE(request.IsCancellationRequested()); } TEST(CallbackHandlerTest, NonStdExceptionFromCallbackDoesNotTerminate) { @@ -61,7 +61,7 @@ TEST(CallbackHandlerTest, NonStdExceptionFromCallbackDoesNotTerminate) { } EXPECT_GE(invocations.load(), 1); - EXPECT_TRUE(request.canceled.load()); + EXPECT_TRUE(request.IsCancellationRequested()); } TEST(CallbackHandlerTest, FurtherPushesAfterExceptionAreNoOps) { @@ -72,11 +72,11 @@ TEST(CallbackHandlerTest, FurtherPushesAfterExceptionAreNoOps) { handler.PushItem(std::make_unique("first")); // Wait until the worker has cancelled the request after catching the throw. - for (int i = 0; i < 200 && !request.canceled.load(); ++i) { + for (int i = 0; i < 200 && !request.IsCancellationRequested(); ++i) { std::this_thread::sleep_for(std::chrono::milliseconds(5)); } - ASSERT_TRUE(request.canceled.load()); + ASSERT_TRUE(request.IsCancellationRequested()); const int invocations_after_first = invocations.load(); @@ -109,7 +109,7 @@ TEST(CallbackHandlerTest, NormalCallbackCancelsViaReturnValue) { handler.Drain(); EXPECT_EQ(invocations.load(), 1); - EXPECT_TRUE(request.canceled.load()); + EXPECT_TRUE(request.IsCancellationRequested()); } TEST(CallbackHandlerTest, DrainPendingWaitsForDeliveryWithoutClosingTheQueue) { diff --git a/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc b/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc index 2b57f66c8..6c4759c2c 100644 --- a/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc +++ b/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc @@ -52,6 +52,16 @@ void AppendSegments(std::vector& destination, const std::vector +void ExpectOperationCancelled(Action&& action) { + try { + action(); + FAIL() << "Expected operation cancellation"; + } catch (const fl::Exception& error) { + EXPECT_EQ(error.code(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + } +} + } // namespace TEST(ChatSessionDecisionTest, HostOutputLimitTruncatesOnlyAnUnfinishedBackendAtTheBoundary) { @@ -204,31 +214,28 @@ TEST(ChatSessionDecisionTest, FinishReasonPrecedenceCoversEveryTerminalSource) { struct TestCase { const char* name; - bool canceled; bool has_tool_calls; bool stop_sequence_matched; bool host_output_limit_reached; std::optional backend_finish_reason; flFinishReason expected; }; - const std::vector cases = { - {"cancellation wins", true, true, true, true, FOUNDRY_LOCAL_FINISH_NONE, FOUNDRY_LOCAL_FINISH_NONE}, - {"tool calls win over stop", false, true, true, false, FOUNDRY_LOCAL_FINISH_STOP, + {"tool calls win over stop", true, true, false, FOUNDRY_LOCAL_FINISH_STOP, FOUNDRY_LOCAL_FINISH_TOOL_CALLS}, - {"tool calls win over host limit", false, true, true, true, FOUNDRY_LOCAL_FINISH_NONE, + {"tool calls win over host limit", true, true, true, FOUNDRY_LOCAL_FINISH_NONE, FOUNDRY_LOCAL_FINISH_TOOL_CALLS}, - {"stop wins over host limit", false, false, true, true, FOUNDRY_LOCAL_FINISH_NONE, + {"stop wins over host limit", false, true, true, FOUNDRY_LOCAL_FINISH_NONE, FOUNDRY_LOCAL_FINISH_STOP}, - {"host limit produces length", false, false, false, true, FOUNDRY_LOCAL_FINISH_NONE, + {"host limit produces length", false, false, true, FOUNDRY_LOCAL_FINISH_NONE, FOUNDRY_LOCAL_FINISH_LENGTH}, - {"backend reason survives natural completion", false, false, false, false, FOUNDRY_LOCAL_FINISH_STOP, + {"backend reason survives natural completion", false, false, false, FOUNDRY_LOCAL_FINISH_STOP, FOUNDRY_LOCAL_FINISH_STOP}, }; for (const auto& test : cases) { SCOPED_TRACE(test.name); - EXPECT_EQ(ResolveGeneratedFinishReason(test.canceled, test.has_tool_calls, test.stop_sequence_matched, + EXPECT_EQ(ResolveGeneratedFinishReason(test.has_tool_calls, test.stop_sequence_matched, test.host_output_limit_reached, test.backend_finish_reason, /*completion_tokens=*/32, /*max_output_tokens=*/32), test.expected); @@ -697,12 +704,12 @@ TEST_F(ChatSessionTest, AppendedClassicGeneratorIsDiscardedAfterStreamingCancell session.SetStreamingCallback(callback_fn); Response response; - session.ProcessRequest(request, response); + ExpectOperationCancelled([&] { session.ProcessRequest(request, response); }); - // Should have stopped early + // The canceled response is not published. EXPECT_EQ(response.finish_reason, FOUNDRY_LOCAL_FINISH_NONE); - // we check cancellation at the start of each loop and we don't use std::atomic to make processing cheaper - // so allow for a couple of extra tokens to come through after the cancellation condition is met + EXPECT_TRUE(response.items.empty()); + // Cancellation is observed between token steps, so allow for a couple of extra tokens after the callback. EXPECT_LE(tokens_received, 6); // The appended canceled turn commits nothing; only the seed turn remains. @@ -748,9 +755,10 @@ TEST_F(ChatSessionTest, CancellationFromTheLastQueuedCallbackPreventsCommit) { }); Response response; - session.ProcessRequest(request, response); + ExpectOperationCancelled([&] { session.ProcessRequest(request, response); }); EXPECT_EQ(response.finish_reason, FOUNDRY_LOCAL_FINISH_NONE); + EXPECT_TRUE(response.items.empty()); EXPECT_EQ(session.MessageCount(), 0u); EXPECT_EQ(session.TurnCount(), 0u); } @@ -795,9 +803,10 @@ TEST_F(ChatSessionTest, OpenAIJsonCancellationFromLastContentCallbackPublishesNo }); Response response; - session.ProcessRequest(request, response); + ExpectOperationCancelled([&] { session.ProcessRequest(request, response); }); EXPECT_EQ(response.finish_reason, FOUNDRY_LOCAL_FINISH_NONE); + EXPECT_TRUE(response.items.empty()); ASSERT_EQ(delivered.size(), 2u); EXPECT_EQ(delivered[0]["choices"][0]["delta"]["role"], "assistant"); EXPECT_FALSE(delivered[0]["choices"][0]["finish_reason"].is_string()); diff --git a/sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc b/sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc index bc87bc619..589d8b66a 100644 --- a/sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc +++ b/sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc @@ -484,8 +484,14 @@ TEST_F(DynamicEngineChatTest, CancellationRebuildsCommittedHistoryWithinBudget) auto canceled = MakeRequest("Write a long essay about mathematics.", 512); Response canceled_response; - session.ProcessRequest(canceled, canceled_response); + try { + session.ProcessRequest(canceled, canceled_response); + FAIL() << "Expected operation cancellation"; + } catch (const fl::Exception& error) { + EXPECT_EQ(error.code(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + } EXPECT_EQ(canceled_response.finish_reason, FOUNDRY_LOCAL_FINISH_NONE); + EXPECT_TRUE(canceled_response.items.empty()); EXPECT_EQ(session.TurnCount(), 1u); EXPECT_GE(streamed_tokens, 3); diff --git a/sdk_v2/cpp/test/internal_api/item_test.cc b/sdk_v2/cpp/test/internal_api/item_test.cc index 02d5284ec..bb497b219 100644 --- a/sdk_v2/cpp/test/internal_api/item_test.cc +++ b/sdk_v2/cpp/test/internal_api/item_test.cc @@ -683,12 +683,28 @@ TEST(RequestTest, MixedOwnedAndBorrowedItems) { EXPECT_TRUE(req.items[1]->type == FOUNDRY_LOCAL_ITEM_MESSAGE); } -TEST(RequestTest, CancellationFlag) { +TEST(RequestTest, CancellationWinsBeforeCompletion) { Request req; - EXPECT_FALSE(req.canceled); + EXPECT_FALSE(req.IsCancellationRequested()); - req.canceled = true; - EXPECT_TRUE(req.canceled); + EXPECT_TRUE(req.Cancel()); + EXPECT_TRUE(req.Cancel()); + EXPECT_TRUE(req.IsCancellationRequested()); + EXPECT_FALSE(req.TryComplete()); +} + +TEST(RequestTest, CompletionMakesLateCancellationANoOp) { + Request req; + + ASSERT_TRUE(req.TryBegin()); + EXPECT_TRUE(req.TryComplete()); + EXPECT_FALSE(req.Cancel()); + EXPECT_FALSE(req.IsCancellationRequested()); + EXPECT_FALSE(req.IsCompleted()); + EXPECT_FALSE(req.TryBegin()); + req.PublishCompletion(); + EXPECT_TRUE(req.IsCompleted()); + EXPECT_TRUE(req.TryBegin()); } // ======================================================================== diff --git a/sdk_v2/cpp/test/internal_api/session_manager_test.cc b/sdk_v2/cpp/test/internal_api/session_manager_test.cc index a7b29eab9..7be49520c 100644 --- a/sdk_v2/cpp/test/internal_api/session_manager_test.cc +++ b/sdk_v2/cpp/test/internal_api/session_manager_test.cc @@ -399,7 +399,7 @@ TEST_F(SessionManagerTest, CheckedOutSessionNotAffectedByCheckIn) { namespace { -/// Test-only Session that blocks inside ProcessRequestImpl until its request's cancel flag is +/// Test-only Session that blocks inside ProcessRequestImpl until its request's cancellation is /// observed. Lets a unit test verify SessionManager::CancelAll() propagates cancellation to every /// registered session without loading a model. Polls the atomic exactly like the real generation /// loop, with a safety deadline so a broken Cancel() fails the test instead of hanging the suite. @@ -417,7 +417,7 @@ class BlockingCancelSession : public Session { in_flight_.store(true, std::memory_order_release); const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); - while (!request.canceled.load(std::memory_order_relaxed)) { + while (!request.IsCancellationRequested()) { if (std::chrono::steady_clock::now() >= deadline) { return; // safety net: a broken Cancel() must not hang the test suite } @@ -430,6 +430,29 @@ class BlockingCancelSession : public Session { std::atomic in_flight_{false}; }; +class CompletingSession : public Session { + public: + CompletingSession(const Model& model, ILogger& logger, ITelemetry& telemetry) + : Session(model, logger, telemetry) {} + + SessionType Type() const override { return SessionType::kChat; } + + protected: + void ProcessRequestImpl(const Request& /*request*/, Response& response) override { + response.finish_reason = FOUNDRY_LOCAL_FINISH_STOP; + } +}; + +flErrorCode ProcessAndGetCode(Session& session, const Request& request) { + try { + Response response; + session.ProcessRequest(request, response); + return FOUNDRY_LOCAL_OK; + } catch (const Exception& error) { + return error.code(); + } +} + /// Spin until `pred` is true or the timeout elapses. Returns pred's final value. template bool WaitUntil(Pred pred, std::chrono::milliseconds timeout) { @@ -463,25 +486,21 @@ TEST(SessionManagerCancelTest, CancelAllCancelsInFlightRequestsOnEverySession) { // Drive each session's blocking ProcessRequest on its own worker so both requests are in-flight // (registered in active_requests_) at the same time — exercising "every registered session". - auto f1 = std::async(std::launch::async, [&] { - Response resp; - s1.ProcessRequest(req1, resp); - }); - auto f2 = std::async(std::launch::async, [&] { - Response resp; - s2.ProcessRequest(req2, resp); - }); + auto f1 = std::async(std::launch::async, [&] { return ProcessAndGetCode(s1, req1); }); + auto f2 = std::async(std::launch::async, [&] { return ProcessAndGetCode(s2, req2); }); ASSERT_TRUE(WaitUntil([&] { return s1.InFlight() && s2.InFlight(); }, std::chrono::seconds(2))) << "worker requests never became in-flight"; mgr.CancelAll(); - // CancelAll set each request's flag; the blocked workers observe it and return promptly. + // CancelAll canceled each request; the blocked workers observe it and return promptly. EXPECT_EQ(f1.wait_for(std::chrono::seconds(2)), std::future_status::ready); EXPECT_EQ(f2.wait_for(std::chrono::seconds(2)), std::future_status::ready); - EXPECT_TRUE(req1.canceled.load(std::memory_order_relaxed)); - EXPECT_TRUE(req2.canceled.load(std::memory_order_relaxed)); + EXPECT_EQ(f1.get(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + EXPECT_EQ(f2.get(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + EXPECT_TRUE(req1.IsCancellationRequested()); + EXPECT_TRUE(req2.IsCancellationRequested()); } TEST(SessionManagerCancelTest, RequestAdmittedAfterCancelIsStampedCanceled) { @@ -505,12 +524,39 @@ TEST(SessionManagerCancelTest, RequestAdmittedAfterCancelIsStampedCanceled) { // Now drive the request. It is admitted after Cancel() ran, so ProcessRequest must stamp it on // insert and the blocking loop must observe cancellation at its first poll. - auto f = std::async(std::launch::async, [&] { - Response resp; - s.ProcessRequest(req, resp); - }); + auto f = std::async(std::launch::async, [&] { return ProcessAndGetCode(s, req); }); EXPECT_EQ(f.wait_for(std::chrono::seconds(2)), std::future_status::ready) << "late-admitted request ran uncanceled — the session_canceled_ latch did not stamp it"; - EXPECT_TRUE(req.canceled.load(std::memory_order_relaxed)); + EXPECT_EQ(f.get(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + EXPECT_TRUE(req.IsCancellationRequested()); + EXPECT_FALSE(s.InFlight()); +} + +TEST(SessionRequestLifecycleTest, PreCanceledRequestNeverReachesBackend) { + fl::test::FakeServiceBindings svc; + Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + TelemetryLogger telemetry{"test", fl::test::NullLog()}; + BlockingCancelSession session(catalog_model, fl::test::NullLog(), telemetry); + Request request; + ASSERT_TRUE(request.Cancel()); + + EXPECT_EQ(ProcessAndGetCode(session, request), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + EXPECT_FALSE(session.InFlight()); +} + +TEST(SessionRequestLifecycleTest, PublishedCompletionMakesLateCancellationNoOp) { + fl::test::FakeServiceBindings svc; + Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + TelemetryLogger telemetry{"test", fl::test::NullLog()}; + CompletingSession session(catalog_model, fl::test::NullLog(), telemetry); + Request request; + Response response; + + session.ProcessRequest(request, response); + + EXPECT_EQ(response.finish_reason, FOUNDRY_LOCAL_FINISH_STOP); + EXPECT_TRUE(request.IsCompleted()); + EXPECT_FALSE(request.Cancel()); + EXPECT_FALSE(request.IsCancellationRequested()); } diff --git a/sdk_v2/cpp/test/sdk_api/cpp_api_test.cc b/sdk_v2/cpp/test/sdk_api/cpp_api_test.cc index 8dc7e67cb..f9abe5213 100644 --- a/sdk_v2/cpp/test/sdk_api/cpp_api_test.cc +++ b/sdk_v2/cpp/test/sdk_api/cpp_api_test.cc @@ -284,9 +284,9 @@ TEST(CppApiTest, RequestSetOptions) { request.SetOptions(opts); } -TEST(CppApiTest, RequestCancelOnIdleRequest) { +TEST(CppApiTest, RequestCancelBeforeAttachmentSucceeds) { foundry_local::Request request; - // Cancel on an idle request should succeed (no-op) + // Cancellation is remembered until a session would otherwise attach the request. EXPECT_NO_THROW(request.Cancel()); } diff --git a/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc b/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc index 3594de61f..4330d950d 100644 --- a/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc +++ b/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc @@ -224,10 +224,12 @@ TEST_F(StreamingAudioFixture, CancellationMidStream) { request.Cancel(); queue.MarkFinished(); - Response response = future.get(); - - EXPECT_EQ(response.GetFinishReason(), FOUNDRY_LOCAL_FINISH_NONE) - << "Cancelled request should have NONE finish reason"; + try { + (void)future.get(); + FAIL() << "Expected operation cancellation"; + } catch (const foundry_local::Error& error) { + EXPECT_EQ(error.Code(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + } } TEST_F(StreamingAudioFixture, StreamingCallbackReceivesTokens) { diff --git a/sdk_v2/js/native/src/session.cc b/sdk_v2/js/native/src/session.cc index 5e92dbc9a..348d888a4 100644 --- a/sdk_v2/js/native/src/session.cc +++ b/sdk_v2/js/native/src/session.cc @@ -189,11 +189,10 @@ class StreamWorker : public Napi::AsyncWorker { } void Execute() override { + bool callback_installed = false; try { auto tsfn = tsfn_; - auto* ctx = ctx_; - sess_->SetStreamingCallback([tsfn, ctx](flStreamingCallbackData data) -> int { - (void)ctx; + sess_->SetStreamingCallback([tsfn](flStreamingCallbackData data) -> int { if (data.item_queue == nullptr) return 0; flItem* raw = nullptr; while (foundry_local::detail::item_api()->ItemQueue_TryPop(data.item_queue, &raw)) { @@ -214,10 +213,8 @@ class StreamWorker : public Napi::AsyncWorker { } return 0; }); + callback_installed = true; ctx_->response = std::make_shared(sess_->ProcessRequest(*req_)); - // Drop the callback so any stale shared state in the lambda is released - // before the Session is re-used for a follow-up request. - sess_->SetStreamingCallback(nullptr); } catch (const foundry_local::Error& e) { ctx_->errored = true; ctx_->err_code = static_cast(e.Code()); @@ -230,6 +227,29 @@ class StreamWorker : public Napi::AsyncWorker { ctx_->errored = true; ctx_->err_msg = "Unknown native exception"; } + + if (callback_installed) { + try { + sess_->SetStreamingCallback(nullptr); + } catch (const foundry_local::Error& e) { + if (!ctx_->errored) { + ctx_->errored = true; + ctx_->err_code = static_cast(e.Code()); + ctx_->err_msg = e.what(); + ctx_->tagged = true; + } + } catch (const std::exception& e) { + if (!ctx_->errored) { + ctx_->errored = true; + ctx_->err_msg = e.what(); + } + } catch (...) { + if (!ctx_->errored) { + ctx_->errored = true; + ctx_->err_msg = "Failed to clear native streaming callback"; + } + } + } } // Promise resolution happens in FinalizeStream — overriding OnOK/OnError diff --git a/sdk_v2/js/src/request.ts b/sdk_v2/js/src/request.ts index 801f0399c..f4e7f05d2 100644 --- a/sdk_v2/js/src/request.ts +++ b/sdk_v2/js/src/request.ts @@ -74,8 +74,8 @@ export class Request { } /** - * Cancel an in-flight request. Safe to call at any time — if the request - * is not currently being processed by a session, this is a no-op. + * Cancel a request. Safe to call at any time; cancellation before processing + * is remembered and prevents the request from reaching the backend. * Cancellation makes the matching `Session.processRequest()` reject with a * `FoundryLocalError` whose `code === FlErrorCode.OperationCancelled`. */ diff --git a/sdk_v2/js/test/items.test.ts b/sdk_v2/js/test/items.test.ts index 53e0003d0..6d89c6503 100644 --- a/sdk_v2/js/test/items.test.ts +++ b/sdk_v2/js/test/items.test.ts @@ -180,7 +180,7 @@ describeIfBuilt("Request round-trip through the native layer", () => { expect(req.itemCount).toBe(3); }); - it("cancel on an unattached request is a no-op", () => { + it("cancel before attachment is accepted", () => { const req = new Request(); expect(() => req.cancel()).not.toThrow(); }); diff --git a/sdk_v2/js/test/streaming.test.ts b/sdk_v2/js/test/streaming.test.ts index dff5127e0..7e7897f2b 100644 --- a/sdk_v2/js/test/streaming.test.ts +++ b/sdk_v2/js/test/streaming.test.ts @@ -124,11 +124,17 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real "early break cancels the stream cleanly and the session remains usable", async () => { if (session === undefined) throw new Error("fixture missing"); + const stream = session.processStreamingRequest(buildPrompt()); let count = 0; - for await (const _item of session.processStreamingRequest(buildPrompt())) { + for await (const _item of stream) { count++; if (count >= 1) break; } + await expect(stream.response).rejects.toMatchObject({ + name: "FoundryLocalError", + code: FlErrorCode.OperationCancelled, + }); + // After the break the session should accept a follow-up send. const resp = await session.processRequest( new Request() @@ -237,9 +243,7 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real expect(["stop", "length", "toolCalls", "error", "none"]).toContain(resp.finishReason); expect(resp.usage.promptTokens).toBeGreaterThan(0); expect(resp.usage.completionTokens).toBeGreaterThan(0); - expect(resp.usage.totalTokens).toBeGreaterThanOrEqual( - resp.usage.promptTokens + resp.usage.completionTokens, - ); + expect(resp.usage.totalTokens).toBeGreaterThanOrEqual(resp.usage.promptTokens + resp.usage.completionTokens); // The Response's text should match what we accumulated from the stream // (modulo possible model post-processing — assert non-empty overlap on // the boundary tokens rather than strict equality). @@ -264,46 +268,40 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real 3 * 60_000, ); - it( - "stream.response rejects with AbortError when pre-aborted", - async () => { - if (session === undefined) throw new Error("fixture missing"); - const ctrl = new AbortController(); - ctrl.abort(); - const stream = session.processStreamingRequest(buildPrompt(), { signal: ctrl.signal }); - await expect(stream.response).rejects.toMatchObject({ name: "AbortError" }); - }, - 60_000, - ); + it("stream.response rejects with AbortError when pre-aborted", async () => { + if (session === undefined) throw new Error("fixture missing"); + const ctrl = new AbortController(); + ctrl.abort(); + const stream = session.processStreamingRequest(buildPrompt(), { signal: ctrl.signal }); + await expect(stream.response).rejects.toMatchObject({ name: "AbortError" }); + }, 60_000); it( - "stream.response resolves with finishReason='none' when request.cancel() is called mid-stream", + "request.cancel rejects iteration and stream.response with OperationCancelled", async () => { if (session === undefined) throw new Error("fixture missing"); - // Native ChatSession::ProcessRequestImpl treats Request::Cancel as a - // graceful early-exit: the generation loop breaks, the generator is - // rewound, and ProcessGeneratedOutput sets finish_reason=NONE. The - // call returns a normal Response — it does NOT throw OperationCancelled - // (that exception is only raised on the pre-call path). The JS layer - // must surface that same contract: `.response` resolves with a - // FinishReason of "none". const req = new Request() .addItem(Item.systemMessage("You are verbose.")) .addItem(Item.userMessage("Write a 500-word essay about the history of bread.")) .setOptions({ search: { maxOutputTokens: 1024, temperature: 0 } }); const stream = session.processStreamingRequest(req); - let observed = 0; - for await (const _item of stream) { - observed++; - if (observed >= 1) { - req.cancel(); - break; + const iteration = async (): Promise => { + let observed = 0; + for await (const _item of stream) { + if (++observed >= 1) { + req.cancel(); + } } - } - const resp = await stream.response; - expect(resp.finishReason).toBe("none"); - // History must NOT be committed on cancel — CommitTurn is skipped - // when request.canceled is true (see ChatSession::ProcessRequestImpl). + }; + + await expect(iteration()).rejects.toMatchObject({ + name: "FoundryLocalError", + code: FlErrorCode.OperationCancelled, + }); + await expect(stream.response).rejects.toMatchObject({ + name: "FoundryLocalError", + code: FlErrorCode.OperationCancelled, + }); expect(session.turnCount).toBe(0); }, 3 * 60_000, @@ -334,9 +332,11 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real }); const req = new Request() - .addItem(Item.systemMessage( - "You are a helpful AI assistant. If necessary, you can use any provided tools to answer the question.", - )) + .addItem( + Item.systemMessage( + "You are a helpful AI assistant. If necessary, you can use any provided tools to answer the question.", + ), + ) .addItem(Item.userMessage("What is the answer to 7 multiplied by 6?")) .setOptions({ search: { temperature: 0, maxOutputTokens: 256 }, @@ -362,7 +362,8 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real expect(itemCount).toBeGreaterThan(0); expect(streamedToolCalls.length).toBeGreaterThanOrEqual(1); - const streamed = streamedToolCalls[0]!; + const streamed = streamedToolCalls[0]; + if (streamed === undefined) throw new Error("expected a streamed tool call"); expect(streamed.name).toBe("multiply_numbers"); expect(streamed.arguments.length).toBeGreaterThan(0); expect(streamed.callId.length).toBeGreaterThan(0); @@ -373,13 +374,11 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real const resp = await stream.response; expect(resp.finishReason).toBe("toolCalls"); - const finalToolCall = resp.output.find((it): it is Extract => - it.type === "toolCall", - ); - expect(finalToolCall).toBeDefined(); - expect(finalToolCall!.name).toBe(streamed.name); - expect(finalToolCall!.arguments).toBe(streamed.arguments); - expect(finalToolCall!.callId).toBe(streamed.callId); + const finalToolCall = resp.output.find((it): it is Extract => it.type === "toolCall"); + if (finalToolCall === undefined) throw new Error("expected a final tool call"); + expect(finalToolCall.name).toBe(streamed.name); + expect(finalToolCall.arguments).toBe(streamed.arguments); + expect(finalToolCall.callId).toBe(streamed.callId); }, 3 * 60_000, ); From 0f341d7fa1ad5d417747546c119dcbcfc349e10c Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 15 Sep 2026 17:36:55 -0500 Subject: [PATCH 02/16] Keep backend cancellation separate from API errors 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 --- .../generative/chat/onnx_engine_chat_stream.cc | 6 ++---- .../chat/dynamic_engine_chat_test.cc | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc b/sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc index 251659ab2..585d493b8 100644 --- a/sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc +++ b/sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc @@ -25,6 +25,8 @@ std::optional MapFinishReason(OgaFinishReason reason) { return FOUNDRY_LOCAL_FINISH_LENGTH; case OgaFinishReason_Failed: return FOUNDRY_LOCAL_FINISH_ERROR; + case OgaFinishReason_Cancelled: + return std::nullopt; default: return std::nullopt; } @@ -188,10 +190,6 @@ void OnnxEngineChatStream::ResetTurnDecoder() { std::optional OnnxEngineChatStream::GetTurnUsage() const { const auto result = engine_.GetTurnResult(conversation_); - if (result.finish_reason == OgaFinishReason_Cancelled) { - FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled"); - } - return ChatTurnUsage{ prompt_token_count_, static_cast(result.generated_tokens), diff --git a/sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc b/sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc index 589d8b66a..a8a340c19 100644 --- a/sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc +++ b/sdk_v2/cpp/test/internal_api/chat/dynamic_engine_chat_test.cc @@ -467,6 +467,23 @@ TEST_F(DynamicEngineChatTest, LateCancelAfterCompletedConversationRemovalIsNoOp) EXPECT_EQ(result_after_cancel.finish_reason, result.finish_reason); } +TEST_F(DynamicEngineChatTest, BackendCancellationIsNotAnApiErrorByItself) { + SearchOptions options; + options.max_output_tokens = 512; + options.temperature = 0.0f; + ToolCallContext tool_context; + std::vector messages = { + {FOUNDRY_LOCAL_ROLE_USER, "Write a long essay about mathematics."}}; + auto stream = OnnxEngineChatStream::Create(messages, options, ModelInstance(), tool_context); + + stream->GenerateNextToken(); + stream->Cancel(); + + const auto usage = stream->GetTurnUsage(); + ASSERT_TRUE(usage.has_value()); + EXPECT_FALSE(usage->finish_reason.has_value()); +} + TEST_F(DynamicEngineChatTest, CancellationRebuildsCommittedHistoryWithinBudget) { ChatSession session(CatalogModel(), ModelInstance(), *logger_, telemetry_); From 3f0098698d3398c3acfd75ddcaefa5d8d6679f25 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 17 Sep 2026 17:54:19 -0500 Subject: [PATCH 03/16] Preserve cancellation causes across SDK boundaries 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 --- .../include/foundry_local/foundry_local_c.h | 2 +- .../include/foundry_local/foundry_local_cpp.h | 3 +- .../inferencing/session/callback_handler.h | 23 ++++- sdk_v2/cpp/src/inferencing/session/request.h | 86 ++++++++++++++++- sdk_v2/cpp/src/inferencing/session/session.cc | 33 ++++++- .../inferencing/session/session_manager.cc | 2 +- .../internal_api/callback_handler_test.cc | 93 +++++++++++++++++++ sdk_v2/cpp/test/internal_api/item_test.cc | 16 ++++ .../test/internal_api/session_manager_test.cc | 19 ++++ sdk_v2/cs/src/Request.cs | 4 + sdk_v2/js/native/src/session.cc | 81 +++++++++------- sdk_v2/js/native/src/session.h | 16 +++- sdk_v2/js/src/request.ts | 5 +- sdk_v2/js/src/session.ts | 17 ++-- sdk_v2/js/test/streaming.test.ts | 84 ++++++++++++++--- .../python/src/foundry_local_sdk/request.py | 7 +- 16 files changed, 405 insertions(+), 86 deletions(-) diff --git a/sdk_v2/cpp/include/foundry_local/foundry_local_c.h b/sdk_v2/cpp/include/foundry_local/foundry_local_c.h index 4bce94459..e9a8476e5 100644 --- a/sdk_v2/cpp/include/foundry_local/foundry_local_c.h +++ b/sdk_v2/cpp/include/foundry_local/foundry_local_c.h @@ -886,7 +886,7 @@ struct flInferenceApi { /// Values are string representations; the implementation parses them for the appropriate type. /// The request copies the data — the caller may release the pairs after this call. FL_API_STATUS(Request_SetOptions, _In_ flRequest* request, _In_ const flKeyValuePairs* options); - /// Cancel an in-progress request. + /// Cancel a request. Pre-processing cancellation is remembered; cancellation after completion is a no-op. FL_API_STATUS(Request_Cancel, _In_ flRequest* request); /* Response */ diff --git a/sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h b/sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h index 3a0e0c16c..22ae78065 100644 --- a/sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h +++ b/sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h @@ -1028,7 +1028,8 @@ class Request { /// Options for this request. Overrides session options for the duration of this request. Request& SetOptions(const RequestOptions& options); - /// Cancel the current request. Inferencing will stop as soon as possible. + /// Cancel this request. Pre-processing cancellation is remembered; in-flight inference stops cooperatively and + /// cancellation after completion is a no-op. void Cancel(); const flRequest* native_handle() const noexcept { return handle_.get(); } diff --git a/sdk_v2/cpp/src/inferencing/session/callback_handler.h b/sdk_v2/cpp/src/inferencing/session/callback_handler.h index f78e1baba..552c27ea0 100644 --- a/sdk_v2/cpp/src/inferencing/session/callback_handler.h +++ b/sdk_v2/cpp/src/inferencing/session/callback_handler.h @@ -28,10 +28,14 @@ namespace fl { /// user callback. This decouples token generation from callback speed /// while guaranteeing delivery order. /// +/// On cancellation, a backlog within the normal 64-item backpressure window is delivered. A larger backlog is +/// discarded so cancellation cannot be delayed indefinitely by a stalled consumer. +/// /// The Request reference is bound at construction — no need to pass it per push. /// Destruction drains the queue and joins the worker thread (RAII). struct CallbackHandler { using CallbackFn = std::function; + static constexpr size_t kMaxCancellationDrainItems = 64; CallbackHandler(const Request& request, CallbackFn callback_fn, ILogger& logger, void* user_data = nullptr) @@ -90,23 +94,28 @@ struct CallbackHandler { // Fire the callback for each available item. // The callback pops from the queue — that is the established contract. while (queue_->Size() > 0) { + if (request_.IsCancellationRequested() && queue_->Size() > kMaxCancellationDrainItems) { + DropPendingItems(); + break; + } + SetCallbackInProgress(true); try { if (fn_(data_, user_data_) != 0) { - request_.Cancel(); + request_.Cancel(Request::CancellationReason::StreamingCallback); } } catch (const std::exception& e) { logger_.Log(LogLevel::Warning, fmt::format("streaming callback threw an exception; cancelling request: {}", e.what())); - DisableAfterException(); + DisableAfterException(e.what()); SetCallbackInProgress(false); return; } catch (...) { logger_.Log(LogLevel::Warning, "streaming callback threw a non-std exception; cancelling request"); - DisableAfterException(); + DisableAfterException("non-standard exception"); SetCallbackInProgress(false); return; } @@ -124,8 +133,12 @@ struct CallbackHandler { /// Called from the worker thread after the user callback throws. Marks the request /// cancelled (so PushItem becomes a no-op and the generator loop stops feeding work) /// and drops any items still queued so the destructor can join cleanly. - void DisableAfterException() { - request_.Cancel(); + void DisableAfterException(std::string_view detail) { + request_.CancelFromStreamingCallbackException(detail); + DropPendingItems(); + } + + void DropPendingItems() { while (queue_->TryPop()) { } } diff --git a/sdk_v2/cpp/src/inferencing/session/request.h b/sdk_v2/cpp/src/inferencing/session/request.h index d822c93a5..fed2910e6 100644 --- a/sdk_v2/cpp/src/inferencing/session/request.h +++ b/sdk_v2/cpp/src/inferencing/session/request.h @@ -9,6 +9,9 @@ #include #include #include +#include +#include +#include #include namespace fl { @@ -24,10 +27,21 @@ inline constexpr const char* kSystemPromptOption = "system_prompt"; /// Generic inference request — pure input data. /// Items are stored as borrowed pointers. Owned items are kept alive in owned_items. struct Request { + enum class CancellationReason : uint8_t { + None, + Caller, + StreamingCallback, + StreamingCallbackException, + SessionShutdown, + }; + enum class State : uint8_t { Ready, Running, - Canceled, + CanceledByCaller, + CanceledByStreamingCallback, + CanceledByStreamingCallbackException, + CanceledBySessionShutdown, Completing, Completed, }; @@ -55,6 +69,7 @@ struct Request { options(std::move(other.options)), item_segment_starts(std::move(other.item_segment_starts)), state_(other.state_.load(std::memory_order_relaxed)), + cancellation_detail_(std::move(other.cancellation_detail_)), owned_items(std::move(other.owned_items)) {} Request& operator=(Request&& other) noexcept { @@ -62,6 +77,7 @@ struct Request { options = std::move(other.options); item_segment_starts = std::move(other.item_segment_starts); state_.store(other.state_.load(std::memory_order_relaxed), std::memory_order_relaxed); + cancellation_detail_ = std::move(other.cancellation_detail_); owned_items = std::move(other.owned_items); return *this; } @@ -87,21 +103,42 @@ struct Request { } /// Atomically wins cancellation against terminal publication. Returns false after completion has won. - bool Cancel() const noexcept { + bool Cancel(CancellationReason reason = CancellationReason::Caller) const noexcept { + const auto canceled_state = CanceledState(reason); auto state = state_.load(std::memory_order_acquire); while (state == State::Ready || state == State::Running) { - if (state_.compare_exchange_weak(state, State::Canceled, + if (state_.compare_exchange_weak(state, canceled_state, std::memory_order_acq_rel, std::memory_order_acquire)) { return true; } } - return state == State::Canceled; + return IsCanceledState(state); + } + + bool CancelFromStreamingCallbackException(std::string_view detail) const noexcept { + try { + std::lock_guard lock(cancellation_detail_mutex_); + cancellation_detail_ = detail; + } catch (...) { + // Cancellation itself must remain reliable if preserving diagnostic text runs out of memory. + } + + return Cancel(CancellationReason::StreamingCallbackException); } bool IsCancellationRequested() const noexcept { - return state_.load(std::memory_order_acquire) == State::Canceled; + return IsCanceledState(state_.load(std::memory_order_acquire)); + } + + CancellationReason GetCancellationReason() const noexcept { + return ReasonFromState(state_.load(std::memory_order_acquire)); + } + + std::string CancellationDetail() const { + std::lock_guard lock(cancellation_detail_mutex_); + return cancellation_detail_; } /// Starts first-time processing or reuses a request whose previous operation completed. @@ -140,7 +177,46 @@ struct Request { } private: + static State CanceledState(CancellationReason reason) noexcept { + switch (reason) { + case CancellationReason::StreamingCallback: + return State::CanceledByStreamingCallback; + case CancellationReason::StreamingCallbackException: + return State::CanceledByStreamingCallbackException; + case CancellationReason::SessionShutdown: + return State::CanceledBySessionShutdown; + case CancellationReason::None: + case CancellationReason::Caller: + default: + return State::CanceledByCaller; + } + } + + static bool IsCanceledState(State state) noexcept { + return state == State::CanceledByCaller || + state == State::CanceledByStreamingCallback || + state == State::CanceledByStreamingCallbackException || + state == State::CanceledBySessionShutdown; + } + + static CancellationReason ReasonFromState(State state) noexcept { + switch (state) { + case State::CanceledByCaller: + return CancellationReason::Caller; + case State::CanceledByStreamingCallback: + return CancellationReason::StreamingCallback; + case State::CanceledByStreamingCallbackException: + return CancellationReason::StreamingCallbackException; + case State::CanceledBySessionShutdown: + return CancellationReason::SessionShutdown; + default: + return CancellationReason::None; + } + } + mutable std::atomic state_{State::Ready}; + mutable std::mutex cancellation_detail_mutex_; + mutable std::string cancellation_detail_; std::vector> owned_items; // owned items (lifetime) }; diff --git a/sdk_v2/cpp/src/inferencing/session/session.cc b/sdk_v2/cpp/src/inferencing/session/session.cc index 2b4ebea4d..d4fb89b86 100644 --- a/sdk_v2/cpp/src/inferencing/session/session.cc +++ b/sdk_v2/cpp/src/inferencing/session/session.cc @@ -22,6 +22,29 @@ namespace fl { +namespace { + +[[noreturn]] void ThrowCancellation(const Request& request) { + switch (request.GetCancellationReason()) { + case Request::CancellationReason::StreamingCallback: + FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled by streaming callback"); + case Request::CancellationReason::StreamingCallbackException: { + const auto detail = request.CancellationDetail(); + FL_THROW(FOUNDRY_LOCAL_ERROR_INTERNAL, + detail.empty() ? "streaming callback threw an exception" + : fmt::format("streaming callback threw an exception: {}", detail)); + } + case Request::CancellationReason::SessionShutdown: + FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled because the session is shutting down"); + case Request::CancellationReason::None: + case Request::CancellationReason::Caller: + default: + FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled by caller"); + } +} + +} // namespace + Session::Session(const fl::Model& catalog_model, ILogger& logger, ITelemetry& telemetry, bool allow_concurrent_requests) : catalog_model_(catalog_model), @@ -150,7 +173,7 @@ void Session::ProcessRequest(const Request& request, Response& response) { // If Cancel() already ran (shutdown began before this request was admitted), stamp it now so the // generation loop exits at its first poll instead of running an uncanceled turn. if (session_canceled_) { - request.Cancel(); + request.Cancel(Request::CancellationReason::SessionShutdown); } if (!request.TryBegin() && !request.IsCancellationRequested()) { @@ -177,7 +200,7 @@ void Session::ProcessRequest(const Request& request, Response& response) { Response staged_response; try { if (request.IsCancellationRequested()) { - FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled"); + ThrowCancellation(request); } ValidateRequestItems(request); @@ -185,7 +208,7 @@ void Session::ProcessRequest(const Request& request, Response& response) { ProcessRequestImpl(request, staged_response); if (!request.TryComplete()) { - FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled"); + ThrowCancellation(request); } response = std::move(staged_response); @@ -199,7 +222,7 @@ void Session::ProcessRequest(const Request& request, Response& response) { } try { - FL_THROW(FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED, "request cancelled"); + ThrowCancellation(request); } catch (const std::exception& cancellation) { tracker.RecordException(cancellation); throw; @@ -213,7 +236,7 @@ void Session::Cancel() { std::lock_guard lock(*active_requests_mutex_); session_canceled_ = true; for (const Request* r : active_requests_) { - r->Cancel(); + r->Cancel(Request::CancellationReason::SessionShutdown); } } diff --git a/sdk_v2/cpp/src/inferencing/session/session_manager.cc b/sdk_v2/cpp/src/inferencing/session/session_manager.cc index 3e35eb45e..4d3e58ee9 100644 --- a/sdk_v2/cpp/src/inferencing/session/session_manager.cc +++ b/sdk_v2/cpp/src/inferencing/session/session_manager.cc @@ -68,7 +68,7 @@ void SessionManager::CancelAll() { logger_.Log(LogLevel::Information, fmt::format("SessionManager: cancelling all sessions ({} active)", sessions_.size())); - // Signal every in-flight request to stop. Cancel() only sets atomic flags — no joins, no + // Signal every in-flight request to stop. Cancel() only updates atomic lifecycle state — no joins, no // re-entrancy into SessionManager — so calling it while holding mutex_ cannot deadlock. for (Session* s : sessions_) { s->Cancel(); diff --git a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc index be3d0609f..0ff8f050e 100644 --- a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc +++ b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc @@ -9,6 +9,8 @@ #include #include +#include +#include #include #include @@ -43,6 +45,8 @@ TEST(CallbackHandlerTest, StdExceptionFromCallbackDoesNotTerminate) { EXPECT_GE(invocations.load(), 1); EXPECT_TRUE(request.IsCancellationRequested()); + EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::StreamingCallbackException); + EXPECT_EQ(request.CancellationDetail(), "boom"); } TEST(CallbackHandlerTest, NonStdExceptionFromCallbackDoesNotTerminate) { @@ -62,6 +66,8 @@ TEST(CallbackHandlerTest, NonStdExceptionFromCallbackDoesNotTerminate) { EXPECT_GE(invocations.load(), 1); EXPECT_TRUE(request.IsCancellationRequested()); + EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::StreamingCallbackException); + EXPECT_EQ(request.CancellationDetail(), "non-standard exception"); } TEST(CallbackHandlerTest, FurtherPushesAfterExceptionAreNoOps) { @@ -110,6 +116,7 @@ TEST(CallbackHandlerTest, NormalCallbackCancelsViaReturnValue) { EXPECT_EQ(invocations.load(), 1); EXPECT_TRUE(request.IsCancellationRequested()); + EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::StreamingCallback); } TEST(CallbackHandlerTest, DrainPendingWaitsForDeliveryWithoutClosingTheQueue) { @@ -132,3 +139,89 @@ TEST(CallbackHandlerTest, DrainPendingWaitsForDeliveryWithoutClosingTheQueue) { handler.Drain(); EXPECT_EQ(invocations.load(), 2); } + +TEST(CallbackHandlerTest, CancellationDrainsSmallBufferedBacklog) { + Request request; + std::atomic invocations{0}; + std::mutex mutex; + std::condition_variable cv; + bool callback_started = false; + bool release_callback = false; + + auto fn = [&](flStreamingCallbackData data, void*) -> int { + auto* queue = reinterpret_cast(data.item_queue); + (void)queue->TryPop(); + ++invocations; + + std::unique_lock lock(mutex); + callback_started = true; + cv.notify_all(); + cv.wait(lock, [&] { return release_callback; }); + return 0; + }; + + CallbackHandler handler(request, fn, fl::test::NullLog()); + handler.PushItem(std::make_unique("first")); + bool started = false; + { + std::unique_lock lock(mutex); + started = cv.wait_for(lock, std::chrono::seconds(2), [&] { return callback_started; }); + } + EXPECT_TRUE(started); + + for (size_t i = 0; i < 3; ++i) { + handler.PushItem(std::make_unique("buffered")); + } + request.Cancel(); + { + std::lock_guard lock(mutex); + release_callback = true; + } + cv.notify_all(); + + handler.Drain(); + EXPECT_EQ(invocations.load(), 4); +} + +TEST(CallbackHandlerTest, CancellationDropsLargeBufferedBacklog) { + Request request; + std::atomic invocations{0}; + std::mutex mutex; + std::condition_variable cv; + bool callback_started = false; + bool release_callback = false; + + auto fn = [&](flStreamingCallbackData data, void*) -> int { + auto* queue = reinterpret_cast(data.item_queue); + (void)queue->TryPop(); + ++invocations; + + std::unique_lock lock(mutex); + callback_started = true; + cv.notify_all(); + cv.wait(lock, [&] { return release_callback; }); + return 0; + }; + + CallbackHandler handler(request, fn, fl::test::NullLog()); + handler.PushItem(std::make_unique("first")); + bool started = false; + { + std::unique_lock lock(mutex); + started = cv.wait_for(lock, std::chrono::seconds(2), [&] { return callback_started; }); + } + EXPECT_TRUE(started); + + for (size_t i = 0; i <= CallbackHandler::kMaxCancellationDrainItems; ++i) { + handler.PushItem(std::make_unique("buffered")); + } + request.Cancel(); + { + std::lock_guard lock(mutex); + release_callback = true; + } + cv.notify_all(); + + handler.Drain(); + EXPECT_EQ(invocations.load(), 1); +} diff --git a/sdk_v2/cpp/test/internal_api/item_test.cc b/sdk_v2/cpp/test/internal_api/item_test.cc index bb497b219..44681859c 100644 --- a/sdk_v2/cpp/test/internal_api/item_test.cc +++ b/sdk_v2/cpp/test/internal_api/item_test.cc @@ -690,9 +690,25 @@ TEST(RequestTest, CancellationWinsBeforeCompletion) { EXPECT_TRUE(req.Cancel()); EXPECT_TRUE(req.Cancel()); EXPECT_TRUE(req.IsCancellationRequested()); + EXPECT_EQ(req.GetCancellationReason(), Request::CancellationReason::Caller); EXPECT_FALSE(req.TryComplete()); } +TEST(RequestTest, CancellationPreservesActionableReason) { + Request callback_request; + EXPECT_TRUE(callback_request.Cancel(Request::CancellationReason::StreamingCallback)); + EXPECT_EQ(callback_request.GetCancellationReason(), Request::CancellationReason::StreamingCallback); + + Request exception_request; + EXPECT_TRUE(exception_request.CancelFromStreamingCallbackException("callback exploded")); + EXPECT_EQ(exception_request.GetCancellationReason(), Request::CancellationReason::StreamingCallbackException); + EXPECT_EQ(exception_request.CancellationDetail(), "callback exploded"); + + Request shutdown_request; + EXPECT_TRUE(shutdown_request.Cancel(Request::CancellationReason::SessionShutdown)); + EXPECT_EQ(shutdown_request.GetCancellationReason(), Request::CancellationReason::SessionShutdown); +} + TEST(RequestTest, CompletionMakesLateCancellationANoOp) { Request req; diff --git a/sdk_v2/cpp/test/internal_api/session_manager_test.cc b/sdk_v2/cpp/test/internal_api/session_manager_test.cc index 7be49520c..2b052cbec 100644 --- a/sdk_v2/cpp/test/internal_api/session_manager_test.cc +++ b/sdk_v2/cpp/test/internal_api/session_manager_test.cc @@ -560,3 +560,22 @@ TEST(SessionRequestLifecycleTest, PublishedCompletionMakesLateCancellationNoOp) EXPECT_FALSE(request.Cancel()); EXPECT_FALSE(request.IsCancellationRequested()); } + +TEST(SessionRequestLifecycleTest, CallbackExceptionSurfacesOriginalCause) { + fl::test::FakeServiceBindings svc; + Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + TelemetryLogger telemetry{"test", fl::test::NullLog()}; + CompletingSession session(catalog_model, fl::test::NullLog(), telemetry); + Request request; + ASSERT_TRUE(request.CancelFromStreamingCallbackException("callback exploded")); + + try { + Response response; + session.ProcessRequest(request, response); + FAIL() << "expected callback failure"; + } catch (const Exception& error) { + EXPECT_EQ(error.code(), FOUNDRY_LOCAL_ERROR_INTERNAL); + EXPECT_NE(std::string(error.what()).find("streaming callback threw an exception: callback exploded"), + std::string::npos); + } +} diff --git a/sdk_v2/cs/src/Request.cs b/sdk_v2/cs/src/Request.cs index 7761f0e61..7baaf1aaf 100644 --- a/sdk_v2/cs/src/Request.cs +++ b/sdk_v2/cs/src/Request.cs @@ -92,6 +92,10 @@ internal Request SetOptions(IntPtr options) return this; } + /// + /// Cancels this request. Cancellation before processing is remembered and prevents the request from reaching the + /// inference backend; cancellation after completion has no effect. + /// public void Cancel() { Api.CheckStatus(Api.Inference.RequestCancel(Ptr)); diff --git a/sdk_v2/js/native/src/session.cc b/sdk_v2/js/native/src/session.cc index 348d888a4..682970693 100644 --- a/sdk_v2/js/native/src/session.cc +++ b/sdk_v2/js/native/src/session.cc @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -97,8 +98,8 @@ foundry_local::Request* UnwrapRequest(Napi::Env env, const Napi::Value& v) { // Pins both the Manager (so the Model handle the Session holds stays alive) // and the Request (so the C++ Request the worker reads stays alive). template -Napi::Value ProcessRequestOn(Napi::Env env, SessT* sess, const Napi::Value& request_arg, - Napi::ObjectReference manager_ref) { +Napi::Value ProcessRequestOn(Napi::Env env, std::shared_ptr sess, const Napi::Value& request_arg, + Napi::ObjectReference manager_ref, std::shared_ptr request_gate) { foundry_local::Request* req = UnwrapRequest(env, request_arg); if (req == nullptr) return env.Undefined(); // pending exception Napi::ObjectReference req_pin = Napi::Reference::New(request_arg.As(), 1); @@ -112,8 +113,9 @@ Napi::Value ProcessRequestOn(Napi::Env env, SessT* sess, const Napi::Value& requ return PromiseWorker::Run( env, - [sess, req, pins]() -> Result { + [sess = std::move(sess), req, pins, request_gate = std::move(request_gate)]() -> Result { (void)pins; // keepalive captured by reference count + std::lock_guard lock(*request_gate); return std::make_shared(sess->ProcessRequest(*req)); }, [](Napi::Env env, Result& resp) -> Napi::Value { return ResponseToJs(env, *resp); }); @@ -180,37 +182,40 @@ void FinalizeStream(Napi::Env env, void* /*data*/, StreamCtx* ctx) { template class StreamWorker : public Napi::AsyncWorker { public: - static Napi::Promise Run(Napi::Env env, SessT* sess, foundry_local::Request* req, - Napi::Function jsCallback, StreamCtx* ctx) { - auto* w = new StreamWorker(env, sess, req, jsCallback, ctx); + static Napi::Promise Run(Napi::Env env, std::shared_ptr sess, foundry_local::Request* req, + Napi::Function jsCallback, StreamCtx* ctx, + std::shared_ptr request_gate) { + auto* w = new StreamWorker(env, std::move(sess), req, jsCallback, ctx, std::move(request_gate)); Napi::Promise p = ctx->deferred.Promise(); w->Queue(); return p; } void Execute() override { + std::lock_guard request_lock(*request_gate_); bool callback_installed = false; try { auto tsfn = tsfn_; sess_->SetStreamingCallback([tsfn](flStreamingCallbackData data) -> int { if (data.item_queue == nullptr) return 0; flItem* raw = nullptr; - while (foundry_local::detail::item_api()->ItemQueue_TryPop(data.item_queue, &raw)) { - if (raw == nullptr) break; - auto* item = new foundry_local::Item(*raw); - napi_status status = tsfn.BlockingCall( - item, [](Napi::Env env, Napi::Function jsCb, foundry_local::Item* it) { - Napi::HandleScope scope(env); - Napi::Value js_item = ItemToJs(env, *it); - delete it; - jsCb.Call({js_item}); - }); - if (status != napi_ok) { - delete item; - return 1; - } - raw = nullptr; + if (!foundry_local::detail::item_api()->ItemQueue_TryPop(data.item_queue, &raw) || raw == nullptr) { + return 0; } + + auto* item = new foundry_local::Item(*raw); + napi_status status = tsfn.BlockingCall( + item, [](Napi::Env env, Napi::Function jsCb, foundry_local::Item* it) { + Napi::HandleScope scope(env); + Napi::Value js_item = ItemToJs(env, *it); + delete it; + jsCb.Call({js_item}); + }); + if (status != napi_ok) { + delete item; + return 1; + } + return 0; }); callback_installed = true; @@ -259,26 +264,29 @@ class StreamWorker : public Napi::AsyncWorker { void OnError(const Napi::Error& /*unused*/) override { tsfn_.Release(); } private: - StreamWorker(Napi::Env env, SessT* sess, foundry_local::Request* req, - Napi::Function jsCallback, StreamCtx* ctx) + StreamWorker(Napi::Env env, std::shared_ptr sess, foundry_local::Request* req, + Napi::Function jsCallback, StreamCtx* ctx, std::shared_ptr request_gate) : Napi::AsyncWorker(env), - sess_(sess), + sess_(std::move(sess)), req_(req), ctx_(ctx), + request_gate_(std::move(request_gate)), tsfn_(Napi::ThreadSafeFunction::New(env, jsCallback, "foundry_local_stream", /*max_queue=*/64, /*threads=*/1, ctx, FinalizeStream, static_cast(nullptr))) {} - SessT* sess_; + std::shared_ptr sess_; foundry_local::Request* req_; StreamCtx* ctx_; + std::shared_ptr request_gate_; Napi::ThreadSafeFunction tsfn_; }; template -Napi::Value ProcessStreamingRequestOn(Napi::Env env, SessT* sess, const Napi::CallbackInfo& info, - Napi::ObjectReference manager_ref) { +Napi::Value ProcessStreamingRequestOn(Napi::Env env, std::shared_ptr sess, const Napi::CallbackInfo& info, + Napi::ObjectReference manager_ref, + std::shared_ptr request_gate) { if (info.Length() < 2 || !info[1].IsFunction()) { Napi::TypeError::New(env, "processStreamingRequest(request: Request, onItem: (item) => void)") .ThrowAsJavaScriptException(); @@ -297,7 +305,8 @@ Napi::Value ProcessStreamingRequestOn(Napi::Env env, SessT* sess, const Napi::Ca 0, false, false}; - return StreamWorker::Run(env, sess, req, info[1].As(), ctx); + return StreamWorker::Run(env, std::move(sess), req, info[1].As(), ctx, + std::move(request_gate)); } } // namespace @@ -342,7 +351,7 @@ ChatSession::ChatSession(const Napi::CallbackInfo& info) : Napi::ObjectWrap(*native); + impl_ = std::make_shared(*native); } catch (const foundry_local::Error& e) { ThrowFoundryLocalError(env, static_cast(e.Code()), e.what()); return; @@ -370,14 +379,14 @@ Napi::Value ChatSession::ProcessRequest(const Napi::CallbackInfo& info) { return env.Undefined(); } Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessRequestOn(env, impl_.get(), info[0], std::move(owner)); + return ProcessRequestOn(env, impl_, info[0], std::move(owner), request_gate_); } Napi::Value ChatSession::ProcessStreamingRequest(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); if (ThrowIfDisposed(env)) return env.Undefined(); Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessStreamingRequestOn(env, impl_.get(), info, std::move(owner)); + return ProcessStreamingRequestOn(env, impl_, info, std::move(owner), request_gate_); } Napi::Value ChatSession::SetOptions(const Napi::CallbackInfo& info) { @@ -524,7 +533,7 @@ EmbeddingsSession::EmbeddingsSession(const Napi::CallbackInfo& info) return; } try { - impl_ = std::make_unique(*native); + impl_ = std::make_shared(*native); } catch (const foundry_local::Error& e) { ThrowFoundryLocalError(env, static_cast(e.Code()), e.what()); return; @@ -552,7 +561,7 @@ Napi::Value EmbeddingsSession::ProcessRequest(const Napi::CallbackInfo& info) { return env.Undefined(); } Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessRequestOn(env, impl_.get(), info[0], std::move(owner)); + return ProcessRequestOn(env, impl_, info[0], std::move(owner), request_gate_); } Napi::Value EmbeddingsSession::SetOptions(const Napi::CallbackInfo& info) { @@ -618,7 +627,7 @@ AudioSession::AudioSession(const Napi::CallbackInfo& info) return; } try { - impl_ = std::make_unique(*native); + impl_ = std::make_shared(*native); } catch (const foundry_local::Error& e) { ThrowFoundryLocalError(env, static_cast(e.Code()), e.what()); return; @@ -646,14 +655,14 @@ Napi::Value AudioSession::ProcessRequest(const Napi::CallbackInfo& info) { return env.Undefined(); } Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessRequestOn(env, impl_.get(), info[0], std::move(owner)); + return ProcessRequestOn(env, impl_, info[0], std::move(owner), request_gate_); } Napi::Value AudioSession::ProcessStreamingRequest(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); if (ThrowIfDisposed(env)) return env.Undefined(); Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessStreamingRequestOn(env, impl_.get(), info, std::move(owner)); + return ProcessStreamingRequestOn(env, impl_, info, std::move(owner), request_gate_); } Napi::Value AudioSession::SetOptions(const Napi::CallbackInfo& info) { diff --git a/sdk_v2/js/native/src/session.h b/sdk_v2/js/native/src/session.h index 2b1db7b86..379f804fc 100644 --- a/sdk_v2/js/native/src/session.h +++ b/sdk_v2/js/native/src/session.h @@ -8,7 +8,7 @@ // flSession_Create is fast. // * session.processRequest(request) -> Promise (PromiseWorker) // * session.processStreamingRequest(request, onItem) -> Promise — streaming bridge via -// Napi::ThreadSafeFunction; resolves with the terminal Response after every item callback drains. +// Napi::ThreadSafeFunction; resolves with the terminal Response after every queued JS callback runs. // The JS layer wraps this in an AsyncIterable whose `.response` promise carries the resolved value. // * session.setOptions(kvp) — session-level options applied to subsequent sends. // * ChatSession adds: turnCount(), undoTurns(count), addToolDefinition({...}). @@ -21,7 +21,9 @@ // // Lifetime: the ChatSession pins the parent Manager via an ObjectReference so // the underlying foundry_local::Model the C++ Session captured can't be -// released out from under it. +// released out from under it. Session implementations are shared with queued +// workers so dispose() can detach the wrapper without blocking the JS thread +// or invalidating work that was already accepted. #pragma once #include @@ -29,6 +31,7 @@ #include #include +#include namespace foundry_local_node { @@ -51,8 +54,9 @@ class ChatSession : public Napi::ObjectWrap { bool ThrowIfDisposed(Napi::Env env); - std::unique_ptr impl_; + std::shared_ptr impl_; Napi::ObjectReference manager_; + std::shared_ptr request_gate_ = std::make_shared(); }; // Napi::ObjectWrap over foundry_local::EmbeddingsSession. @@ -82,8 +86,9 @@ class EmbeddingsSession : public Napi::ObjectWrap { bool ThrowIfDisposed(Napi::Env env); - std::unique_ptr impl_; + std::shared_ptr impl_; Napi::ObjectReference manager_; + std::shared_ptr request_gate_ = std::make_shared(); }; // Napi::ObjectWrap over foundry_local::AudioSession. @@ -111,8 +116,9 @@ class AudioSession : public Napi::ObjectWrap { bool ThrowIfDisposed(Napi::Env env); - std::unique_ptr impl_; + std::shared_ptr impl_; Napi::ObjectReference manager_; + std::shared_ptr request_gate_ = std::make_shared(); }; } // namespace foundry_local_node diff --git a/sdk_v2/js/src/request.ts b/sdk_v2/js/src/request.ts index f4e7f05d2..fe5e6d364 100644 --- a/sdk_v2/js/src/request.ts +++ b/sdk_v2/js/src/request.ts @@ -75,9 +75,10 @@ export class Request { /** * Cancel a request. Safe to call at any time; cancellation before processing - * is remembered and prevents the request from reaching the backend. + * is remembered, while cancellation after completion has no effect. * Cancellation makes the matching `Session.processRequest()` reject with a - * `FoundryLocalError` whose `code === FlErrorCode.OperationCancelled`. + * `FoundryLocalError` whose `code === FlErrorCode.OperationCancelled` and + * whose message identifies caller-requested cancellation. */ cancel(): void { this.#native.cancel(); diff --git a/sdk_v2/js/src/session.ts b/sdk_v2/js/src/session.ts index 932c6638d..67d247817 100644 --- a/sdk_v2/js/src/session.ts +++ b/sdk_v2/js/src/session.ts @@ -45,10 +45,9 @@ export interface StreamOptions { * once the native call completes — carrying stop reason, usage, and any * non-streamed items (e.g. the final aggregated text item). * - * `response` settles after the iterator finishes draining. It rejects with - * the same error the iterator would throw (including `AbortError` when the - * stream is cancelled, and `OperationCancelled` when the consumer breaks - * early without an `AbortSignal`). + * `response` settles after the native call completes and all queued item callbacks have run. Breaking iteration early + * requests cancellation, but native completion can win that race; in that case `response` resolves normally. Otherwise + * it rejects with the cancellation error (`AbortError` for an aborted signal or `OperationCancelled` for an early break). */ export interface StreamingResponse extends AsyncIterable { readonly response: Promise; @@ -111,12 +110,12 @@ function modelToNativeAudioSession(model: IModel): NativeAudioSession { * Drive a native streaming session and yield each item to the consumer. * Handles backpressure (the JS-side queue grows; the native TSFN backpressure * caps producer-side queueing), abort signal wiring, error mapping, and - * deterministic cleanup on early break. + * cleanup on early break. * * The native call starts eagerly so the returned `response` promise is * meaningful even if the caller never iterates (e.g. awaits `.response` - * directly). The promise settles only after the consumer has fully drained - * the iterator, mirroring native finalize-on-drain semantics. + * directly). The promise settles after the native call and queued item callbacks complete, independently of whether the + * consumer drains the JS iterator. */ function streamItems(native: NativeSession, request: Request, signal: AbortSignal | undefined): StreamingResponse { const queue: Item[] = []; @@ -278,8 +277,8 @@ export abstract class Session { * * Cancellation: pass `{ signal }`; aborting the signal cancels the native * request and causes the iterator to throw an `Error` with - * `name === "AbortError"`. Breaking out of the `for await` loop also - * cancels the underlying request. + * `name === "AbortError"`. Breaking out of the `for await` loop requests cancellation of the underlying request. If + * native completion wins the race, `response` resolves normally; otherwise it rejects with `OperationCancelled`. * * Non-cancellation failures throw a `FoundryLocalError`. */ diff --git a/sdk_v2/js/test/streaming.test.ts b/sdk_v2/js/test/streaming.test.ts index 7e7897f2b..502210c11 100644 --- a/sdk_v2/js/test/streaming.test.ts +++ b/sdk_v2/js/test/streaming.test.ts @@ -121,7 +121,7 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real ); it( - "early break cancels the stream cleanly and the session remains usable", + "early break requests cancellation while permitting prior native completion", async () => { if (session === undefined) throw new Error("fixture missing"); const stream = session.processStreamingRequest(buildPrompt()); @@ -130,10 +130,18 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real count++; if (count >= 1) break; } - await expect(stream.response).rejects.toMatchObject({ - name: "FoundryLocalError", - code: FlErrorCode.OperationCancelled, - }); + const outcome = await stream.response.then( + (response) => ({ response, error: null }), + (error: unknown) => ({ response: null, error }), + ); + if (outcome.error !== null) { + expect(outcome.error).toMatchObject({ + name: "FoundryLocalError", + code: FlErrorCode.OperationCancelled, + }); + } else { + expect(outcome.response.finishReason).not.toBe("none"); + } // After the break the session should accept a follow-up send. const resp = await session.processRequest( @@ -254,6 +262,37 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real 3 * 60_000, ); + it( + "serializes overlapping streams without replacing either callback", + async () => { + if (session === undefined) throw new Error("fixture missing"); + const first = session.processStreamingRequest(buildPrompt()); + const second = session.processStreamingRequest( + new Request() + .addItem(Item.userMessage("Name three primary colors.")) + .setOptions({ search: { maxOutputTokens: 64, temperature: 0 } }), + ); + + const collect = async (stream: AsyncIterable): Promise => { + const items: Item[] = []; + for await (const item of stream) items.push(item); + return items; + }; + const [firstItems, secondItems, firstResponse, secondResponse] = await Promise.all([ + collect(first), + collect(second), + first.response, + second.response, + ]); + + expect(firstItems.length).toBeGreaterThan(0); + expect(secondItems.length).toBeGreaterThan(0); + expect(firstResponse.finishReason).not.toBe("none"); + expect(secondResponse.finishReason).not.toBe("none"); + }, + 4 * 60_000, + ); + it( "stream.response resolves without iteration (eager native start)", async () => { @@ -277,7 +316,7 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real }, 60_000); it( - "request.cancel rejects iteration and stream.response with OperationCancelled", + "request.cancel yields OperationCancelled unless native completion wins", async () => { if (session === undefined) throw new Error("fixture missing"); const req = new Request() @@ -294,15 +333,30 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real } }; - await expect(iteration()).rejects.toMatchObject({ - name: "FoundryLocalError", - code: FlErrorCode.OperationCancelled, - }); - await expect(stream.response).rejects.toMatchObject({ - name: "FoundryLocalError", - code: FlErrorCode.OperationCancelled, - }); - expect(session.turnCount).toBe(0); + const iterationOutcome = await iteration().then( + () => null, + (error: unknown) => error, + ); + const responseOutcome = await stream.response.then( + (response) => ({ response, error: null }), + (error: unknown) => ({ response: null, error }), + ); + + if (responseOutcome.error !== null) { + expect(iterationOutcome).toMatchObject({ + name: "FoundryLocalError", + code: FlErrorCode.OperationCancelled, + }); + expect(responseOutcome.error).toMatchObject({ + name: "FoundryLocalError", + code: FlErrorCode.OperationCancelled, + }); + expect(session.turnCount).toBe(0); + } else { + expect(iterationOutcome).toBeNull(); + expect(responseOutcome.response.finishReason).not.toBe("none"); + expect(session.turnCount).toBe(1); + } }, 3 * 60_000, ); diff --git a/sdk_v2/python/src/foundry_local_sdk/request.py b/sdk_v2/python/src/foundry_local_sdk/request.py index fe6879e81..4e87c336d 100644 --- a/sdk_v2/python/src/foundry_local_sdk/request.py +++ b/sdk_v2/python/src/foundry_local_sdk/request.py @@ -103,7 +103,12 @@ def set_options(self, options: "RequestOptions") -> "Request": return self def cancel(self) -> None: - """Signal cancellation for an in-flight request.""" + """Cancel this request. + + Cancellation before processing is remembered and prevents the request + from reaching the inference backend. Cancellation after completion has + no effect. + """ self._check_open() from foundry_local_sdk._native.api import api From 132dfada2e9a7d80ccb1bfb8afa7d53d06b0b6bc Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 17 Sep 2026 18:53:47 -0500 Subject: [PATCH 04/16] Schedule session work without blocking libuv 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 --- sdk_v2/cpp/src/inferencing/session/session.cc | 14 +- .../test/internal_api/session_manager_test.cc | 24 ++ sdk_v2/js/native/src/session.cc | 297 +++++++++++++++--- sdk_v2/js/native/src/session.h | 20 +- sdk_v2/js/test/streaming.test.ts | 39 +++ 5 files changed, 345 insertions(+), 49 deletions(-) diff --git a/sdk_v2/cpp/src/inferencing/session/session.cc b/sdk_v2/cpp/src/inferencing/session/session.cc index d4fb89b86..4afcecbdc 100644 --- a/sdk_v2/cpp/src/inferencing/session/session.cc +++ b/sdk_v2/cpp/src/inferencing/session/session.cc @@ -170,16 +170,18 @@ void Session::ProcessRequest(const Request& request, Response& response) { { std::lock_guard active_lock(*active_requests_mutex_); - // If Cancel() already ran (shutdown began before this request was admitted), stamp it now so the - // generation loop exits at its first poll instead of running an uncanceled turn. - if (session_canceled_) { - request.Cancel(Request::CancellationReason::SessionShutdown); - } + const bool began = request.TryBegin(); - if (!request.TryBegin() && !request.IsCancellationRequested()) { + if (!began && !request.IsCancellationRequested()) { FL_THROW(FOUNDRY_LOCAL_ERROR_INVALID_USAGE, "request is already being processed"); } + // Stamp only an invocation that successfully claimed the request. This preserves an existing caller cancellation + // while ensuring a completed reusable request admitted after shutdown cannot enter the backend. + if (began && session_canceled_) { + request.Cancel(Request::CancellationReason::SessionShutdown); + } + active_requests_.insert(&request); } diff --git a/sdk_v2/cpp/test/internal_api/session_manager_test.cc b/sdk_v2/cpp/test/internal_api/session_manager_test.cc index 2b052cbec..bae975255 100644 --- a/sdk_v2/cpp/test/internal_api/session_manager_test.cc +++ b/sdk_v2/cpp/test/internal_api/session_manager_test.cc @@ -436,11 +436,16 @@ class CompletingSession : public Session { : Session(model, logger, telemetry) {} SessionType Type() const override { return SessionType::kChat; } + size_t ProcessCount() const { return process_count_; } protected: void ProcessRequestImpl(const Request& /*request*/, Response& response) override { + ++process_count_; response.finish_reason = FOUNDRY_LOCAL_FINISH_STOP; } + + private: + size_t process_count_ = 0; }; flErrorCode ProcessAndGetCode(Session& session, const Request& request) { @@ -561,6 +566,25 @@ TEST(SessionRequestLifecycleTest, PublishedCompletionMakesLateCancellationNoOp) EXPECT_FALSE(request.IsCancellationRequested()); } +TEST(SessionRequestLifecycleTest, ReusedRequestAfterSessionCancelNeverReachesBackend) { + fl::test::FakeServiceBindings svc; + Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + TelemetryLogger telemetry{"test", fl::test::NullLog()}; + CompletingSession session(catalog_model, fl::test::NullLog(), telemetry); + Request request; + Response response; + + session.ProcessRequest(request, response); + ASSERT_EQ(session.ProcessCount(), 1u); + ASSERT_TRUE(request.IsCompleted()); + + session.Cancel(); + + EXPECT_EQ(ProcessAndGetCode(session, request), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::SessionShutdown); + EXPECT_EQ(session.ProcessCount(), 1u); +} + TEST(SessionRequestLifecycleTest, CallbackExceptionSurfacesOriginalCause) { fl::test::FakeServiceBindings svc; Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); diff --git a/sdk_v2/js/native/src/session.cc b/sdk_v2/js/native/src/session.cc index 682970693..dccd8f155 100644 --- a/sdk_v2/js/native/src/session.cc +++ b/sdk_v2/js/native/src/session.cc @@ -6,21 +6,46 @@ #include "errors.h" #include "items.h" #include "model.h" -#include "promise_worker.h" #include "request.h" #include "request_options.h" #include #include +#include +#include #include -#include #include #include #include namespace foundry_local_node { +void SessionScheduler::Enqueue(std::function start) { + pending_.push_back(std::move(start)); + StartNext(); +} + +void SessionScheduler::Complete() { + if (!running_) { + return; + } + + running_ = false; + StartNext(); +} + +void SessionScheduler::StartNext() { + if (running_ || pending_.empty()) { + return; + } + + running_ = true; + auto start = std::move(pending_.front()); + pending_.pop_front(); + start(); +} + namespace { const char* FinishReasonToString(flFinishReason r) { @@ -94,31 +119,145 @@ foundry_local::Request* UnwrapRequest(Napi::Env env, const Napi::Value& v) { return req->native(); } -// Process a Request on the worker thread, converting to JS in the resolver. -// Pins both the Manager (so the Model handle the Session holds stays alive) -// and the Request (so the C++ Request the worker reads stays alive). +// Process a Request on a worker thread, converting to JS after completion. Chat/audio workers are queued by their +// per-session scheduler before consuming a libuv worker; embeddings workers start immediately. +template +class SessionPromiseWorker : public Napi::AsyncWorker { + public: + using Result = std::shared_ptr; + + static Napi::Promise Run(Napi::Env env, std::shared_ptr sess, foundry_local::Request* req, + Napi::ObjectReference manager, Napi::ObjectReference request, + std::shared_ptr scheduler) { + auto* worker = new SessionPromiseWorker(env, std::move(sess), req, std::move(manager), std::move(request), + std::move(scheduler)); + Napi::Promise promise = worker->deferred_.Promise(); + try { + if (worker->scheduler_ != nullptr) { + worker->scheduler_->Enqueue([worker] { worker->Start(); }); + } else { + worker->Start(); + } + } catch (const std::exception& e) { + worker->FailBeforeQueue(e.what()); + } catch (...) { + worker->FailBeforeQueue("Failed to schedule native request"); + } + + return promise; + } + + void Execute() override { + try { + response_ = std::make_shared(sess_->ProcessRequest(*req_)); + } catch (const foundry_local::Error& e) { + err_code_ = static_cast(e.Code()); + err_msg_ = e.what(); + tagged_ = true; + SetError(err_msg_); + } catch (const std::exception& e) { + err_msg_ = e.what(); + SetError(err_msg_); + } catch (...) { + err_msg_ = "Unknown native exception"; + SetError(err_msg_); + } + } + + void OnOK() override { + Napi::Env env = Env(); + Napi::HandleScope scope(env); + try { + deferred_.Resolve(ResponseToJs(env, *response_)); + } catch (const Napi::Error& e) { + deferred_.Reject(e.Value()); + } catch (const std::exception& e) { + deferred_.Reject(Napi::Error::New(env, e.what()).Value()); + } catch (...) { + deferred_.Reject(Napi::Error::New(env, "Failed to convert native response").Value()); + } + + CompleteScheduler(); + } + + void OnError(const Napi::Error& /*unused*/) override { + Napi::Env env = Env(); + Napi::HandleScope scope(env); + if (tagged_) { + Napi::Error err = Napi::Error::New(env, err_msg_); + Napi::Object value = err.Value(); + value.Set("name", Napi::String::New(env, "FoundryLocalError")); + value.Set("code", Napi::Number::New(env, err_code_)); + deferred_.Reject(value); + } else { + deferred_.Reject(Napi::Error::New(env, err_msg_).Value()); + } + + CompleteScheduler(); + } + + private: + SessionPromiseWorker(Napi::Env env, std::shared_ptr sess, foundry_local::Request* req, + Napi::ObjectReference manager, Napi::ObjectReference request, + std::shared_ptr scheduler) + : Napi::AsyncWorker(env), + deferred_(Napi::Promise::Deferred::New(env)), + sess_(std::move(sess)), + req_(req), + manager_(std::move(manager)), + request_(std::move(request)), + scheduler_(std::move(scheduler)) {} + + void Start() { + try { + Queue(); + } catch (const std::exception& e) { + deferred_.Reject(Napi::Error::New(Env(), e.what()).Value()); + CompleteScheduler(); + delete this; + } catch (...) { + deferred_.Reject(Napi::Error::New(Env(), "Failed to queue native request").Value()); + CompleteScheduler(); + delete this; + } + } + + void CompleteScheduler() { + if (!scheduler_completed_ && scheduler_ != nullptr) { + scheduler_completed_ = true; + auto scheduler = std::move(scheduler_); + scheduler->Complete(); + } + } + + void FailBeforeQueue(const std::string& message) { + scheduler_.reset(); + deferred_.Reject(Napi::Error::New(Env(), message).Value()); + delete this; + } + + Napi::Promise::Deferred deferred_; + std::shared_ptr sess_; + foundry_local::Request* req_; + Napi::ObjectReference manager_; + Napi::ObjectReference request_; + std::shared_ptr scheduler_; + Result response_; + std::string err_msg_; + int err_code_ = 0; + bool tagged_ = false; + bool scheduler_completed_ = false; +}; + template Napi::Value ProcessRequestOn(Napi::Env env, std::shared_ptr sess, const Napi::Value& request_arg, - Napi::ObjectReference manager_ref, std::shared_ptr request_gate) { + Napi::ObjectReference manager_ref, std::shared_ptr scheduler) { foundry_local::Request* req = UnwrapRequest(env, request_arg); if (req == nullptr) return env.Undefined(); // pending exception Napi::ObjectReference req_pin = Napi::Reference::New(request_arg.As(), 1); - using Result = std::shared_ptr; - struct Pins { - Napi::ObjectReference manager; - Napi::ObjectReference request; - }; - auto pins = std::make_shared(Pins{std::move(manager_ref), std::move(req_pin)}); - - return PromiseWorker::Run( - env, - [sess = std::move(sess), req, pins, request_gate = std::move(request_gate)]() -> Result { - (void)pins; // keepalive captured by reference count - std::lock_guard lock(*request_gate); - return std::make_shared(sess->ProcessRequest(*req)); - }, - [](Napi::Env env, Result& resp) -> Napi::Value { return ResponseToJs(env, *resp); }); + return SessionPromiseWorker::Run(env, std::move(sess), req, std::move(manager_ref), std::move(req_pin), + std::move(scheduler)); } // ────────────────────────────────────────────────────────────────────────── @@ -150,6 +289,7 @@ struct StreamCtx { Napi::Promise::Deferred deferred; Napi::ObjectReference manager; Napi::ObjectReference request; + std::shared_ptr scheduler; std::shared_ptr response; std::string err_msg; int err_code = 0; @@ -176,6 +316,12 @@ void FinalizeStream(Napi::Env env, void* /*data*/, StreamCtx* ctx) { // anyway so we never leave the deferred pending. ctx->deferred.Resolve(env.Undefined()); } + + if (ctx->scheduler != nullptr) { + auto scheduler = std::move(ctx->scheduler); + scheduler->Complete(); + } + delete ctx; } @@ -184,15 +330,44 @@ class StreamWorker : public Napi::AsyncWorker { public: static Napi::Promise Run(Napi::Env env, std::shared_ptr sess, foundry_local::Request* req, Napi::Function jsCallback, StreamCtx* ctx, - std::shared_ptr request_gate) { - auto* w = new StreamWorker(env, std::move(sess), req, jsCallback, ctx, std::move(request_gate)); + std::shared_ptr scheduler) { Napi::Promise p = ctx->deferred.Promise(); - w->Queue(); + StreamWorker* w = nullptr; + try { + w = new StreamWorker(env, std::move(sess), req, jsCallback, ctx, std::move(scheduler)); + } catch (const Napi::Error& e) { + ctx->scheduler.reset(); + ctx->deferred.Reject(e.Value()); + delete ctx; + return p; + } catch (const std::exception& e) { + ctx->scheduler.reset(); + ctx->deferred.Reject(Napi::Error::New(env, e.what()).Value()); + delete ctx; + return p; + } catch (...) { + ctx->scheduler.reset(); + ctx->deferred.Reject(Napi::Error::New(env, "Failed to initialize native streaming request").Value()); + delete ctx; + return p; + } + + try { + if (w->scheduler_ != nullptr) { + w->scheduler_->Enqueue([w] { w->Start(); }); + } else { + w->Start(); + } + } catch (const std::exception& e) { + w->FailBeforeQueue(e.what()); + } catch (...) { + w->FailBeforeQueue("Failed to schedule native streaming request"); + } + return p; } void Execute() override { - std::lock_guard request_lock(*request_gate_); bool callback_installed = false; try { auto tsfn = tsfn_; @@ -260,33 +435,66 @@ class StreamWorker : public Napi::AsyncWorker { // Promise resolution happens in FinalizeStream — overriding OnOK/OnError // here only releases the TSFN so its finalizer can run on the JS thread // once all queued item callbacks have drained. - void OnOK() override { tsfn_.Release(); } - void OnError(const Napi::Error& /*unused*/) override { tsfn_.Release(); } + void OnOK() override { ReleaseTsfn(); } + void OnError(const Napi::Error& /*unused*/) override { ReleaseTsfn(); } private: StreamWorker(Napi::Env env, std::shared_ptr sess, foundry_local::Request* req, - Napi::Function jsCallback, StreamCtx* ctx, std::shared_ptr request_gate) + Napi::Function jsCallback, StreamCtx* ctx, std::shared_ptr scheduler) : Napi::AsyncWorker(env), sess_(std::move(sess)), req_(req), ctx_(ctx), - request_gate_(std::move(request_gate)), + scheduler_(std::move(scheduler)), tsfn_(Napi::ThreadSafeFunction::New(env, jsCallback, "foundry_local_stream", /*max_queue=*/64, /*threads=*/1, ctx, FinalizeStream, static_cast(nullptr))) {} + void Start() { + try { + Queue(); + } catch (const std::exception& e) { + ctx_->errored = true; + ctx_->err_msg = e.what(); + ReleaseTsfn(); + delete this; + } catch (...) { + ctx_->errored = true; + ctx_->err_msg = "Failed to queue native streaming request"; + ReleaseTsfn(); + delete this; + } + } + + void ReleaseTsfn() { + if (!tsfn_released_) { + tsfn_released_ = true; + tsfn_.Release(); + } + } + + void FailBeforeQueue(const std::string& message) { + scheduler_.reset(); + ctx_->scheduler.reset(); + ctx_->errored = true; + ctx_->err_msg = message; + ReleaseTsfn(); + delete this; + } + std::shared_ptr sess_; foundry_local::Request* req_; StreamCtx* ctx_; - std::shared_ptr request_gate_; + std::shared_ptr scheduler_; Napi::ThreadSafeFunction tsfn_; + bool tsfn_released_ = false; }; template Napi::Value ProcessStreamingRequestOn(Napi::Env env, std::shared_ptr sess, const Napi::CallbackInfo& info, Napi::ObjectReference manager_ref, - std::shared_ptr request_gate) { + std::shared_ptr scheduler) { if (info.Length() < 2 || !info[1].IsFunction()) { Napi::TypeError::New(env, "processStreamingRequest(request: Request, onItem: (item) => void)") .ThrowAsJavaScriptException(); @@ -300,13 +508,14 @@ Napi::Value ProcessStreamingRequestOn(Napi::Env env, std::shared_ptr sess auto* ctx = new StreamCtx{Napi::Promise::Deferred::New(env), std::move(manager_ref), std::move(req_pin), + scheduler, nullptr, "", 0, false, false}; return StreamWorker::Run(env, std::move(sess), req, info[1].As(), ctx, - std::move(request_gate)); + std::move(scheduler)); } } // namespace @@ -332,7 +541,8 @@ Napi::Function ChatSession::Init(Napi::Env env) { }); } -ChatSession::ChatSession(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) { +ChatSession::ChatSession(const Napi::CallbackInfo& info) + : Napi::ObjectWrap(info), scheduler_(std::make_shared()) { Napi::Env env = info.Env(); auto* data = env.GetInstanceData(); if (info.Length() != 1 || !info[0].IsObject() || @@ -379,14 +589,18 @@ Napi::Value ChatSession::ProcessRequest(const Napi::CallbackInfo& info) { return env.Undefined(); } Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessRequestOn(env, impl_, info[0], std::move(owner), request_gate_); + auto impl = impl_; + auto scheduler = scheduler_; + return ProcessRequestOn(env, std::move(impl), info[0], std::move(owner), std::move(scheduler)); } Napi::Value ChatSession::ProcessStreamingRequest(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); if (ThrowIfDisposed(env)) return env.Undefined(); Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessStreamingRequestOn(env, impl_, info, std::move(owner), request_gate_); + auto impl = impl_; + auto scheduler = scheduler_; + return ProcessStreamingRequestOn(env, std::move(impl), info, std::move(owner), std::move(scheduler)); } Napi::Value ChatSession::SetOptions(const Napi::CallbackInfo& info) { @@ -561,7 +775,8 @@ Napi::Value EmbeddingsSession::ProcessRequest(const Napi::CallbackInfo& info) { return env.Undefined(); } Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessRequestOn(env, impl_, info[0], std::move(owner), request_gate_); + auto impl = impl_; + return ProcessRequestOn(env, std::move(impl), info[0], std::move(owner), nullptr); } Napi::Value EmbeddingsSession::SetOptions(const Napi::CallbackInfo& info) { @@ -608,7 +823,7 @@ Napi::Function AudioSession::Init(Napi::Env env) { } AudioSession::AudioSession(const Napi::CallbackInfo& info) - : Napi::ObjectWrap(info) { + : Napi::ObjectWrap(info), scheduler_(std::make_shared()) { Napi::Env env = info.Env(); auto* data = env.GetInstanceData(); if (info.Length() != 1 || !info[0].IsObject() || @@ -655,14 +870,18 @@ Napi::Value AudioSession::ProcessRequest(const Napi::CallbackInfo& info) { return env.Undefined(); } Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessRequestOn(env, impl_, info[0], std::move(owner), request_gate_); + auto impl = impl_; + auto scheduler = scheduler_; + return ProcessRequestOn(env, std::move(impl), info[0], std::move(owner), std::move(scheduler)); } Napi::Value AudioSession::ProcessStreamingRequest(const Napi::CallbackInfo& info) { Napi::Env env = info.Env(); if (ThrowIfDisposed(env)) return env.Undefined(); Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - return ProcessStreamingRequestOn(env, impl_, info, std::move(owner), request_gate_); + auto impl = impl_; + auto scheduler = scheduler_; + return ProcessStreamingRequestOn(env, std::move(impl), info, std::move(owner), std::move(scheduler)); } Napi::Value AudioSession::SetOptions(const Napi::CallbackInfo& info) { diff --git a/sdk_v2/js/native/src/session.h b/sdk_v2/js/native/src/session.h index 379f804fc..37ad3dae2 100644 --- a/sdk_v2/js/native/src/session.h +++ b/sdk_v2/js/native/src/session.h @@ -30,11 +30,24 @@ #include +#include +#include #include -#include namespace foundry_local_node { +class SessionScheduler { + public: + void Enqueue(std::function start); + void Complete(); + + private: + void StartNext(); + + std::deque> pending_; + bool running_ = false; +}; + class ChatSession : public Napi::ObjectWrap { public: static Napi::Function Init(Napi::Env env); @@ -56,7 +69,7 @@ class ChatSession : public Napi::ObjectWrap { std::shared_ptr impl_; Napi::ObjectReference manager_; - std::shared_ptr request_gate_ = std::make_shared(); + std::shared_ptr scheduler_; }; // Napi::ObjectWrap over foundry_local::EmbeddingsSession. @@ -88,7 +101,6 @@ class EmbeddingsSession : public Napi::ObjectWrap { std::shared_ptr impl_; Napi::ObjectReference manager_; - std::shared_ptr request_gate_ = std::make_shared(); }; // Napi::ObjectWrap over foundry_local::AudioSession. @@ -118,7 +130,7 @@ class AudioSession : public Napi::ObjectWrap { std::shared_ptr impl_; Napi::ObjectReference manager_; - std::shared_ptr request_gate_ = std::make_shared(); + std::shared_ptr scheduler_; }; } // namespace foundry_local_node diff --git a/sdk_v2/js/test/streaming.test.ts b/sdk_v2/js/test/streaming.test.ts index 502210c11..c15f1987a 100644 --- a/sdk_v2/js/test/streaming.test.ts +++ b/sdk_v2/js/test/streaming.test.ts @@ -47,6 +47,7 @@ function countUkTokens(text: string): number { // Used by the multi-turn streaming test: a context-dependent follow-up // ("What is the capital of each?") should mention the UK capitals. const UK_CAPITAL_TOKENS = ["london", "edinburgh", "cardiff", "belfast"] as const; +const PRIMARY_COLOR_TOKENS = ["red", "blue", "yellow"] as const; function countUkCapitalTokens(text: string): number { const lower = text.toLowerCase(); @@ -287,12 +288,50 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real expect(firstItems.length).toBeGreaterThan(0); expect(secondItems.length).toBeGreaterThan(0); + expect(countUkTokens(firstItems.map(extractText).join(""))).toBeGreaterThanOrEqual(2); + expect( + PRIMARY_COLOR_TOKENS.filter((token) => secondItems.map(extractText).join("").toLowerCase().includes(token)) + .length, + ).toBeGreaterThanOrEqual(2); expect(firstResponse.finishReason).not.toBe("none"); expect(secondResponse.finishReason).not.toBe("none"); }, 4 * 60_000, ); + it( + "finishes accepted queued work after dispose and rejects future work", + async () => { + if (session === undefined) throw new Error("fixture missing"); + const active = session.processStreamingRequest(buildPrompt()); + const queued = session.processRequest( + new Request() + .addItem(Item.userMessage("Reply with the single word 'ok'.")) + .setOptions({ search: { maxOutputTokens: 4, temperature: 0 } }), + ); + + session.dispose(); + expect(session.disposed).toBe(true); + + await expect( + session.processRequest(new Request().addItem(Item.userMessage("This must not be accepted."))), + ).rejects.toMatchObject({ + name: "FoundryLocalError", + code: FlErrorCode.InvalidUsage, + }); + + const activeItems: Item[] = []; + for await (const item of active) activeItems.push(item); + const [activeResponse, queuedResponse] = await Promise.all([active.response, queued]); + + expect(activeItems.length).toBeGreaterThan(0); + expect(activeResponse.finishReason).not.toBe("none"); + expect(queuedResponse.finishReason).not.toBe("none"); + expect(queuedResponse.output.map(extractText).join("").toLowerCase()).toContain("ok"); + }, + 4 * 60_000, + ); + it( "stream.response resolves without iteration (eager native start)", async () => { From 8b538d0e0b7ffd42c6574635f769d6c9c46aac20 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 17 Sep 2026 19:11:27 -0500 Subject: [PATCH 05/16] Wake callback drain waiters after backlog drops 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 --- sdk_v2/cpp/src/inferencing/session/callback_handler.h | 1 + sdk_v2/cpp/test/internal_api/callback_handler_test.cc | 1 + sdk_v2/js/src/session.ts | 3 ++- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/sdk_v2/cpp/src/inferencing/session/callback_handler.h b/sdk_v2/cpp/src/inferencing/session/callback_handler.h index 552c27ea0..9396d1c0b 100644 --- a/sdk_v2/cpp/src/inferencing/session/callback_handler.h +++ b/sdk_v2/cpp/src/inferencing/session/callback_handler.h @@ -96,6 +96,7 @@ struct CallbackHandler { while (queue_->Size() > 0) { if (request_.IsCancellationRequested() && queue_->Size() > kMaxCancellationDrainItems) { DropPendingItems(); + SetCallbackInProgress(false); break; } diff --git a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc index 0ff8f050e..9b1b3cbe3 100644 --- a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc +++ b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc @@ -222,6 +222,7 @@ TEST(CallbackHandlerTest, CancellationDropsLargeBufferedBacklog) { } cv.notify_all(); + handler.DrainPending(); handler.Drain(); EXPECT_EQ(invocations.load(), 1); } diff --git a/sdk_v2/js/src/session.ts b/sdk_v2/js/src/session.ts index 67d247817..ebe607e3a 100644 --- a/sdk_v2/js/src/session.ts +++ b/sdk_v2/js/src/session.ts @@ -47,7 +47,8 @@ export interface StreamOptions { * * `response` settles after the native call completes and all queued item callbacks have run. Breaking iteration early * requests cancellation, but native completion can win that race; in that case `response` resolves normally. Otherwise - * it rejects with the cancellation error (`AbortError` for an aborted signal or `OperationCancelled` for an early break). + * it rejects with the cancellation error (`AbortError` for an aborted signal or `OperationCancelled` for an early + * break). */ export interface StreamingResponse extends AsyncIterable { readonly response: Promise; From 2b3a6821df82c54359bf9fba90919058f641c1f7 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 11:17:59 -0500 Subject: [PATCH 06/16] Limit request cancellation to active inference 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 --- .../include/foundry_local/foundry_local_c.h | 2 +- .../include/foundry_local/foundry_local_cpp.h | 3 +- sdk_v2/cpp/src/inferencing/session/request.h | 28 ++- sdk_v2/cpp/src/inferencing/session/session.cc | 28 ++- sdk_v2/cpp/test/internal_api/c_api_test.cc | 4 +- .../internal_api/callback_handler_test.cc | 7 + .../internal_api/chat/chat_session_test.cc | 12 +- sdk_v2/cpp/test/internal_api/item_test.cc | 21 ++- .../test/internal_api/session_manager_test.cc | 173 ++++++++++++++++-- sdk_v2/cpp/test/sdk_api/cpp_api_test.cc | 4 +- sdk_v2/cs/src/Request.cs | 3 +- sdk_v2/js/docs/PortJsToSdkV2.md | 6 +- sdk_v2/js/src/request.ts | 8 +- sdk_v2/js/src/session.ts | 22 ++- sdk_v2/js/test/items.test.ts | 2 +- .../python/src/foundry_local_sdk/request.py | 5 +- 16 files changed, 261 insertions(+), 67 deletions(-) diff --git a/sdk_v2/cpp/include/foundry_local/foundry_local_c.h b/sdk_v2/cpp/include/foundry_local/foundry_local_c.h index e9a8476e5..1c26056d0 100644 --- a/sdk_v2/cpp/include/foundry_local/foundry_local_c.h +++ b/sdk_v2/cpp/include/foundry_local/foundry_local_c.h @@ -886,7 +886,7 @@ struct flInferenceApi { /// Values are string representations; the implementation parses them for the appropriate type. /// The request copies the data — the caller may release the pairs after this call. FL_API_STATUS(Request_SetOptions, _In_ flRequest* request, _In_ const flKeyValuePairs* options); - /// Cancel a request. Pre-processing cancellation is remembered; cancellation after completion is a no-op. + /// Cancel the in-flight invocation of a request. This is a no-op while idle or after completion. FL_API_STATUS(Request_Cancel, _In_ flRequest* request); /* Response */ diff --git a/sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h b/sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h index 22ae78065..9a2f2eeeb 100644 --- a/sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h +++ b/sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h @@ -1028,8 +1028,7 @@ class Request { /// Options for this request. Overrides session options for the duration of this request. Request& SetOptions(const RequestOptions& options); - /// Cancel this request. Pre-processing cancellation is remembered; in-flight inference stops cooperatively and - /// cancellation after completion is a no-op. + /// Cancel this request's in-flight invocation. This is a no-op while idle or after completion. void Cancel(); const flRequest* native_handle() const noexcept { return handle_.get(); } diff --git a/sdk_v2/cpp/src/inferencing/session/request.h b/sdk_v2/cpp/src/inferencing/session/request.h index 3d0eb120b..5619c6288 100644 --- a/sdk_v2/cpp/src/inferencing/session/request.h +++ b/sdk_v2/cpp/src/inferencing/session/request.h @@ -117,25 +117,35 @@ struct Request { item_segment_starts.push_back(items.size()); } - /// Atomically wins cancellation against terminal publication. Returns false after completion has won. + /// Cancels only the invocation currently being processed. Idle and completed requests are unchanged. bool Cancel(CancellationReason reason = CancellationReason::Caller) const noexcept { const auto canceled_state = CanceledState(reason); - auto state = state_.load(std::memory_order_acquire); - while (state == State::Ready || state == State::Running) { - if (state_.compare_exchange_weak(state, canceled_state, + auto expected = State::Running; + if (state_.compare_exchange_strong(expected, canceled_state, std::memory_order_acq_rel, std::memory_order_acquire)) { - return true; - } + return true; } - return IsCanceledState(state); + return IsCanceledState(expected); } bool CancelFromStreamingCallbackException(std::string_view detail) const noexcept { try { std::lock_guard lock(cancellation_detail_mutex_); + auto expected = state_.load(std::memory_order_acquire); + if (expected != State::Running) { + return IsCanceledState(expected); + } + cancellation_detail_ = detail; + if (state_.compare_exchange_strong(expected, State::CanceledByStreamingCallbackException, + std::memory_order_acq_rel, + std::memory_order_acquire)) { + return true; + } + + return IsCanceledState(expected); } catch (...) { // Cancellation itself must remain reliable if preserving diagnostic text runs out of memory. } @@ -157,9 +167,11 @@ struct Request { } /// Starts first-time processing or reuses a request whose previous operation completed. - bool TryBegin() const noexcept { + bool TryBegin() const { + std::lock_guard lock(cancellation_detail_mutex_); auto state = state_.load(std::memory_order_acquire); while (state == State::Ready || state == State::Completed) { + cancellation_detail_.clear(); if (state_.compare_exchange_weak(state, State::Running, std::memory_order_acq_rel, std::memory_order_acquire)) { diff --git a/sdk_v2/cpp/src/inferencing/session/session.cc b/sdk_v2/cpp/src/inferencing/session/session.cc index 4afcecbdc..04e0f718a 100644 --- a/sdk_v2/cpp/src/inferencing/session/session.cc +++ b/sdk_v2/cpp/src/inferencing/session/session.cc @@ -170,15 +170,13 @@ void Session::ProcessRequest(const Request& request, Response& response) { { std::lock_guard active_lock(*active_requests_mutex_); - const bool began = request.TryBegin(); - - if (!began && !request.IsCancellationRequested()) { + if (!request.TryBegin()) { FL_THROW(FOUNDRY_LOCAL_ERROR_INVALID_USAGE, "request is already being processed"); } - // Stamp only an invocation that successfully claimed the request. This preserves an existing caller cancellation - // while ensuring a completed reusable request admitted after shutdown cannot enter the backend. - if (began && session_canceled_) { + // A late shutdown admission must first claim the request, then be canceled under the same lock that protects + // active registration. This keeps TryBegin as the sole admission gate while preventing backend entry. + if (session_canceled_) { request.Cancel(Request::CancellationReason::SessionShutdown); } @@ -190,10 +188,22 @@ void Session::ProcessRequest(const Request& request, Response& response) { struct ActiveRequestGuard { Session& session; const Request& request; - ~ActiveRequestGuard() { + + void Deregister() { + if (!registered) { + return; + } + std::lock_guard active_lock(*session.active_requests_mutex_); session.active_requests_.erase(&request); + registered = false; } + + ~ActiveRequestGuard() { + Deregister(); + } + + bool registered = true; } active_guard{*this, request}; ActionTracker tracker(Action::kSessionProcessRequest, telemetry_); @@ -214,10 +224,12 @@ void Session::ProcessRequest(const Request& request, Response& response) { } response = std::move(staged_response); + active_guard.Deregister(); request.PublishCompletion(); tracker.SetStatus(ActionStatus::kSuccess); } catch (const std::exception& ex) { if (request.TryComplete()) { + active_guard.Deregister(); request.PublishCompletion(); tracker.RecordException(ex); throw; @@ -226,6 +238,8 @@ void Session::ProcessRequest(const Request& request, Response& response) { try { ThrowCancellation(request); } catch (const std::exception& cancellation) { + active_guard.Deregister(); + request.PublishCompletion(); tracker.RecordException(cancellation); throw; } diff --git a/sdk_v2/cpp/test/internal_api/c_api_test.cc b/sdk_v2/cpp/test/internal_api/c_api_test.cc index e9223d426..11836f669 100644 --- a/sdk_v2/cpp/test/internal_api/c_api_test.cc +++ b/sdk_v2/cpp/test/internal_api/c_api_test.cc @@ -1712,7 +1712,7 @@ TEST(CApiTest, ItemQueuePushPopAndFinish) { // Inference API — Request_Cancel // ======================================================================== -TEST(CApiTest, RequestCancelBeforeAttachmentSucceeds) { +TEST(CApiTest, RequestCancelWhileIdleSucceeds) { const flApi* api = GetApi(); const flInferenceApi* inf_api = api->GetInferenceApi(); @@ -1720,7 +1720,7 @@ TEST(CApiTest, RequestCancelBeforeAttachmentSucceeds) { ASSERT_TRUE(IsOk(inf_api->Request_Create(&req))); ASSERT_NE(req, nullptr); - // Cancellation is remembered until a session would otherwise attach the request. + // Idle cancellation is a successful no-op. EXPECT_TRUE(IsOk(inf_api->Request_Cancel(req))); inf_api->Request_Release(req); diff --git a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc index 9b1b3cbe3..0e6c4e0da 100644 --- a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc +++ b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc @@ -33,6 +33,7 @@ CallbackHandler::CallbackFn MakeThrowingCallback(std::atomic& invocations) TEST(CallbackHandlerTest, StdExceptionFromCallbackDoesNotTerminate) { Request request; + ASSERT_TRUE(request.TryBegin()); std::atomic invocations{0}; { @@ -51,6 +52,7 @@ TEST(CallbackHandlerTest, StdExceptionFromCallbackDoesNotTerminate) { TEST(CallbackHandlerTest, NonStdExceptionFromCallbackDoesNotTerminate) { Request request; + ASSERT_TRUE(request.TryBegin()); std::atomic invocations{0}; auto fn = [&invocations](flStreamingCallbackData, void*) -> int { @@ -72,6 +74,7 @@ TEST(CallbackHandlerTest, NonStdExceptionFromCallbackDoesNotTerminate) { TEST(CallbackHandlerTest, FurtherPushesAfterExceptionAreNoOps) { Request request; + ASSERT_TRUE(request.TryBegin()); std::atomic invocations{0}; CallbackHandler handler(request, MakeThrowingCallback(invocations), fl::test::NullLog()); @@ -98,6 +101,7 @@ TEST(CallbackHandlerTest, FurtherPushesAfterExceptionAreNoOps) { TEST(CallbackHandlerTest, NormalCallbackCancelsViaReturnValue) { Request request; + ASSERT_TRUE(request.TryBegin()); std::atomic invocations{0}; auto fn = [&invocations, &request](flStreamingCallbackData data, void*) -> int { @@ -121,6 +125,7 @@ TEST(CallbackHandlerTest, NormalCallbackCancelsViaReturnValue) { TEST(CallbackHandlerTest, DrainPendingWaitsForDeliveryWithoutClosingTheQueue) { Request request; + ASSERT_TRUE(request.TryBegin()); std::atomic invocations{0}; auto fn = [&invocations](flStreamingCallbackData data, void*) -> int { @@ -142,6 +147,7 @@ TEST(CallbackHandlerTest, DrainPendingWaitsForDeliveryWithoutClosingTheQueue) { TEST(CallbackHandlerTest, CancellationDrainsSmallBufferedBacklog) { Request request; + ASSERT_TRUE(request.TryBegin()); std::atomic invocations{0}; std::mutex mutex; std::condition_variable cv; @@ -185,6 +191,7 @@ TEST(CallbackHandlerTest, CancellationDrainsSmallBufferedBacklog) { TEST(CallbackHandlerTest, CancellationDropsLargeBufferedBacklog) { Request request; + ASSERT_TRUE(request.TryBegin()); std::atomic invocations{0}; std::mutex mutex; std::condition_variable cv; diff --git a/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc b/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc index 5fc7be2e7..60c6bf5f6 100644 --- a/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc +++ b/sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc @@ -1888,8 +1888,8 @@ TEST_F(QwenNativeProductionIntegrationTest, Response response; ExpectOperationCancelled([&] { session.ProcessRequest(request, response); }); - EXPECT_TRUE(request.IsCancellationRequested()); - EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::Caller); + EXPECT_TRUE(request.IsCompleted()); + EXPECT_FALSE(request.IsCancellationRequested()); EXPECT_EQ(counters->created, 2); EXPECT_EQ(counters->closed, 1); EXPECT_EQ(counters->canceled, 1); @@ -1928,8 +1928,8 @@ TEST_F(QwenNativeProductionIntegrationTest, Response response; ExpectOperationCancelled([&] { session.ProcessRequest(request, response); }); - EXPECT_TRUE(request.IsCancellationRequested()); - EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::StreamingCallback); + EXPECT_TRUE(request.IsCompleted()); + EXPECT_FALSE(request.IsCancellationRequested()); EXPECT_EQ(counters->created, 1); EXPECT_EQ(counters->canceled, 1); EXPECT_EQ(counters->usage_after_cancel, 1); @@ -1987,8 +1987,8 @@ TEST_F(QwenNativeProductionIntegrationTest, Response response; ExpectOperationCancelled([&] { session.ProcessRequest(request, response); }); - EXPECT_TRUE(request.IsCancellationRequested()); - EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::StreamingCallback); + EXPECT_TRUE(request.IsCompleted()); + EXPECT_FALSE(request.IsCancellationRequested()); EXPECT_EQ(counters->created, 1); EXPECT_EQ(counters->canceled, 1); EXPECT_EQ(counters->usage_after_cancel, 1); diff --git a/sdk_v2/cpp/test/internal_api/item_test.cc b/sdk_v2/cpp/test/internal_api/item_test.cc index 44681859c..b25f746ff 100644 --- a/sdk_v2/cpp/test/internal_api/item_test.cc +++ b/sdk_v2/cpp/test/internal_api/item_test.cc @@ -683,12 +683,21 @@ TEST(RequestTest, MixedOwnedAndBorrowedItems) { EXPECT_TRUE(req.items[1]->type == FOUNDRY_LOCAL_ITEM_MESSAGE); } -TEST(RequestTest, CancellationWinsBeforeCompletion) { +TEST(RequestTest, ReadyCancellationIsNoOpAndTryBeginSucceeds) { Request req; EXPECT_FALSE(req.IsCancellationRequested()); + EXPECT_FALSE(req.Cancel()); + EXPECT_FALSE(req.IsCancellationRequested()); + EXPECT_TRUE(req.TryBegin()); +} + +TEST(RequestTest, CancellationWinsBeforeCompletion) { + Request req; + ASSERT_TRUE(req.TryBegin()); + EXPECT_TRUE(req.Cancel()); - EXPECT_TRUE(req.Cancel()); + EXPECT_TRUE(req.Cancel(Request::CancellationReason::SessionShutdown)); EXPECT_TRUE(req.IsCancellationRequested()); EXPECT_EQ(req.GetCancellationReason(), Request::CancellationReason::Caller); EXPECT_FALSE(req.TryComplete()); @@ -696,20 +705,24 @@ TEST(RequestTest, CancellationWinsBeforeCompletion) { TEST(RequestTest, CancellationPreservesActionableReason) { Request callback_request; + ASSERT_TRUE(callback_request.TryBegin()); EXPECT_TRUE(callback_request.Cancel(Request::CancellationReason::StreamingCallback)); EXPECT_EQ(callback_request.GetCancellationReason(), Request::CancellationReason::StreamingCallback); Request exception_request; + ASSERT_TRUE(exception_request.TryBegin()); EXPECT_TRUE(exception_request.CancelFromStreamingCallbackException("callback exploded")); + EXPECT_TRUE(exception_request.CancelFromStreamingCallbackException("replacement detail")); EXPECT_EQ(exception_request.GetCancellationReason(), Request::CancellationReason::StreamingCallbackException); EXPECT_EQ(exception_request.CancellationDetail(), "callback exploded"); Request shutdown_request; + ASSERT_TRUE(shutdown_request.TryBegin()); EXPECT_TRUE(shutdown_request.Cancel(Request::CancellationReason::SessionShutdown)); EXPECT_EQ(shutdown_request.GetCancellationReason(), Request::CancellationReason::SessionShutdown); } -TEST(RequestTest, CompletionMakesLateCancellationANoOp) { +TEST(RequestTest, CompletedCancellationIsNoOpAndRequestCanBeReused) { Request req; ASSERT_TRUE(req.TryBegin()); @@ -720,6 +733,8 @@ TEST(RequestTest, CompletionMakesLateCancellationANoOp) { EXPECT_FALSE(req.TryBegin()); req.PublishCompletion(); EXPECT_TRUE(req.IsCompleted()); + EXPECT_FALSE(req.Cancel()); + EXPECT_FALSE(req.IsCancellationRequested()); EXPECT_TRUE(req.TryBegin()); } diff --git a/sdk_v2/cpp/test/internal_api/session_manager_test.cc b/sdk_v2/cpp/test/internal_api/session_manager_test.cc index bae975255..c33e40de4 100644 --- a/sdk_v2/cpp/test/internal_api/session_manager_test.cc +++ b/sdk_v2/cpp/test/internal_api/session_manager_test.cc @@ -448,6 +448,88 @@ class CompletingSession : public Session { size_t process_count_ = 0; }; +class ControlledCancelSession : public Session { + public: + ControlledCancelSession(const Model& model, ILogger& logger, ITelemetry& telemetry) + : Session(model, logger, telemetry, /*allow_concurrent_requests=*/true) {} + + SessionType Type() const override { return SessionType::kChat; } + bool FirstInvocationEntered() const { return first_invocation_entered_.load(std::memory_order_acquire); } + size_t ProcessCount() const { return process_count_.load(std::memory_order_acquire); } + + void ReleaseFirstInvocation() { + release_first_invocation_.store(true, std::memory_order_release); + } + + protected: + void ProcessRequestImpl(const Request& request, Response& response) override { + const auto invocation = process_count_.fetch_add(1, std::memory_order_acq_rel); + if (invocation == 0) { + first_invocation_entered_.store(true, std::memory_order_release); + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (!request.IsCancellationRequested() || !release_first_invocation_.load(std::memory_order_acquire)) { + if (std::chrono::steady_clock::now() >= deadline) { + return; + } + + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + } + + response.finish_reason = FOUNDRY_LOCAL_FINISH_STOP; + } + + private: + std::atomic process_count_{0}; + std::atomic first_invocation_entered_{false}; + std::atomic release_first_invocation_{false}; +}; + +class CallbackExceptionSession : public Session { + public: + CallbackExceptionSession(const Model& model, ILogger& logger, ITelemetry& telemetry) + : Session(model, logger, telemetry) {} + + SessionType Type() const override { return SessionType::kChat; } + size_t ProcessCount() const { return process_count_; } + + protected: + void ProcessRequestImpl(const Request& request, Response& response) override { + ++process_count_; + if (process_count_ == 1) { + request.CancelFromStreamingCallbackException("callback exploded"); + return; + } + + response.finish_reason = FOUNDRY_LOCAL_FINISH_STOP; + } + + private: + size_t process_count_ = 0; +}; + +class ThrowThenCompleteSession : public Session { + public: + ThrowThenCompleteSession(const Model& model, ILogger& logger, ITelemetry& telemetry) + : Session(model, logger, telemetry) {} + + SessionType Type() const override { return SessionType::kChat; } + size_t ProcessCount() const { return process_count_; } + + protected: + void ProcessRequestImpl(const Request& /*request*/, Response& response) override { + ++process_count_; + if (process_count_ == 1) { + throw std::runtime_error("backend exploded"); + } + + response.finish_reason = FOUNDRY_LOCAL_FINISH_STOP; + } + + private: + size_t process_count_ = 0; +}; + flErrorCode ProcessAndGetCode(Session& session, const Request& request) { try { Response response; @@ -504,8 +586,10 @@ TEST(SessionManagerCancelTest, CancelAllCancelsInFlightRequestsOnEverySession) { EXPECT_EQ(f2.wait_for(std::chrono::seconds(2)), std::future_status::ready); EXPECT_EQ(f1.get(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); EXPECT_EQ(f2.get(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); - EXPECT_TRUE(req1.IsCancellationRequested()); - EXPECT_TRUE(req2.IsCancellationRequested()); + EXPECT_TRUE(req1.IsCompleted()); + EXPECT_TRUE(req2.IsCompleted()); + EXPECT_FALSE(req1.IsCancellationRequested()); + EXPECT_FALSE(req2.IsCancellationRequested()); } TEST(SessionManagerCancelTest, RequestAdmittedAfterCancelIsStampedCanceled) { @@ -534,20 +618,21 @@ TEST(SessionManagerCancelTest, RequestAdmittedAfterCancelIsStampedCanceled) { EXPECT_EQ(f.wait_for(std::chrono::seconds(2)), std::future_status::ready) << "late-admitted request ran uncanceled — the session_canceled_ latch did not stamp it"; EXPECT_EQ(f.get(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); - EXPECT_TRUE(req.IsCancellationRequested()); + EXPECT_TRUE(req.IsCompleted()); + EXPECT_FALSE(req.IsCancellationRequested()); EXPECT_FALSE(s.InFlight()); } -TEST(SessionRequestLifecycleTest, PreCanceledRequestNeverReachesBackend) { +TEST(SessionRequestLifecycleTest, IdleCancelIsNoOpAndRequestReachesBackend) { fl::test::FakeServiceBindings svc; Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); TelemetryLogger telemetry{"test", fl::test::NullLog()}; - BlockingCancelSession session(catalog_model, fl::test::NullLog(), telemetry); + CompletingSession session(catalog_model, fl::test::NullLog(), telemetry); Request request; - ASSERT_TRUE(request.Cancel()); + ASSERT_FALSE(request.Cancel()); - EXPECT_EQ(ProcessAndGetCode(session, request), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); - EXPECT_FALSE(session.InFlight()); + EXPECT_EQ(ProcessAndGetCode(session, request), FOUNDRY_LOCAL_OK); + EXPECT_EQ(session.ProcessCount(), 1u); } TEST(SessionRequestLifecycleTest, PublishedCompletionMakesLateCancellationNoOp) { @@ -581,17 +666,76 @@ TEST(SessionRequestLifecycleTest, ReusedRequestAfterSessionCancelNeverReachesBac session.Cancel(); EXPECT_EQ(ProcessAndGetCode(session, request), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); - EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::SessionShutdown); + EXPECT_TRUE(request.IsCompleted()); + EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::None); EXPECT_EQ(session.ProcessCount(), 1u); } -TEST(SessionRequestLifecycleTest, CallbackExceptionSurfacesOriginalCause) { +TEST(SessionRequestLifecycleTest, InFlightCancellationAllowsRequestReuse) { fl::test::FakeServiceBindings svc; Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); TelemetryLogger telemetry{"test", fl::test::NullLog()}; - CompletingSession session(catalog_model, fl::test::NullLog(), telemetry); + ControlledCancelSession session(catalog_model, fl::test::NullLog(), telemetry); + Request request; + + auto first = std::async(std::launch::async, [&] { return ProcessAndGetCode(session, request); }); + ASSERT_TRUE(WaitUntil([&] { return session.FirstInvocationEntered(); }, std::chrono::seconds(2))); + ASSERT_TRUE(request.Cancel()); + session.ReleaseFirstInvocation(); + + EXPECT_EQ(first.get(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::None); + EXPECT_TRUE(request.IsCompleted()); + + EXPECT_EQ(ProcessAndGetCode(session, request), FOUNDRY_LOCAL_OK); + EXPECT_EQ(session.ProcessCount(), 2u); +} + +TEST(SessionRequestLifecycleTest, ConcurrentReuseOfCanceledActiveRequestIsRejected) { + fl::test::FakeServiceBindings svc; + Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + TelemetryLogger telemetry{"test", fl::test::NullLog()}; + ControlledCancelSession session(catalog_model, fl::test::NullLog(), telemetry); + Request request; + + auto first = std::async(std::launch::async, [&] { return ProcessAndGetCode(session, request); }); + ASSERT_TRUE(WaitUntil([&] { return session.FirstInvocationEntered(); }, std::chrono::seconds(2))); + ASSERT_TRUE(request.Cancel()); + + auto concurrent = std::async(std::launch::async, [&] { return ProcessAndGetCode(session, request); }); + EXPECT_EQ(concurrent.wait_for(std::chrono::seconds(2)), std::future_status::ready); + EXPECT_EQ(concurrent.get(), FOUNDRY_LOCAL_ERROR_INVALID_USAGE); + EXPECT_EQ(session.ProcessCount(), 1u); + + session.ReleaseFirstInvocation(); + EXPECT_EQ(first.get(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); +} + +TEST(SessionRequestLifecycleTest, OrdinaryExceptionAllowsRequestReuse) { + fl::test::FakeServiceBindings svc; + Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + TelemetryLogger telemetry{"test", fl::test::NullLog()}; + ThrowThenCompleteSession session(catalog_model, fl::test::NullLog(), telemetry); + Request request; + + EXPECT_THROW( + { + Response response; + session.ProcessRequest(request, response); + }, + std::runtime_error); + EXPECT_TRUE(request.IsCompleted()); + + EXPECT_EQ(ProcessAndGetCode(session, request), FOUNDRY_LOCAL_OK); + EXPECT_EQ(session.ProcessCount(), 2u); +} + +TEST(SessionRequestLifecycleTest, CallbackExceptionSurfacesOriginalCauseAndAllowsReuse) { + fl::test::FakeServiceBindings svc; + Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + TelemetryLogger telemetry{"test", fl::test::NullLog()}; + CallbackExceptionSession session(catalog_model, fl::test::NullLog(), telemetry); Request request; - ASSERT_TRUE(request.CancelFromStreamingCallbackException("callback exploded")); try { Response response; @@ -602,4 +746,9 @@ TEST(SessionRequestLifecycleTest, CallbackExceptionSurfacesOriginalCause) { EXPECT_NE(std::string(error.what()).find("streaming callback threw an exception: callback exploded"), std::string::npos); } + + EXPECT_TRUE(request.IsCompleted()); + EXPECT_EQ(request.GetCancellationReason(), Request::CancellationReason::None); + EXPECT_EQ(ProcessAndGetCode(session, request), FOUNDRY_LOCAL_OK); + EXPECT_EQ(session.ProcessCount(), 2u); } diff --git a/sdk_v2/cpp/test/sdk_api/cpp_api_test.cc b/sdk_v2/cpp/test/sdk_api/cpp_api_test.cc index f9abe5213..316df305d 100644 --- a/sdk_v2/cpp/test/sdk_api/cpp_api_test.cc +++ b/sdk_v2/cpp/test/sdk_api/cpp_api_test.cc @@ -284,9 +284,9 @@ TEST(CppApiTest, RequestSetOptions) { request.SetOptions(opts); } -TEST(CppApiTest, RequestCancelBeforeAttachmentSucceeds) { +TEST(CppApiTest, RequestCancelWhileIdleSucceeds) { foundry_local::Request request; - // Cancellation is remembered until a session would otherwise attach the request. + // Idle cancellation is a successful no-op. EXPECT_NO_THROW(request.Cancel()); } diff --git a/sdk_v2/cs/src/Request.cs b/sdk_v2/cs/src/Request.cs index 7baaf1aaf..4ece717a6 100644 --- a/sdk_v2/cs/src/Request.cs +++ b/sdk_v2/cs/src/Request.cs @@ -93,8 +93,7 @@ internal Request SetOptions(IntPtr options) } /// - /// Cancels this request. Cancellation before processing is remembered and prevents the request from reaching the - /// inference backend; cancellation after completion has no effect. + /// Cancels this request's in-flight invocation. This has no effect while the request is idle or after completion. /// public void Cancel() { diff --git a/sdk_v2/js/docs/PortJsToSdkV2.md b/sdk_v2/js/docs/PortJsToSdkV2.md index 439def37b..4dab8e7d0 100644 --- a/sdk_v2/js/docs/PortJsToSdkV2.md +++ b/sdk_v2/js/docs/PortJsToSdkV2.md @@ -119,9 +119,9 @@ underlying C ABI call is a memory copy, not I/O. native streaming-callback push lands on a `Napi::ThreadSafeFunction` acquired in the session's constructor and released when the iterable is closed. -- Cancellation: each async API accepts an `AbortSignal`. The signal is - bound to `Request::Cancel()`, which the C++ wrapper translates into a - cancellation signal observed by the streaming callback. +- Cancellation: streaming APIs accept an `AbortSignal`. A signal already aborted at call time rejects before native + work is submitted. Once submitted, the signal calls `Request::Cancel()`, which affects only an invocation currently + inside native `Session::ProcessRequest`; work still waiting in the native session FIFO is not canceled. - Live PCM input (audio transcription with chunks arriving over time) is expressed by adding an `AudioItem` descriptor to the `Request` and pushing PCM bytes through a paired `ItemQueue`. The session consumes the diff --git a/sdk_v2/js/src/request.ts b/sdk_v2/js/src/request.ts index fe5e6d364..64ad49775 100644 --- a/sdk_v2/js/src/request.ts +++ b/sdk_v2/js/src/request.ts @@ -74,11 +74,9 @@ export class Request { } /** - * Cancel a request. Safe to call at any time; cancellation before processing - * is remembered, while cancellation after completion has no effect. - * Cancellation makes the matching `Session.processRequest()` reject with a - * `FoundryLocalError` whose `code === FlErrorCode.OperationCancelled` and - * whose message identifies caller-requested cancellation. + * Cancel this request's invocation only while it is inside native `Session::ProcessRequest`. Calling this while the + * request is idle, waiting in a session's native FIFO, or already completed has no effect. Active cancellation makes + * the matching `Session.processRequest()` reject with `code === FlErrorCode.OperationCancelled`. */ cancel(): void { this.#native.cancel(); diff --git a/sdk_v2/js/src/session.ts b/sdk_v2/js/src/session.ts index ebe607e3a..63468565e 100644 --- a/sdk_v2/js/src/session.ts +++ b/sdk_v2/js/src/session.ts @@ -32,9 +32,10 @@ import type { Response } from "./response.js"; /** Options accepted by streaming Session APIs. */ export interface StreamOptions { /** - * Optional cancellation signal. When the signal aborts, the underlying - * `Request` is cancelled and the async iterator rejects with an `Error` - * whose `name === "AbortError"` (mirroring the Web/Node standard). + * Optional cancellation signal. Once native processing is active, aborting cancels the `Request` and the iterator + * rejects with an `Error` whose `name === "AbortError"`. An abort while work is only waiting in the native session + * FIFO does not prevent that work from running. A signal already aborted when this method is called rejects before + * native work is submitted. */ readonly signal?: AbortSignal; } @@ -46,9 +47,9 @@ export interface StreamOptions { * non-streamed items (e.g. the final aggregated text item). * * `response` settles after the native call completes and all queued item callbacks have run. Breaking iteration early - * requests cancellation, but native completion can win that race; in that case `response` resolves normally. Otherwise - * it rejects with the cancellation error (`AbortError` for an aborted signal or `OperationCancelled` for an early - * break). + * requests cancellation of an active native invocation, but native completion can win that race. A request still + * waiting in the native session FIFO is unaffected. When cancellation wins, `response` rejects with `AbortError` for + * an aborted signal or `OperationCancelled` for an early break. */ export interface StreamingResponse extends AsyncIterable { readonly response: Promise; @@ -276,10 +277,11 @@ export abstract class Session { * `Response` (stop reason, usage, aggregate text item, etc.) once the * native call completes. * - * Cancellation: pass `{ signal }`; aborting the signal cancels the native - * request and causes the iterator to throw an `Error` with - * `name === "AbortError"`. Breaking out of the `for await` loop requests cancellation of the underlying request. If - * native completion wins the race, `response` resolves normally; otherwise it rejects with `OperationCancelled`. + * Cancellation: pass `{ signal }`; aborting while the invocation is active cancels the native request and causes the + * iterator to throw an `Error` with `name === "AbortError"`. Aborting while work is only waiting in the native FIFO + * does not prevent execution. A signal already aborted at call time rejects before submission. Breaking out of the + * `for await` loop similarly requests active cancellation. If native completion wins the race, `response` resolves + * normally; otherwise it rejects with `OperationCancelled`. * * Non-cancellation failures throw a `FoundryLocalError`. */ diff --git a/sdk_v2/js/test/items.test.ts b/sdk_v2/js/test/items.test.ts index 6d89c6503..2ffa6f1b0 100644 --- a/sdk_v2/js/test/items.test.ts +++ b/sdk_v2/js/test/items.test.ts @@ -180,7 +180,7 @@ describeIfBuilt("Request round-trip through the native layer", () => { expect(req.itemCount).toBe(3); }); - it("cancel before attachment is accepted", () => { + it("cancel while idle is an accepted no-op", () => { const req = new Request(); expect(() => req.cancel()).not.toThrow(); }); diff --git a/sdk_v2/python/src/foundry_local_sdk/request.py b/sdk_v2/python/src/foundry_local_sdk/request.py index 4e87c336d..c7d8d9a62 100644 --- a/sdk_v2/python/src/foundry_local_sdk/request.py +++ b/sdk_v2/python/src/foundry_local_sdk/request.py @@ -105,9 +105,8 @@ def set_options(self, options: "RequestOptions") -> "Request": def cancel(self) -> None: """Cancel this request. - Cancellation before processing is remembered and prevents the request - from reaching the inference backend. Cancellation after completion has - no effect. + Only an invocation currently being processed is affected. Calling this + while the request is idle or after completion has no effect. """ self._check_open() from foundry_local_sdk._native.api import api From ff5fab4dc4b632ce5cdab5899d1b68dd9dd73796 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 13:16:16 -0500 Subject: [PATCH 07/16] Restore model download cancellation parity Carry the legacy AbortSignal and threading.Event contracts into the v2 bindings so callers can stop native downloads instead of receiving progress-only APIs. Preserve completion races and callback exceptions while mapping signal-driven JavaScript cancellation to AbortError. Files changed: - sdk_v2/js/native/src/model.cc: bridge AbortSignal state into the native progress callback - sdk_v2/js/src and test: restore overloads and verify cancellation semantics - sdk_v2/python/src and test: restore cancel_event and preserve callback errors - sdk_v2/python/README.md: document cooperative download cancellation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d2c76d2-0c86-4672-b91b-3fcb54ed7906 --- sdk_v2/js/native/src/model.cc | 99 ++++++++++++++++--- sdk_v2/js/src/detail/native.ts | 2 +- sdk_v2/js/src/imodel.ts | 5 +- sdk_v2/js/src/model.ts | 49 ++++++++- sdk_v2/js/test/model-download.types.ts | 11 +++ sdk_v2/js/test/model-lifecycle.test.ts | 17 ++++ sdk_v2/js/test/model.test.ts | 23 +++++ sdk_v2/js/tsconfig.types.json | 2 +- sdk_v2/python/README.md | 13 +++ sdk_v2/python/src/foundry_local_sdk/imodel.py | 49 ++++++--- .../unit/test_model_download_cancellation.py | 99 +++++++++++++++++++ 11 files changed, 335 insertions(+), 34 deletions(-) create mode 100644 sdk_v2/js/test/model-download.types.ts create mode 100644 sdk_v2/python/test/unit/test_model_download_cancellation.py diff --git a/sdk_v2/js/native/src/model.cc b/sdk_v2/js/native/src/model.cc index 6d1844c9e..d948d3897 100644 --- a/sdk_v2/js/native/src/model.cc +++ b/sdk_v2/js/native/src/model.cc @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -283,27 +284,41 @@ namespace { class DownloadWorker : public Napi::AsyncWorker { public: DownloadWorker(Napi::Env env, foundry_local::IModel* impl, Napi::ObjectReference owner, - Napi::ThreadSafeFunction tsfn) + Napi::ThreadSafeFunction tsfn, std::shared_ptr> abort_requested, + Napi::ObjectReference abort_signal, Napi::FunctionReference abort_listener) : Napi::AsyncWorker(env), deferred_(Napi::Promise::Deferred::New(env)), impl_(impl), owner_(std::move(owner)), - tsfn_(std::move(tsfn)) {} + tsfn_(std::move(tsfn)), + abort_requested_(std::move(abort_requested)), + abort_signal_(std::move(abort_signal)), + abort_listener_(std::move(abort_listener)) {} Napi::Promise Promise() { return deferred_.Promise(); } void Execute() override { try { - auto progress_cb = tsfn_ ? std::function([this](float percent) { - // BlockingCall keeps backpressure on the worker thread: if JS is - // slow to drain the queue we'll wait rather than dropping reports. - // Callback return value is unused on the JS side; we always continue. - tsfn_.BlockingCall([percent](Napi::Env env, Napi::Function js_cb) { - js_cb.Call({Napi::Number::New(env, static_cast(percent))}); - }); - return 0; // 0 = continue per flProgressCallback contract. + const bool has_cancellation = abort_requested_ != nullptr; + auto progress_cb = (tsfn_ || has_cancellation) ? std::function([this](float percent) { + if (IsAbortRequested()) { + cancelled_by_signal_ = true; + return 1; + } + if (tsfn_) { + // BlockingCall keeps backpressure on the worker thread: if JS is slow to drain the queue we'll wait rather + // than dropping reports. + tsfn_.BlockingCall([percent](Napi::Env env, Napi::Function js_cb) { + js_cb.Call({Napi::Number::New(env, static_cast(percent))}); + }); + } + if (IsAbortRequested()) { + cancelled_by_signal_ = true; + return 1; + } + return 0; }) - : std::function(nullptr); + : std::function(nullptr); impl_->Download(std::move(progress_cb)); } catch (const foundry_local::Error& e) { err_code_ = static_cast(e.Code()); @@ -321,18 +336,20 @@ class DownloadWorker : public Napi::AsyncWorker { void OnOK() override { Napi::HandleScope scope(Env()); - ReleaseTsfn(); + CleanupJsReferences(); deferred_.Resolve(Env().Undefined()); } void OnError(const Napi::Error& /*unused*/) override { Napi::Env env = Env(); Napi::HandleScope scope(env); - ReleaseTsfn(); + CleanupJsReferences(); if (tagged_) { Napi::Error err = Napi::Error::New(env, err_msg_); Napi::Object value = err.Value(); - value.Set("name", Napi::String::New(env, "FoundryLocalError")); + const bool is_signal_cancellation = + cancelled_by_signal_ && err_code_ == FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED; + value.Set("name", Napi::String::New(env, is_signal_cancellation ? "AbortError" : "FoundryLocalError")); value.Set("code", Napi::Number::New(env, err_code_)); deferred_.Reject(value); } else { @@ -341,7 +358,21 @@ class DownloadWorker : public Napi::AsyncWorker { } private: - void ReleaseTsfn() { + bool IsAbortRequested() const { + return abort_requested_ != nullptr && abort_requested_->load(std::memory_order_acquire); + } + + void CleanupJsReferences() { + if (!abort_signal_.IsEmpty() && !abort_listener_.IsEmpty()) { + Napi::Object signal = abort_signal_.Value(); + Napi::Value remove_value = signal.Get("removeEventListener"); + if (remove_value.IsFunction()) { + remove_value.As().Call( + signal, {Napi::String::New(Env(), "abort"), abort_listener_.Value()}); + } + abort_listener_.Reset(); + abort_signal_.Reset(); + } if (tsfn_) { tsfn_.Release(); tsfn_ = Napi::ThreadSafeFunction(); @@ -352,9 +383,13 @@ class DownloadWorker : public Napi::AsyncWorker { foundry_local::IModel* impl_; Napi::ObjectReference owner_; Napi::ThreadSafeFunction tsfn_; + std::shared_ptr> abort_requested_; + Napi::ObjectReference abort_signal_; + Napi::FunctionReference abort_listener_; std::string err_msg_; int err_code_ = 0; bool tagged_ = false; + bool cancelled_by_signal_ = false; }; } // namespace @@ -378,8 +413,40 @@ Napi::Value Model::Download(const Napi::CallbackInfo& info) { return env.Undefined(); } + std::shared_ptr> abort_requested; + Napi::ObjectReference abort_signal; + Napi::FunctionReference abort_listener; + if (info.Length() >= 2 && !info[1].IsUndefined() && !info[1].IsNull()) { + if (!info[1].IsObject()) { + Napi::TypeError::New(env, "Model.download: signal must be an AbortSignal").ThrowAsJavaScriptException(); + return env.Undefined(); + } + Napi::Object signal = info[1].As(); + Napi::Value aborted = signal.Get("aborted"); + Napi::Value add_value = signal.Get("addEventListener"); + Napi::Value remove_value = signal.Get("removeEventListener"); + if (!aborted.IsBoolean() || !add_value.IsFunction() || !remove_value.IsFunction()) { + Napi::TypeError::New(env, "Model.download: signal must be an AbortSignal").ThrowAsJavaScriptException(); + return env.Undefined(); + } + + abort_requested = std::make_shared>(aborted.As().Value()); + Napi::Function listener = Napi::Function::New( + env, [abort_requested](const Napi::CallbackInfo&) { + abort_requested->store(true, std::memory_order_release); + }); + add_value.As().Call( + signal, {Napi::String::New(env, "abort"), listener}); + if (signal.Get("aborted").As().Value()) { + abort_requested->store(true, std::memory_order_release); + } + abort_signal = Napi::Persistent(signal); + abort_listener = Napi::Persistent(listener); + } + Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - auto* w = new DownloadWorker(env, impl_, std::move(owner), std::move(tsfn)); + auto* w = new DownloadWorker(env, impl_, std::move(owner), std::move(tsfn), std::move(abort_requested), + std::move(abort_signal), std::move(abort_listener)); Napi::Promise p = w->Promise(); w->Queue(); return p; diff --git a/sdk_v2/js/src/detail/native.ts b/sdk_v2/js/src/detail/native.ts index 4167acdd3..bd74857ac 100644 --- a/sdk_v2/js/src/detail/native.ts +++ b/sdk_v2/js/src/detail/native.ts @@ -92,7 +92,7 @@ export interface NativeModel { selectVariant(variant: NativeModel): void; load(): Promise; unload(): Promise; - download(progress?: (percent: number) => void): Promise; + download(progress?: (percent: number) => void, signal?: AbortSignal): Promise; removeFromCache(): void; } diff --git a/sdk_v2/js/src/imodel.ts b/sdk_v2/js/src/imodel.ts index fe5449df0..e653df933 100644 --- a/sdk_v2/js/src/imodel.ts +++ b/sdk_v2/js/src/imodel.ts @@ -19,7 +19,10 @@ export interface IModel { get capabilities(): string | null; get supportsToolCalling(): boolean | null; - download(progressCallback?: (progress: number) => void): Promise; + download(): Promise; + download(signal: AbortSignal): Promise; + download(progressCallback: (progress: number) => void, signal?: AbortSignal): Promise; + download(progressCallback: undefined, signal: AbortSignal): Promise; get path(): string; load(): Promise; removeFromCache(): void; diff --git a/sdk_v2/js/src/model.ts b/sdk_v2/js/src/model.ts index 6bb5d52ff..8053395e7 100644 --- a/sdk_v2/js/src/model.ts +++ b/sdk_v2/js/src/model.ts @@ -16,6 +16,22 @@ const internalCtorKey = Symbol("Model.internal"); const nativeByModel = new WeakMap(); +function isAbortSignal(value: unknown): value is AbortSignal { + return ( + typeof value === "object" && + value !== null && + typeof (value as AbortSignal).aborted === "boolean" && + typeof (value as AbortSignal).addEventListener === "function" && + typeof (value as AbortSignal).removeEventListener === "function" + ); +} + +function makeAbortError(message: string): Error { + const error = new Error(message); + error.name = "AbortError"; + return error; +} + function toDeviceType(value: NativeModelInfo["deviceType"]): DeviceType { switch (value) { case "CPU": @@ -24,7 +40,6 @@ function toDeviceType(value: NativeModelInfo["deviceType"]): DeviceType { return DeviceType.GPU; case "NPU": return DeviceType.NPU; - case "Invalid": default: return DeviceType.Invalid; } @@ -157,8 +172,36 @@ export class Model implements IModel { await this.#native.unload(); } - async download(progressCallback?: (progress: number) => void): Promise { - await this.#native.download(progressCallback); + async download(): Promise; + async download(signal: AbortSignal): Promise; + async download(progressCallback: (progress: number) => void, signal?: AbortSignal): Promise; + async download(progressCallback: undefined, signal: AbortSignal): Promise; + async download( + progressCallbackOrSignal?: ((progress: number) => void) | AbortSignal, + signal?: AbortSignal, + ): Promise { + const progressCallback = + typeof progressCallbackOrSignal === "function" ? progressCallbackOrSignal : undefined; + const abortSignal = isAbortSignal(progressCallbackOrSignal) ? progressCallbackOrSignal : signal; + + if ( + progressCallbackOrSignal !== undefined && + typeof progressCallbackOrSignal !== "function" && + !isAbortSignal(progressCallbackOrSignal) + ) { + throw new TypeError("Model.download: first argument must be a progress callback or AbortSignal"); + } + if (signal !== undefined && !isAbortSignal(signal)) { + throw new TypeError("Model.download: second argument must be an AbortSignal"); + } + if (isAbortSignal(progressCallbackOrSignal) && signal !== undefined) { + throw new TypeError("Model.download: signal must not be provided twice"); + } + if (abortSignal?.aborted === true) { + throw makeAbortError("Model download aborted before start"); + } + + await this.#native.download(progressCallback, abortSignal); } removeFromCache(): void { diff --git a/sdk_v2/js/test/model-download.types.ts b/sdk_v2/js/test/model-download.types.ts new file mode 100644 index 000000000..20f01068e --- /dev/null +++ b/sdk_v2/js/test/model-download.types.ts @@ -0,0 +1,11 @@ +import type { IModel } from "../src/imodel.js"; + +declare const model: IModel; +declare const progress: (percent: number) => void; +declare const signal: AbortSignal; + +void model.download(); +void model.download(signal); +void model.download(progress); +void model.download(progress, signal); +void model.download(undefined, signal); diff --git a/sdk_v2/js/test/model-lifecycle.test.ts b/sdk_v2/js/test/model-lifecycle.test.ts index 0596cef3e..69bbfb1d1 100644 --- a/sdk_v2/js/test/model-lifecycle.test.ts +++ b/sdk_v2/js/test/model-lifecycle.test.ts @@ -55,6 +55,23 @@ describe.skipIf(!haveTestModelCache)("Model lifecycle (real model)", () => { 2 * 60_000, ); + it("download() accepts an AbortSignal and preserves completion when a cache hit wins the race", async () => { + const m = fixture?.model; + if (m === undefined) throw new Error("fixture missing"); + const controller = new AbortController(); + + await expect(m.download(() => controller.abort(), controller.signal)).resolves.toBeUndefined(); + }); + + it("download() rejects a pre-aborted AbortSignal before native submission", async () => { + const m = fixture?.model; + if (m === undefined) throw new Error("fixture missing"); + const controller = new AbortController(); + controller.abort(); + + await expect(m.download(controller.signal)).rejects.toMatchObject({ name: "AbortError" }); + }); + it("calling load() on an already-loaded model is idempotent (or surfaces a clear error)", async () => { const m = fixture?.model; if (m === undefined) throw new Error("fixture missing"); diff --git a/sdk_v2/js/test/model.test.ts b/sdk_v2/js/test/model.test.ts index 07bc32b2d..a930b2019 100644 --- a/sdk_v2/js/test/model.test.ts +++ b/sdk_v2/js/test/model.test.ts @@ -4,6 +4,7 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; import type { Catalog } from "../src/catalog.js"; +import { FlErrorCode } from "../src/detail/errors.js"; import { Model } from "../src/model.js"; import { @@ -88,6 +89,28 @@ describeIfBuilt("Model (cache-only)", () => { expect(typeof model.path).toBe("string"); }); + it("download() maps AbortSignal cancellation from a native progress checkpoint to AbortError", async () => { + const controller = new AbortController(); + + await expect( + model.download((progress) => { + if (progress === 0) controller.abort(); + }, controller.signal), + ).rejects.toMatchObject({ + name: "AbortError", + code: FlErrorCode.OperationCancelled, + }); + expect(model.isCached).toBe(false); + }); + + it("download() rejects a pre-aborted AbortSignal before native submission", async () => { + const controller = new AbortController(); + controller.abort(); + + await expect(model.download(controller.signal)).rejects.toMatchObject({ name: "AbortError" }); + expect(model.isCached).toBe(false); + }); + it("id and alias match info", () => { expect(model.id).toBe(model.info.id); expect(model.alias).toBe(model.info.alias); diff --git a/sdk_v2/js/tsconfig.types.json b/sdk_v2/js/tsconfig.types.json index d78e2d054..e5c1de9f5 100644 --- a/sdk_v2/js/tsconfig.types.json +++ b/sdk_v2/js/tsconfig.types.json @@ -5,5 +5,5 @@ "moduleResolution": "NodeNext", "noEmit": true }, - "include": ["src/**/*", "test/tool-definition.types.ts"] + "include": ["src/**/*", "test/model-download.types.ts", "test/tool-definition.types.ts"] } diff --git a/sdk_v2/python/README.md b/sdk_v2/python/README.md index 69990b531..e4bcdc6cf 100644 --- a/sdk_v2/python/README.md +++ b/sdk_v2/python/README.md @@ -110,6 +110,19 @@ with ChatSession(model) as session: model.unload() ``` +Pass a `threading.Event` as `cancel_event` to cancel an active download at the next native progress checkpoint: + +```python +from threading import Event + +cancel_event = Event() + +def on_progress(percent: float) -> None: + print(f"\rDownloading: {percent:.1f}%", end="", flush=True) + +model.download(progress_callback=on_progress, cancel_event=cancel_event) +``` + Runnable end-to-end examples live under [`samples/python/`](https://github.com/microsoft/Foundry-Local/tree/main/samples/python). ## Usage diff --git a/sdk_v2/python/src/foundry_local_sdk/imodel.py b/sdk_v2/python/src/foundry_local_sdk/imodel.py index 5b9043d2a..2052df475 100644 --- a/sdk_v2/python/src/foundry_local_sdk/imodel.py +++ b/sdk_v2/python/src/foundry_local_sdk/imodel.py @@ -5,6 +5,7 @@ from __future__ import annotations from abc import ABC, abstractmethod +from threading import Event from typing import TYPE_CHECKING, Callable from typing_extensions import deprecated @@ -78,12 +79,17 @@ def supports_tool_calling(self) -> bool | None: """Whether the model supports tool/function calling, or ``None`` if unknown.""" @abstractmethod - def download(self, progress_callback: Callable[[float], None] | None = None) -> None: + def download( + self, + progress_callback: Callable[[float], None] | None = None, + cancel_event: Event | None = None, + ) -> None: """Download the model to the local cache if not already present. Args: progress_callback: Optional callback receiving download progress as a percentage (0.0–100.0). + cancel_event: Optional event that cancels the download when set. """ @abstractmethod @@ -326,29 +332,48 @@ def supports_tool_calling(self) -> bool | None: # Model lifecycle # ------------------------------------------------------------------ - def download(self, progress_callback: Callable[[float], None] | None = None) -> None: + def download( + self, + progress_callback: Callable[[float], None] | None = None, + cancel_event: Event | None = None, + ) -> None: from foundry_local_sdk._native.api import api, ffi cb = ffi.NULL user_data = ffi.NULL + callback_error: BaseException | None = None - if progress_callback is not None: - self._progress_cb_handle = ffi.new_handle(progress_callback) + if progress_callback is not None or cancel_event is not None: + callback_state = (progress_callback, cancel_event) + progress_cb_handle = ffi.new_handle(callback_state) - @ffi.callback("flProgressCallback") - def _cb(value: float, ud: object) -> int: + def _progress_callback(value: float, ud: object) -> int: + nonlocal callback_error try: - fn = ffi.from_handle(ud) - fn(float(value)) + fn, event = ffi.from_handle(ud) + if event is not None and event.is_set(): + return 1 + if fn is not None: + fn(float(value)) + if event is not None and event.is_set(): + return 1 return 0 - except Exception: + except BaseException as exc: + callback_error = exc return 1 - self._progress_cb = _cb # keep alive + _cb = ffi.callback("flProgressCallback")(_progress_callback) cb = _cb - user_data = self._progress_cb_handle + user_data = progress_cb_handle - api.check_status(api.model.Download(self._ptr, cb, user_data)) + try: + api.check_status(api.model.Download(self._ptr, cb, user_data)) + except FoundryLocalException: + if callback_error is not None: + raise callback_error + raise + if callback_error is not None: + raise callback_error def get_path(self) -> str: from foundry_local_sdk._native.api import api, ffi diff --git a/sdk_v2/python/test/unit/test_model_download_cancellation.py b/sdk_v2/python/test/unit/test_model_download_cancellation.py new file mode 100644 index 000000000..5d3053788 --- /dev/null +++ b/sdk_v2/python/test/unit/test_model_download_cancellation.py @@ -0,0 +1,99 @@ +from __future__ import annotations + +import sys +from threading import Event +from types import SimpleNamespace + +import pytest + +from foundry_local_sdk.exception import FoundryLocalException +from foundry_local_sdk.imodel import _ModelImpl + + +class FakeFfi: + NULL = None + + @staticmethod + def new_handle(value): + return value + + @staticmethod + def from_handle(value): + return value + + @staticmethod + def callback(_signature): + return lambda fn: fn + + +def make_model(monkeypatch, invoke_callback): + def download(_ptr, callback, user_data): + return invoke_callback(callback, user_data) + + def check_status(status): + if status is not None: + raise FoundryLocalException("download cancelled", error_code=5) + + fake_api = SimpleNamespace(model=SimpleNamespace(Download=download), check_status=check_status) + monkeypatch.setitem( + sys.modules, + "foundry_local_sdk._native.api", + SimpleNamespace(api=fake_api, ffi=FakeFfi()), + ) + model = _ModelImpl.__new__(_ModelImpl) + model._ptr = object() + return model + + +def test_download_cancel_event_returns_nonzero(monkeypatch): + cancel_event = Event() + cancel_event.set() + + def invoke(callback, user_data): + assert callback(25.0, user_data) == 1 + return object() + + model = make_model(monkeypatch, invoke) + + with pytest.raises(FoundryLocalException, match="download cancelled") as exc: + model.download(cancel_event=cancel_event) + + assert exc.value.error_code == 5 + + +def test_download_cancel_event_is_checked_after_progress(monkeypatch): + cancel_event = Event() + progress: list[float] = [] + + def on_progress(value: float) -> None: + progress.append(value) + cancel_event.set() + + def invoke(callback, user_data): + assert callback(50.0, user_data) == 1 + return object() + + model = make_model(monkeypatch, invoke) + + with pytest.raises(FoundryLocalException, match="download cancelled"): + model.download(on_progress, cancel_event) + + assert progress == [50.0] + + +def test_download_preserves_progress_callback_exception(monkeypatch): + expected = RuntimeError("progress failed") + + def on_progress(_value: float) -> None: + raise expected + + def invoke(callback, user_data): + assert callback(10.0, user_data) == 1 + return object() + + model = make_model(monkeypatch, invoke) + + with pytest.raises(RuntimeError, match="progress failed") as exc: + model.download(on_progress) + + assert exc.value is expected From b662da914badcbfcc975d705db1668b1e3ec28c9 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 14:51:14 -0500 Subject: [PATCH 08/16] Harden cancellation cleanup after review Prevent lifecycle and callback races that could strand requests or queued items after exceptional exits. Restore TypeScript source compatibility and make the Python cancellation example functional. Files changed: - sdk_v2/cpp session lifecycle, callback handler, and focused tests - sdk_v2/js download overloads and type tests - sdk_v2/python cancellation documentation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d2c76d2-0c86-4672-b91b-3fcb54ed7906 --- .../inferencing/session/callback_handler.h | 22 +++++--- sdk_v2/cpp/src/inferencing/session/session.cc | 56 +++++++++---------- .../internal_api/callback_handler_test.cc | 1 + .../test/internal_api/session_manager_test.cc | 42 ++++++++++++++ sdk_v2/js/src/imodel.ts | 6 +- sdk_v2/js/src/model.ts | 9 +-- sdk_v2/js/test/model-download.types.ts | 7 +++ sdk_v2/python/README.md | 1 + 8 files changed, 97 insertions(+), 47 deletions(-) diff --git a/sdk_v2/cpp/src/inferencing/session/callback_handler.h b/sdk_v2/cpp/src/inferencing/session/callback_handler.h index 9396d1c0b..8ca47634b 100644 --- a/sdk_v2/cpp/src/inferencing/session/callback_handler.h +++ b/sdk_v2/cpp/src/inferencing/session/callback_handler.h @@ -60,7 +60,8 @@ struct CallbackHandler { /// Push an item into the queue and wake the worker. /// Called from the generator thread — returns immediately. void PushItem(std::unique_ptr item) { - if (request_.IsCancellationRequested()) { + std::lock_guard lock(callback_mutex_); + if (disabled_after_exception_ || request_.IsCancellationRequested()) { return; } @@ -95,8 +96,7 @@ struct CallbackHandler { // The callback pops from the queue — that is the established contract. while (queue_->Size() > 0) { if (request_.IsCancellationRequested() && queue_->Size() > kMaxCancellationDrainItems) { - DropPendingItems(); - SetCallbackInProgress(false); + DropPendingItemsAndNotify(false); break; } @@ -111,13 +111,11 @@ struct CallbackHandler { fmt::format("streaming callback threw an exception; cancelling request: {}", e.what())); DisableAfterException(e.what()); - SetCallbackInProgress(false); return; } catch (...) { logger_.Log(LogLevel::Warning, "streaming callback threw a non-std exception; cancelling request"); DisableAfterException("non-standard exception"); - SetCallbackInProgress(false); return; } @@ -136,12 +134,19 @@ struct CallbackHandler { /// and drops any items still queued so the destructor can join cleanly. void DisableAfterException(std::string_view detail) { request_.CancelFromStreamingCallbackException(detail); - DropPendingItems(); + DropPendingItemsAndNotify(true); } - void DropPendingItems() { - while (queue_->TryPop()) { + void DropPendingItemsAndNotify(bool disable_after_exception) { + { + std::lock_guard lock(callback_mutex_); + disabled_after_exception_ = disabled_after_exception_ || disable_after_exception; + while (queue_->TryPop()) { + } + callback_in_progress_ = false; } + + callback_cv_.notify_all(); } void SetCallbackInProgress(bool value) { @@ -165,6 +170,7 @@ struct CallbackHandler { std::mutex callback_mutex_; std::condition_variable callback_cv_; bool callback_in_progress_ = false; + bool disabled_after_exception_ = false; std::thread worker_; }; diff --git a/sdk_v2/cpp/src/inferencing/session/session.cc b/sdk_v2/cpp/src/inferencing/session/session.cc index 04e0f718a..1a6cf1d5b 100644 --- a/sdk_v2/cpp/src/inferencing/session/session.cc +++ b/sdk_v2/cpp/src/inferencing/session/session.cc @@ -13,6 +13,7 @@ #include "model.h" #include "telemetry/telemetry.h" #include "telemetry/telemetry_action_tracker.h" +#include "util/scope_guard.h" #include "utils.h" #include @@ -168,11 +169,30 @@ void Session::ProcessRequest(const Request& request, Response& response) { lock.lock(); } + bool admitted = false; + bool registered = false; + auto finish_lifecycle = [&]() noexcept { + if (!admitted) { + return; + } + + if (registered) { + std::lock_guard active_lock(*active_requests_mutex_); + active_requests_.erase(&request); + registered = false; + } + + request.PublishCompletion(); + admitted = false; + }; + ScopeGuard lifecycle_guard([&]() noexcept { finish_lifecycle(); }); + { std::lock_guard active_lock(*active_requests_mutex_); if (!request.TryBegin()) { FL_THROW(FOUNDRY_LOCAL_ERROR_INVALID_USAGE, "request is already being processed"); } + admitted = true; // A late shutdown admission must first claim the request, then be canceled under the same lock that protects // active registration. This keeps TryBegin as the sole admission gate while preventing backend entry. @@ -181,31 +201,9 @@ void Session::ProcessRequest(const Request& request, Response& response) { } active_requests_.insert(&request); + registered = true; } - // RAII: deregister the request even if ProcessRequestImpl throws, so Cancel() never - // dereferences a dangling Request after this call unwinds. - struct ActiveRequestGuard { - Session& session; - const Request& request; - - void Deregister() { - if (!registered) { - return; - } - - std::lock_guard active_lock(*session.active_requests_mutex_); - session.active_requests_.erase(&request); - registered = false; - } - - ~ActiveRequestGuard() { - Deregister(); - } - - bool registered = true; - } active_guard{*this, request}; - ActionTracker tracker(Action::kSessionProcessRequest, telemetry_); tracker.SetModelId(CatalogModel().Id()); @@ -224,13 +222,13 @@ void Session::ProcessRequest(const Request& request, Response& response) { } response = std::move(staged_response); - active_guard.Deregister(); - request.PublishCompletion(); + finish_lifecycle(); + lifecycle_guard.Dismiss(); tracker.SetStatus(ActionStatus::kSuccess); } catch (const std::exception& ex) { if (request.TryComplete()) { - active_guard.Deregister(); - request.PublishCompletion(); + finish_lifecycle(); + lifecycle_guard.Dismiss(); tracker.RecordException(ex); throw; } @@ -238,8 +236,8 @@ void Session::ProcessRequest(const Request& request, Response& response) { try { ThrowCancellation(request); } catch (const std::exception& cancellation) { - active_guard.Deregister(); - request.PublishCompletion(); + finish_lifecycle(); + lifecycle_guard.Dismiss(); tracker.RecordException(cancellation); throw; } diff --git a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc index 0e6c4e0da..233e15fd9 100644 --- a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc +++ b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc @@ -93,6 +93,7 @@ TEST(CallbackHandlerTest, FurtherPushesAfterExceptionAreNoOps) { handler.PushItem(std::make_unique("second")); handler.PushItem(std::make_unique("third")); + handler.DrainPending(); handler.Drain(); // Worker exited after the throw — no further callback invocations. diff --git a/sdk_v2/cpp/test/internal_api/session_manager_test.cc b/sdk_v2/cpp/test/internal_api/session_manager_test.cc index c33e40de4..13b4c9e8f 100644 --- a/sdk_v2/cpp/test/internal_api/session_manager_test.cc +++ b/sdk_v2/cpp/test/internal_api/session_manager_test.cc @@ -530,6 +530,28 @@ class ThrowThenCompleteSession : public Session { size_t process_count_ = 0; }; +class NonStdThrowThenCompleteSession : public Session { + public: + NonStdThrowThenCompleteSession(const Model& model, ILogger& logger, ITelemetry& telemetry) + : Session(model, logger, telemetry) {} + + SessionType Type() const override { return SessionType::kChat; } + size_t ProcessCount() const { return process_count_; } + + protected: + void ProcessRequestImpl(const Request& /*request*/, Response& response) override { + ++process_count_; + if (process_count_ == 1) { + throw 42; + } + + response.finish_reason = FOUNDRY_LOCAL_FINISH_STOP; + } + + private: + size_t process_count_ = 0; +}; + flErrorCode ProcessAndGetCode(Session& session, const Request& request) { try { Response response; @@ -730,6 +752,26 @@ TEST(SessionRequestLifecycleTest, OrdinaryExceptionAllowsRequestReuse) { EXPECT_EQ(session.ProcessCount(), 2u); } +TEST(SessionRequestLifecycleTest, NonStdExceptionAllowsRequestReuse) { + fl::test::FakeServiceBindings svc; + Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + TelemetryLogger telemetry{"test", fl::test::NullLog()}; + NonStdThrowThenCompleteSession session(catalog_model, fl::test::NullLog(), telemetry); + Request request; + + try { + Response response; + session.ProcessRequest(request, response); + FAIL() << "expected non-standard exception"; + } catch (int value) { + EXPECT_EQ(value, 42); + } + EXPECT_TRUE(request.IsCompleted()); + + EXPECT_EQ(ProcessAndGetCode(session, request), FOUNDRY_LOCAL_OK); + EXPECT_EQ(session.ProcessCount(), 2u); +} + TEST(SessionRequestLifecycleTest, CallbackExceptionSurfacesOriginalCauseAndAllowsReuse) { fl::test::FakeServiceBindings svc; Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); diff --git a/sdk_v2/js/src/imodel.ts b/sdk_v2/js/src/imodel.ts index e653df933..4af8fc076 100644 --- a/sdk_v2/js/src/imodel.ts +++ b/sdk_v2/js/src/imodel.ts @@ -19,10 +19,8 @@ export interface IModel { get capabilities(): string | null; get supportsToolCalling(): boolean | null; - download(): Promise; - download(signal: AbortSignal): Promise; - download(progressCallback: (progress: number) => void, signal?: AbortSignal): Promise; - download(progressCallback: undefined, signal: AbortSignal): Promise; + download(signal?: AbortSignal): Promise; + download(progressCallback: ((progress: number) => void) | undefined, signal?: AbortSignal): Promise; get path(): string; load(): Promise; removeFromCache(): void; diff --git a/sdk_v2/js/src/model.ts b/sdk_v2/js/src/model.ts index 8053395e7..f7e0b06e7 100644 --- a/sdk_v2/js/src/model.ts +++ b/sdk_v2/js/src/model.ts @@ -172,16 +172,13 @@ export class Model implements IModel { await this.#native.unload(); } - async download(): Promise; - async download(signal: AbortSignal): Promise; - async download(progressCallback: (progress: number) => void, signal?: AbortSignal): Promise; - async download(progressCallback: undefined, signal: AbortSignal): Promise; + async download(signal?: AbortSignal): Promise; + async download(progressCallback: ((progress: number) => void) | undefined, signal?: AbortSignal): Promise; async download( progressCallbackOrSignal?: ((progress: number) => void) | AbortSignal, signal?: AbortSignal, ): Promise { - const progressCallback = - typeof progressCallbackOrSignal === "function" ? progressCallbackOrSignal : undefined; + const progressCallback = typeof progressCallbackOrSignal === "function" ? progressCallbackOrSignal : undefined; const abortSignal = isAbortSignal(progressCallbackOrSignal) ? progressCallbackOrSignal : signal; if ( diff --git a/sdk_v2/js/test/model-download.types.ts b/sdk_v2/js/test/model-download.types.ts index 20f01068e..1bfc07608 100644 --- a/sdk_v2/js/test/model-download.types.ts +++ b/sdk_v2/js/test/model-download.types.ts @@ -2,10 +2,17 @@ import type { IModel } from "../src/imodel.js"; declare const model: IModel; declare const progress: (percent: number) => void; +declare const maybeProgress: ((percent: number) => void) | undefined; declare const signal: AbortSignal; +declare const secondSignal: AbortSignal; void model.download(); +void model.download(undefined); void model.download(signal); void model.download(progress); +void model.download(maybeProgress); void model.download(progress, signal); void model.download(undefined, signal); + +// @ts-expect-error A second signal is only valid when the first argument is a progress callback. +void model.download(signal, secondSignal); diff --git a/sdk_v2/python/README.md b/sdk_v2/python/README.md index e4bcdc6cf..3f184ec80 100644 --- a/sdk_v2/python/README.md +++ b/sdk_v2/python/README.md @@ -119,6 +119,7 @@ cancel_event = Event() def on_progress(percent: float) -> None: print(f"\rDownloading: {percent:.1f}%", end="", flush=True) + cancel_event.set() model.download(progress_callback=on_progress, cancel_event=cancel_event) ``` From f8545808eb86fdd609777583d45e8eff051938a4 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 15:51:43 -0500 Subject: [PATCH 09/16] Synchronize streaming audio cancellation test Wait for the backend to consume queued audio before cancelling so the test exercises active-request cancellation instead of racing the documented idle no-op behavior. 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 --- sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc b/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc index 4330d950d..fa1e5cd8a 100644 --- a/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc +++ b/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc @@ -9,12 +9,14 @@ #include "utils/string_utils.h" #include +#include #include #include #include #include #include #include +#include #include using fl::test::ToLower; @@ -202,6 +204,7 @@ TEST_F(StreamingAudioFixture, CancellationMidStream) { auto pcm = LoadPcm(); auto chunks = SplitIntoChunks(pcm, 3200); + ASSERT_GT(chunks.size(), 1u); auto audio = Item::AudioFromData("pcm", nullptr, 0, /*sample_rate=*/16000, /*channels=*/1); ItemQueue queue; @@ -221,6 +224,20 @@ TEST_F(StreamingAudioFixture, CancellationMidStream) { queue.Push(Item::Bytes(FOUNDRY_LOCAL_ITEM_BYTES, chunks[i].data(), chunks[i].size())); } + const auto admission_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); + while (queue.Size() == half && std::chrono::steady_clock::now() < admission_deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + + if (queue.Size() == half) { + queue.MarkFinished(); + future.wait(); + FAIL() << "Audio session did not consume input before the cancellation deadline"; + } + + ASSERT_EQ(future.wait_for(std::chrono::milliseconds(0)), std::future_status::timeout) + << "Audio session completed before cancellation"; + request.Cancel(); queue.MarkFinished(); From 63bb3b1497317a2204a895591b74ba4c10b9369d Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 17:37:24 -0500 Subject: [PATCH 10/16] Document cancellation reason fallback Clarify why Cancel defensively maps a missing or unknown reason to caller cancellation instead of leaving active work running. Files changed: - sdk_v2/cpp/src/inferencing/session/request.h Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d2c76d2-0c86-4672-b91b-3fcb54ed7906 --- sdk_v2/cpp/src/inferencing/session/request.h | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk_v2/cpp/src/inferencing/session/request.h b/sdk_v2/cpp/src/inferencing/session/request.h index 5619c6288..c1011e370 100644 --- a/sdk_v2/cpp/src/inferencing/session/request.h +++ b/sdk_v2/cpp/src/inferencing/session/request.h @@ -213,6 +213,7 @@ struct Request { case CancellationReason::SessionShutdown: return State::CanceledBySessionShutdown; case CancellationReason::None: + // Cancel() always means cancellation; defensively classify a missing or unknown reason as caller-initiated. case CancellationReason::Caller: default: return State::CanceledByCaller; From 8b2c9f0320dc51b4a520cd12dafb85a36775776b Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 18:15:39 -0500 Subject: [PATCH 11/16] Separate model download cancellation changes 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 --- sdk_v2/js/native/src/model.cc | 99 +++---------------- sdk_v2/js/src/detail/native.ts | 2 +- sdk_v2/js/src/imodel.ts | 3 +- sdk_v2/js/src/model.ts | 46 +-------- sdk_v2/js/test/model-download.types.ts | 18 ---- sdk_v2/js/test/model-lifecycle.test.ts | 17 ---- sdk_v2/js/test/model.test.ts | 23 ----- sdk_v2/js/test/streaming.test.ts | 13 --- sdk_v2/js/tsconfig.types.json | 2 +- sdk_v2/python/README.md | 14 --- sdk_v2/python/src/foundry_local_sdk/imodel.py | 49 +++------ .../unit/test_model_download_cancellation.py | 99 ------------------- 12 files changed, 34 insertions(+), 351 deletions(-) delete mode 100644 sdk_v2/js/test/model-download.types.ts delete mode 100644 sdk_v2/python/test/unit/test_model_download_cancellation.py diff --git a/sdk_v2/js/native/src/model.cc b/sdk_v2/js/native/src/model.cc index d948d3897..6d1844c9e 100644 --- a/sdk_v2/js/native/src/model.cc +++ b/sdk_v2/js/native/src/model.cc @@ -9,7 +9,6 @@ #include #include -#include #include #include #include @@ -284,41 +283,27 @@ namespace { class DownloadWorker : public Napi::AsyncWorker { public: DownloadWorker(Napi::Env env, foundry_local::IModel* impl, Napi::ObjectReference owner, - Napi::ThreadSafeFunction tsfn, std::shared_ptr> abort_requested, - Napi::ObjectReference abort_signal, Napi::FunctionReference abort_listener) + Napi::ThreadSafeFunction tsfn) : Napi::AsyncWorker(env), deferred_(Napi::Promise::Deferred::New(env)), impl_(impl), owner_(std::move(owner)), - tsfn_(std::move(tsfn)), - abort_requested_(std::move(abort_requested)), - abort_signal_(std::move(abort_signal)), - abort_listener_(std::move(abort_listener)) {} + tsfn_(std::move(tsfn)) {} Napi::Promise Promise() { return deferred_.Promise(); } void Execute() override { try { - const bool has_cancellation = abort_requested_ != nullptr; - auto progress_cb = (tsfn_ || has_cancellation) ? std::function([this](float percent) { - if (IsAbortRequested()) { - cancelled_by_signal_ = true; - return 1; - } - if (tsfn_) { - // BlockingCall keeps backpressure on the worker thread: if JS is slow to drain the queue we'll wait rather - // than dropping reports. - tsfn_.BlockingCall([percent](Napi::Env env, Napi::Function js_cb) { - js_cb.Call({Napi::Number::New(env, static_cast(percent))}); - }); - } - if (IsAbortRequested()) { - cancelled_by_signal_ = true; - return 1; - } - return 0; + auto progress_cb = tsfn_ ? std::function([this](float percent) { + // BlockingCall keeps backpressure on the worker thread: if JS is + // slow to drain the queue we'll wait rather than dropping reports. + // Callback return value is unused on the JS side; we always continue. + tsfn_.BlockingCall([percent](Napi::Env env, Napi::Function js_cb) { + js_cb.Call({Napi::Number::New(env, static_cast(percent))}); + }); + return 0; // 0 = continue per flProgressCallback contract. }) - : std::function(nullptr); + : std::function(nullptr); impl_->Download(std::move(progress_cb)); } catch (const foundry_local::Error& e) { err_code_ = static_cast(e.Code()); @@ -336,20 +321,18 @@ class DownloadWorker : public Napi::AsyncWorker { void OnOK() override { Napi::HandleScope scope(Env()); - CleanupJsReferences(); + ReleaseTsfn(); deferred_.Resolve(Env().Undefined()); } void OnError(const Napi::Error& /*unused*/) override { Napi::Env env = Env(); Napi::HandleScope scope(env); - CleanupJsReferences(); + ReleaseTsfn(); if (tagged_) { Napi::Error err = Napi::Error::New(env, err_msg_); Napi::Object value = err.Value(); - const bool is_signal_cancellation = - cancelled_by_signal_ && err_code_ == FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED; - value.Set("name", Napi::String::New(env, is_signal_cancellation ? "AbortError" : "FoundryLocalError")); + value.Set("name", Napi::String::New(env, "FoundryLocalError")); value.Set("code", Napi::Number::New(env, err_code_)); deferred_.Reject(value); } else { @@ -358,21 +341,7 @@ class DownloadWorker : public Napi::AsyncWorker { } private: - bool IsAbortRequested() const { - return abort_requested_ != nullptr && abort_requested_->load(std::memory_order_acquire); - } - - void CleanupJsReferences() { - if (!abort_signal_.IsEmpty() && !abort_listener_.IsEmpty()) { - Napi::Object signal = abort_signal_.Value(); - Napi::Value remove_value = signal.Get("removeEventListener"); - if (remove_value.IsFunction()) { - remove_value.As().Call( - signal, {Napi::String::New(Env(), "abort"), abort_listener_.Value()}); - } - abort_listener_.Reset(); - abort_signal_.Reset(); - } + void ReleaseTsfn() { if (tsfn_) { tsfn_.Release(); tsfn_ = Napi::ThreadSafeFunction(); @@ -383,13 +352,9 @@ class DownloadWorker : public Napi::AsyncWorker { foundry_local::IModel* impl_; Napi::ObjectReference owner_; Napi::ThreadSafeFunction tsfn_; - std::shared_ptr> abort_requested_; - Napi::ObjectReference abort_signal_; - Napi::FunctionReference abort_listener_; std::string err_msg_; int err_code_ = 0; bool tagged_ = false; - bool cancelled_by_signal_ = false; }; } // namespace @@ -413,40 +378,8 @@ Napi::Value Model::Download(const Napi::CallbackInfo& info) { return env.Undefined(); } - std::shared_ptr> abort_requested; - Napi::ObjectReference abort_signal; - Napi::FunctionReference abort_listener; - if (info.Length() >= 2 && !info[1].IsUndefined() && !info[1].IsNull()) { - if (!info[1].IsObject()) { - Napi::TypeError::New(env, "Model.download: signal must be an AbortSignal").ThrowAsJavaScriptException(); - return env.Undefined(); - } - Napi::Object signal = info[1].As(); - Napi::Value aborted = signal.Get("aborted"); - Napi::Value add_value = signal.Get("addEventListener"); - Napi::Value remove_value = signal.Get("removeEventListener"); - if (!aborted.IsBoolean() || !add_value.IsFunction() || !remove_value.IsFunction()) { - Napi::TypeError::New(env, "Model.download: signal must be an AbortSignal").ThrowAsJavaScriptException(); - return env.Undefined(); - } - - abort_requested = std::make_shared>(aborted.As().Value()); - Napi::Function listener = Napi::Function::New( - env, [abort_requested](const Napi::CallbackInfo&) { - abort_requested->store(true, std::memory_order_release); - }); - add_value.As().Call( - signal, {Napi::String::New(env, "abort"), listener}); - if (signal.Get("aborted").As().Value()) { - abort_requested->store(true, std::memory_order_release); - } - abort_signal = Napi::Persistent(signal); - abort_listener = Napi::Persistent(listener); - } - Napi::ObjectReference owner = Napi::Reference::New(manager_.Value(), 1); - auto* w = new DownloadWorker(env, impl_, std::move(owner), std::move(tsfn), std::move(abort_requested), - std::move(abort_signal), std::move(abort_listener)); + auto* w = new DownloadWorker(env, impl_, std::move(owner), std::move(tsfn)); Napi::Promise p = w->Promise(); w->Queue(); return p; diff --git a/sdk_v2/js/src/detail/native.ts b/sdk_v2/js/src/detail/native.ts index bd74857ac..4167acdd3 100644 --- a/sdk_v2/js/src/detail/native.ts +++ b/sdk_v2/js/src/detail/native.ts @@ -92,7 +92,7 @@ export interface NativeModel { selectVariant(variant: NativeModel): void; load(): Promise; unload(): Promise; - download(progress?: (percent: number) => void, signal?: AbortSignal): Promise; + download(progress?: (percent: number) => void): Promise; removeFromCache(): void; } diff --git a/sdk_v2/js/src/imodel.ts b/sdk_v2/js/src/imodel.ts index 4af8fc076..fe5449df0 100644 --- a/sdk_v2/js/src/imodel.ts +++ b/sdk_v2/js/src/imodel.ts @@ -19,8 +19,7 @@ export interface IModel { get capabilities(): string | null; get supportsToolCalling(): boolean | null; - download(signal?: AbortSignal): Promise; - download(progressCallback: ((progress: number) => void) | undefined, signal?: AbortSignal): Promise; + download(progressCallback?: (progress: number) => void): Promise; get path(): string; load(): Promise; removeFromCache(): void; diff --git a/sdk_v2/js/src/model.ts b/sdk_v2/js/src/model.ts index f7e0b06e7..6bb5d52ff 100644 --- a/sdk_v2/js/src/model.ts +++ b/sdk_v2/js/src/model.ts @@ -16,22 +16,6 @@ const internalCtorKey = Symbol("Model.internal"); const nativeByModel = new WeakMap(); -function isAbortSignal(value: unknown): value is AbortSignal { - return ( - typeof value === "object" && - value !== null && - typeof (value as AbortSignal).aborted === "boolean" && - typeof (value as AbortSignal).addEventListener === "function" && - typeof (value as AbortSignal).removeEventListener === "function" - ); -} - -function makeAbortError(message: string): Error { - const error = new Error(message); - error.name = "AbortError"; - return error; -} - function toDeviceType(value: NativeModelInfo["deviceType"]): DeviceType { switch (value) { case "CPU": @@ -40,6 +24,7 @@ function toDeviceType(value: NativeModelInfo["deviceType"]): DeviceType { return DeviceType.GPU; case "NPU": return DeviceType.NPU; + case "Invalid": default: return DeviceType.Invalid; } @@ -172,33 +157,8 @@ export class Model implements IModel { await this.#native.unload(); } - async download(signal?: AbortSignal): Promise; - async download(progressCallback: ((progress: number) => void) | undefined, signal?: AbortSignal): Promise; - async download( - progressCallbackOrSignal?: ((progress: number) => void) | AbortSignal, - signal?: AbortSignal, - ): Promise { - const progressCallback = typeof progressCallbackOrSignal === "function" ? progressCallbackOrSignal : undefined; - const abortSignal = isAbortSignal(progressCallbackOrSignal) ? progressCallbackOrSignal : signal; - - if ( - progressCallbackOrSignal !== undefined && - typeof progressCallbackOrSignal !== "function" && - !isAbortSignal(progressCallbackOrSignal) - ) { - throw new TypeError("Model.download: first argument must be a progress callback or AbortSignal"); - } - if (signal !== undefined && !isAbortSignal(signal)) { - throw new TypeError("Model.download: second argument must be an AbortSignal"); - } - if (isAbortSignal(progressCallbackOrSignal) && signal !== undefined) { - throw new TypeError("Model.download: signal must not be provided twice"); - } - if (abortSignal?.aborted === true) { - throw makeAbortError("Model download aborted before start"); - } - - await this.#native.download(progressCallback, abortSignal); + async download(progressCallback?: (progress: number) => void): Promise { + await this.#native.download(progressCallback); } removeFromCache(): void { diff --git a/sdk_v2/js/test/model-download.types.ts b/sdk_v2/js/test/model-download.types.ts deleted file mode 100644 index 1bfc07608..000000000 --- a/sdk_v2/js/test/model-download.types.ts +++ /dev/null @@ -1,18 +0,0 @@ -import type { IModel } from "../src/imodel.js"; - -declare const model: IModel; -declare const progress: (percent: number) => void; -declare const maybeProgress: ((percent: number) => void) | undefined; -declare const signal: AbortSignal; -declare const secondSignal: AbortSignal; - -void model.download(); -void model.download(undefined); -void model.download(signal); -void model.download(progress); -void model.download(maybeProgress); -void model.download(progress, signal); -void model.download(undefined, signal); - -// @ts-expect-error A second signal is only valid when the first argument is a progress callback. -void model.download(signal, secondSignal); diff --git a/sdk_v2/js/test/model-lifecycle.test.ts b/sdk_v2/js/test/model-lifecycle.test.ts index 69bbfb1d1..0596cef3e 100644 --- a/sdk_v2/js/test/model-lifecycle.test.ts +++ b/sdk_v2/js/test/model-lifecycle.test.ts @@ -55,23 +55,6 @@ describe.skipIf(!haveTestModelCache)("Model lifecycle (real model)", () => { 2 * 60_000, ); - it("download() accepts an AbortSignal and preserves completion when a cache hit wins the race", async () => { - const m = fixture?.model; - if (m === undefined) throw new Error("fixture missing"); - const controller = new AbortController(); - - await expect(m.download(() => controller.abort(), controller.signal)).resolves.toBeUndefined(); - }); - - it("download() rejects a pre-aborted AbortSignal before native submission", async () => { - const m = fixture?.model; - if (m === undefined) throw new Error("fixture missing"); - const controller = new AbortController(); - controller.abort(); - - await expect(m.download(controller.signal)).rejects.toMatchObject({ name: "AbortError" }); - }); - it("calling load() on an already-loaded model is idempotent (or surfaces a clear error)", async () => { const m = fixture?.model; if (m === undefined) throw new Error("fixture missing"); diff --git a/sdk_v2/js/test/model.test.ts b/sdk_v2/js/test/model.test.ts index a930b2019..07bc32b2d 100644 --- a/sdk_v2/js/test/model.test.ts +++ b/sdk_v2/js/test/model.test.ts @@ -4,7 +4,6 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; import type { Catalog } from "../src/catalog.js"; -import { FlErrorCode } from "../src/detail/errors.js"; import { Model } from "../src/model.js"; import { @@ -89,28 +88,6 @@ describeIfBuilt("Model (cache-only)", () => { expect(typeof model.path).toBe("string"); }); - it("download() maps AbortSignal cancellation from a native progress checkpoint to AbortError", async () => { - const controller = new AbortController(); - - await expect( - model.download((progress) => { - if (progress === 0) controller.abort(); - }, controller.signal), - ).rejects.toMatchObject({ - name: "AbortError", - code: FlErrorCode.OperationCancelled, - }); - expect(model.isCached).toBe(false); - }); - - it("download() rejects a pre-aborted AbortSignal before native submission", async () => { - const controller = new AbortController(); - controller.abort(); - - await expect(model.download(controller.signal)).rejects.toMatchObject({ name: "AbortError" }); - expect(model.isCached).toBe(false); - }); - it("id and alias match info", () => { expect(model.id).toBe(model.info.id); expect(model.alias).toBe(model.info.alias); diff --git a/sdk_v2/js/test/streaming.test.ts b/sdk_v2/js/test/streaming.test.ts index c15f1987a..58d9e688e 100644 --- a/sdk_v2/js/test/streaming.test.ts +++ b/sdk_v2/js/test/streaming.test.ts @@ -108,19 +108,6 @@ describe.skipIf(!haveTestModelCache)("ChatSession.processStreamingRequest (real 2 * 60_000, ); - it( - "concatenated streamed text contains the expected answer content", - async () => { - if (session === undefined) throw new Error("fixture missing"); - let text = ""; - for await (const item of session.processStreamingRequest(buildPrompt())) { - text += extractText(item); - } - expect(countUkTokens(text)).toBeGreaterThanOrEqual(2); - }, - 2 * 60_000, - ); - it( "early break requests cancellation while permitting prior native completion", async () => { diff --git a/sdk_v2/js/tsconfig.types.json b/sdk_v2/js/tsconfig.types.json index e5c1de9f5..d78e2d054 100644 --- a/sdk_v2/js/tsconfig.types.json +++ b/sdk_v2/js/tsconfig.types.json @@ -5,5 +5,5 @@ "moduleResolution": "NodeNext", "noEmit": true }, - "include": ["src/**/*", "test/model-download.types.ts", "test/tool-definition.types.ts"] + "include": ["src/**/*", "test/tool-definition.types.ts"] } diff --git a/sdk_v2/python/README.md b/sdk_v2/python/README.md index 3f184ec80..69990b531 100644 --- a/sdk_v2/python/README.md +++ b/sdk_v2/python/README.md @@ -110,20 +110,6 @@ with ChatSession(model) as session: model.unload() ``` -Pass a `threading.Event` as `cancel_event` to cancel an active download at the next native progress checkpoint: - -```python -from threading import Event - -cancel_event = Event() - -def on_progress(percent: float) -> None: - print(f"\rDownloading: {percent:.1f}%", end="", flush=True) - cancel_event.set() - -model.download(progress_callback=on_progress, cancel_event=cancel_event) -``` - Runnable end-to-end examples live under [`samples/python/`](https://github.com/microsoft/Foundry-Local/tree/main/samples/python). ## Usage diff --git a/sdk_v2/python/src/foundry_local_sdk/imodel.py b/sdk_v2/python/src/foundry_local_sdk/imodel.py index 2052df475..5b9043d2a 100644 --- a/sdk_v2/python/src/foundry_local_sdk/imodel.py +++ b/sdk_v2/python/src/foundry_local_sdk/imodel.py @@ -5,7 +5,6 @@ from __future__ import annotations from abc import ABC, abstractmethod -from threading import Event from typing import TYPE_CHECKING, Callable from typing_extensions import deprecated @@ -79,17 +78,12 @@ def supports_tool_calling(self) -> bool | None: """Whether the model supports tool/function calling, or ``None`` if unknown.""" @abstractmethod - def download( - self, - progress_callback: Callable[[float], None] | None = None, - cancel_event: Event | None = None, - ) -> None: + def download(self, progress_callback: Callable[[float], None] | None = None) -> None: """Download the model to the local cache if not already present. Args: progress_callback: Optional callback receiving download progress as a percentage (0.0–100.0). - cancel_event: Optional event that cancels the download when set. """ @abstractmethod @@ -332,48 +326,29 @@ def supports_tool_calling(self) -> bool | None: # Model lifecycle # ------------------------------------------------------------------ - def download( - self, - progress_callback: Callable[[float], None] | None = None, - cancel_event: Event | None = None, - ) -> None: + def download(self, progress_callback: Callable[[float], None] | None = None) -> None: from foundry_local_sdk._native.api import api, ffi cb = ffi.NULL user_data = ffi.NULL - callback_error: BaseException | None = None - if progress_callback is not None or cancel_event is not None: - callback_state = (progress_callback, cancel_event) - progress_cb_handle = ffi.new_handle(callback_state) + if progress_callback is not None: + self._progress_cb_handle = ffi.new_handle(progress_callback) - def _progress_callback(value: float, ud: object) -> int: - nonlocal callback_error + @ffi.callback("flProgressCallback") + def _cb(value: float, ud: object) -> int: try: - fn, event = ffi.from_handle(ud) - if event is not None and event.is_set(): - return 1 - if fn is not None: - fn(float(value)) - if event is not None and event.is_set(): - return 1 + fn = ffi.from_handle(ud) + fn(float(value)) return 0 - except BaseException as exc: - callback_error = exc + except Exception: return 1 - _cb = ffi.callback("flProgressCallback")(_progress_callback) + self._progress_cb = _cb # keep alive cb = _cb - user_data = progress_cb_handle + user_data = self._progress_cb_handle - try: - api.check_status(api.model.Download(self._ptr, cb, user_data)) - except FoundryLocalException: - if callback_error is not None: - raise callback_error - raise - if callback_error is not None: - raise callback_error + api.check_status(api.model.Download(self._ptr, cb, user_data)) def get_path(self) -> str: from foundry_local_sdk._native.api import api, ffi diff --git a/sdk_v2/python/test/unit/test_model_download_cancellation.py b/sdk_v2/python/test/unit/test_model_download_cancellation.py deleted file mode 100644 index 5d3053788..000000000 --- a/sdk_v2/python/test/unit/test_model_download_cancellation.py +++ /dev/null @@ -1,99 +0,0 @@ -from __future__ import annotations - -import sys -from threading import Event -from types import SimpleNamespace - -import pytest - -from foundry_local_sdk.exception import FoundryLocalException -from foundry_local_sdk.imodel import _ModelImpl - - -class FakeFfi: - NULL = None - - @staticmethod - def new_handle(value): - return value - - @staticmethod - def from_handle(value): - return value - - @staticmethod - def callback(_signature): - return lambda fn: fn - - -def make_model(monkeypatch, invoke_callback): - def download(_ptr, callback, user_data): - return invoke_callback(callback, user_data) - - def check_status(status): - if status is not None: - raise FoundryLocalException("download cancelled", error_code=5) - - fake_api = SimpleNamespace(model=SimpleNamespace(Download=download), check_status=check_status) - monkeypatch.setitem( - sys.modules, - "foundry_local_sdk._native.api", - SimpleNamespace(api=fake_api, ffi=FakeFfi()), - ) - model = _ModelImpl.__new__(_ModelImpl) - model._ptr = object() - return model - - -def test_download_cancel_event_returns_nonzero(monkeypatch): - cancel_event = Event() - cancel_event.set() - - def invoke(callback, user_data): - assert callback(25.0, user_data) == 1 - return object() - - model = make_model(monkeypatch, invoke) - - with pytest.raises(FoundryLocalException, match="download cancelled") as exc: - model.download(cancel_event=cancel_event) - - assert exc.value.error_code == 5 - - -def test_download_cancel_event_is_checked_after_progress(monkeypatch): - cancel_event = Event() - progress: list[float] = [] - - def on_progress(value: float) -> None: - progress.append(value) - cancel_event.set() - - def invoke(callback, user_data): - assert callback(50.0, user_data) == 1 - return object() - - model = make_model(monkeypatch, invoke) - - with pytest.raises(FoundryLocalException, match="download cancelled"): - model.download(on_progress, cancel_event) - - assert progress == [50.0] - - -def test_download_preserves_progress_callback_exception(monkeypatch): - expected = RuntimeError("progress failed") - - def on_progress(_value: float) -> None: - raise expected - - def invoke(callback, user_data): - assert callback(10.0, user_data) == 1 - return object() - - model = make_model(monkeypatch, invoke) - - with pytest.raises(RuntimeError, match="progress failed") as exc: - model.download(on_progress) - - assert exc.value is expected From bf0692a1a25ead23b877b1b59f7881750b5100f6 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 18:34:21 -0500 Subject: [PATCH 12/16] Prevent streaming settlement from stalling sessions 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 --- sdk_v2/js/native/src/session.cc | 60 +++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 21 deletions(-) diff --git a/sdk_v2/js/native/src/session.cc b/sdk_v2/js/native/src/session.cc index dccd8f155..0b40197db 100644 --- a/sdk_v2/js/native/src/session.cc +++ b/sdk_v2/js/native/src/session.cc @@ -297,32 +297,50 @@ struct StreamCtx { bool errored = false; }; +class StreamFinalizeGuard { + public: + explicit StreamFinalizeGuard(StreamCtx* ctx) : ctx_(ctx) {} + + ~StreamFinalizeGuard() { + if (ctx_->scheduler != nullptr) { + auto scheduler = std::move(ctx_->scheduler); + scheduler->Complete(); + } + delete ctx_; + } + + private: + StreamCtx* ctx_; +}; + void FinalizeStream(Napi::Env env, void* /*data*/, StreamCtx* ctx) { + StreamFinalizeGuard finalize_guard(ctx); Napi::HandleScope scope(env); - if (ctx->errored) { - if (ctx->tagged) { - Napi::Error err = Napi::Error::New(env, ctx->err_msg); - Napi::Object v = err.Value(); - v.Set("name", Napi::String::New(env, "FoundryLocalError")); - v.Set("code", Napi::Number::New(env, ctx->err_code)); - ctx->deferred.Reject(v); + try { + if (ctx->errored) { + if (ctx->tagged) { + Napi::Error err = Napi::Error::New(env, ctx->err_msg); + Napi::Object v = err.Value(); + v.Set("name", Napi::String::New(env, "FoundryLocalError")); + v.Set("code", Napi::Number::New(env, ctx->err_code)); + ctx->deferred.Reject(v); + } else { + ctx->deferred.Reject(Napi::Error::New(env, ctx->err_msg).Value()); + } + } else if (ctx->response != nullptr) { + ctx->deferred.Resolve(ResponseToJs(env, *ctx->response)); } else { - ctx->deferred.Reject(Napi::Error::New(env, ctx->err_msg).Value()); + // Should not happen: successful path always captures a Response. Guard anyway so we never leave the deferred + // pending. + ctx->deferred.Resolve(env.Undefined()); } - } else if (ctx->response != nullptr) { - ctx->deferred.Resolve(ResponseToJs(env, *ctx->response)); - } else { - // Should not happen: successful path always captures a Response. Guard - // anyway so we never leave the deferred pending. - ctx->deferred.Resolve(env.Undefined()); - } - - if (ctx->scheduler != nullptr) { - auto scheduler = std::move(ctx->scheduler); - scheduler->Complete(); + } catch (const Napi::Error& e) { + ctx->deferred.Reject(e.Value()); + } catch (const std::exception& e) { + ctx->deferred.Reject(Napi::Error::New(env, e.what()).Value()); + } catch (...) { + ctx->deferred.Reject(Napi::Error::New(env, "Failed to settle native streaming response").Value()); } - - delete ctx; } template From bd9bac336707d59a3d35798b6673de8fe2f7f821 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 18:57:17 -0500 Subject: [PATCH 13/16] Arbitrate cancellation after non-standard exceptions 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 --- sdk_v2/cpp/src/inferencing/session/session.cc | 15 +++++++ .../test/internal_api/session_manager_test.cc | 42 +++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/sdk_v2/cpp/src/inferencing/session/session.cc b/sdk_v2/cpp/src/inferencing/session/session.cc index 1a6cf1d5b..8d686ed4e 100644 --- a/sdk_v2/cpp/src/inferencing/session/session.cc +++ b/sdk_v2/cpp/src/inferencing/session/session.cc @@ -233,6 +233,21 @@ void Session::ProcessRequest(const Request& request, Response& response) { throw; } + try { + ThrowCancellation(request); + } catch (const std::exception& cancellation) { + finish_lifecycle(); + lifecycle_guard.Dismiss(); + tracker.RecordException(cancellation); + throw; + } + } catch (...) { + if (request.TryComplete()) { + finish_lifecycle(); + lifecycle_guard.Dismiss(); + throw; + } + try { ThrowCancellation(request); } catch (const std::exception& cancellation) { diff --git a/sdk_v2/cpp/test/internal_api/session_manager_test.cc b/sdk_v2/cpp/test/internal_api/session_manager_test.cc index 13b4c9e8f..1e465514c 100644 --- a/sdk_v2/cpp/test/internal_api/session_manager_test.cc +++ b/sdk_v2/cpp/test/internal_api/session_manager_test.cc @@ -552,6 +552,33 @@ class NonStdThrowThenCompleteSession : public Session { size_t process_count_ = 0; }; +class NonStdThrowAfterCancelSession : public Session { + public: + NonStdThrowAfterCancelSession(const Model& model, ILogger& logger, ITelemetry& telemetry) + : Session(model, logger, telemetry) {} + + SessionType Type() const override { return SessionType::kChat; } + bool InFlight() const { return in_flight_.load(std::memory_order_acquire); } + + protected: + void ProcessRequestImpl(const Request& request, Response& /*response*/) override { + in_flight_.store(true, std::memory_order_release); + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (!request.IsCancellationRequested()) { + if (std::chrono::steady_clock::now() >= deadline) { + throw 42; + } + + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + + throw 42; + } + + private: + std::atomic in_flight_{false}; +}; + flErrorCode ProcessAndGetCode(Session& session, const Request& request) { try { Response response; @@ -772,6 +799,21 @@ TEST(SessionRequestLifecycleTest, NonStdExceptionAllowsRequestReuse) { EXPECT_EQ(session.ProcessCount(), 2u); } +TEST(SessionRequestLifecycleTest, CancellationWinsOverNonStdException) { + fl::test::FakeServiceBindings svc; + Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); + TelemetryLogger telemetry{"test", fl::test::NullLog()}; + NonStdThrowAfterCancelSession session(catalog_model, fl::test::NullLog(), telemetry); + Request request; + + auto processing = std::async(std::launch::async, [&] { return ProcessAndGetCode(session, request); }); + ASSERT_TRUE(WaitUntil([&] { return session.InFlight(); }, std::chrono::seconds(2))); + ASSERT_TRUE(request.Cancel()); + + EXPECT_EQ(processing.get(), FOUNDRY_LOCAL_ERROR_OPERATION_CANCELLED); + EXPECT_TRUE(request.IsCompleted()); +} + TEST(SessionRequestLifecycleTest, CallbackExceptionSurfacesOriginalCauseAndAllowsReuse) { fl::test::FakeServiceBindings svc; Model catalog_model = Model::FromModelInfo(ModelInfo{}, "", svc.download_manager, svc.model_load_manager); From a8ca3b5cc593bf284d4bedb7f9e8fdd659f526f4 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 20:10:21 -0500 Subject: [PATCH 14/16] Clarify promise worker ownership 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 --- sdk_v2/js/native/src/manager.cc | 1 - sdk_v2/js/native/src/promise_worker.h | 2 +- sdk_v2/js/native/src/session.h | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/sdk_v2/js/native/src/manager.cc b/sdk_v2/js/native/src/manager.cc index e94f29803..d78e67262 100644 --- a/sdk_v2/js/native/src/manager.cc +++ b/sdk_v2/js/native/src/manager.cc @@ -5,7 +5,6 @@ #include "addon_data.h" #include "catalog.h" #include "errors.h" -#include "promise_worker.h" #include #include diff --git a/sdk_v2/js/native/src/promise_worker.h b/sdk_v2/js/native/src/promise_worker.h index b0a338229..a495d0b66 100644 --- a/sdk_v2/js/native/src/promise_worker.h +++ b/sdk_v2/js/native/src/promise_worker.h @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. // -// Used by model download/load/unload and session inference. +// Used by asynchronous catalog queries and model load/unload operations. // // PromiseWorker — generic Napi::AsyncWorker that runs a std::function // on a libuv worker thread and resolves / rejects a JS Promise with the diff --git a/sdk_v2/js/native/src/session.h b/sdk_v2/js/native/src/session.h index 37ad3dae2..b8a4f0051 100644 --- a/sdk_v2/js/native/src/session.h +++ b/sdk_v2/js/native/src/session.h @@ -6,7 +6,7 @@ // Surface: // * new ChatSession(model) — sync construction; underlying // flSession_Create is fast. -// * session.processRequest(request) -> Promise (PromiseWorker) +// * session.processRequest(request) -> Promise — scheduled on a per-session worker queue. // * session.processStreamingRequest(request, onItem) -> Promise — streaming bridge via // Napi::ThreadSafeFunction; resolves with the terminal Response after every queued JS callback runs. // The JS layer wraps this in an AsyncIterable whose `.response` promise carries the resolved value. From 4de72098a72d31078cbe42e464d3d7ebad7b324e Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 20:36:00 -0500 Subject: [PATCH 15/16] Remove stale cancellation terminology 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 --- sdk_v2/cpp/src/inferencing/session/session.h | 6 +++--- .../cpp/test/internal_api/callback_handler_test.cc | 2 +- sdk_v2/cs/src/StreamingResponse.cs | 4 ++-- sdk_v2/js/docs/PortJsToSdkV2.md | 9 ++++----- sdk_v2/js/src/request.ts | 4 ++-- sdk_v2/js/src/session.ts | 12 ++++++------ 6 files changed, 18 insertions(+), 19 deletions(-) diff --git a/sdk_v2/cpp/src/inferencing/session/session.h b/sdk_v2/cpp/src/inferencing/session/session.h index e46dfac2c..cbeea7dfc 100644 --- a/sdk_v2/cpp/src/inferencing/session/session.h +++ b/sdk_v2/cpp/src/inferencing/session/session.h @@ -167,9 +167,9 @@ class Session { const bool allow_concurrent_requests_; mutable std::unique_ptr request_mutex_ = std::make_unique(); - // In-flight requests tracked so Cancel() can flip their cancel flags from another thread. Guarded - // by its own mutex (not request_mutex_) because concurrent sessions (e.g. audio) may hold several - // at once, and Cancel() must run without waiting on an active generation holding request_mutex_. + // In-flight requests tracked so Cancel() can transition their lifecycle state from another thread. Guarded by its + // own mutex (not request_mutex_) because concurrent sessions (e.g. audio) may hold several at once, and Cancel() + // must run without waiting on an active generation holding request_mutex_. // unique_ptr keeps Session movable (std::mutex is not movable), matching request_mutex_. std::unordered_set active_requests_; mutable std::unique_ptr active_requests_mutex_ = std::make_unique(); diff --git a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc index 233e15fd9..566fa276f 100644 --- a/sdk_v2/cpp/test/internal_api/callback_handler_test.cc +++ b/sdk_v2/cpp/test/internal_api/callback_handler_test.cc @@ -89,7 +89,7 @@ TEST(CallbackHandlerTest, FurtherPushesAfterExceptionAreNoOps) { const int invocations_after_first = invocations.load(); - // Subsequent pushes must be dropped (canceled is set, so PushItem skips). + // Subsequent pushes must be dropped once the request's cancellation state is set. handler.PushItem(std::make_unique("second")); handler.PushItem(std::make_unique("third")); diff --git a/sdk_v2/cs/src/StreamingResponse.cs b/sdk_v2/cs/src/StreamingResponse.cs index b8f2dd682..b349b251d 100644 --- a/sdk_v2/cs/src/StreamingResponse.cs +++ b/sdk_v2/cs/src/StreamingResponse.cs @@ -146,8 +146,8 @@ private async IAsyncEnumerable EnumerateAsync([EnumeratorCancellation] Can // TryComplete (channel completion is always preceded by ProcessRequest returning). The // native request has therefore already produced its full output, so cleanup MUST NOT // signal abort — doing so races the native streaming-callback worker thread and can - // flip request.canceled on a still-finalizing request, truncating the output. Only an - // early break / dispose / external-token cancellation (which skip this line) should abort. + // transition a still-finalizing request to canceled, truncating the output. Only an early + // break / dispose / external-token cancellation (which skip this line) should abort. Interlocked.Exchange(ref _drainedNaturally, 1); } finally diff --git a/sdk_v2/js/docs/PortJsToSdkV2.md b/sdk_v2/js/docs/PortJsToSdkV2.md index 4dab8e7d0..30369ee41 100644 --- a/sdk_v2/js/docs/PortJsToSdkV2.md +++ b/sdk_v2/js/docs/PortJsToSdkV2.md @@ -115,13 +115,12 @@ underlying C ABI call is a memory copy, not I/O. ## Streaming -- `Session.processRequestStreaming` returns an `AsyncIterable`. Each - native streaming-callback push lands on a `Napi::ThreadSafeFunction` - acquired in the session's constructor and released when the iterable is - closed. +- `Session.processStreamingRequest` returns an `AsyncIterable`. Each request owns a worker whose + `Napi::ThreadSafeFunction` forwards native streaming-callback items and is released after the worker completes and + its queued callbacks drain. - Cancellation: streaming APIs accept an `AbortSignal`. A signal already aborted at call time rejects before native work is submitted. Once submitted, the signal calls `Request::Cancel()`, which affects only an invocation currently - inside native `Session::ProcessRequest`; work still waiting in the native session FIFO is not canceled. + inside native `Session::ProcessRequest`; work still waiting in the addon's per-session queue is not canceled. - Live PCM input (audio transcription with chunks arriving over time) is expressed by adding an `AudioItem` descriptor to the `Request` and pushing PCM bytes through a paired `ItemQueue`. The session consumes the diff --git a/sdk_v2/js/src/request.ts b/sdk_v2/js/src/request.ts index 64ad49775..77e7fe16a 100644 --- a/sdk_v2/js/src/request.ts +++ b/sdk_v2/js/src/request.ts @@ -75,8 +75,8 @@ export class Request { /** * Cancel this request's invocation only while it is inside native `Session::ProcessRequest`. Calling this while the - * request is idle, waiting in a session's native FIFO, or already completed has no effect. Active cancellation makes - * the matching `Session.processRequest()` reject with `code === FlErrorCode.OperationCancelled`. + * request is idle, waiting in the addon's per-session queue, or already completed has no effect. Active cancellation + * makes the matching `Session.processRequest()` reject with `code === FlErrorCode.OperationCancelled`. */ cancel(): void { this.#native.cancel(); diff --git a/sdk_v2/js/src/session.ts b/sdk_v2/js/src/session.ts index 63468565e..09053de20 100644 --- a/sdk_v2/js/src/session.ts +++ b/sdk_v2/js/src/session.ts @@ -48,8 +48,8 @@ export interface StreamOptions { * * `response` settles after the native call completes and all queued item callbacks have run. Breaking iteration early * requests cancellation of an active native invocation, but native completion can win that race. A request still - * waiting in the native session FIFO is unaffected. When cancellation wins, `response` rejects with `AbortError` for - * an aborted signal or `OperationCancelled` for an early break. + * waiting in the addon's per-session queue is unaffected. When cancellation wins, `response` rejects with + * `AbortError` for an aborted signal or `OperationCancelled` for an early break. */ export interface StreamingResponse extends AsyncIterable { readonly response: Promise; @@ -278,10 +278,10 @@ export abstract class Session { * native call completes. * * Cancellation: pass `{ signal }`; aborting while the invocation is active cancels the native request and causes the - * iterator to throw an `Error` with `name === "AbortError"`. Aborting while work is only waiting in the native FIFO - * does not prevent execution. A signal already aborted at call time rejects before submission. Breaking out of the - * `for await` loop similarly requests active cancellation. If native completion wins the race, `response` resolves - * normally; otherwise it rejects with `OperationCancelled`. + * iterator to throw an `Error` with `name === "AbortError"`. Aborting while work is only waiting in the addon's + * per-session queue does not prevent execution. A signal already aborted at call time rejects before submission. + * Breaking out of the `for await` loop similarly requests active cancellation. If native completion wins the race, + * `response` resolves normally; otherwise it rejects with `OperationCancelled`. * * Non-cancellation failures throw a `FoundryLocalError`. */ From 764e8b3126d21af352a6d5d7278d794075490145 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 18 Sep 2026 20:51:18 -0500 Subject: [PATCH 16/16] Cancel stalled audio inference before waiting 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 --- sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc b/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc index fa1e5cd8a..60a90d6ec 100644 --- a/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc +++ b/sdk_v2/cpp/test/sdk_api/streaming_audio_test.cc @@ -230,6 +230,7 @@ TEST_F(StreamingAudioFixture, CancellationMidStream) { } if (queue.Size() == half) { + request.Cancel(); queue.MarkFinished(); future.wait(); FAIL() << "Audio session did not consume input before the cancellation deadline";