[PROF-13273] Fix profiler causing Bundler::PermissionError
#5146
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**What does this PR do?** This PR fixes a regression introduced in #4999: In that PR we added a call to `Bundler.bin_path` but did not realize that bundler not only returns the path **BUT CRUCIALLY** it makes sure the path exists by going as far as trying to create it if it doesn't. If the directory does not exist, and the application directory is read-only (or the app does not have enough permissions), the application will fail with: > There was an error while trying to write to (directory). It is likely > that you need to grant write permissions for that path. > (Bundler::PermissionError) Fixes #5137 **Motivation:** Fix a regression introduced in #4999 that shipped in v2.23.0. **Additional Notes:** The bundler's `bin_path` method has had a similar behavior/implementation for 15+ years: * master -> https://github.com/ruby/rubygems/blob/570c3419c7f111d8665710f4e2cb15ca878e606d/bundler/lib/bundler.rb#L119-L126 * Jul 10, 2010 -> ruby/rubygems@f799489 So we should be fine in replicating its behavior + having a test that checks we're matching it. Worst case, we get `CodeProvenance` slightly off, which compared to "broke the app", seems better. I've also added a `rescue` to the method so hopefully we don't run into more surprises here. (Note that while we could've kept calling `Bundler.bin_path`, wrapping it with the `rescue`, this would still produce the side-effect of creating the folder and it seems weird to me to end up in this situation of "turning on the profiler causes bin/ to be created". This is why I chose to reimplement the behavior, minus folder creation) **How to test the change?** I've updated the test coverage. Furthermore, the repro mentioned in #5137 also works to show it's fixed: ```bash $ rm -rf bin/ $ chmod -w . $ bundle exec rake compile && DD_PROFILING_ENABLED=true bundle exec ruby -rdatadog/profiling/preload -e "puts 'hello'" ```
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage * Fix with Cursor requires Datadog plugin ≥v2.17.0 🔗 Commit SHA: bbdbf93 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
BenchmarksBenchmark execution time: 2025-12-15 14:27:47 Comparing candidate commit bbdbf93 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 43 metrics, 2 unstable metrics. scenario:error - error tracking, with_error=true - third_party only
|
p-datadog
approved these changes
Dec 15, 2025
Member
|
Reported upstream: ruby/rubygems#9197 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR fixes a regression introduced in #4999: In that PR we added a call to
Bundler.bin_pathbut did not realize that bundler not only returns the path BUT CRUCIALLY it makes sure the path exists by going as far as trying to create it if it doesn't.If the directory does not exist, and the application directory is read-only (or the app does not have enough permissions), the application will fail with:
Fixes #5137
Motivation:
Fix a regression introduced in #4999 that shipped in v2.23.0.
Change log entry
Yes. Fix profiler causing
Bundler::PermissionErrorAdditional Notes:
The bundler's
bin_pathmethod has had a similarbehavior/implementation for 15+ years:
So we should be fine in replicating its behavior + having a test that checks we're matching it.
Worst case, we get
CodeProvenanceslightly off, which compared to "broke the app", seems better.I've also added a
rescueto the method so hopefully we don't run into more surprises here.(Note that while we could've kept calling
Bundler.bin_path, wrapping it with therescue, this would still produce the side-effect of creating the folder and it seems weird to me to end up in this situation of "turning on the profiler causes bin/ to be created". This is why I chose to reimplement the behavior, minus folder creation)How to test the change?
I've updated the test coverage.
Furthermore, the repro mentioned in #5137 also works to show it's fixed: