prevent client deadlock and incorrect timing on stop_on_client_after

[tgross]

The disconnect.stop_on_client_after feature is implemented as a loop on the client that's intended to wait on the shortest timeout of all the allocations on the node and then check whether the interval since the last heartbeat has been longer than the timeout. It uses a buffered channel of allocations written and read from the same goroutine to push "stops" from the timeout expiring to the next pass through the loop. Unfortunately if there are multiple allocations that need to be stopped in the same timeout event, or even if a previous event has not yet been dequeued, then sending on the channel will block and the entire goroutine deadlocks itself.

While fixing this, I also discovered that the stop_on_client_after and heartbeat loops can synchronize in a pathological way that extends the stop_on_client_after window. If a heartbeat fails close to the beginning of the shortest stop_on_client_after window, the loop will end up waiting until almost 2x the intended wait period.

While fixing both of those issues, I discovered that the existing tests had a bug such that we were asserting that an allocrunner was being destroyed when it had already exited.

This commit includes the following:

• Rework the watch loop so that we handle the stops in the same case as the timer expiration, rather than using a channel in the method scope.
• Remove the alloc intervals map field from the struct and keep it in the method scope, in order to discourage writing racy tests that read its value.
• Reset the timer whenever we receive a heartbeat, which forces the two intervals to synchronize correctly.
• Minor refactoring of the disconnect timeout lookup to improve brevity.
• Fix the test assertions and expand them to cover more ground.

Fixes: #24679
Ref: https://hashicorp.atlassian.net/browse/NMD-407

Contributor Checklist

• Changelog Entry If this PR changes user-facing behavior, please generate and add a
  changelog entry using the make cl command.
• Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
  ensure regressions will be caught.
• Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
  and job configuration, please update the Nomad website documentation to reflect this. Refer to
  the website README for docs guidelines. Please also consider whether the
  change requires notes within the upgrade guide.

Reviewer Checklist

• Backport Labels Please add the correct backport labels as described by the internal
  backporting document.
• Commit Type Ensure the correct merge method is selected which should be "squash and merge"
  in the majority of situations. The main exceptions are long-lived feature branches or merges where
  history should be preserved.
• Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
  within the public repository.

[tgross]

Changing this buffer isn't strictly necessary but seemed like a safe belt-and-suspenders thing to do. During client restart it's possible to get a bunch of allocs on this channel in a short window when we restore them, so having a larger buffer gives us some room to avoid putting backpressure on the allocrunner setup.

[gulducat]

the code proper LGTM!

however, I do have a concern about what the last test case is actually covering, if you wanna noodle on that.

[gulducat]

is this hour arbitrary? I don't see it in docs.

if I'm reading all this right, it represents the amount of time between loop iterations, if the select doesn't get poked by anything else, right?

so it isn't a documented value that users care about (or might be surprised by), it just keeps the loop looping (very slowly) in lieu of any other input?

[tgross]

Right! We need to find the minimum interval and because we have a map instead of a slice we don't have a good starting value to compare against. And we want to reset the timer to something in the case where we're not watching any allocs, so having it be an hour keeps it idle for a good long while.

The alternative would be to copy the values of allocIntervals into a slice every loop and take the min, or track the lowest interval at all times whenever we add/delete an alloc. And then we'd stop the timer if len(allocIntervals) == 0. Let me see if we can reasonably track the lowest interval because that might make this a bit more legible.

[tgross]

Ok, in 6ad1aae I've moved where we declare the interval and check for 0 when minimizing the interval. This avoids the arbitrary 1 hour interval and also stops the timer's internal goroutine when not in use.

[gulducat]

so now not having any allocs to watch (interval remains 0) calls timer.Stop(), which means the loop will only turn over on heartbeats or new allocs (or bail on shutdown). that seems right to me 👍

[gulducat]

lgtm!

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.