feat: background task architecture refactoring#65
Merged
yishuiliunian merged 10 commits intomainfrom Apr 3, 2026
Merged
Conversation
…cOutcome, TUI injection Resolves 6 architectural issues in the background task system: 1. (P0) Remove backend → tool-background reverse dependency: exec_background now returns opaque ProcessHandle; bash tool registers in store via bg_convert::register_spawned. 2. (P0) Decouple TUI from tool-background: new BgTaskSnapshot/BgTaskStatus protocol types; App.bg_provider injection replaces direct store access; bootstrap injects snapshot_running at startup. 3. (P1) Replace ToolIoError::TimedOutProcess with ExecOutcome enum: exec_streaming returns ExecOutcome::Completed | TimedOut, making timeout a valid control-flow outcome rather than an error variant. 4. (P2) Unify lock order in bg_stop/monitor: consistently acquire child before status; monitor only updates status when still Running. 5. (P3) Add zero-height guard in render.rs: skip panel split when layout.agents.height == 0 to prevent small-terminal rendering issues. 6. (P3) Add reader task cleanup via AbortHandle: shell_stream stores abort handles in TimedOutProcessData; monitor aborts stragglers after child exits; watch_tx always fires so blocking bg_output callers wake up.
- Fix ~30 collapsible-if warnings across the workspace by converting nested if/if-let to Rust let-chain syntax (if cond && let pat = ...) - Add snapshot_running() unit tests (5 tests) - Add streaming timeout integration tests (2 tests) - Add render zero-height guard tests (2 tests) - Remove unused import in snapshot_test
The monitor was aborting reader tasks immediately after child exit, causing a race where short-lived commands (like `echo bg_hello`) would have their output lost before the reader could write it to the buffer. Now waits 50ms for pipes to drain before aborting stragglers. Also passes abort handles from register_spawned for proper cleanup.
bg_stop now always returns success with "stopped" regardless of whether the monitor already set a terminal status. Fixes flaky CI test where the monitor races to set Failed before bg_stop checks status.
Increase sleep before stop from 100ms to 500ms to reduce flakiness on slow CI runners where the child process may not have started yet.
…diagnostics - path::resolve now rejects absolute write paths outside cwd even without sandbox policy (defence-in-depth). This fixes the pre-existing file-ops path traversal test failures. - Improve bg_stop test assertion messages for CI debugging.
All background task tests share a global LazyLock store. Without serialization, snapshot_test::clear_store() races with other tests, causing "Process not found" errors on CI. Add BG_STORE_LOCK to suite.rs and acquire in all test functions. Also fix pre-existing file-ops path traversal failures by enforcing cwd containment for absolute write paths without sandbox policy.
…e instance The global `LazyLock<Mutex<HashMap>>` caused test flakiness: parallel tests sharing a single store, with `clear_store()` wiping other tests' data. Prior workarounds (BG_STORE_LOCK, #[allow(clippy::await_holding_lock)]) masked the architectural issue. Now: - `BackgroundTaskStore` is an injectable Arc instance, not a global - `BashTool::new(store)` receives the store at construction - `register_all(registry, bg_store)` threads it through Kernel - TUI's `App.bg_store` replaces the `bg_provider` function pointer - Each test creates its own `BackgroundTaskStore::new()` — fully isolated - Zero `clear_store()`, zero `BG_STORE_LOCK`, zero `allow(clippy)` hacks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ToolIoError::TimedOutProcesserror variant withExecOutcome::Completed | TimedOut, making timeout-to-background a valid control flowChanges
New files (12):
loopal-protocol/src/bg_task.rs—BgTaskSnapshot,BgTaskStatustypesloopal-tui/src/panel_ops.rs— unified panel focus navigationloopal-tui/src/views/bg_tasks_panel.rs— background tasks panel viewbash/src/bg_convert.rs— timed-out/spawned process → store registrationbash/src/bg_monitor.rs— monitor spawning, AbortHandle cleanupbash/src/bg_ops.rs— bg_output / bg_stop operationsbash/src/format.rs— output formatting helpersModified layers (bottom-up):
loopal-error: RemoveTimedOutProcessvariant; keepProcessHandleforExecOutcomeloopal-tool-api: AddExecOutcomeenum;exec_streamingreturnsResult<ExecOutcome>;exec_backgrounddropsdescparamloopal-backend:shell_streamreturnsExecOutcome::TimedOutwithAbortHandle;shell::exec_backgroundreturnsSpawnedBackgroundDatatool-background: Addsnapshot_running()returning protocol typesbash tool: MatchExecOutcomevariants; register viabg_convert; unified lock orderloopal-tui:App.bg_providerinjection;bg_tasks_panelreads&[BgTaskSnapshot]; zero-height render guardbootstrap: Injectsnapshot_runninginto TUITest plan
bazel build //...passesbazel test //...— 47/48 pass (1 pre-existing file-ops failure)