Skip to content

Conversation

@mar-cf
Copy link
Contributor

@mar-cf mar-cf commented Dec 12, 2025

No description provided.

@mar-cf mar-cf requested review from a team as code owners December 12, 2025 13:16
@fhanau
Copy link
Contributor

fhanau commented Dec 12, 2025

@mar-cf I'm sorry, is this not more or less the same PR as #5577 which has been open for 3 weeks now?

@mar-cf
Copy link
Contributor Author

mar-cf commented Dec 12, 2025

@mar-cf I'm sorry, is this not more or less the same PR as #5577 which has been open for 3 weeks now?

It's more or less the same. I never saw tests nor an explanation that made sense. Check the internal PR

@mar-cf mar-cf requested a review from fhanau December 12, 2025 16:06
@mar-cf
Copy link
Contributor Author

mar-cf commented Dec 16, 2025

@fhanau can you either approve this PR or push some changes to the internal PR I opened to support landing the other (update to #5577 + description)

Copy link
Contributor

@fhanau fhanau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code itself LGTM, but there is one complication with downstream usage: We support marking a WorkerTracer as unused, but that does not extend to the RPC equivalent, which is set to just have a no-op markUnused function, it would have to propagate the unused signal. However, that might just be fine if the duplicate alarm scenario cannot happen under the RPC case as you suggested? We'd have to support it in the future though if we end up marking tracers as unused in more cases.

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 18, 2025

CodSpeed Performance Report

Merging #5685 will improve performances by 9.56%

Comparing mar/stw-alarm (d3ade51) with main (7b5d410)

Summary

⚡ 1 improvement
✅ 56 untouched
⏩ 34 skipped1

Benchmarks breakdown

Benchmark BASE HEAD Change
simpleStringBody[Response] 21.4 µs 19.6 µs +9.56%

Footnotes

  1. 34 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.

@mar-cf
Copy link
Contributor Author

mar-cf commented Dec 18, 2025

However, that might just be fine if the duplicate alarm scenario cannot happen under the RPC case as you suggested? We'd have to support it in the future though if we end up marking tracers as unused in more cases.

Exactly!

@mar-cf mar-cf merged commit 117eda7 into main Dec 18, 2025
31 of 35 checks passed
@mar-cf mar-cf deleted the mar/stw-alarm branch December 18, 2025 21:53
fhanau added a commit that referenced this pull request Dec 23, 2025
With #5685, we managed to reduce the volume of "destructed WorkerTracer"
warnings by >80% by eliminating it in the case of duplicate alarm events.
Another case where this happens are R2 API calls where we create a new
WorkerInterface for the R2 call before completing error checking, as seen in the
r2-test itself. We can easily avoid this by moving getHttpClient() calls behind
error checks.
While this PR is not aiming for completion I also cleaned up a call in
web-socket.c++ that may be susceptible to the same issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants