📝 docs(plan): implement async executor cancellation

2026-07-12 20:17:33 +08:00
parent 627613f586
commit edea37202d
2 changed files with 375 additions and 1 deletions
@@ -0,0 +1,374 @@
+# AsyncExecutor Cooperative Cancellation Implementation Plan
+
+> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
+
+**Goal:** Replace the scheduler's ambiguous ID-based cancellation and weak task state with instance-safe handles, cooperative stop tokens, stable results, and accurate active-task tracking.
+
+**Architecture:** `TaskHandle` strongly owns a private per-submission state containing a `stop_source`, phase, final `TaskResult`, and completion synchronization. `AsyncExecutor` separately indexes the current task for each ID and every active task instance; long-running symbol and workspace operations receive the task's `stop_token` and check it at file boundaries.
+
+**Tech Stack:** C++23 Modules, Taskflow 4, `std::stop_source` / `std::stop_token`, repository test harness, CMake/Ninja/CTest.
+
+---
+
+## Plan Meta
+
+- **Plan Group:** scheduler-cancellation
+- **Parent Plan:** none
+- **Verification Scope:** `test_scheduler`, `test_provider`, `tsl-server`, and LSP transport smoke tests
+- **Verification Gate:** all new scheduler lifecycle cases pass; provider tests and production server build without API migration errors; LSP transport smoke tests pass
+- **Executor:** `executing-plans`
+- **Constraints:** `karpathy-guidelines`, `.agents/`, `AGENT_RULES.md`, test-first RED/GREEN evidence, preserve unrelated worktree changes
+
+## File map
+
+- `lsp-server/src/scheduler/async_executor.cppm`: owns the public scheduler API and all private per-task state, cancellation, completion, metrics, and registration logic.
+- `lsp-server/test/test_scheduler/test_async_executor.cppm`: specifies handle lifetime, per-instance cancellation, queued/running cancellation, duplicate-ID accounting, zero concurrency, exception, callback, and metrics behavior.
+- `lsp-server/test/test_scheduler/main.cc`: calls the scheduler test module through its namespace instead of a global exported function.
+- `lsp-server/src/core/server.cppm`, `lsp-server/src/core/dispatcher.cppm`, `lsp-server/src/manager/bootstrap.cppm`: migrate scheduler types and bootstrap closures/callbacks to the new namespace and result model.
+- `lsp-server/src/manager/symbol.cppm`: accepts optional stop tokens and checks them at safe file-loop boundaries before publishing accumulated changes.
+- `lsp-server/src/provider/initialize/initialize.cppm`, `lsp-server/src/provider/workspace/execute_command.cppm`, `lsp-server/src/provider/workspace/did_change_workspace_folders.cppm`: pass stop tokens through background workspace operations and consume `TaskResult` callbacks.
+- `lsp-server/test/test_provider/*.cppm`: mechanically migrate scheduler type names and the two direct scheduler closures.
+
+### Task 1: Specify the new scheduler lifecycle API and edge cases
+
+**Files:**
+- Modify: `lsp-server/test/test_scheduler/test_async_executor.cppm`
+- Modify: `lsp-server/test/test_scheduler/main.cc`
+
+- [ ] **Step 1: Replace the old scheduler tests with tests against the desired API**
+
+Use `lsp::scheduler::async_executor::{AsyncExecutor, TaskStatus}` and make the module entry `lsp::test::scheduler::async_executor::Run`. Add deterministic synchronization with `std::latch`, `std::promise`, atomics, and handle waits. The suite must contain these independent cases:
+
+```cpp
+suite.Add("Retains completed task result through handle", [] {
+    AsyncExecutor executor{ 1 };
+    auto handle = executor.Submit("fast", [](std::stop_token) {
+        return std::optional<std::string>{ "done" };
+    });
+    auto result = handle.Wait();
+    Expect(handle.Valid(), "completed handle should remain valid");
+    Expect(result && result->status == TaskStatus::kCompleted, "task should complete");
+    Expect(result->value == "done", "task value should remain available");
+    Expect(handle.TryGetResult().has_value(), "completed result should be non-blocking");
+});
+
+suite.Add("Skips a queued task cancelled through its handle", [] {
+    AsyncExecutor executor{ 1 };
+    std::latch blocker_started{ 1 };
+    std::latch release_blocker{ 1 };
+    auto blocker = executor.Submit("blocker", [&](std::stop_token) {
+        blocker_started.count_down();
+        release_blocker.wait();
+        return std::optional<std::string>{};
+    });
+    blocker_started.wait();
+    std::atomic<bool> ran{ false };
+    auto queued = executor.Submit("queued", [&](std::stop_token) {
+        ran = true;
+        return std::optional<std::string>{};
+    });
+    Expect(queued.Cancel(), "first queued cancellation should succeed");
+    Expect(!queued.Cancel(), "repeated cancellation should fail");
+    release_blocker.count_down();
+    auto result = queued.Wait();
+    Expect(result && result->status == TaskStatus::kCancelled, "queued task should cancel");
+    Expect(!ran, "cancelled queued closure must not execute");
+    Expect(blocker.Wait().has_value(), "blocker should finish");
+});
+```
+
+Also add cases that assert:
+
+- `Wait` returns only after callback completion.
+- an old same-ID handle cannot cancel the replacement task.
+- a running closure observes `stop_requested()` and exits.
+- two same-ID instances produce `GetRunningTaskCount() == 2` while active and `WaitAll()` waits for both.
+- `AsyncExecutor{0}` runs a task successfully.
+- task exceptions yield `kFailed` and a rethrowable original `exception_ptr`.
+- callback exceptions do not deadlock or overwrite a completed task result.
+- completed/cancelled/failed metrics match terminal results.
+- a default-constructed handle is invalid and its methods return failure/empty values.
+
+- [ ] **Step 2: Run the scheduler target and capture RED**
+
+Run:
+
+```bash
+cmake --build lsp-server/build/codex43-clean/Release --target test_scheduler -j1
+```
+
+Expected: compile failure because `lsp::scheduler::async_executor`, token-taking closures, `TaskStatus`, result-returning `Wait`, and `TryGetResult` do not exist yet. This is the required RED evidence.
+
+- [ ] **Step 3: Commit the executable specification only if it can be isolated from the breaking implementation**
+
+Because the desired tests intentionally do not compile against the old public API, keep them in the implementation commit unless an intermediate compiling contract test is possible. Do not create a permanently broken commit.
+
+### Task 2: Implement stable state and cooperative cancellation
+
+**Files:**
+- Modify: `lsp-server/src/scheduler/async_executor.cppm`
+
+- [ ] **Step 1: Define the new exported result and handle API**
+
+Export only the public namespace and types:
+
+```cpp
+export namespace lsp::scheduler::async_executor
+{
+    enum class TaskStatus { kCompleted, kCancelled, kFailed };
+
+    struct TaskResult
+    {
+        TaskStatus status;
+        std::optional<std::string> value;
+        std::exception_ptr error;
+    };
+
+    struct ExecutorMetrics
+    {
+        std::size_t running = 0;
+        std::uint64_t submitted = 0;
+        std::uint64_t completed = 0;
+        std::uint64_t failed = 0;
+        std::uint64_t cancelled = 0;
+    };
+
+    class TaskHandle
+    {
+    public:
+        TaskHandle() = default;
+        bool Valid() const;
+        bool Cancel() const;
+        std::optional<TaskResult> Wait() const;
+        std::optional<TaskResult> TryGetResult() const;
+
+    private:
+        struct State;
+        explicit TaskHandle(std::shared_ptr<State> state);
+        std::shared_ptr<State> state_;
+        friend class AsyncExecutor;
+    };
+}
+```
+
+`TaskHandle::State` must be defined only in the module implementation and contain the task ID, start time, `std::stop_source`, mutex, condition variable, pending/running/completed phase, optional final result, and callback-completed flag.
+
+- [ ] **Step 2: Replace the executor API and registration model**
+
+Use these public aliases and methods:
+
+```cpp
+using TaskClosure = std::function<std::optional<std::string>(std::stop_token)>;
+using TaskCallback = std::function<void(const TaskResult&)>;
+
+explicit AsyncExecutor(std::size_t concurrency = std::thread::hardware_concurrency());
+TaskHandle Submit(const std::string& task_id, TaskClosure task,
+                  TaskCallback callback = nullptr);
+bool Cancel(const std::string& task_id);
+void WaitAll();
+std::size_t GetRunningTaskCount() const;
+void LogStatus() const;
+ExecutorMetrics GetStatistics() const;
+```
+
+Remove `WaitForTask`, the exported `detail` namespace, the raw executor pointer from `TaskHandle`, `ActiveEntry`, and `kStatusLogInterval`. Store current-ID states in `current_tasks_` and every unfinished instance in `active_tasks_`. Document beside `TaskCallback` that a callback must not call `Wait()` on its own handle or call `WaitAll()` on the same executor, because either operation would self-wait.
+
+- [ ] **Step 3: Implement the cancellation synchronization point**
+
+`TaskHandle::Cancel()` calls `request_stop()` on its own state only while its phase is not completed. `AsyncExecutor::Cancel(id)` snapshots only `current_tasks_[id]` and performs the same operation. `ExecuteTask` locks the state before starting: if stop is already requested, it completes as cancelled without invoking the closure; otherwise it changes pending to running and calls the closure with `state->stop_source.get_token()`.
+
+- [ ] **Step 4: Implement terminal results and callback completion**
+
+Build exactly one `TaskResult` per task:
+
+```cpp
+TaskResult result;
+try {
+    auto value = task(token);
+    result = token.stop_requested()
+        ? TaskResult{ TaskStatus::kCancelled, std::nullopt, nullptr }
+        : TaskResult{ TaskStatus::kCompleted, std::move(value), nullptr };
+} catch (...) {
+    result = TaskResult{ TaskStatus::kFailed, std::nullopt,
+                         std::current_exception() };
+}
+```
+
+Write the result before invoking the callback. Catch and log callback exceptions without changing `result`. Remove the exact state instance from both registries, mark callback completion, update exactly one terminal metric, and then notify waiters. This ordering makes `Wait()` imply that callback execution and registry cleanup are both complete.
+
+- [ ] **Step 5: Normalize concurrency and wait for all Taskflow work**
+
+Construct Taskflow with `std::max<std::size_t>(1, concurrency)`. `WaitAll()` must use Taskflow's `wait_for_all()` so callback-submitted work is drained; completed states must already be removed from `active_tasks_`. The destructor continues to call `WaitAll()` and `LogStatus()`.
+
+- [ ] **Step 6: Build and run the scheduler tests for GREEN**
+
+Run:
+
+```bash
+cmake --build lsp-server/build/codex43-clean/Release --target test_scheduler -j1
+ctest --test-dir lsp-server/build/codex43-clean/Release -R '^test_scheduler$' --output-on-failure
+```
+
+Expected: target builds and the complete scheduler suite passes.
+
+### Task 3: Migrate namespaces and scheduler call signatures
+
+**Files:**
+- Modify: `lsp-server/src/core/server.cppm`
+- Modify: `lsp-server/src/core/dispatcher.cppm`
+- Modify: `lsp-server/src/manager/bootstrap.cppm`
+- Modify: `lsp-server/src/provider/initialize/initialize.cppm`
+- Modify: `lsp-server/src/provider/workspace/execute_command.cppm`
+- Modify: `lsp-server/src/provider/workspace/did_change_workspace_folders.cppm`
+- Modify: `lsp-server/test/test_provider/completion_test.cppm`
+- Modify: `lsp-server/test/test_provider/definitions_test.cppm`
+- Modify: `lsp-server/test/test_provider/interpreter_test.cppm`
+- Modify: `lsp-server/test/test_provider/json_flow_test.cppm`
+- Modify: `lsp-server/test/test_provider/json_provider_coverage_test.cppm`
+- Modify: `lsp-server/test/test_provider/provider_misc_test.cppm`
+- Modify: `lsp-server/test/test_provider/provider_surface_test.cppm`
+
+- [ ] **Step 1: Mechanically migrate scheduler type names**
+
+Replace every `scheduler::AsyncExecutor` with `scheduler::async_executor::AsyncExecutor` and every fully qualified `lsp::scheduler::AsyncExecutor` with `lsp::scheduler::async_executor::AsyncExecutor`. Do not alter unrelated namespace migrations already present in the worktree.
+
+- [ ] **Step 2: Migrate every submitted closure and callback**
+
+Every closure accepts a `std::stop_token`, using `[[maybe_unused]]` only for genuinely short tasks. Every callback accepts `const scheduler::async_executor::TaskResult&` (or a local alias) and switches on `result.status`; successful values are read from `result.value`. For example:
+
+```cpp
+async_executor.Submit(
+    task_name,
+    [&hub, path](std::stop_token stop_token) -> std::optional<std::string> {
+        hub.symbols().LoadSystemLibrary(path, stop_token);
+        return std::format("Loaded system library: {}", path);
+    },
+    [path](const scheduler::async_executor::TaskResult& result) {
+        if (result.status == scheduler::async_executor::TaskStatus::kCancelled)
+            spdlog::info("System library load task cancelled: {}", path);
+        else if (result.status == scheduler::async_executor::TaskStatus::kCompleted && result.value)
+            spdlog::info("{}", *result.value);
+    });
+```
+
+- [ ] **Step 3: Prove the migration is complete statically**
+
+Run:
+
+```bash
+rg -n 'scheduler::AsyncExecutor|lsp::scheduler::AsyncExecutor|\[.*\]\(\) -> std::optional<std::string>|const std::optional<std::string>&.*, bool cancelled|WaitForTask|GetResult\(' lsp-server/src lsp-server/test
+```
+
+Expected: no scheduler-related legacy matches. Any unrelated optional-returning lambda must be inspected rather than blindly changed.
+
+### Task 4: Propagate stop tokens through symbol and workspace loops
+
+**Files:**
+- Modify: `lsp-server/src/manager/symbol.cppm`
+- Modify: `lsp-server/src/manager/bootstrap.cppm`
+- Modify: `lsp-server/src/provider/initialize/initialize.cppm`
+- Modify: `lsp-server/src/provider/workspace/execute_command.cppm`
+- Modify: `lsp-server/src/provider/workspace/did_change_workspace_folders.cppm`
+
+- [ ] **Step 1: Add optional stop tokens to long symbol operations**
+
+Use these signatures so synchronous callers need no artificial token while scheduler callers pass one explicitly:
+
+```cpp
+void LoadSystemLibrary(const std::string& lib_path,
+                       std::stop_token stop_token = {});
+void LoadWorkspace(const protocol::DocumentUri& workspace_uri,
+                   std::stop_token stop_token = {});
+void IndexWorkspaceFiles(const std::vector<protocol::DocumentUri>& uris,
+                         std::stop_token stop_token = {});
+void RemoveWorkspaceFiles(const std::vector<protocol::DocumentUri>& uris,
+                          std::stop_token stop_token = {});
+```
+
+At the start of every file/directory loop iteration, return when `stop_token.stop_requested()`. Check once more before acquiring `mutex_` and publishing accumulated map changes, so cancelled work does not publish a partial replacement assembled before cancellation.
+
+- [ ] **Step 2: Make workspace enumeration cooperative**
+
+Change the helper to:
+
+```cpp
+std::vector<protocol::DocumentUri> EnumerateWorkspaceFiles(
+    const protocol::DocumentUri& workspace_uri,
+    std::stop_token stop_token)
+```
+
+Check the token before filesystem setup and at the start of each recursive iterator iteration. The submitted closure must check again before calling `IndexWorkspaceFiles` or `RemoveWorkspaceFiles`, then pass the same token into those methods.
+
+- [ ] **Step 3: Build production and provider targets**
+
+Run:
+
+```bash
+cmake --build lsp-server/build/codex43-clean/Release --target test_provider tsl-server -j1
+```
+
+Expected: both targets build without stale scheduler namespace, closure, callback, or symbol signatures.
+
+### Task 5: Format and verify the complete behavior
+
+**Files:**
+- Modify only files listed in Tasks 1-4 if formatting changes are needed.
+- Modify: `memory-bank/progress.md`
+
+- [ ] **Step 1: Format touched C++ files**
+
+Run `clang-format -i` only on the C++ files changed by this plan. Do not format unrelated dirty files.
+
+- [ ] **Step 2: Rebuild and run focused tests**
+
+Run:
+
+```bash
+cmake --build lsp-server/build/codex43-clean/Release --target test_scheduler test_provider tsl-server -j1
+ctest --test-dir lsp-server/build/codex43-clean/Release -R '^test_scheduler$|^test_provider$' --output-on-failure
+```
+
+Expected: build succeeds and both tests pass.
+
+- [ ] **Step 3: Run LSP transport smoke tests without fixture validation**
+
+Run the repository's existing LSP test entry with `--no-validate` against `lsp-server/build/codex43-clean/Release/src/tsl-server`, using the same invocation recorded by the preceding UTF-16 plan or discover it with `rg -- '--no-validate' lsp-server/test`.
+
+Expected: transport startup, initialize, shutdown, and exit complete successfully.
+
+- [ ] **Step 4: Inspect final diffs and legacy API absence**
+
+Run:
+
+```bash
+git diff --check
+rg -n 'WaitForTask|namespace lsp::scheduler\s*$|scheduler::AsyncExecutor|lsp::scheduler::AsyncExecutor' lsp-server/src/scheduler lsp-server/src/core lsp-server/src/manager lsp-server/src/provider lsp-server/test
+git status --short
+```
+
+Expected: no whitespace errors or legacy scheduler API references; unrelated pre-existing dirty files remain unstaged.
+
+- [ ] **Step 5: Commit the implementation with the required emoji convention**
+
+Stage only the files from Tasks 1-4 and commit:
+
+```bash
+git commit -m ":bug: fix(async_executor): implement cooperative cancellation"
+```
+
+- [ ] **Step 6: Finish the main-loop record and update the human summary**
+
+Run:
+
+```bash
+python docs/standards/playbook/scripts/main_loop.py finish \
+  -plan docs/superpowers/plans/2026-07-12-async-executor-cancellation.md \
+  -status done \
+  -progress memory-bank/progress.md
+```
+
+Then update `memory-bank/progress.md` Current Focus, Recent Changes, Next Steps, and Open Risks with the verified scheduler outcome, stage only that file, and commit:
+
+```bash
+git commit -m ":memo: docs(progress): finish async executor cancellation plan"
+```
@@ -54,7 +54,7 @@
 <!-- workflow-state:start -->
 phase: planning
 spec: docs/superpowers/specs/2026-07-12-async-executor-cancellation-design.md
-plan: docs/superpowers/plans/2026-07-12-text-coordinates-utf16.md
+plan: docs/superpowers/plans/2026-07-12-async-executor-cancellation.md
 executor: executing-plans
 constraints: karpathy-guidelines,.agents,AGENT_RULES
 <!-- workflow-state:end -->