Skip to content

feat: shell command as bg job#2678

Open
ssddOnTop wants to merge 13 commits intomainfrom
feat/shell-command-as-bg-job1
Open

feat: shell command as bg job#2678
ssddOnTop wants to merge 13 commits intomainfrom
feat/shell-command-as-bg-job1

Conversation

@ssddOnTop
Copy link
Collaborator

@ssddOnTop ssddOnTop commented Mar 24, 2026

Fixes: #2635

Pending:
Session management

@tusharmath tusharmath closed this Mar 24, 2026
@tusharmath tusharmath reopened this Mar 24, 2026
@ssddOnTop ssddOnTop marked this pull request as ready for review March 25, 2026 04:31
Comment on lines +86 to +100
pub async fn register(
&self,
pid: u32,
command: String,
cwd: PathBuf,
log_file: PathBuf,
log_handle: tempfile::NamedTempFile,
) -> Result<BackgroundProcess> {
let process = BackgroundProcess { pid, command, cwd, log_file, started_at: Utc::now() };
self.lock_processes()?.push(process.clone());
self.lock_log_handles()?
.push(OwnedLogFile { _handle: log_handle, pid });
self.metadata.save_process(&process).await?;
Ok(process)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition: If save_process() fails on line 98, the in-memory state (lines 95-97) is already corrupted with the process added to both vectors. Subsequent operations will see the process in memory but it won't be persisted to disk, causing inconsistency. Fix by saving to disk first:

pub async fn register(...) -> Result<BackgroundProcess> {
    let process = BackgroundProcess { pid, command, cwd, log_file, started_at: Utc::now() };
    self.metadata.save_process(&process).await?;  // Disk first
    self.lock_processes()?.push(process.clone());
    self.lock_log_handles()?.push(OwnedLogFile { _handle: log_handle, pid });
    Ok(process)
}
Suggested change
pub async fn register(
&self,
pid: u32,
command: String,
cwd: PathBuf,
log_file: PathBuf,
log_handle: tempfile::NamedTempFile,
) -> Result<BackgroundProcess> {
let process = BackgroundProcess { pid, command, cwd, log_file, started_at: Utc::now() };
self.lock_processes()?.push(process.clone());
self.lock_log_handles()?
.push(OwnedLogFile { _handle: log_handle, pid });
self.metadata.save_process(&process).await?;
Ok(process)
}
pub async fn register(
&self,
pid: u32,
command: String,
cwd: PathBuf,
log_file: PathBuf,
log_handle: tempfile::NamedTempFile,
) -> Result<BackgroundProcess> {
let process = BackgroundProcess { pid, command, cwd, log_file, started_at: Utc::now() };
self.metadata.save_process(&process).await?;
self.lock_processes()?.push(process.clone());
self.lock_log_handles()?
.push(OwnedLogFile { _handle: log_handle, pid });
Ok(process)
}

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
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.

[Feature]: Ability to run a shell command as a background job

2 participants