EW-6888 Improve finishScheduled() outcome reporting

[fhanau]

No description provided.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR changes finishScheduled() to return EventOutcome directly instead of the internal FinishScheduledResult enum, improving outcome reporting by propagating richer error information from outcomeFromException().

Issues found (ranked by severity):

1. [HIGH] All three new DetailTypeId constants are placeholder 0x0ull — they collide with each other and will cause incorrect dispatching in outcomeFromException().
2. [MEDIUM] Leftover commented-out code and bare // TODO comments appear to be debug artifacts that should not be merged.
3. [LOW] The outcomeFromException function takes a non-const lvalue reference but doesn't modify the exception — should be const.

---

This review was generated by an AI assistant and may contain inaccuracies.

[ask-bonk]

[HIGH] These three DetailTypeId constants are all 0x0ull, meaning they're indistinguishable at runtime. getDetail() uses the ID as a key, so SCRIPT_KILLED_DETAIL_ID, INACTIVE_WEBSOCKETS_DETAIL_ID, and SCRIPT_NOT_FOUND_DETAIL_ID will all match the same detail entry. The first branch that checks any of these will match (or none will), making the subsequent branches dead code.

The existing IDs in the codebase all use unique random 64-bit values (e.g. CPU_LIMIT_DETAIL_ID = 0xfdcb787ba4240576ull). These need real, distinct IDs — or if the detail IDs don't exist yet on the exception-producing side, this dispatch logic can't work and should be restructured.

I understand the // TODO comment suggests this is WIP, but as-is this would silently misclassify outcomes if merged.

[ask-bonk]

[MEDIUM] Bare // TODO without explanation — if this is a reminder to assign real IDs, please make the TODO actionable (e.g. // TODO(felix): Assign unique DetailTypeId values; these are placeholders).

[ask-bonk]

[MEDIUM] Leftover debug artifacts — the bare // TODO on line 965 and the commented-out old code on line 967 look like they shouldn't be merged. If there's an open question here, make the TODO actionable; otherwise please clean these up.

Suggested change

	// TODO
	if (scheduledResult == EventOutcome::EXCEEDED_CPU) {
	// if (result == IoContext_IncomingRequest::FinishScheduledResult::TIMEOUT) {
	if (scheduledResult == EventOutcome::EXCEEDED_CPU) {

[ask-bonk]

[LOW] This function doesn't modify the exception — consider taking const kj::Exception& instead of kj::Exception&.

Suggested change

	EventOutcome outcomeFromException(kj::Exception& e) {
	EventOutcome outcomeFromException(const kj::Exception& e) {

[ask-bonk]

Review posted on PR #6606 with 4 inline comments. Summary of findings:

1. [HIGH] Three new DetailTypeId constants (SCRIPT_KILLED_DETAIL_ID, INACTIVE_WEBSOCKETS_DETAIL_ID, SCRIPT_NOT_FOUND_DETAIL_ID) are all 0x0ull — they collide with each other, making the branching logic in outcomeFromException() broken. All existing IDs in the codebase use unique random 64-bit values.

2. [MEDIUM] Leftover debug artifacts — bare // TODO comments and a commented-out old code line (// if (result == IoContext_IncomingRequest::FinishScheduledResult::TIMEOUT)) in queue.c++:967.

3. [LOW] outcomeFromException takes a mutable reference but doesn't modify the exception — should be const kj::Exception&.

github run

[codecov-commenter]

Codecov Report

❌ Patch coverage is 21.95122% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.84%. Comparing base (556d2f6) to head (bdfac0b).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
src/workerd/io/io-context.c++	11.53%	23 Missing ⚠️
src/workerd/api/queue.c++	25.00%	5 Missing and 1 partial ⚠️
src/workerd/io/worker-entrypoint.c++	57.14%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6606      +/-   ##
==========================================
- Coverage   70.85%   70.84%   -0.02%     
==========================================
  Files         438      438              
  Lines      123717   123738      +21     
  Branches    19470    19476       +6     
==========================================
  Hits        87660    87660              
- Misses      24530    24552      +22     
+ Partials    11527    11526       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codspeed-hq]

Merging this PR will degrade performance by 28.41%

❌ 1 regressed benchmark
✅ 71 untouched benchmarks
⏩ 129 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
❌	simpleStringBody[Response]	19.3 µs	26.9 µs	-28.41%

---

_{Comparing felix/041726-outcome-reporting (bdfac0b) with main (1c691a9)}

Footnotes

1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩