Our run group idiom is confusing

[directionless]

Digging through a debug log, I find myself wondering about

launcher/cmd/launcher/internal/updater/updater.go

Lines 143 to 158 in 7217b19

	func (u *updaterCmd) interrupt(err error) {
	level.Info(u.config.Logger).Log("msg", "updater interrupted", "err", err)
	// non-blocking channel send
	select {
	case u.stopChan <- true:
	level.Info(u.config.Logger).Log("msg", "updater interrupt sent signal over stop channel")
	default:
	level.Info(u.config.Logger).Log("msg", "updater interrupt without sending signal over stop channel (no one to receive)")
	}
	if u.stopExecution != nil {
	u.stopExecution()
	}
	}

This is the interrupt function for the updater. u.stopExecution() makes sense, but signaling u.stopChan <- true feels weird. That stops the main launcher thread, which should have it's own interrupt function. I might be missing something clever, but my hunch is that it's superfluous.

[RebeccaMahany]

Investigated -- u.stopChan is a separate channel from the sigChannel that gets passed in, so signaling u.stopChan only stops the updater from attempting to init an autoupdater if it hasn't yet, and does not stop the main launcher thread.

Discussed offline the need for better logging and reporting for when our many rungroups exit.

[directionless]

Thank you for digging through that Becca!

My sense here, is that part of what's confusing is that we don't have clear enough logging around what happens. (And this is because the rungroup idiom feels like it's missing something)

Inturrupt takes an err which is meant to be the err of the thing triggering the exit. But that feels like it's a bit weird? why would some routine care why it's being asked to exit. 🤷 maybe sometimes it wants to trigger an extra cleanup routine.

But I think one of the cores here, is that we don't log consistently. When something starts, or when it's interrupted, and why are all very muddled. My gut sense is that we should write a replacement (or wrapper) for oklog/run that adds some simple logging and cleans up the interfaces

[RebeccaMahany]

Some specific wishlist items while working through #1272:

• The rungroup replacement should be able to identify actors, and log which actor initiated the shutdown and what the error was: Rungroup improvements: logging #1316
• We should audit all the Execute / Interrupt functions to remove superfluous logging of an interrupting error not coming from that actor, because they really muck up the logs: Remove superfluous logs in actor.Interrupt #1273
• Audit all Interrupt functions and ensure they can be called multiple times and still return promptly; add tests for all: partially addressed in Allow desktop runner Interrupt to be called multiple times #1344, Allow runner.Shutdown to be called more than once #1354
• The rungroup replacement should have a timeout on waiting for all actors to Interrupt successfully, so that we aren't stuck forever waiting for a broken Interrupt => Execute return: Add timeout to rungroup shutdown #1481
• The rungroup replacement should log when an actor cannot be interrupted successfully