From edea37202d930a70b25258a9cfeb673cbe481331 Mon Sep 17 00:00:00 2001 From: csh Date: Sun, 12 Jul 2026 20:17:33 +0800 Subject: [PATCH] :memo: docs(plan): implement async executor cancellation --- .../2026-07-12-async-executor-cancellation.md | 374 ++++++++++++++++++ memory-bank/progress.md | 2 +- 2 files changed, 375 insertions(+), 1 deletion(-) create mode 100644 docs/superpowers/plans/2026-07-12-async-executor-cancellation.md diff --git a/docs/superpowers/plans/2026-07-12-async-executor-cancellation.md b/docs/superpowers/plans/2026-07-12-async-executor-cancellation.md new file mode 100644 index 0000000..3ec75c2 --- /dev/null +++ b/docs/superpowers/plans/2026-07-12-async-executor-cancellation.md @@ -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{ "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{}; + }); + blocker_started.wait(); + std::atomic ran{ false }; + auto queued = executor.Submit("queued", [&](std::stop_token) { + ran = true; + return std::optional{}; + }); + 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 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 Wait() const; + std::optional TryGetResult() const; + + private: + struct State; + explicit TaskHandle(std::shared_ptr state); + std::shared_ptr 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::stop_token)>; +using TaskCallback = std::function; + +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(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 { + 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|const std::optional&.*, 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& uris, + std::stop_token stop_token = {}); +void RemoveWorkspaceFiles(const std::vector& 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 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" +``` diff --git a/memory-bank/progress.md b/memory-bank/progress.md index 075c016..b835708 100644 --- a/memory-bank/progress.md +++ b/memory-bank/progress.md @@ -54,7 +54,7 @@ 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