fix: lifecycle stop hooks not running for detached sessions

[fkondej]

Part of DEV-1490

[claude]

Claude finished @fkondej's task in 1m 8s —— View job

---

Review of PR #407

• Read PR diff and changed files
• Review code quality, bugs, edge cases
• Post review feedback

Summary

The goal — running lifecycle stop hooks for detached sessions — makes sense. The approach of reading the persisted manifest to discover stop commands is reasonable.

One bug found: StopSession is called from two paths:

1. Non-detached: LocalRunner.Stop() → runAllLifecycleStopCommands() → StopSession() (stop commands run twice)
2. Detached: StopSession() directly (stop commands run once — correct)

The new RunLifecycleStopFromManifest call inside StopSession causes double execution of stop commands for non-detached sessions. See inline comment for suggested fixes.

Minor: The new function duplicates the stop-command loop logic from runLifecycleStopCommands without the logHeader call. Could be consolidated into a shared helper.

No other issues found — the error handling (warn-and-continue) and directory resolution logic look correct.

[claude]

Claude finished @fkondej's task in 1m 24s —— View job

---

Review of PR #407 (re-review after update)

• Read updated PR diff
• Review code changes for correctness
• Post review feedback

Summary

The latest commit (460ed98) addresses the duplicate-execution bug from my previous review. The fix consolidates stop command execution into a single manifest-based path inside StopSession, which is clean and correct. Both non-detached and detached paths now go through the same code.

Issues

Dead code: lifecycleServiceInfo, lifecycleServices, lifecycleMu

The lifecycleServiceInfo struct, lifecycleServices field, and lifecycleMu mutex on LocalRunner are now only written to (in startWithLifecycleHooks) but never read for actual functionality — stop commands now use the manifest-based path. This is dead code that should be cleaned up. The struct fields, the append in startWithLifecycleHooks (lines 98–104), and the related test assertions (require.Len, require.Equal on lifecycleServices) can be removed.

Minor: ForceKillSession skips lifecycle stop commands

ForceKillSession (called from the force-kill context path at local_runner.go:295) does not call RunAllLifecycleStopCommands. This means lifecycle stop hooks (e.g. QEMU VM cleanup) won't run during force kill. This may be intentional, but worth confirming — if a QEMU VM is left running after force kill, that could leak host resources.

No other issues found. The error handling, manifest loading, and directory resolution logic all look correct.

[claude]

Claude finished @fkondej's task in 1m 2s —— View job

---

Review of PR #407 (re-review after sync)

• Read updated PR diff and changed files
• Review code changes for correctness and quality
• Post review feedback

Summary

The latest commits (460ed98, 1c5d2b2) correctly address the duplicate-execution bug from my first review. Stop command execution is now consolidated into a single manifest-based path inside StopSession, which works for both detached and non-detached sessions. The core logic is sound.

Remaining issues

Dead code: lifecycleServiceInfo, lifecycleServices, lifecycleMu

The lifecycleServiceInfo struct (line 25), the lifecycleServices / lifecycleMu fields on LocalRunner (lines 71–72), and the append block in startWithLifecycleHooks (lines 98–104) are now write-only — stop commands use the manifest-based path exclusively. This dead code should be cleaned up. The test file also still initializes lifecycleServices: []*lifecycleServiceInfo{} in several places (lines 24, 61, 87, 124) which can be removed.

Fix this →

ForceKillSession skips lifecycle stop commands

ForceKillSession (called from the force-kill context at local_runner.go:295) does not call RunAllLifecycleStopCommands. This means lifecycle stop hooks (e.g. QEMU VM cleanup) won't run during force kill, potentially leaking host resources. If intentional, a comment clarifying this would be helpful.

No other issues found.

[fkondej]

Minor: ForceKillSession skips lifecycle stop commands

ForceKillSession (called from the force-kill context path at local_runner.go:295) does not call RunAllLifecycleStopCommands. This means lifecycle stop hooks (e.g. QEMU VM cleanup) won't run during force kill. This may be intentional, but worth confirming — if a QEMU VM is left running after force kill, that could leak host resources.

Pre-existing behavior — ForceKillSession never called lifecycle stop hooks before this PR either. This is most likely intentional and correct — force kill is the escape hatch when graceful shutdown is stuck, and running stop hooks there could block if the process is unresponsive. Out of scope for this change.

[claude]

Claude finished @fkondej's task in 1m 0s —— View job

---

Review of PR #407 (re-review after dead code removal)

• Read updated PR diff
• Review code changes for correctness and quality
• Post review feedback

Summary

All three issues from previous reviews have been addressed:

1. Double execution of stop commands — Fixed. Stop commands now run exclusively via the manifest-based path inside StopSession, used by both detached and non-detached flows.
2. Dead code cleanup — Done. lifecycleServiceInfo struct, lifecycleServices/lifecycleMu fields, the tracking append block, and related test assertions are all removed.
3. ForceKillSession skipping lifecycle stop — Acknowledged as intentional pre-existing behavior, agreed out of scope.

Remaining issues

None found. The code is clean and correct. The consolidation into runLifecycleStopForServices + RunAllLifecycleStopCommands is a good design — single code path, testable without LocalRunner, and works for both session types.

LGTM.

[claude]

Claude finished @fkondej's task in 42s —— View job

---

Review of PR #407 (re-review after sync)

• Read PR diff and changed files
• Review code quality, bugs, edge cases
• Post review feedback

Summary

All previously identified issues have been addressed. The code is clean and correct.

• Double execution bug — Fixed. runAllLifecycleStopCommands was removed from LocalRunner.Stop() and consolidated into StopSession, so both detached and non-detached paths use the same manifest-based stop.
• Dead code cleanup — Done. lifecycleServiceInfo struct, lifecycleServices/lifecycleMu fields, and related test code removed.
• Tests updated — Stop command tests now call runLifecycleStopForServices directly, which is cleaner and doesn't require a full LocalRunner instance.

No new issues found. LGTM.

[claude]

Claude finished @fkondej's task in 47s —— View job

---

Review of PR #407 (re-review after sync)

• Read PR diff and changed files
• Review code quality, bugs, edge cases
• Post review feedback

Summary

All previously identified issues have been addressed. The code is clean and correct.

• Double execution bug — Fixed. runAllLifecycleStopCommands removed from LocalRunner.Stop() and consolidated into StopSession, so both detached and non-detached paths use the same manifest-based stop.
• Dead code cleanup — Done. lifecycleServiceInfo struct, lifecycleServices/lifecycleMu fields, and related test code removed.
• Tests updated — Stop command tests now call runLifecycleStopForServices directly, which is cleaner and doesn't require a full LocalRunner instance.

No new issues found. LGTM.