Reload defunct runners

[p1-0tr]

In case a runner becomes defunct, e.g. as a result of a backend crash it would be neat to be able to reload it. So, if the loader finds runner, have it check if the runner is still alive, and create a new one if the runner is defunct.

[doringeman]

Suggested change

	l.evictRunner(backendName, model)
	// Reset the reference count to zero so that we can evict the runner and then start a new one.
	l.references[existing] = 0
	l.evictRunner(backendName, model)

[p1-0tr]

Makes sense. Though I wonder if it would not be safer to let the reference counting work normally, issue and idle check here, and expand the idle check logic to look for defunct or stale runners. WDYT?

[doringeman]

expand the idle check logic to look for defunct or stale runners

I like this!

Although, in this specific case, this code which comes right after the code you're changing will evict all (1, currently, but still) runners if all the slots are full and the current one that's attempted to be loaded is defunct and not clean up, right?

// If there's not sufficient memory or all slots are full, then try
// evicting unused runners.
if memory > l.availableMemory || len(l.runners) == len(l.slots) {
	l.evict(false)
}

[p1-0tr]

I'm pretty sure forcing the refcount to 0 does put us at a risk of panicing in loader.release. I've opted not to force the refcount to 0, and added logic in evict to remove defunct runners.

[xenoscopic]

I agree that we can't force the refcount to 0 here.

The bigger issue I see with the new logic is that evictRunner in this case might not actually evict if there's a non-zero reference count for the defunct runner (e.g. a client that hasn't realized its backend is defunct yet). The problem is that this code would then continue and override the l.runners entry for runnerKey{backend, model, mode} with a newly created runner, so when that hypothetical outstanding defunct runner is finally released, it will decrement the reference count for the new runner in release (since it uses the same key to look up the slot).

I think what I would do is put a label (say WaitForChange:) just above the last block of code in this loop (grep for "Wait for something to change") and then in the case <-l.slots[existing].done: path, I would goto WaitForChange. Then, in release, add a check for <-runner.done and immediately evict if l.references[slot] == 0. Because realistically any client using a defunct runner will find out quite quickly once the socket connection closes, which means the runner will be release'd quickly, which will call broadcast and break the waiting load call out of its waiting loop.

[p1-0tr]

Ok, it took me a while to convince myself that run() and load() will play nice with this approach. Should be all good, though :)

[p1-0tr]

Ended up having to add an error handler on the proxy, where we can wait for the runner process to exit. Otherwise we race, and often end up releaseing the runner in the gap between it closing its socket and the done channel being closed.

[xenoscopic]

I like the idea, but I think we'll need a slightly different approach.

[xenoscopic]

I agree that we can't force the refcount to 0 here.

The bigger issue I see with the new logic is that evictRunner in this case might not actually evict if there's a non-zero reference count for the defunct runner (e.g. a client that hasn't realized its backend is defunct yet). The problem is that this code would then continue and override the l.runners entry for runnerKey{backend, model, mode} with a newly created runner, so when that hypothetical outstanding defunct runner is finally released, it will decrement the reference count for the new runner in release (since it uses the same key to look up the slot).

I think what I would do is put a label (say WaitForChange:) just above the last block of code in this loop (grep for "Wait for something to change") and then in the case <-l.slots[existing].done: path, I would goto WaitForChange. Then, in release, add a check for <-runner.done and immediately evict if l.references[slot] == 0. Because realistically any client using a defunct runner will find out quite quickly once the socket connection closes, which means the runner will be release'd quickly, which will call broadcast and break the waiting load call out of its waiting loop.

To fix the issue, the model variable should be sanitized before being used in the log entry. Since the logs appear to be plain text, we can remove potentially harmful characters such as newlines (\n, \r) using strings.ReplaceAll. This ensures that the log entry cannot be manipulated by malicious input. The sanitization should be applied directly before the log statement on line 389 in loader.go.

[xenoscopic]

Looks good, just two thoughts:

[xenoscopic]

If we want to get REALLY pedantic, we could also add an optional mode specification to evictRunner so that eviction of a defunct runner in one mode doesn't force early eviction of an idle runner in another mode. Most (all?) models aren't used bi-modally though, so maybe it's a non-issue.

[p1-0tr]

Makes the semantics clearer, I like it :) done.

[doringeman]

LGTM!
I wonder if it makes sense to also change idleCheckDuration to check immediately if a defunct runner exists.

[xenoscopic]

Looks great to me!

[xenoscopic]

@doringeman I don't know if one defunct runner would indicate a high likelihood of other defunct runners.

[doringeman]

@xenoscopic I meant to modify idleCheckDuration so that when it's called randomly, it also verifies whether there's a defunct runner, and if so, returns a maximum value to trigger an immediate check.

[xenoscopic]

@doringeman Sorry, I misunderstood. If I'm following correctly now, then I'm definitely in agreement. Instead of "maximum value" though, just 0 (to check immediately), right?

[doringeman]

Yeah, 0 to check immediately, sorry.

[doringeman]

#77