Skip to content

Remove force kill listener cleanup#382

Open
canercidam wants to merge 1 commit intomainfrom
caner/remove-force-kill-listener-cleanup
Open

Remove force kill listener cleanup#382
canercidam wants to merge 1 commit intomainfrom
caner/remove-force-kill-listener-cleanup

Conversation

@canercidam
Copy link
Member

No description provided.

@claude
Copy link
Contributor

claude bot commented Feb 24, 2026

Claude finished @canercidam's task in 1m 19s —— View job


Review: Remove force kill listener cleanup

  • Gather context and read changed files
  • Analyze changes and provide review feedback

Summary

This PR simplifies the force-kill listener goroutine in Stop() by removing the local cancellation context that previously ensured goroutine cleanup when Stop() returned normally.

One concern raised inline: the goroutine now leaks in the normal (non-force-kill) shutdown path, since mainctx.GetForceKillCtx() is only cancelled on the 3rd interrupt signal. The previous select with a local context ensured proper cleanup. This is practically benign for a CLI tool (process exit cleans it up), but worth noting if Stop() is ever called in longer-lived contexts (tests, session rotation).

No other issues found. The change is straightforward and the context import remains used elsewhere in the file.

Comment on lines +287 to +291
go func() {
<-mainctx.GetForceKillCtx().Done()
d.stopAllProcessesWithSignal(os.Kill)
ForceKillSession(d.manifest.ID, keepResources)
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

This goroutine will never terminate during normal (non-force-kill) shutdown. The forceKillCtx from mainctx is only cancelled on the 3rd interrupt signal. In the common case (graceful exit), the goroutine blocks on this channel indefinitely.

The previous code used a local context.WithCancel + defer cancel() to ensure the goroutine was cleaned up when Stop() returned. Without that, this goroutine leaks for the remainder of the process lifetime.

Since this is a CLI tool that exits shortly after Stop(), this is practically benign — the leaked goroutine gets cleaned up by process exit. But it's worth noting that the old code was more correct in terms of resource management. If Stop() were ever called in a context where the process continues (e.g., tests, session rotation), the leak would accumulate.

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.

1 participant