[apeacock1991]

Stacked on #8124.

Worker::Actor::runOnHibernate() takes a tracer but never set the event's onset. A buffered tail worker got a trace with no event, and the streaming tail writer asserts if no onset was reported, so an embedder couldn't pass a tracer at all.

The handler now reports a CustomEventInfo onset before it's delivered, the same as the [restore]() hook. It also reports a return once the handler settles, as runAlarm() does. A canceled handler reports no return.

Two new incoming-request-test cases run a hook with a tracer. One checks that it reports a CustomEventInfo onset and then a return. The other checks that a canceled hook reports the onset and no return.

[clanky-the-clanker]

Superseded by the review of 38db8750.

This review, of dd329b85

💬 Approved with comments — The onset and return reporting is correct and follows the existing [restore] and alarm patterns. The only comment is optional: the test could also check the return event and the canceled path.

Findings: 1 suggestion

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

📋 Code Quality: 💬 1 suggestion

The onset and return reporting is correct. The new test checks only the onset.

Recommendation: 💬 Optional Nits

• [SUGGESTION] The new test checks the CustomEventInfo onset but not the return event, and it doesn't cover a canceled handler, which should report no return. Both behaviours are stated in the description. (src/workerd/io/incoming-request-test.c++:667)

View full review

Production change checked, no defects found

• The CustomEventInfo onset is set right after the IncomingRequest is constructed, before the handler is delivered. That's the order the streaming tail writer requires.
• setReturn(context.now()) runs only on the settled path, after reported = true. A canceled handler goes through the KJ_DEFER path and reports no return, as the description says.
• A handler that throws still gets a return, because execution leaves the KJ_CATCH block and reaches setReturn.

The test covers only part of the change (src/workerd/io/incoming-request-test.c++:646-669)

• The new test checks that eventInfo is a CustomEventInfo. It doesn't check that a return was recorded, or that a canceled handler records none. The description states both behaviours, so tests for them would guard against regressions.

---

✅ All other checks passed

📝 Documentation: ✅ LGTM

No documentation issues. The new code comments and the test name match what the change does.

---

_{Reviewed commit: dd329b85}

[clanky-the-clanker]

Superseded by the review of 37ef6316.

This review, of 38db8750

✅ Approved — No findings. The previous test-coverage suggestion is fixed, and the onset and return reporting follows the existing [restore] and alarm patterns.

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

✅ All checks passed

📋 Code Quality: ✅ LGTM

• Production change (src/workerd/io/worker.c++):
  • The CustomEventInfo onset is set right after the IncomingRequest is constructed and before delivered(). This is the order the streaming tail writer requires.
  • setReturn(context.now()) runs only on the settled path, after reported = true. That path includes a handler that throws.
  • A canceled handler only goes through the KJ_DEFER block, so it reports no return.
• Tests (src/workerd/io/incoming-request-test.c++:647-720):
  • The settled test asserts "customOnset;return;".
  • The canceled test drops a handler that never settles and asserts only "customOnset;".

---

📝 Documentation: ✅ LGTM

No documentation issues. The code comments and test names match what the change does.

---

_{Reviewed commit: 38db8750}

[clanky-the-clanker]

Superseded by the review of e088e80d.

This review, of 37ef6316

✅ Approved — No findings. The onset and return reporting follows the existing [restore] and alarm patterns, and the updated tests cover both the settled and the canceled handler.

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

✅ All checks passed

📋 Code Quality: ✅ LGTM

No findings.

• The RecordingTracer in incoming-request-test.c++ overrides every pure virtual of BaseTracer with matching signatures.
• The settled-handler test expects "customOnset;return;". That matches the onset being set before delivered() and setReturn being called on the settled path in worker.c++.
• The canceled-handler test drops the pending promise and expects only "customOnset;". That matches a canceled handler leaving through the KJ_DEFER block without reaching setReturn.

---

📝 Documentation: ✅ LGTM

No documentation issues. The code comments and test names match what the change does.

---

_{Reviewed commit: 37ef6316}

[clanky-the-clanker]

Superseded by the review of 31941061.

This review, of e088e80d

✅ Approved — No findings. The onset and return reporting follows the existing [restore] and alarm patterns, and the updated tests cover both the settled and the canceled handler.

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

✅ All checks passed

📋 Code Quality: ✅ LGTM

No findings.

• The RecordingTracer in incoming-request-test.c++ overrides every pure virtual of BaseTracer with matching signatures.
• The settled-handler test expects "customOnset;return;". That matches the onset being set before delivered() and setReturn being called on the settled path in worker.c++.
• The canceled-handler test drops the pending promise and expects only "customOnset;". That matches a canceled handler leaving through the KJ_DEFER block without reaching setReturn.

---

📝 Documentation: ✅ LGTM

No documentation issues. The code comments and test names match what the change does.

---

_{Reviewed commit: e088e80d — same changes as 37ef6316, so this is that commit's review (no new review was run; line numbers are as of 37ef6316)}

[clanky-the-clanker]

✅ Approved — No findings. The onset and return reporting follows the existing [restore] and alarm patterns, and the updated tests cover both the settled and the canceled handler.

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

✅ All checks passed

📋 Code Quality: ✅ LGTM

No findings.

• The RecordingTracer in incoming-request-test.c++ overrides every pure virtual of BaseTracer with matching signatures.
• The settled-handler test expects "customOnset;return;". That matches the onset being set before delivered() and setReturn being called on the settled path in worker.c++.
• The canceled-handler test drops the pending promise and expects only "customOnset;". That matches a canceled handler leaving through the KJ_DEFER block without reaching setReturn.

---

📝 Documentation: ✅ LGTM

No documentation issues. The code comments and test names match what the change does.

---

_{Reviewed commit: 31941061 — same changes as 37ef6316, so this is that commit's review (no new review was run; line numbers are as of 37ef6316)}