fix(worker): bound blocking ops so hung builtin workers are recoverable (#148)

[marcmantei]

Problem (shipyard#148, P1c)

A builtin worker hung ~26.4h 'running' past its 1800s wall-clock budget, unkillable except by restarting spacebot (P0).

Root cause

The abort machinery already exists — worker.rs::run() has select! vs sleep(1800s), channel.rs tracks worker_handles + cancel_worker_with_reason calls handle.abort(), and spawn_worker_task cleans up on every outcome. But run() polled run_inner inline in the select!. A synchronous, non-yielding stretch inside run_inner (a tool/index path that never reaches an .await) pins that task's poll → the sleep branch is never evaluated, abort() (also .await-gated) can't fire → run() never completes → cleanup never runs → slot/DB row stay running forever.

Fix (one file, surgical)

Drive run_inner on its own tokio task; select! on its JoinHandle vs the timer. The timer + external cancel now run on a task the inner work cannot pin → slot/record freed promptly on timeout. An AbortOnDrop guard tears the inner task down on timeout/completion/cancel. Panic + cancel semantics preserved.

Residual (honest)

A sync-pinned inner still leaks one blocking thread until it returns or the process restarts — true OS-level force-kill needs process isolation (a larger follow-up). P0's restart stays the ultimate backstop; this stops the pool-level zombie, complementing spacedriveapp#147's slot reclamation.

Tests

abort_guard_tests: AbortOnDrop cancels on drop; a sync-pinned spawned task doesn't starve a concurrent select! timer.

⚠️ Build gate

Native deps (lance/fastembed) need the full build env — must be gated by docker compose build spacebot before deploy. refs shipyard#148

[shipyard-ci]

Review: fix(worker): bound blocking ops so hung builtin workers are recoverable

Summary

This PR fixes a critical issue (spacedriveapp#148) where hung builtin workers could survive past their wall-clock timeout by running synchronous, non-yielding code that pins the tokio runtime thread. The fix spawns run_inner on its own dedicated tokio task and wraps it with AbortOnDrop to ensure cleanup on timeout or external cancellation, keeping timers responsive regardless of blocking operations.

Findings

P1 (Blocking)

1. Potential thread leak acknowledged but not mitigated — The PR acknowledges that a sync-pinned inner task leaks one blocking thread until it returns or the process restarts. While the worker pool slot is freed promptly (good), this could accumulate over time in long-running services. Consider documenting a monitoring/alerting strategy or adding a backstop mechanism.

P2 (Should Fix)

1. Test isolation concern — The timer_fires_despite_sync_pinned_inner_task test uses flavor = "multi_thread" with 2 threads to demonstrate the issue, but doesn't verify behavior on single-threaded runtimes. Consider adding a comment explaining why multi-threaded is essential.

2. Panic handling clarity — The code re-raises panics from run_inner via resume_unwind, which is correct, but the error message for the cancelled path ("worker inner task aborted") could be more descriptive about whether this was timeout-driven or externally cancelled.

P3 (Nit)

1. Minor: _abort_inner variable naming — The leading underscore suppresses the "unused" warning, but _abort_guard or similar would be more idiomatic Rust.

2. Test: redundant comment — Line 1580's comment is slightly repetitive with the function docstring; could be condensed.

Security

No security concerns identified. The change is internal task management with no new trust boundaries or input validation changes.

Verdict

APPROVE — The fix correctly addresses the root cause of spacedriveapp#148 with a well-designed solution. Tests are comprehensive and the implementation is sound. The acknowledged thread-leak edge case is acceptable given the process-level backstop. Consider the P2 suggestions for future robustness, but they are not blockers.

---

AI Review · Verdict: COMMENT
Routed as code_review (100%) → github_code_review_flow · View AI traces