[ML] fixes testWatchdog test verifying matcher is interrupted on timeout

[benwtrent]

Constructing the timout checker FIRST and THEN registering the watcher allows the test to have a race condition.

The timeout value could be reached BEFORE the matcher is added. To prevent the matcher never being interrupted, a new timedOut value is added to the watcher thread entry. Then when a new matcher is registered, if the thread was previously timedout, we interrupt the matcher immediately.

closes #48861

[elasticmachine]

Pinging @elastic/ml-core (:ml)

[benwtrent]

@elasticmachine update branch

[droberts195]

I think the problem you've identified could affect the production code as well as the test code. But you've fixed the test by introducing a new code path that works well for the test. So the production code is left with a flaw while the test starts working.

It seems like a solution that should work both for testing and production is to say that if a matcher is registered after a timeout then it's instantly interrupted, i.e. change:

        @Override
        public void register(Matcher matcher) {
            WatchDogEntry value = registry.get(Thread.currentThread());
            if (value != null) {
                boolean wasFalse = value.registered.compareAndSet(false, true);
                assert wasFalse;
                value.matchers.add(matcher);
            }
        }

to something like:

        @Override
        public void register(Matcher matcher) {
            WatchDogEntry value = registry.get(Thread.currentThread());
            if (value != null) {
                synchronized (TimeoutChecker.this) {
                    boolean wasFalse = value.registered.compareAndSet(false, true);
                    assert wasFalse;
                    value.matchers.add(matcher);
                    if (TimeoutChecker.this.timeoutExceeded) {
                        matcher.interrupt();
                    }
                }
            }
        }

Then the test can stay more-or-less as it was, but the timeout should be a random value between 10 and 500 to increase the chance of testing both code paths.

What do you think?

[benwtrent]

It seems like a solution that should work both for testing and production is to say that if a matcher is registered after a timeout then it's instantly interrupted,

Definitely! I will make the change

[benwtrent]

run elasticsearch-ci/packaging-sample-windows

[droberts195]

LGTM