@danlapid @jasnell any questions/concerns here? Based on Sentry reporting this should address the issue at hand here – fetch spans are sometimes being reported after the outcome event, likely when the span only wraps up after a worker invocation has returned (I tried to cause this in a test using ctx.waitUntil(), but couldn't actually reproduce it). I think this is best addressed by deferring outcome event reporting.

Can't say I believe in or like this change.
I get that it seems to resolve this issue but this is just more guesswork.
As I said previously, tracing is an IO based thing.
If the WorkerTracer lives longer than the IoContext (onset happens before/as we construct the IoContext, outcome happens after/as we destruct the IoContext) then all of these problems should be gone for good.
I'd rather see this represented clearly in the IoContext class or in a class encompassing the IoContext.

Can't say I believe in or like this change. I get that it seems to resolve this issue but this is just more guesswork. As I said previously, tracing is an IO based thing. If the WorkerTracer lives longer than the IoContext (onset happens before/as we construct the IoContext, outcome happens after/as we destruct the IoContext) then all of these problems should be gone for good. I'd rather see this represented clearly in the IoContext class or in a class encompassing the IoContext.

Kindly note that the WorkerTracer already outlives the IoContext (or rather all events that can be reported from it) since IoContext_IncomingRequest owns WorkerTracer. (Otherwise we would not be able to print the warning in question here, which is reported within WorkerTracer).
An elegant way to avoid this issue would be to delay when the outcome event is reported to WorkerTracer – this currently happens in the destructor of a RequestObserver (RequestObserverWithTracer for workerd and its internal equivalent downstream). If we were to move this reporting to another class or change when this class gets deallocated that could work, but I believe that either would be a significant lift.

Can't say I believe in or like this change. I get that it seems to resolve this issue but this is just more guesswork. As I said previously, tracing is an IO based thing. If the WorkerTracer lives longer than the IoContext (onset happens before/as we construct the IoContext, outcome happens after/as we destruct the IoContext) then all of these problems should be gone for good. I'd rather see this represented clearly in the IoContext class or in a class encompassing the IoContext.

Kindly note that the WorkerTracer already outlives the IoContext (or rather all events that can be reported from it) since IoContext_IncomingRequest owns WorkerTracer. (Otherwise we would not be able to print the warning in question here, which is reported within WorkerTracer).

An elegant way to avoid this issue would be to delay when the outcome event is reported to WorkerTracer – this currently happens in the destructor of a RequestObserver (RequestObserverWithTracer for workerd and its internal equivalent downstream). If we were to move this reporting to another class or change when this class gets deallocated that could work, but I believe that either would be a significant lift.

Sounds like changing it so the IoContext destructor tells the WorkerTracer to report the outcome event would be simple then.
Since it already is outlived by it.

Let's go with that.

Sounds like changing it so the IoContext destructor tells the WorkerTracer to report the outcome event would be simple then. Since it already is outlived by it.

Let's go with that.

Based on local testing, I believe this can't work as intended: In the common case, the IoContext/IoContext_IncomingRequest shuts down before metrics get reported (at least in workerd). The WorkerTracer is still being kept alive based on being co-owned by the RequestObserver though, and the outcome gets reported when the RequestObserver shuts down (right before WorkerTracer is deallocated). In this case we can't dispatch the outcome event when the IoContext shuts down because it is simply not available yet.
In the case this PR seeks to address, the RequestObserver is destructed before the IoContext and accordingly reports the outcome event at that time, the IoContext may still add more events so we need to defer dispatching the outcome.

Sounds like changing it so the IoContext destructor tells the WorkerTracer to report the outcome event would be simple then. Since it already is outlived by it.
Let's go with that.

Based on local testing, I believe this can't work as intended: In the common case, the IoContext/IoContext_IncomingRequest shuts down before metrics get reported (at least in workerd). The WorkerTracer is still being kept alive based on being co-owned by the RequestObserver though, and the outcome gets reported when the RequestObserver shuts down (right before WorkerTracer is deallocated). In this case we can't dispatch the outcome event when the IoContext shuts down because it is simply not available yet. In the case this PR seeks to address, the RequestObserver is destructed before the IoContext and accordingly reports the outcome event at that time, the IoContext may still add more events so we need to defer dispatching the outcome.

I understand that this PR seems like a simple fix to you that addresses the immediate need but this comment just proves how convoluted the request lifecycle is.
We cannot continue like this.
I have yet to hear any valid logical reason why the outcome can change after the IoContext is destructed.
The fact that it could is a bug.

I understand that this PR seems like a simple fix to you that addresses the immediate need but this comment just proves how convoluted the request lifecycle is. We cannot continue like this. I have yet to hear any valid logical reason why the outcome can change after the IoContext is destructed. The fact that it could is a bug.

I'm not aware of the outcome changing after the IoContext shuts down here, or that it would need to be supported. What I see happening is that the order in which RequestObserver and IoContext shut down is not fixed, which puts us into this position. Do you think that needs to be changed so that the RequestObserver destructs first? Based on a brief look on the internal and workerd implementations, it looks to me like the outcome in RequestObserver is being set by RequestObserver::reportFailure, RequestObserver::setOutcome and when checking the return value of invoking a (custom)event. To me it looks like the issue with when outcome is reported is inherent in this design and hard to avoid, please let me know if you can think of a workaround/new approach.

Ok looked together.
Our codebase is too much of a mess at the moment.
We should clean this up later.

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉