Skip to content

Report internalError outcome for overloaded exceptions#6668

Open
fhanau wants to merge 1 commit intomainfrom
felix/042426-overloaded-outcome
Open

Report internalError outcome for overloaded exceptions#6668
fhanau wants to merge 1 commit intomainfrom
felix/042426-overloaded-outcome

Conversation

@fhanau
Copy link
Copy Markdown
Contributor

@fhanau fhanau commented Apr 25, 2026

overloaded-type exceptions are generally due to some internal limit being hit – they are generally unrelated to the worker CPU limit and not something directly caused by a worker script, so internalError fits better than exceededCpu. Note that only exceptions without a qualifying exception detail are affected, for those we continue to report a more precise outcome.

=====================
As suggested by @dom96 when overloaded exceptions came up.
Also see the downstream PR.

overloaded-type exceptions are generally due to some internal limit being hit –
they are generally unrelated to the worker CPU limit and not something directly
caused by a worker script, so internalError fits better than exceededCpu.
Note that only exceptions without a qualifying exception detail are affected,
for those we continue to report a more precise outcome.
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 25, 2026

The change looks correct and well-placed. Let me review the full analysis:

  1. If-else chain ordering is correct: The new OVERLOADED check at line 1616 is placed after the CPU_LIMIT_DETAIL_ID and MEMORY_LIMIT_DETAIL_ID checks. This is important because some OVERLOADED exceptions carry these details (e.g., alarm CPU timeout at global-scope.c++:565, memory limit at io-context.h:1448), and those should continue to be classified as EXCEEDED_CPU / EXCEEDED_MEMORY rather than INTERNAL_ERROR.

  2. The comment is accurate: It correctly explains the rationale for why detail-bearing overloaded exceptions are not affected.

  3. No backward compatibility concern: This only changes the outcome reporting for undetailed overloaded exceptions (from EXCEPTION to INTERNAL_ERROR). This is purely a metrics/observability change, not a behavioral change visible to workers. No compat flag needed.

  4. No security concern: This is outcome classification logic only.

  5. No test coverage exists for RequestObserverWithTracer in general, so the lack of tests for this change is consistent with the existing pattern. The class is tested indirectly through integration tests.

The change is small, correct, and well-reasoned. The ordering in the if-else chain is safe.

LGTM

github run

@fhanau fhanau marked this pull request as ready for review April 25, 2026 01:52
@fhanau fhanau requested review from a team as code owners April 25, 2026 01:52
@fhanau fhanau requested a review from dom96 April 25, 2026 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant