Run commands in background

[rumpl]

Draft for now, the runtime stops the toolsets once it finishes running the stream, have to remove that before we can get this one in

[rumpl]

Code Review - Areas for Improvement

Great work on implementing background command execution! The core functionality is solid and the API design is intuitive. Here are some areas that could be strengthened:

1. Resource Management & Memory Leaks

The current implementation stores command outputs indefinitely:

h.commandOutputs.Store(pid, currentOutput.(string)+string(stdoutData[:n]))

Issues:

• Long-running commands could accumulate large outputs in memory
• No cleanup mechanism for completed background commands
• Outputs persist until shell tool restart

Suggestions:

• Add output size limits or rotation
• Implement cleanup for completed commands
• Consider adding a "forget" or "clear" command

2. Output Reading Race Conditions

for {
    n, err := stdout.Read(stdoutData)
    // ... update shared state
    n, err = stderr.Read(stderrData) 
    // ... update same shared state
}

Issues:

• No coordination between stdout/stderr reading
• Potential race conditions when both streams write simultaneously
• Could miss proper output ordering

Suggestion: Use separate goroutines with proper synchronization or io.MultiWriter.

3. Error Handling

currentOutput, _ := h.commandOutputs.Load(pid)
h.commandOutputs.Store(pid, currentOutput.(string)+string(stdoutData[:n]))

Risk: Panic if stored value isn't a string. Add proper type checking.

4. Missing Test Coverage

The new background functionality lacks tests:

• Background command execution
• Output retrieval via get_command_output
• Process cleanup in Stop() method
• Error scenarios (invalid PID, failed process start)

5. Security/Limits

Consider adding:

• Maximum number of background processes
• Timeout mechanisms for runaway processes
• Output size limits per command

6. Runtime Change Clarification

The removal of the defer block in runtime.go:

// REMOVED: defer func() { a.StopToolSets() }()

This seems unrelated to background commands. Could you clarify why this was removed? This could potentially cause resource leaks for other toolsets.

Minor: Process Management

Consider adding a way to:

• List active background processes
• Check if a background process is still running
• Kill specific background processes

The implementation is solid overall - these improvements would make it production-ready!