refactor: harden async runtime and UI boundaries

docs/performance-audit.md

@@ -0,0 +1,82 @@
+# Runtime and maintainability audit
+
+This audit reviewed transport, event batching and subscriptions, timers, file completion,
+promise settlement, history persistence, snapshot/review commands, and renderer update
+primitives. It includes targeted fixes and regression tests. It is not a guarantee that
+every runtime or platform combination is free of defects.
+
+## Fixed
+
+| Priority | Finding | Change |
+| --- | --- | --- |
+| High | File completion synchronously waited for server responses and shell searches. Server errors prevented fallback; Lua also treats `executable()` returning `0` as true. | Yield through promises, catch failed searches, skip unavailable executables, and enforce the configured result limit. |
+| High | Delayed server-ready callbacks could reconnect after stop; an old server's shutdown could close the replacement subscription. Buffered and late events could cross subscription boundaries. | Guard callbacks with lifecycle/subscription identities, check the current server, and clear pending batches and cached parts at subscription cleanup. |
+| High | The server test suite passed fake PIDs to real process enumeration and signal functions. The first full run terminated with exit 143. | Stub both OS operations for server tests, including restoration after each test. The termination's precise cause was not independently proven. |
+| Medium | Collapsing repeated part updates rescanned all intervening events, producing quadratic work. | Track the most recent permission-event index, preserving the existing permission ordering rule in constant time per update. |
+| Medium | Streaming line extraction copied the remaining buffer for each line. | Scan by offset and retain the remaining suffix once per chunk. Extremely long, incomplete lines can still cause repeated concatenation across chunks. |
+| Medium | HTTP parsing stopped at a proxy CONNECT or informational response, potentially reporting the wrong status and treating final headers as body text. | Consume preliminary header blocks and parse the final response; preserve bodies that happen to resemble HTTP headers. |
+| Medium | A cleared throttle callback could consume a new batch before its own deadline. | Invalidate cancelled callbacks using a generation counter. |
+| Medium | Queued timer ticks could run after stop/restart; an old tick could stop a replacement timer created inside the callback. | Check timer identity before invocation and before stopping. |
+| Medium | A listener unsubscribing itself during event emission could cause the next listener to be skipped. | Iterate a snapshot of the listener list. |
+
+## Measurements and verification
+
+`./run_tests.sh`: **1,224 passed, zero failed**, including minimal, unit, and replay tests.
+The sandboxed run could not create Neovim swap files; the successful complete run used
+approved filesystem access. Targeted tests cover pending completion responses, fallback,
+subscription replacement, timer restart, cancellation, HTTP headers, and stream boundaries.
+Changed Lua files were formatted with the repository's StyLua configuration; `git diff --check` passed.
+
+A synthetic benchmark compared the original and updated event manager in headless Neovim.
+Each batch contained repeated full updates to one text part; event delivery was disabled
+to isolate normalization and collapsing. Results are the median of five runs, with GC
+before each run, on the audit machine:
+
+| Updates per batch | Before | After |
+| --- | ---: | ---: |
+| 1,000 | 1.49 ms | 0.67 ms |
+| 4,000 | 18.05 ms | 2.33 ms |
+| 8,000 | 78.79 ms | 4.80 ms |
+
+This demonstrates the removed quadratic scan. It is not an end-to-end rendering or typing
+latency measurement. Live server, Windows shell, and large-repository interactive testing
+remain useful follow-up validation.
+
+## Remaining findings, in recommended order
+
+1. **Addressed: history encoding and deletion.** New writes use JSON lines in `history.jsonl`,
+   with legacy `history.txt` reading and migration on the first write. Existing ambiguous
+   legacy records retain their previous interpretation; the legacy file remains intact.
+   Deletion deduplicates indices and commits rewrites atomically without mutating the cache
+   on failure. Regression tests cover round trips, migration, clear, duplicate indices and
+   failed writes.
+2. **Addressed: snapshot/review and API startup waits.** Snapshot APIs now return promises;
+   revert operations resolve to `{ id, deleted_files }`. Callers await Git processes and
+   picker choices, and command handlers return the complete promise chain. Operations capture
+   their session/directory and serialize access to each snapshot index. Stale review results
+   are ignored. API startup and version detection yield, with cancellation available before
+   an event subscription starts. Real temporary Git repositories exercise revert and recovery;
+   mocked delayed processes exercise concurrency and workspace switches. Checkout errors no
+   longer imply that a file should be deleted, and diff previews preserve original bytes.
+3. **Addressed: promise retention and falsy rejections.** Settlement clears both callback
+   queues and the waiting coroutine list after scheduling consumers. Rejection has its own
+   state flag, preserving `false` and `nil` rejection reasons consistently through chaining,
+   `finally`, synchronous waiting and coroutine awaiting. Both early and late consumers have
+   regression coverage.
+4. **Architecture remains tightly coupled.** The required topology scanner reports **5 cycles**,
+   a **largest strongly connected component of 41 modules**, **18 policy violations**, and
+   **11 ungrouped modules**. The HEAD-to-worktree diff adds/removes zero dependency edges and
+   introduces zero violations. Of the violations, 12 are capability-to-entry dependencies,
+   4 entry-to-infrastructure, and 2 capability-to-dispatch. Prioritize renderer/formatter
+   dependencies on permission/question windows and the `ui.ui` orchestration boundary.
+   Expand scanner group coverage before using its totals as a comprehensive architecture gate.
+
+The remaining findings above are based on source inspection, not newly added reproductions.
+They are intentionally recorded separately from the tested fixes.
+
+Scanner commands (install `scripts/dependency-topology/requirements.txt` first):
+
+```sh
+python3 scripts/dependency-topology/scan_topology.py scan --json
+python3 scripts/dependency-topology/scan_topology.py diff --from HEAD --to worktree --json
+```

lua/opencode/api_client.lua

@@ -1,4 +1,5 @@
 local server_job = require('opencode.server_job')
+local Promise = require('opencode.promise')
 local state = require('opencode.state')
 local url_encode = require('opencode.util').url_encode
 local apply_path_map = require('opencode.util').apply_path_map
@@ -65,47 +66,54 @@ local function normalize_global_event(event)
   }
 end
 
----Ensure that base_url is set. Even thought we're subscribed to
----state.opencode_server, we still need this check because
----it's possible someone will try to make an api call in their event
----handler (e.g. event_manager or header)
----@return boolean
-function OpencodeApiClient:_ensure_base_url()
-  -- NOTE: eventhough we're subscribed opencode_server, we need this check for
-  -- base_url because the notification about opencode_server being set to
-  -- non-nil my not have gotten to us in time
+---@return Promise<boolean>
+OpencodeApiClient._ensure_base_url = Promise.async(function(self)
   if self.base_url then
     return true
   end
-
-  if not state.opencode_server then
-    -- this is last resort - try to start the server and could be blocking
-    state.jobs.set_server(server_job.ensure_server():wait() --[[@as OpencodeServer]])
-    -- shouldn't normally happen but prevents error in replay tester
-    if not state.opencode_server then
+  if self._connecting then
+    return self._connecting:await()
+  end
+  local connecting = Promise.new()
+  self._connecting = connecting
+  local ok, result = pcall(function()
+    local server = state.opencode_server or server_job.ensure_server():await()
+    if not server then
       return false
     end
-  end
-
-  if not state.opencode_server.url then
-    state.opencode_server:get_spawn_promise():wait()
-    if not state.opencode_server.url then
+    if not server.url then
+      server:get_spawn_promise():await()
+    end
+    if not server.url then
       return false
     end
+    if state.opencode_server and state.opencode_server ~= server then
+      error('Server changed while connecting')
+    end
+    self.base_url = server.url:gsub('/$', '')
+    return true
+  end)
+  self._connecting = nil
+  if not ok then
+    connecting:reject(result)
+    error(result, 0)
   end
-
-  self.base_url = state.opencode_server.url:gsub('/$', '')
-  return true
-end
+  connecting:resolve(result)
+  return result
+end)
 
 --- Make a typed API call
 --- @param endpoint string The API endpoint path
 --- @param method string|nil HTTP method (default: 'GET')
 --- @param body table|nil|boolean Request body
 --- @param query table|nil Query parameters
 --- @return Promise<any> promise
-function OpencodeApiClient:_call(endpoint, method, body, query)
-  if not self:_ensure_base_url() then
+OpencodeApiClient._call = Promise.async(function(self, endpoint, method, body, query)
+  if query then
+    query = vim.deepcopy(query)
+    query.directory = query.directory or state.current_cwd or vim.fn.getcwd()
+  end
+  if not self:_ensure_base_url():await() then
     return require('opencode.promise').new():reject('No server base url')
   end
   local url = self.base_url .. endpoint
@@ -137,7 +145,7 @@ function OpencodeApiClient:_call(endpoint, method, body, query)
   return server_job.call_api(url, method, body):and_then(function(result)
     return reverse_transform_paths_recursive(result)
   end)
-end
+end)
 
 -- Project endpoints
 
@@ -207,10 +215,7 @@ end
 --- directories instead of being filtered to the current cwd.
 --- @return Promise<GlobalSession[]>
 function OpencodeApiClient:list_sessions_global()
-  if not self:_ensure_base_url() then
-    return require('opencode.promise').new():reject('No server base url')
-  end
-  return server_job.call_api(self.base_url .. '/experimental/session', 'GET')
+  return self:_call('/experimental/session', 'GET')
 end
 
 --- Create a new session
@@ -527,66 +532,53 @@ end
 --- @param on_event fun(event: table) Event callback
 --- @return table The streaming job handle
 function OpencodeApiClient:subscribe_to_events(directory, on_event)
-  -- Make sure we have a base URL before attempting to subscribe. If we
-  -- cannot determine a base URL (server not running), return nil so
-  -- callers can handle the absence of a subscription without an error.
-  if not self:_ensure_base_url() then
-    return nil
-  end
-
-  local version = assert(state.opencode_cli_version):wait()
-  if is_version_greater_or_equal(version, '1.14.42') then
-    return self:_subscribe_to_global_events(directory, on_event)
-  end
-
-  local url = self.base_url .. '/event'
-  if directory then
-    local mapped_directory = apply_path_map(directory)
-    url = url .. '?directory=' .. url_encode(mapped_directory)
-  end
-
-  return server_job.stream_api(url, 'GET', nil, function(chunk)
-    chunk = chunk:gsub('^data:%s*', '')
-    local ok, event = pcall(vim.json.decode, vim.trim(chunk))
-    if ok and event then
-      local transformed_event = reverse_transform_paths_recursive(event)
-      on_event(transformed_event --[[@as table]])
-    end
-  end)
-end
-
---- Subscribe to events (streaming)
---- @param directory string|nil Directory path
---- @param on_event fun(event: table) Event callback
---- @return table The streaming job handle
-function OpencodeApiClient:_subscribe_to_global_events(directory, on_event)
-  -- Ensure base_url is available. If not, return nil instead of erroring.
-  if not self:_ensure_base_url() then
-    return nil
-  end
-
-  local version = assert(state.opencode_cli_version):wait()
-  if not is_version_greater_or_equal(version, '1.14.42') then
-    error('subscribe_to_global_events should not be called directly')
-  end
-
-  local url = self.base_url .. '/global/event'
-  if directory then
-    local mapped_directory = apply_path_map(directory)
-    url = url .. '?directory=' .. url_encode(mapped_directory)
-  end
-
-  return server_job.stream_api(url, 'GET', nil, function(chunk)
-    chunk = chunk:gsub('^data:%s*', '')
-    local ok, event = pcall(vim.json.decode, vim.trim(chunk))
-    if ok and event then
-      local normalized_event = normalize_global_event(event)
-      if normalized_event then
-        local transformed_event = reverse_transform_paths_recursive(normalized_event)
-        on_event(transformed_event --[[@as table]])
+  local stopped = false
+  local job
+  local handle = {
+    shutdown = function()
+      stopped = true
+      if job and job.shutdown then
+        job:shutdown()
       end
+    end,
+    is_running = function()
+      return not stopped and (not job or not job.is_running or job:is_running())
+    end,
+  }
+  Promise.spawn(function()
+    if not self:_ensure_base_url():await() or stopped then
+      stopped = true
+      return
+    end
+    local version = assert(state.opencode_cli_version):await()
+    if stopped then
+      return
+    end
+    local global = is_version_greater_or_equal(version, '1.14.42')
+    local url = self.base_url .. (global and '/global/event' or '/event')
+    if directory then
+      url = url .. '?directory=' .. url_encode(apply_path_map(directory))
     end
+    job = server_job.stream_api(url, 'GET', nil, function(chunk)
+      if stopped then
+        return
+      end
+      chunk = chunk:gsub('^data:%s*', '')
+      local ok, event = pcall(vim.json.decode, vim.trim(chunk))
+      if ok and event then
+        if global then
+          event = normalize_global_event(event)
+        end
+        if event then
+          on_event(reverse_transform_paths_recursive(event))
+        end
+      end
+    end)
+  end):catch(function(err)
+    stopped = true
+    require('opencode.log').notify('Failed to subscribe to events: ' .. vim.inspect(err), vim.log.levels.ERROR)
   end)
+  return handle
 end
 
 -- Skill endpoints

lua/opencode/commands/handlers/diff.lua

@@ -14,8 +14,8 @@ local function with_output_open(callback, open_if_closed)
   local open_fn = open_if_closed and session_runtime.open_if_closed or session_runtime.open
   return function(...)
     local args = { ... }
-    open_fn({ new_session = false, focus = 'output' }):and_then(function()
-      callback(unpack(args))
+    return open_fn({ new_session = false, focus = 'output' }):and_then(function()
+      return callback(unpack(args))
     end)
   end
 end
@@ -43,30 +43,30 @@ end
 ---@param from_snapshot_id? string
 ---@param _to_snapshot_id? string|number
 M.actions.diff_open = with_output_open(function(from_snapshot_id, _to_snapshot_id)
-  git_review.review(extract_hash_arg(from_snapshot_id))
+  return git_review.review(extract_hash_arg(from_snapshot_id))
 end, true)
 
 M.actions.diff_next = with_output_open(function()
-  git_review.next_diff()
+  return git_review.next_diff()
 end, false)
 
 M.actions.diff_prev = with_output_open(function()
-  git_review.prev_diff()
+  return git_review.prev_diff()
 end, false)
 
 M.actions.diff_close = with_output_open(function()
-  git_review.close_diff()
+  return git_review.close_diff()
 end, false)
 
 ---@param from_snapshot_id? string
 M.actions.diff_revert_all = with_output_open(function(from_snapshot_id)
-  git_review.revert_all(from_snapshot_id)
+  return git_review.revert_all(from_snapshot_id)
 end, false)
 
 ---@param from_snapshot_id? string
 ---@param _to_snapshot_id? string
 M.actions.diff_revert_selected_file = with_output_open(function(from_snapshot_id, _to_snapshot_id)
-  git_review.revert_selected_file(from_snapshot_id)
+  return git_review.revert_selected_file(from_snapshot_id)
 end, false)
 
 ---@return string|nil
@@ -87,12 +87,12 @@ M.actions.diff_revert_all_last_prompt = with_output_open(function()
     return
   end
 
-  git_review.revert_all(snapshot_id)
+  return git_review.revert_all(snapshot_id)
 end, false)
 
 ---@param snapshot_id? string
 M.actions.diff_revert_this = with_output_open(function(snapshot_id)
-  git_review.revert_current(snapshot_id)
+  return git_review.revert_current(snapshot_id)
 end, false)
 
 M.actions.diff_revert_this_last_prompt = with_output_open(function()
@@ -101,21 +101,21 @@ M.actions.diff_revert_this_last_prompt = with_output_open(function()
     return
   end
 
-  git_review.revert_current(snapshot_id)
+  return git_review.revert_current(snapshot_id)
 end, false)
 
 ---@param restore_point_id? string
 M.actions.diff_restore_snapshot_file = with_output_open(function(restore_point_id)
-  git_review.restore_snapshot_file(restore_point_id)
+  return git_review.restore_snapshot_file(restore_point_id)
 end, false)
 
 ---@param restore_point_id? string
 M.actions.diff_restore_snapshot_all = with_output_open(function(restore_point_id)
-  git_review.restore_snapshot_all(restore_point_id)
+  return git_review.restore_snapshot_all(restore_point_id)
 end, false)
 
 M.actions.set_review_breakpoint = with_output_open(function()
-  git_review.create_snapshot()
+  return git_review.create_snapshot()
 end, false)
 
 ---@type table<string, fun(): any>