[dajiaohuang]

Summary

Preserve terminal cancellation when a producer already running later returns a value, returns void, or throws. Result setters check cancellation under the existing result lock. Added a threaded regression against the real async headers and corrected the stale get_future cancellation warning.

Fixes #3233.

Caller and contract

async::run_on_main/run_on_bg create a bg_queue_promise through impl::run_; the submitted functor calls bg_queue_promise::call. Real callers include dataset API mutations in cpp/deeplake_api/dataset.hpp and refresh/read operations in cpp/deeplake_pg/table_data_impl.hpp. call checks cancellation only before entering the producer, so cancellation during that work is a legitimate late-completion path. Queue task removal explicitly does not interrupt an already running task.

Dropping that late completion preserves promise::cancel's callback suppression contract. The previous ASSERT(!has_result(...)) does not detect this case: cancelled results are not fulfilled. Replacing the result can erase terminal cancellation and admit a later subscriber.

Successful cancellation destroys the callback's captured std::promise; get_future therefore becomes ready with std::future_errc::broken_promise. The existing warning claiming it stayed locked was stale. No new waiter-notification mechanism or future-result contract is introduced.

Validation

• C++20 threaded test compiles and runs with actual bg_queue_promise, handle_base, result, promise, locking and Abseil headers.
• Public promise.cancel() followed by a late set_value preserves cancellation and suppresses existing/later callbacks; the future test also supplies a late value before checking broken_promise readiness.
• Four cancelled-running-producer cases pass (int value, void value, int exception, void exception), plus normal callback completion and get_future cancellation/broken-promise readiness.
• With only the pre-fix handle_base.hpp restored, the executable aborts because the late result resurrects the cancelled handle (exit 134); the fixed header passes (exit 0).
• git diff --check passes.
• Standalone release-header build uses a minimal generated config.hpp and explicitly isolated queue-linkage stubs for an unsubmitted queue ID. Callbacks use the null/inline queue; actual scheduler execution would terminate the test. Real queue scheduling, the complete async suite and the full Deep Lake extension were not exercised.

Review follow-up

Removed the standalone native test files requested by review. Production changes are unchanged. Native gtests need integration into cpp/tests/async. Earlier isolated component checks are historical evidence only; full native integration and patched-extension acceptance remain unverified.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 266661b6-48ad-4352-8d7f-a2a648e7291a

📥 Commits

Reviewing files that changed from the base of the PR and between f432041 and 31fabed.

📒 Files selected for processing (1)

• cpp/async/handle_base.hpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.

---

📝 Walkthrough

Walkthrough

Changes

The completion setters in handle_base now return without updating the result when the handle is cancelled. This applies to value-setting overloads for Windows and non-Windows builds, void results, and exception-setting.

Async cancellation

Layer / File(s)	Summary
Guard completion setters cpp/async/handle_base.hpp	The Windows, non-Windows, and void set_value overloads, and set_exception, now return when the handle is cancelled.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 31fab

A task that is successfully cancelled while running retains its cancelled state when its work later completes; no actionable merge-blocking risk remains.

Architecture Summary

Architecture risk: 🔵 Low · up to 31fab

The change affects 1 system.

Changed systems: cpp

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — cpp (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in cpp/async/handle_base.hpp: The Windows set_value overload now exits when the handle is cancelled; previously it proceeded to assert and update the result.
• observed — Modified behavior in cpp/async/handle_base.hpp: The non-Windows set_value overload now exits when the handle is cancelled; previously it proceeded to assert and update the result.
• observed — Modified behavior in cpp/async/handle_base.hpp: The void set_value overload now exits when the handle is cancelled instead of proceeding to fulfill the result.
• observed — Modified behavior in cpp/async/handle_base.hpp: set_exception now exits when the handle is cancelled instead of proceeding to assert and replace the result.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR addresses issue #3233. The change guards all handle_base::set_value overloads and set_exception under the cancellation state. The guards cover value results, void results, Windows and non-W…
Out of Scope Changes check	✅ Passed	The PR changes only cpp/async/handle_base.hpp. The changes apply directly to the handle completion setters named in issue #3233. No unrelated implementation or documentation change is shown.
Title check	✅ Passed	The title clearly and concisely describes the main change: preserving async cancellation when a running task completes.
Description check	✅ Passed	The description explains the problem, affected execution path, technical fix, issue reference, behavior, and validation limits. It does not use the template headings or impact checkboxes exactly, but …

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks the task’s small gate,
“If cancelled, I will not update.”
No value hops, no exception flies,
The cancelled result stays as it lies.
I nibble clover and celebrate!}

_{Comment @coderabbitai help to get the list of available commands.}

[khustup2]

The guard changes a core semantic of the async layer, so it needs more than a source review:

1. Which caller observed set_value() after cancel()? Give the call path.
2. Today that path hits ASSERT(!has_result) in debug builds. Is the intended behavior to drop the value silently, or to assert? Explain why silent drop is correct for every caller.
3. on_result_updated() is skipped when cancelled. Confirm no waiter depends on that notification after a cancel, or show that cancel() already notified them.
4. Add a unit test in the async test suite: cancel a promise, then set_value, then assert is_cancelled() and that no callback fires.

[dajiaohuang]

Confirmed the real call path: dataset API and PostgreSQL refresh/read callers use run_on_main; impl::run_ creates bg_queue_promise, whose call checks cancellation before invoking the producer. Cancellation while that producer runs is legitimate because queue removal does not interrupt already-running work. ASSERT(!has_result) does not reject a cancelled state, since cancellation is not fulfilled; a late value can erase it and allow a later subscriber.

Added a threaded test using the actual producer/handle/result/promise headers. Four cancelled-producer cases, normal completion, and future readiness pass. Restoring only the pre-fix handle header reproduces resurrection and exits 134. This is a standalone header test with queue linkage isolated, not a complete queue/extension suite.

For waiters, actual get_future becomes ready with broken_promise when cancellation destroys its captured std::promise. Verified that directly and corrected the stale “forever locked” warning. The fix preserves this behavior rather than adding another notification path.

[dajiaohuang]

Added explicit public-API coverage in 4ccb3a2: construct an async::promise from its producer handle, register a callback, call promise.cancel(), then supply the late set_value. The test asserts the promise remains cancelled, is not ready, and neither the original callback nor a later subscriber runs. The get_future case now also supplies a late value after cancellation and verifies broken_promise readiness remains intact.

The updated native C++20 executable passes these cases plus the four running-producer cases and normal completion. Restoring the pre-fix handle header still aborts in the running-producer regression (exit 134); the fixed header passes (exit 0). That negative control belongs to the running-producer case, not a claim that each public-API control independently fails on old code. Queue linkage remains explicitly isolated; no actual scheduler or complete-extension execution is claimed.

[khustup2]

Confirmed against the current source. has_result() is result_.has_value() && result_->fulfilled(), and a cancelled handle is not fulfilled (cancelled is index 1 of the result variant; fulfilled is value or exception). So a late set_value()/set_exception() passes ASSERT(!has_result) and overwrites the cancelled state — the resurrection you describe. The four guards are the right fix and your call-path analysis (bg_queue_promise checks cancellation before the producer runs, not during) matches ours. The standalone test under cpp/async/tests will not be picked up by our test tree; we will carry the fix with a gtest in cpp/tests/async. Approving the source change.