feat: spawn worker process #4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive worker process management feature that allows users to configure, start, stop, and monitor a custom worker process directly from the Standalone app's Settings page. The implementation spans backend (Rust/Tauri), frontend (Svelte), and includes improvements to the TypeScript sample client for better CLI integration.
- Adds full lifecycle management for worker processes with crash detection and log streaming
- Extends Settings UI with worker controls showing PID, status indicators, and real-time logs
- Updates TypeScript sample client to use environment variables for database and extension paths
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| standalone/src-tauri/src/worker.rs | Complete worker process implementation with state management, crash tracking, command parsing, and log buffering |
| standalone/src-tauri/src/lib.rs | Registers worker commands, initializes WorkerState, and implements auto-start on launch |
| standalone/src-tauri/src/dev_api.rs | Adds tRPC endpoints for worker status, logs, and control operations |
| standalone/src-tauri/src/db_commands.rs | Extends SettingsInfo to include database file size |
| standalone/src-tauri/src/db.rs | Adds helper function to resolve SQLite extension path for worker environment |
| standalone/src-tauri/Cargo.toml | Adds libc dependency for Unix process signaling |
| standalone/src/routes/settings/+page.svelte | Adds comprehensive Worker UI section with controls, status display, log viewer, and polling logic |
| standalone/src/lib/providers/absurdData.ts | Extends provider interface with worker-related types and methods for Tauri, tRPC, and mock providers |
| samples/typescript-client/src/index.ts | Replaces hardcoded paths with required environment variables for better CLI usage |
| samples/typescript-client/package.json | Adds bin entry and configures esbuild to generate executable with shebang |
| docs/specs/standalone-worker-process.md | Complete specification document for the worker process feature |
| Cargo.lock | Updates dependency graph with libc addition |
Files not reviewed (1)
- samples/typescript-client/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let (changed, was_running) = { | ||
| let stored = state.binary_path.lock().unwrap(); | ||
| (*stored != next, state.running.lock().unwrap().is_some()) | ||
| }; |
There was a problem hiding this comment.
There's a potential deadlock risk in set_worker_binary_path_inner. At line 208, two mutex locks are acquired within the same scope (binary_path and running). While they're released immediately, if another thread tries to acquire these locks in a different order, it could lead to a deadlock.
Consider acquiring only one lock at a time, or ensuring consistent lock ordering throughout the codebase.
| let (changed, was_running) = { | |
| let stored = state.binary_path.lock().unwrap(); | |
| (*stored != next, state.running.lock().unwrap().is_some()) | |
| }; | |
| let changed = { | |
| let stored = state.binary_path.lock().unwrap(); | |
| *stored != next | |
| }; | |
| let was_running = { | |
| let running = state.running.lock().unwrap(); | |
| running.is_some() | |
| }; |
| if let Some(next) = chars.next() { | ||
| current.push(next); |
There was a problem hiding this comment.
The command parsing doesn't handle escaped quotes within quoted strings correctly. For example, the command echo "He said \"hello\"" would not parse as expected because the backslash escape logic at lines 377-380 consumes the backslash and the next character unconditionally, even inside quotes where it should only apply to specific escape sequences.
Consider implementing proper quote-aware escape handling, where backslashes only escape specific characters when inside quotes (like the quote character itself and backslash).
| if let Some(next) = chars.next() { | |
| current.push(next); | |
| if in_single { | |
| // Inside single quotes, treat backslash as a literal. | |
| current.push('\\'); | |
| } else if in_double { | |
| // Inside double quotes, only escape " and \. | |
| match chars.peek() { | |
| Some('"') | Some('\\') => { | |
| if let Some(escaped) = chars.next() { | |
| current.push(escaped); | |
| } | |
| } | |
| _ => { | |
| // Backslash before other characters is literal. | |
| current.push('\\'); | |
| } | |
| } | |
| } else { | |
| // Outside quotes, backslash escapes the next character if present. | |
| if let Some(next) = chars.next() { | |
| current.push(next); | |
| } |
| const refreshData = async () => { | ||
| settings = await provider.getSettingsInfo(); | ||
| migrations = await provider.getMigrations(); | ||
| workerStatus = await provider.getWorkerStatus(); | ||
| if (workerStatus && !workerPathTouched) { | ||
| workerPathDraft = workerStatus.configuredPath ?? ""; | ||
| } | ||
| if (showDevApi) { | ||
| devApiStatus = await invoke<DevApiStatus>("get_dev_api_status"); | ||
| } | ||
| if (workerLogsOpen) { | ||
| await fetchWorkerLogs(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The refreshData function doesn't handle errors from any of the provider calls. If getWorkerStatus, getWorkerLogs, or other provider methods fail, the error will silently propagate and could leave the UI in an inconsistent state. The handleRefresh function also just uses void to suppress the promise, which means errors won't be visible to users.
Consider adding error handling to refreshData and displaying errors to the user, similar to how other worker operations handle errors (e.g., workerError state).
| function formatBytes(bytes: number | null) { | ||
| if (bytes === null || Number.isNaN(bytes)) { | ||
| return "—"; | ||
| } | ||
| if (bytes < 1024) { | ||
| return `${bytes} B`; | ||
| } | ||
| const units = ["KB", "MB", "GB", "TB"]; | ||
| let value = bytes / 1024; | ||
| let unitIndex = 0; | ||
| while (value >= 1024 && unitIndex < units.length - 1) { | ||
| value /= 1024; | ||
| unitIndex += 1; | ||
| } | ||
| return `${value.toFixed(value >= 10 ? 0 : 1)} ${units[unitIndex]}`; | ||
| } |
There was a problem hiding this comment.
The formatBytes function doesn't handle negative numbers, which could occur if there's a filesystem error or corrupted metadata. While the backend returns Option<u64> which should always be non-negative, TypeScript's number type includes negative values, and this could potentially be an issue if bad data is received.
Consider adding a check for negative numbers and returning "—" or "Invalid" in that case.
| let app_handle = app_handle.clone(); | ||
| async_runtime::spawn(async move { | ||
| if let Err(err) = worker::start_worker_inner(&app_handle) { | ||
| log::error!("Failed to start worker on launch: {}", err); |
There was a problem hiding this comment.
The auto-start logic spawns a task to start the worker but doesn't wait for the WorkerState to be fully initialized. Since app_handle.manage(worker::WorkerState::new(...)) happens at line 88 and the spawn happens immediately after, there's a potential race where worker commands from the frontend could be called before the WorkerState is managed. However, this is likely safe in practice since Tauri setup completes before handling commands.
A more significant issue is that the worker is started asynchronously without any feedback to the user. If the worker fails to start on launch (line 102), the error is only logged but the user won't know unless they check the logs. Consider tracking this initial start failure in the WorkerState so the UI can display it.
| log::error!("Failed to start worker on launch: {}", err); | |
| log::error!("Failed to start worker on launch: {}", err); | |
| // Notify the frontend so it can surface the failure to the user. | |
| let _ = app_handle.emit_all("worker_start_failed", err.to_string()); |
| pub fn stop_worker_inner(app_handle: &AppHandle) -> Result<WorkerStatus, String> { | ||
| let state = app_handle.state::<WorkerState>(); | ||
| let worker = state.running.lock().unwrap().take(); | ||
| let Some(worker) = worker else { | ||
| return Ok(state.status()); | ||
| }; | ||
|
|
||
| *state.stop_requested.lock().unwrap() = true; | ||
| let pid = worker.pid; | ||
| let stopped = stop_worker_process(worker, pid); | ||
| if let Err(err) = stopped { | ||
| *state.stop_requested.lock().unwrap() = false; | ||
| return Err(err); | ||
| } | ||
|
|
||
| Ok(state.status()) |
There was a problem hiding this comment.
There's a potential race condition between checking if a worker is running and stopping it. Between lines 310 and 318, if the worker exits naturally (through handle_worker_exit), the stop_requested flag could be set to true but the worker might already be None. This could lead to sending SIGTERM to a PID that has already been cleaned up or potentially reused by the system.
Consider checking if the worker is still in the running state after setting stop_requested, or using a more atomic approach to coordinate the stop operation with the exit handler.
| *state.stop_requested.lock().unwrap() = false; | ||
| return Err(err); | ||
| } | ||
|
|
||
| Ok(state.status()) | ||
| } |
There was a problem hiding this comment.
The stop_worker_inner function has a bug when an error occurs during stop_worker_process. If the stop fails (line 318-320), the function resets stop_requested to false but doesn't restore the worker to the running state. This leaves the system in an inconsistent state where the worker process might still be running but is no longer tracked in state.running.
Consider either restoring the RunningWorker to state.running on failure, or accepting that the stop operation is best-effort and not resetting the stop_requested flag.
| *state.stop_requested.lock().unwrap() = false; | |
| return Err(err); | |
| } | |
| Ok(state.status()) | |
| } | |
| return Err(err); | |
| } | |
| Ok(state.status()) | |
| } | |
| } |
| pub fn set_worker_binary_path_inner( | ||
| app_handle: &AppHandle, | ||
| path: &str, | ||
| ) -> Result<WorkerStatus, String> { | ||
| let normalized = path.trim(); | ||
| let next = if normalized.is_empty() { | ||
| None | ||
| } else { | ||
| Some(normalized.to_string()) | ||
| }; |
There was a problem hiding this comment.
The set_worker_binary_path function doesn't validate or sanitize the command path input. A user could potentially configure an arbitrary command to be executed with elevated environment variables. While this is a desktop application where the user has full control, there's no validation that:
- The path points to an actual executable file
- The command doesn't contain potentially dangerous characters or shell injection patterns
- The command isn't attempting to access system resources inappropriately
Consider adding basic validation such as checking if the first argument (program) is an absolute path that exists, or if it's a known safe command like "npx" or "uvx". At minimum, document that users should only configure trusted worker commands.
| "scripts": { | ||
| "build": "esbuild src/index.ts --bundle --outfile=dist/bundle.js --platform=node --external:better-sqlite3", | ||
| "start": "node dist/bundle.js", | ||
| "build": "esbuild src/index.ts --bundle --outfile=dist/absurd-sqlite-sample.js --platform=node --external:better-sqlite3 --banner:js='#!/usr/bin/env node'", |
There was a problem hiding this comment.
The bin field points to "dist/absurd-sqlite-sample.js" but this file needs to be executable on Unix systems. While the shebang is added via the esbuild banner, the file permissions need to be set appropriately (chmod +x) for the bin to work correctly when installed globally.
Consider documenting that users need to run chmod +x on the generated file, or add a postbuild step to set the executable permission automatically.
| "build": "esbuild src/index.ts --bundle --outfile=dist/absurd-sqlite-sample.js --platform=node --external:better-sqlite3 --banner:js='#!/usr/bin/env node'", | |
| "build": "esbuild src/index.ts --bundle --outfile=dist/absurd-sqlite-sample.js --platform=node --external:better-sqlite3 --banner:js='#!/usr/bin/env node'", | |
| "postbuild": "chmod +x dist/absurd-sqlite-sample.js", |
| if *stored != next { | ||
| let mut tracker = state.crash_tracker.lock().unwrap(); | ||
| tracker.clear(); | ||
| } |
There was a problem hiding this comment.
The crash tracker only clears its history when the configured path changes (line 221), but not when a worker successfully starts and runs. This means if a worker crashes 3 times, then the user fixes the issue and the worker runs successfully for a long time, the crash indicator will still show "crashing" until 60 seconds have passed since the last crash.
Consider clearing the crash history when a worker starts successfully (perhaps after it has been running for a short time, like 5 seconds), or provide a manual way to clear the crash state.
| if *stored != next { | |
| let mut tracker = state.crash_tracker.lock().unwrap(); | |
| tracker.clear(); | |
| } | |
| // Clear the crash tracker whenever the worker binary path is (re)configured, | |
| // so that users can manually reset the "crashing" indicator even if the path | |
| // value itself does not change. | |
| let mut tracker = state.crash_tracker.lock().unwrap(); | |
| tracker.clear(); |
This pull request introduces a comprehensive "Standalone Worker Process Control" feature, enabling users to configure, start, stop, and monitor a custom worker process from the app's Settings page. The implementation includes backend support for managing the worker's lifecycle, capturing logs, and surfacing status (including crash detection), as well as frontend and API changes to expose these controls and information. Additionally, the TypeScript sample client is updated for better CLI usability, and some minor improvements are made to the settings information.
Standalone Worker Process Control
docs/specs/standalone-worker-process.md) for managing a user-specified worker process, including goals, UI/UX, backend/tauri design, frontend integration, error handling, and testing.Backend (Rust/Tauri) Implementation
WorkerStateand related types to manage the worker process, including tracking its status, logs, and crash history. Exposed new Tauri commands:get_worker_status,set_worker_binary_path,start_worker,stop_worker, andget_worker_logs. The worker is started on launch if configured, and crash detection logic is implemented. [1] [2] [3] [4] [5] [6] [7] [8]Frontend (TypeScript/Svelte) Integration
AbsurdDataProviderinterface and implementations to support worker status, logs, and control commands. Added corresponding types. The mock and TRPC providers are also updated. [1] [2] [3] [4] [5] [6]TypeScript Sample Client Improvements
binentry and generating a proper executable bundle with a shebang. The sample now requiresABSURD_DATABASE_PATHandABSURD_DATABASE_EXTENSION_PATHenvironment variables, instead of hardcoded paths. [1] [2] [3]Dependency and Platform Support
libcdependency for Unix platforms to support process management.References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22] [23]