Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[o11y] Fix segfault with tail worker self loop #3439

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Jan 30, 2025

To detect the main worker having a self loop, we need to compare the current WorkerService with the WorkerService corresponding to the tail worker. If service refers to an EntrypointService, that means we have to compare to the underlying worker instead.

As reported by @penalosa. Tracked this down together with @jmorrell.

To detect the main worker having a self loop, we need to compare the current
WorkerService with the WorkerService corresponding to the tail worker. If
service refers to an EntrypointService, that means we have to compare to the
underlying worker instead.
@fhanau fhanau requested a review from jasnell January 30, 2025 17:13
@fhanau fhanau requested review from a team as code owners January 30, 2025 17:13
@fhanau fhanau requested a review from harrishancock January 30, 2025 17:13
@danlapid
Copy link
Collaborator

Can we add a test?

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Nice catch

@fhanau
Copy link
Collaborator Author

fhanau commented Jan 30, 2025

Can we add a test?

Testing streaming tail worker is not really amenable to the testing framework we have in workerd (i.e. wd_test) since we don't define a number of tests in JS, but rather need to look at if communication between two worker stages works properly. This is easier to test within the downstream test environment.

@fhanau fhanau merged commit 100606c into main Jan 30, 2025
17 checks passed
@fhanau fhanau deleted the felix/012925-STW-segfault-fix branch January 30, 2025 22:28
@danlapid
Copy link
Collaborator

Can we add a test?

Testing streaming tail worker is not really amenable to the testing framework we have in workerd (i.e. wd_test) since we don't define a number of tests in JS, but rather need to look at if communication between two worker stages works properly. This is easier to test within the downstream test environment.

Sadly the downstream test environment is unable to test the specific modified file.

Perhaps we need to add some testing infrastructure to workerd.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants