[bug] Finalize command sessions only after descendants and output pipes settle

[douglasjarquin]

Severity and category

Severity: High correctness and process-lifecycle defect

Category: Command completion / late output / descendant ownership

Problem and intended outcome

Pinchos can report a command as successfully completed before the full command session it owns has actually ended.

When the shell exits, CommandExecutionEngine.run records a CommandExecution immediately. If members of the same process group are still alive, the engine returns that execution plus a LingeringProcess. CommandRunner.finishActiveRun() then returns .completed(execution) to the caller while retaining the lingering process internally.

This creates several externally visible inconsistencies:

1. ManagedItem.refresh() commits the shell's current stdout as the item's last successful value as soon as it receives .exited(code: 0).
2. A background descendant can write additional stdout after the shell exits. CommandRunner eventually updates its internal lastExecution, but ManagedItem.lastSuccessfulOutput, formatted title, tooltip output, and lastUpdatedAt are never updated from that final stdout.
3. If the lingering group reaches the command timeout and is killed, LingeringProcess.currentExecution() keeps the original .exited(code: 0) terminal reason. Diagnostics can therefore record a successful shell exit even though Pinchos terminated the remaining owned work for timeout.
4. pinchos run <item> treats the preliminary shell result as final, prints it, returns its exit code, and then the top-level executable calls Darwin.exit. A same-group background descendant can outlive the CLI because the local runner still had a lingering session when the process exited.

The intended outcome is a coherent command-session contract. A caller must be able to distinguish the shell's exit from the final owned session result, and Pinchos must not publish final success, final output, timeout status, or CLI exit until all same-group descendants and output pipes are settled or deliberately cancelled.

Current evidence

Relevant implementation in Sources/PinchosCore/CommandExecution.swift:

• The engine computes terminalAt and execution when the first process/timeout/cancellation event wins.
• keepProcessGroup = controller.hasMembers() allows the engine to return while descendants remain.
• LingeringProcess stores the preliminary execution and collectors.
• currentExecution() refreshes output snapshots but preserves execution.terminalReason and execution.duration.
• CommandRunner.finishActiveRun() returns .completed(completedExecution) even when it appends a still-live LingeringProcess.
• settleLingeringProcesses() updates only CommandRunner.lastExecution after the group disappears.

Relevant implementation in Sources/pinchos/ManagedItem.swift:

• refresh() treats any returned .exited(code: 0) as final success.
• It copies stdout into lastSuccessfulOutput, updates lastUpdatedAt, computes the title, and schedules staleness.
• There is no subscription/callback for the later settled execution.

Relevant implementation in Sources/pinchos/PinchosCLI.swift and main.swift:

• runItem maps the preliminary execution directly to output and an exit code.
• CLI mode exits the entire process after PinchosCLI.run returns.

Existing tests verify that late stderr eventually appears in CommandRunner.snapshot(), but they do not prove that the menu-bar value, timeout classification, or CLI lifetime uses the definitive session result.

Required session semantics

Define and document a session as the shell process, all same-process-group descendants, and both captured output pipes.

The final result should be emitted only when one of these conditions is true:

• The shell and all owned descendants have exited and stdout/stderr are fully drained.
• The configured timeout ended the session and cleanup reached its bounded terminal state.
• Cancellation ended the session and cleanup reached its bounded terminal state.
• Launch failed before a session was established.

A preliminary shell-exit event may still be exposed internally for UI feedback, but it must not be confused with final success. If Pinchos intentionally continues to display “running” while descendants exist, the eventual final event must update the item's output, timestamps, status, and diagnostics exactly once.

Timeout duration should describe the owned session, not only the shell leader. If background work outlives the shell and reaches the timeout, the final reason must be .timedOut rather than .exited(0).

Bounded scope and non-goals

In scope:

• Establish a definitive command-session completion API.
• Preserve late stdout and stderr through final pipe closure.
• Correctly classify timeout/cancellation that occurs after shell exit.
• Ensure ManagedItem commits success and lastUpdatedAt from the final session result.
• Ensure late final output can update the rendered title and tooltip.
• Ensure pinchos run waits for, times out, or cancels the full session before exiting.
• Preserve no-overlap behavior for the entire session lifetime.
• Preserve bounded retained output and skipped-refresh diagnostics.
• Add deterministic unit and executable-level tests.

Explicit non-goals:

• Do not support intentionally detached/daemonized processes as part of the session.
• Do not treat every child process on the system as owned; ownership remains the established process group/session boundary.
• Do not wait forever for an escaped process that intentionally created a new session and only retained a pipe; existing bounded drain behavior should remain.
• Do not change display formatting or error policy beyond using the correct final result.
• Do not solve PID/PGID reuse here; compose with [security] Prevent recycled process-group IDs from signaling unrelated processes #43.
• Do not solve external process signals here; compose with [reliability] Gracefully cancel managed commands on SIGTERM, SIGINT, and CLI interruption #44.

Testable acceptance criteria

• A shell that exits 0 while a same-group child writes stdout later produces a final execution containing the late stdout.
• The corresponding ManagedItem ultimately renders the final late value, updates {output} and lastUpdatedAt, and does not remain stuck on the preliminary value.
• A shell that exits 0 while a same-group child writes late stderr retains that stderr in final diagnostics.
• A lingering child that exceeds the configured timeout is killed and the definitive terminal reason is .timedOut, not .exited(code: 0).
• Cancellation after shell exit yields .cancelled and removes every owned descendant.
• The item remains running or another explicitly documented non-final state while owned descendants exist; it does not mark a final success early.
• Staleness is scheduled from final successful completion, not from preliminary shell exit.
• on_error = "keep_last" and replace apply to the definitive result.
• pinchos run <item> does not exit while the runner still owns a process group or output drain.
• A normal CLI run with a finite background child waits for the final child/output and returns the correct final exit status.
• A CLI run with an indefinite same-group child obeys the configured timeout and leaves no orphan.
• A detached new-session pipe holder cannot make the CLI or GUI wait without bound.
• Repeated refreshes remain coalesced for the complete session lifetime.
• Duration semantics are documented and tested for shell-only and lingering sessions.
• Existing high-volume stdout/stderr, timeout, cancellation, and subsequent-run tests continue to pass.
• swift test and swift build -c release pass.

Likely implementation areas

• Sources/PinchosCore/CommandExecution.swift
  • split preliminary shell status from definitive session result
  • carry final termination reason and duration through lingering settlement
  • expose a final completion awaitable/event to callers
• Sources/pinchos/ManagedItem.swift
  • update state only from the definitive result, or consume both preliminary and final events deliberately
• Sources/pinchos/PinchosCLI.swift
• Sources/pinchos/main.swift
• Tests/PinchosCoreTests/CommandExecutionTests.swift
• Tests/PinchosCoreTests/CommandRunnerTests.swift
• Tests/pinchosTests/RecoveryLifecycleTests.swift
• Tests/pinchosTests/PinchosCLITests.swift
• README command-lifecycle documentation

Verification plan

1. Add deterministic commands that exit the shell and then emit delayed stdout/stderr from same-group children.
2. Assert preliminary and final states separately if both remain part of the API.
3. Add late timeout and late cancellation cases after shell exit.
4. Exercise the same fixtures through headless ManagedItem and the built CLI executable.
5. Verify title, tooltip, status, timestamps, final reason, output byte counts, and child-process absence.
6. Stress repeated runs where descendants finish just before and just after timeout.
7. Run the full suite and release build.
8. Manually inspect a release app item whose background child changes the final visible value.

Risks and dependencies

Waiting for full owned-session completion may expose commands that accidentally background long-lived work. That is correct: such an item is still consuming resources owned by Pinchos and must remain running until it exits or times out.

The API should make the shell/session distinction explicit rather than hiding another settlement callback inside snapshot(). Silent internal mutation after callers were told a run was complete is the root design problem.

[douglasjarquin]

Additional concrete failure path from the current implementation: ProcessGroupController.beginTermination(_:) treats an existing .natural claim as eligible for timeout/cancellation, but it assigns the new reason only when claim == nil. After the shell has claimed .natural, a later timeout can therefore request process-group termination while the stored claim remains .natural. LingeringProcess.currentExecution() then preserves the preliminary .exited(code: 0) reason and duration. This makes the late-timeout misclassification deterministic, not merely a stale-presentation race, and should be covered by a focused red test.