Add overall plans

Mikayla Maki created 2 days ago

Change summary

async-app-result-removal-tracker.json | 288 +++++++++++++++++++++++++++++
async-app-result-removal.md           | 241 ++++++++++++++++++++++++
2 files changed, 529 insertions(+)

Detailed changes

async-app-result-removal-tracker.json

@@ -0,0 +1,288 @@
+{
+  "project": "async-app-result-removal",
+  "description": "Remove Result from AsyncApp return types by moving app-alive checks to executor",
+  "phases": [
+    {
+      "id": 1,
+      "name": "Add Trampoline Check Infrastructure",
+      "status": "not_started",
+      "tasks": [
+        {
+          "id": "1.1",
+          "description": "Add Option<Weak<AppCell>> field to RunnableMeta struct",
+          "file": "crates/gpui/src/executor.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "1.2",
+          "description": "Update trampoline function in Mac dispatcher to check app status before run()",
+          "file": "crates/gpui/src/platform/mac/dispatcher.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "1.3",
+          "description": "Update trampoline function in Linux dispatcher to check app status before run()",
+          "file": "crates/gpui/src/platform/linux/dispatcher.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "1.4",
+          "description": "Update trampoline function in Windows dispatcher to check app status before run()",
+          "file": "crates/gpui/src/platform/windows/dispatcher.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "1.5",
+          "description": "Update trampoline function in Test dispatcher to check app status before run()",
+          "file": "crates/gpui/src/platform/test/dispatcher.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "1.6",
+          "description": "Modify ForegroundExecutor::spawn_with_priority to accept optional app weak pointer",
+          "file": "crates/gpui/src/executor.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "1.7",
+          "description": "Update AsyncApp::spawn to pass app weak pointer to executor",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "1.8",
+          "description": "Write unit test verifying task cancellation when app is dropped",
+          "file": "crates/gpui/src/executor.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "1.9",
+          "description": "Write unit test for nested tasks both cancelling cleanly",
+          "file": "crates/gpui/src/executor.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "1.10",
+          "description": "Create async_cancellation.rs example demonstrating behavior",
+          "file": "crates/gpui/examples/async_cancellation.rs",
+          "status": "not_started"
+        }
+      ]
+    },
+    {
+      "id": 2,
+      "name": "Update AsyncApp API",
+      "status": "not_started",
+      "tasks": [
+        {
+          "id": "2.1",
+          "description": "Rename update() -> Result<R> to try_update() -> Option<R>",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "2.2",
+          "description": "Add new update() -> R that panics if app is gone",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "2.3",
+          "description": "Apply try_/non-try pattern to read_entity and update_entity",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "2.4",
+          "description": "Apply try_/non-try pattern to read_global and update_global",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "2.5",
+          "description": "Apply try_/non-try pattern to read_window and update_window",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "2.6",
+          "description": "Apply try_/non-try pattern to new, reserve_entity, insert_entity",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "2.7",
+          "description": "Apply try_/non-try pattern to refresh, open_window, subscribe, has_global",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "2.8",
+          "description": "Update AppContext trait Result associated type for AsyncApp",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "2.9",
+          "description": "Apply same changes to AsyncWindowContext",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        }
+      ]
+    },
+    {
+      "id": 3,
+      "name": "Audit Cross-Boundary Awaits",
+      "status": "not_started",
+      "tasks": [
+        {
+          "id": "3.1",
+          "description": "Search for background tasks awaiting foreground tasks",
+          "file": null,
+          "status": "not_started",
+          "notes": "grep for patterns like background_executor().spawn(...).await"
+        },
+        {
+          "id": "3.2",
+          "description": "Identify all Task<T> values passed across thread boundaries",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "3.3",
+          "description": "Migrate identified cases to use try_update or handle Option",
+          "file": null,
+          "status": "not_started"
+        }
+      ]
+    },
+    {
+      "id": 4,
+      "name": "Codebase Migration",
+      "status": "not_started",
+      "tasks": [
+        {
+          "id": "4.1",
+          "description": "Update callsites in crates/gpui",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "4.2",
+          "description": "Update callsites in crates/editor",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "4.3",
+          "description": "Update callsites in crates/workspace",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "4.4",
+          "description": "Update callsites in crates/project",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "4.5",
+          "description": "Update callsites in crates/language",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "4.6",
+          "description": "Update callsites in crates/agent and crates/agent_ui",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "4.7",
+          "description": "Update callsites in remaining crates",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "4.8",
+          "description": "Fix all compilation errors",
+          "file": null,
+          "status": "not_started"
+        }
+      ]
+    },
+    {
+      "id": 5,
+      "name": "Testing and Cleanup",
+      "status": "not_started",
+      "tasks": [
+        {
+          "id": "5.1",
+          "description": "Remove dead error handling code",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "5.2",
+          "description": "Update AsyncApp documentation",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "5.3",
+          "description": "Add test: try_update returns None when app gone",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "5.4",
+          "description": "Add test: update works normally when app alive",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "5.5",
+          "description": "Add test: spawn_executes_when_app_alive",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "5.6",
+          "description": "Add test: update_entity works via AsyncApp",
+          "file": "crates/gpui/src/app/async_context.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "5.7",
+          "description": "Verify async_cancellation example runs without panics",
+          "file": "crates/gpui/examples/async_cancellation.rs",
+          "status": "not_started"
+        },
+        {
+          "id": "5.8",
+          "description": "Run cargo test -p gpui",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "5.9",
+          "description": "Run ./script/clippy",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "5.10",
+          "description": "Run full test suite and fix any regressions",
+          "file": null,
+          "status": "not_started"
+        },
+        {
+          "id": "5.11",
+          "description": "Manual test: close windows with pending tasks, verify no panics",
+          "file": null,
+          "status": "not_started"
+        }
+      ]
+    }
+  ]
+}

async-app-result-removal.md

@@ -0,0 +1,241 @@
+# Project: Remove Result from AsyncApp Return Types
+
+## Problem
+
+`AsyncApp` holds a `Weak<AppCell>` to avoid leaking the app through cyclic references. Every method must upgrade this weak pointer and return `Result<T>` to handle the case where the app was dropped. This pushes error handling complexity to every callsite throughout the codebase.
+
+```rust
+// Current: Every call requires error handling
+cx.update(|app| { ... })?
+cx.update(|app| { ... }).ok();
+cx.update(|app| { ... }).unwrap();
+```
+
+## Solution
+
+Move the app-alive check into the executor's task execution path. Before running any task that needs the app, check if the app is still alive. If not, cancel the task by dropping its runnable. This guarantees that any code inside a foreground task only executes when the app is alive, allowing us to remove the `Result` wrapper.
+
+## Technical Background
+
+### How async-task Works
+
+The `async-task` crate's `spawn()` returns a `(Runnable, Task)` pair:
+- `Runnable`: Executes the future via `runnable.run()`
+- `Task`: A handle to await the future's output
+- Dropping a `Runnable` cancels the task
+- The `schedule` closure determines where/when the runnable is dispatched
+
+### Current Executor Architecture
+
+Tasks are spawned with metadata and a schedule closure:
+```rust
+async_task::Builder::new()
+    .metadata(RunnableMeta { location })
+    .spawn(
+        move |_| future,
+        move |runnable| {
+            dispatcher.dispatch(RunnableVariant::Meta(runnable), label, priority)
+        },
+    )
+```
+
+The dispatcher queues the runnable, and a trampoline function executes it:
+```rust
+extern "C" fn trampoline(runnable: *mut c_void) {
+    let task = unsafe { Runnable::<RunnableMeta>::from_raw(...) };
+    task.run();  // <-- We add the check BEFORE this
+}
+```
+
+### Why This Is Safe
+
+1. **Foreground tasks run on the main thread.** The App also lives on the main thread. While a task runs synchronously, nothing else on the main thread can run, so the App cannot be dropped mid-execution.
+
+2. **The check happens before each poll.** Between await points, the task yields. When rescheduled, the trampoline checks the app status again. If dead, the task is cancelled before user code runs.
+
+3. **Recursive cancellation handles nested awaits.** If an outer foreground task awaits an inner foreground task and the app dies:
+   - Inner task scheduled → trampoline sees app dead → cancels (drops runnable)
+   - Outer task woken → scheduled → trampoline sees app dead → cancels
+   - Neither task's code runs after app death → no panic
+
+4. **Background tasks cannot hold AsyncApp.** `AsyncApp` contains `Weak<Rc<...>>` which is `!Send`, so it cannot be moved into a `Send` future on a background thread.
+
+### The One Edge Case
+
+A background task (no app check) awaiting a foreground task (has app check):
+- Foreground task cancelled when app dies
+- Background task's await sees cancellation → panic
+
+Solution: Provide `try_update()` returning `Option<T>` for these cases.
+
+## API Changes
+
+### AsyncApp Methods
+
+Rename existing methods and add non-Result versions:
+
+| Old Signature | New Signature |
+|--------------|---------------|
+| `fn update(...) -> Result<R>` | `fn update(...) -> R` (panics if app gone) |
+| (new) | `fn try_update(...) -> Option<R>` (returns None if app gone) |
+
+Apply the same pattern to:
+- `update`
+- `read_entity` / `update_entity`
+- `read_global` / `update_global`
+- `read_window` / `update_window`
+- `new` / `reserve_entity` / `insert_entity`
+- `refresh`
+- `open_window`
+- `subscribe`
+- `has_global`
+
+### AppContext Trait
+
+Change the associated type:
+```rust
+// Before
+impl AppContext for AsyncApp {
+    type Result<T> = Result<T>;
+}
+
+// After
+impl AppContext for AsyncApp {
+    type Result<T> = T;
+}
+```
+
+### RunnableMeta
+
+Add optional app weak pointer:
+```rust
+pub struct RunnableMeta {
+    pub location: &'static Location<'static>,
+    pub app: Option<Weak<AppCell>>,  // NEW
+}
+```
+
+## Implementation Phases
+
+### Phase 1: Add Trampoline Check Infrastructure
+
+1. Modify `RunnableMeta` to include `Option<Weak<AppCell>>`
+2. Update trampoline functions in all dispatchers to check app status before running:
+   - `crates/gpui/src/platform/mac/dispatcher.rs`
+   - `crates/gpui/src/platform/linux/dispatcher.rs`
+   - `crates/gpui/src/platform/windows/dispatcher.rs`
+   - `crates/gpui/src/platform/test/dispatcher.rs`
+3. Modify spawn paths in `AsyncApp` to populate the app weak pointer in metadata
+
+### Phase 2: Update AsyncApp API
+
+1. Rename `update() -> Result<R>` to `try_update() -> Option<R>`
+2. Add new `update() -> R` that panics if app is gone
+3. Apply same pattern to all fallible methods
+4. Update `AppContext` trait's associated `Result` type
+
+### Phase 3: Audit Cross-Boundary Awaits
+
+Search for patterns where background code awaits foreground tasks:
+```
+background_executor().spawn(...).await  // awaiting foreground work
+```
+
+Migrate these to use `try_update()` or handle the `Option`.
+
+Known patterns to check:
+- `cx.spawn(...).await` called from background context
+- Channels receiving from foreground tasks
+- Any `Task<T>` held across thread boundaries
+
+### Phase 4: Codebase Migration
+
+1. Update all callsites to remove `.unwrap()`, `?`, `.ok()` from `update()` calls
+2. Use `try_update()` where `Option` handling is needed
+3. This is a large mechanical change across ~500+ callsites
+
+### Phase 5: Cleanup
+
+1. Remove now-dead error handling code
+2. Update documentation
+3. Add tests validating cancellation behavior
+
+## Files to Modify
+
+### Core Changes
+- `crates/gpui/src/app/async_context.rs` - AsyncApp implementation
+- `crates/gpui/src/executor.rs` - RunnableMeta, spawn functions
+- `crates/gpui/src/platform.rs` - RunnableVariant if needed
+- `crates/gpui/src/gpui.rs` - AppContext trait
+
+### Dispatcher Changes
+- `crates/gpui/src/platform/mac/dispatcher.rs` - trampoline function
+- `crates/gpui/src/platform/linux/dispatcher.rs` - trampoline function
+- `crates/gpui/src/platform/windows/dispatcher.rs` - trampoline function
+- `crates/gpui/src/platform/test/dispatcher.rs` - trampoline function
+
+### Realtime Tasks (No Changes Needed)
+Realtime/audio tasks use a separate code path with their own thread and channel. They don't use `AsyncApp` and won't be affected.
+
+## Testing Strategy
+
+### Unit Tests (`crates/gpui/src/executor.rs`)
+
+**`test_task_cancelled_when_app_dropped`**
+Verifies the core mechanism: tasks don't execute after the app is gone.
+
+Test spec:
+```
+spawn task that sets flag on completion
+run_until_parked (task starts, blocks on channel)
+quit app
+assert flag was never set
+```
+
+**`test_nested_tasks_both_cancel`**
+Validates recursive cancellation: awaiting a cancelled task doesn't panic when both have app checks.
+
+Test spec:
+```
+spawn outer task that awaits inner task
+both tasks set flags if they run after quit
+quit app
+assert neither flag was set (no panic from awaiting cancelled inner)
+```
+
+**`test_try_update_returns_none_when_app_gone`**
+Confirms the fallback API works for edge cases like background-awaits-foreground.
+
+Test spec:
+```
+get async_cx, quit app
+call try_update
+assert returns None
+```
+
+### GPUI Example (`crates/gpui/examples/async_cancellation.rs`)
+
+Interactive demo for manual validation of cancellation behavior.
+```
+window with "Spawn Task" button
+each task runs 10-second timer, updating status each second
+closing window cancels pending tasks (no panic, no zombie tasks)
+```
+
+Run with: `cargo run -p gpui --example async_cancellation`
+
+### Integration Tests (`crates/gpui/src/app/async_context.rs`)
+
+**`test_spawn_executes_when_app_alive`**
+Basic spawn/await still works.
+
+**`test_update_entity_works`**
+Entity operations via AsyncApp still work.
+
+### Validation Checklist
+
+1. `cargo test -p gpui` passes
+2. `cargo run -p gpui --example async_cancellation` works correctly
+3. `./script/clippy` passes
+4. Full test suite `cargo test` passes
+5. Manual test: close windows with pending tasks, verify no panics