Skip to content

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

Merged
fhanau merged 1 commit intomainfrom
felix/012925-STW-segfault-fix
Jan 30, 2025
Merged

[o11y] Fix segfault with tail worker self loop#3439
fhanau merged 1 commit intomainfrom
felix/012925-STW-segfault-fix

Conversation

@fhanau
Copy link
Contributor

@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
Collaborator

@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
Contributor 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.

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.

3 participants