Skip to content

fix(terminal): bound hidden exec lifecycle so calls always complete - #6

Open
yoruuuchan wants to merge 1 commit into
AAswordman:masterfrom
yoruuuchan:fix/hidden-exec-lifecycle
Open

yoruuuchan wants to merge 1 commit into
AAswordman:masterfrom
yoruuuchan:fix/hidden-exec-lifecycle

Conversation

@yoruuuchan

Copy link
Copy Markdown

Summary

LocalTerminalProvider.executeHiddenCommand could stay pending forever. The caller budget was applied with withTimeout(timeoutMs), but the phases that can block were outside its guarantee, and cancellation had to join non-cancellable IO children:

  1. getOrCreateHiddenExecShell() ran before the timeout: shell startup, readiness (a fixed internal 30s wait) and the global creation mutex were not bounded by the caller deadline.
  2. The command was written through structured withContext(Dispatchers.IO) { writer.write(...); writer.flush() }. When a hidden shell stops draining its pipe, that write blocks a thread; coroutine cancellation only marks the job and still waits for the child to return.
  3. awaitHiddenExecReady() used its own fixed internal timeout instead of the caller budget.
  4. On timeout the shell was closed asynchronously via hiddenExecScope.launch { ... }, so the next call with the same executorKey could race a half-closed shell while the result read was skipped entirely.

Because of (2) and (4) the calling Kotlin tool never returned, so the ToolPkg Tools.System.terminal.hiddenExec Promise in Operit never settled. QQbot triggers it reliably when it starts its background gateway, but the defect is in the generic hidden executor chain, not in that caller.

Fix

  • executeHiddenCommand now runs prepare, queueing, write and read inside a single withTimeoutOrNull(timeoutMs) budget and reports the phase that expired.
  • The pipe write runs as a cancellable child; the call cancels it instead of joining it, then retires the shell that owned it.
  • closeHiddenExecShell is idempotent (AtomicBoolean + ConcurrentHashMap.remove(key, shell)) and destroys the process before closing the buffered writer, so a blocked writer cannot hold cleanup.
  • A queued call that finds its shell retired by the previous owner re-resolves the key inside the same deadline, so the same key still succeeds afterwards.
  • A process whose creation raced with cancellation is destroyed instead of leaking.
  • disconnect() cancels the scope before closing shells.
  • SSHFileConnectionManager.executeHiddenCommand consumes the same budget for setup, connect() and draining, and disconnects the channel in finally.
  • TerminalProvider.executeHiddenCommand documents that timeoutMs covers the whole lifecycle.

Tests

13 new JVM unit tests in LocalTerminalProviderTest, green on JDK 21 (./gradlew :terminal:testDebugUnitTest from the Operit build):

  • success plus same-key reuse without output mixing
  • start failure does not poison the next call
  • readiness deadline destroys an unready process and still allows both keys
  • blocked process start returns by the deadline and retires its process
  • waiting for the creation mutex consumes the call deadline
  • a queued call timing out does not terminate the active owner
  • a queued call continues on a new shell after the owner times out
  • a stalled writer is deadline-bounded and retired before the next call
  • cancellation retires the shell and the same key works again
  • process exit ends the call without waiting for its deadline
  • reader failure ends the call and retires the shell
  • a split end marker waits for the complete exit code line
  • disconnect rejects further calls promptly without starting processes

Against the unfixed implementation, the readiness, creation-mutex and blocked-write cases fail with a wall-clock TimeoutException — the reported permanent pending behavior.

Hidden exec calls could wait forever while the shell became ready, while the creation mutex was held, or while a blocked pipe write was joined through cancellation, because the caller deadline did not cover those phases.

- Run prepare, queueing, write and read inside the caller deadline and report the phase that expired
- Cancel a blocked writer instead of joining it, and destroy the shell that owned it
- Retire failed, timed-out or cancelled executors through an idempotent close
- Re-resolve the executor key when a queued call finds a retired shell
- Destroy processes whose creation raced with cancellation
- Cover success, start failure, queueing, cancellation, process exit and disconnect paths with JVM unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant