This actually reverts logic I've added in a very recent PR that is required to land python-isolate-pools.
Clearing up the ownership of metrics so that the api object owns it and is responsible for destroying it is very critical.
The co-ownership of the metrics object is already too confusing and should be handled better but we need something better than Owning it again here.
Perhaps a better solution would be to move TeardownFinishedGuard into the Api object? seems to me like it would serve the purpose just as well?

In general this is part of future work I wanted to get to at some point so if you already need it now I wonder why not to go ahead and do it

Clearing up the ownership of metrics so that the api object owns it and is responsible for destroying it is very critical.

Could you elaborate on why it is critical?

Perhaps a better solution would be to move TeardownFinishedGuard into the Api object? seems to me like it would serve the purpose just as well?

Currently, teardownStarted() and teardownLockAcquired() are called in the Worker::Isolate destructor so I think it makes sense for Worker::Isolate to also be in control of when teardownFinished() is called.

I guess one way we could keep the unique ownership of metrics whilst also keeping teardownFinished() in Worker::Isolate is to pass a Worker::Api factory function to the Worker::Isolate constructor since the API object is always created just before the Worker::Isolate anyway. That would make construction/destruction of the API object deterministic/symmetrical wrt. other Worker::Isolate members.

Well the whole idea is to decouple the api class and the Isolate class.
With isolate pools the api will be created ahead of time and held in a pool and the Isolate class will only be constructed when a request comes in.

As per offline, if the API object is created ahead of time in a request-agnostic way, does it actually need an IsolateObserver before it's injected into an actual Worker::Isolate (it looks like IsolateObserver::created() is still called only when the Worker::Isolate is created)? If not, couldn't we just have Worker::Isolate create/own the observer and give it a ref to Worker::Api when needed?

If the API object does need it ahead of time (because for example we want to consider the initialization of the API object as part of observable isolate creation), then I'm tempted to suggest that the object we pre-initialize should be a Worker::Isolate which could just create the API object inside it during a pre-initialization phase and then later be turned into a fully initialized isolate with request-specific state (using a variant with two states internally, for example).

If these aren't really viable/good ideas for whatever reason, I suppose we could move TeardownFinishedGuard to the API object, but as I say above, that seems a bit dirty from an encapsulation POV.

@danlapid and I discussed this offline.

tl;dr - this change is a good enough short term change to fix the fact we're currently calling TeardownFinished() too early - longer term, we might want to look at refactoring the Isolate/Api abstractions and/or the IsolateObserver so we can have the metrics object(s) be uniquely owned

Will merge this now.