Skip to content

Conversation

@fhanau
Copy link
Contributor

@fhanau fhanau commented 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.

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.
@fhanau fhanau requested review from a team as code owners December 23, 2025 18:53
@fhanau fhanau requested a review from mar-cf December 23, 2025 18:53
@fhanau
Copy link
Contributor Author

fhanau commented Dec 23, 2025

@mar-cf: Re. tests: This is already being tested in //src/workerd/api/tests:r2-test – convince yourself that the warning is present on main but disappears after this change. Due to the number of different functions where this can happen, I can't write a regression test for each affected API.
Note that this PR is not aiming to exhaustively cover all cases where the issue appears – I merely noticed that on the edge we frequently get this warning for R2-related code, so this should reduce the error volume further. That being said the warning is of limited priority at this point since the volume is quite low and it merely indicates a bit of waste happening (we create a WorkerInterface and associated WorkerTracer that ends up unused due to an error we didn't check for yet). This PR achieves incremental progress, lmk if you think that isn't the right approach here.

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 23, 2025

CodSpeed Performance Report

Merging #5756 will degrade performance by 11.71%

Comparing felix/122325-stw-unused-tracer (346bcd3) with main (7a5c452)

Summary

❌ 1 regression
✅ 56 untouched
⏩ 34 skipped1

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

Benchmarks breakdown

Benchmark BASE HEAD Efficiency
Encode_ASCII_32[TextEncoder][0/0/32] 2.9 ms 3.3 ms -11.71%

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.

@danlapid
Copy link
Collaborator

Discussed privately and while this might improve things in local dev and so there's no reason not to run it - it won't actually sort out the prod errors because of some added layers in the prod environment between the user worker and R2's worker.

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.

2 participants