Skip to content

fix: fix data race in echo provisioner #17142

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 28, 2025
Merged

fix: fix data race in echo provisioner #17142

merged 1 commit into from
Mar 28, 2025

Conversation

aslilac
Copy link
Member

@aslilac aslilac commented Mar 27, 2025

Closes coder/internal#546
Closes coder/internal#543

Fixes a data race in the echo provisioner

What happened

If you're curious, two things:

  • I was under the assumption that the incoming responses would always be under the ownership of the provisioner (no need to worry about synchronization or locking) because it would be coming from freshly decoded protobuf messages. Apparently this is very wrong, because we just call the echo provisioner functions directly using protobuf types in a bunch of tests. Serialization/deserialization doesn't happen anywhere in the process, we're just passing pointers around.

  • The incoming responses are a mixture of values which could be interpreted as "transferring ownership", and a few global/"static" values that we have around for common scenarios. Mutating the unique "owned" responses is fine, but mutating those global helpers is where the data race was occurring. The code was checking to see if the plan was "empty" (as in len(plan.Plan) == 0) and then replacing it with a "valid empty" plan (as in []byte("{}"), empty JSON object). The problem with this is that there should not be a valid plan if plan.Error is set. The "valid empty" plan was already added as a field to the PlanComplete static helper, but was intentionally omitted from a few other helpers that have Error values.

So with all that context, the fix is to just not mutate plan.Plan if plan.Error is set.

Also, since in production environments the data will actually be owned by this code, freshly deserialized from a protobuf message, I don't think there's any way this write could affect a real deployment.

@aslilac aslilac added the hotfix label Mar 27, 2025
Copy link

@cdr-bot cdr-bot bot left a comment

Choose a reason for hiding this comment

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

This PR is a hotfix and has been automatically approved.

  • ✅ Base is main or release branch
  • ✅ Has hotfix label
  • ✅ Head is from coder/coder
  • ✅ Less than 100 lines

@aslilac aslilac merged commit ca414b0 into main Mar 28, 2025
36 checks passed
@aslilac aslilac deleted the lilac/echo-race branch March 28, 2025 00:04
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flake: data race in echo provisioner flake: data race: TestUpdateWithRichParameters/EphemeralParameterFlags
1 participant