Continue heartbeat after stop signal

[stevenmatthewt]

This addresses a regression that was added in #971.

Heartbeats were no longer sent after a SIGTERM or SIGINT was received, which interferes with the graceful shutdown. While the buildkite-agent shuts down gracefully, buildkite.com will display "Exited with status -1 (agent lost)" after ~5 minutes. This limits the graceful shutdown feature to a duration of only 5 minutes.

I understand the previous change addressed a race condition, so please double check that I haven't accidentally reintroduced that issue.

[lox]

Argh, sorry about this regression! We need to figure out a way to test some of these edge cases.

Looking at the code, it seems like there might be a race condition on checking a.running. Whilst this is preferable to the bug I introduced, I wonder how we can fix it.

The intention of my change in #971 was to use a channel closure to signal the heartbeater and the pinger, perhaps we just need to move the closure in to a defer so it doesn't run until the agent actually terminates?

[lox]

The other way to do this would be with a context with cancel:

ctx, cancel := context.WithCancel(context.Background()
defer cancel()
...
case <-ctx.Done():

[lox]

I think the introduction of a.stop was confusing, I think what we wanted was to signal that the agent had entered stopping state, not stopped state, which should end the ping timer, but not the heartbeat timer, which should only stop when the agent reaches a stopped state.

[lox]

I wonder if a state machine would make this clearer 🤔

[lox]

I made this change with a context.Context in #992.

[stevenmatthewt]

Changes in #992 look great. Closing this in favor of it.