[DNM] [TEST] fix(config-cache): Defer CLI path resolution to execution phase#1224
[DNM] [TEST] fix(config-cache): Defer CLI path resolution to execution phase#1224runningcode wants to merge 17 commits into
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Defer CLI path resolution to execution phase ([#1224](https://github.com/getsentry/sentry-android-gradle-plugin/pull/1224))If none of the above apply, you can opt out of this check by adding |
| @get:Internal abstract val sentryTelemetryService: Property<SentryTelemetryService> | ||
|
|
||
| private val buildDirectory: Provider<File> = project.layout.buildDirectory.asFile | ||
| @get:Internal abstract val sentryProjectDir: DirectoryProperty |
There was a problem hiding this comment.
we don't actually want the contents of these directories as inputs
d4734af to
1a2f01e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fe25f47. Configure here.
b0fe22e to
73d9f21
Compare
| } | ||
| buildEvents.onOperationCompletion(sentryTelemetryProvider) |
There was a problem hiding this comment.
Bug: The unsynchronized started() method in SentryTelemetryService creates a race condition when accessing pendingOrgLookupParams, which is set by the synchronized start() method.
Severity: MEDIUM
Suggested Fix
Add the @Synchronized annotation to the started() method in SentryTelemetryService to ensure thread-safe access to the pendingOrgLookupParams field. Alternatively, annotate the pendingOrgLookupParams field with @Volatile to ensure memory visibility across threads.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt#L275-L276
Potential issue: A race condition exists in `SentryTelemetryService`. The `start()`
method is `@Synchronized` and sets the `pendingOrgLookupParams` field. However, the
`started()` method, which reads and nullifies this field, is not synchronized. During
parallel Gradle builds, multiple threads can call `started()` concurrently. This can
lead to a race condition where the write to `pendingOrgLookupParams` during the
configuration phase may not be visible to threads reading it in the execution phase, due
to a lack of memory visibility guarantees.
Also affects:
plugin-build/src/main/kotlin/io/sentry/android/gradle/telemetry/SentryTelemetryService.kt:115~119
Add a pull_request workflow that benchmarks this project's `help` task with the configuration cache disabled (2 warm-ups, 5 builds), comparing the PR base commit against the head commit in a single gradle-profiler run via the git-checkout mutator. This is separate from the existing duckduckgo benchmark build: it profiles configuration time of this project rather than the runtime cost of applying the plugin to a sample app, and it runs automatically on every PR. Results are uploaded as an artifact and summarized as a sticky PR comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gradle-profiler does not write summary rows (mean, min, max) to benchmark.csv; those only appear in the HTML report. The comment parser looked for a non-existent `mean` row and always fell back to "Could not parse benchmark results". Average the `measured build #N` rows instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The sentry-cli binary path was resolved during Gradle's configuration phase via SentryCliValueSource and cached. When the binary was deleted (OS temp cleanup, build dir cleaned) or the plugin was upgraded (different CLI version), the cached path became stale, causing build failures like "Could not start sentry-cli*.exe". Move CLI path resolution entirely to the execution phase: - Remove SentryCliValueSource, cliExecutableProvider(), and the cliExecutable @input property from all tasks - Resolve the CLI path fresh inside computeCommandLineArgs() at execution time via getSentryCliPath() + maybeExtractFromResources() - Remove SentryCliInfoValueSource and SentryCliVersionValueSource which spawned sentry-cli processes during configuration phase; use BuildConfig.CliVersion and URL-based heuristics instead - Improve maybeExtractFromResources() to handle CLI version changes by extracting the current bundled version when a stale path is detected in build/tmp/ Fixes #613 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace private val fields that captured project.projectDir and project.rootDir with abstract DirectoryProperty fields annotated with @internal. Properties are wired during configuration via asSentryCliExec() and resolved during execution, following Gradle best practices for configuration cache compatibility. Also removes duplicate buildDir property from SentryUploadNativeSymbolsTask in favor of the base class property. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace fileValue(project.rootDir) with project.rootProject.layout.projectDirectory so the property is serialized as a project-relative reference by the config cache instead of an absolute path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The synchronous SentryCliInfoValueSource was removed because it ran sentry-cli during configuration phase, which is incompatible with Gradle's configuration cache. This restores the default organization lookup by running sentry-cli info in a background daemon thread after telemetry is initialized. If the process times out or fails, the scope is left unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move fetchDefaultOrgInBackground() from start() (called during configuration via taskGraph.whenReady) to the started() callback of BuildOperationListener which fires during execution phase. Gradle's config cache instrumentation detects ProcessBuilder.start() calls during configuration regardless of which thread they run on. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ovider Add DirectoryProperty overloads for getSentryCliPath, getSentryPropertiesPath, searchCliInPropertiesFile, getCliResourcesExtractionPath, and maybeExtractFromResources to match existing test expectations. Keep File-based methods for the telemetry background thread. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use isolated.rootProject.projectDirectory on Gradle 8.8+ to avoid deprecated project.rootProject access that will break when project isolation is enforced. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the File overload and keep only the DirectoryProperty signature. Update SentryTelemetryService to pass DirectoryProperty instead of File. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Revert SentryCliProvider to its main branch state, preserving memoization, SentryCliValueSource, cliExecutableProvider, and getIsolatedRootProjectDir. Only add File overloads for getSentryCliPath, searchCliInPropertiesFile, getSentryPropertiesPath, and getCliResourcesExtractionPath needed by the telemetry service's execution-phase CLI resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SentryCliProvider now matches main exactly. Updated SentryTelemetryService to use DirectoryProperty for all CLI dir params so it can call getSentryCliPath(DirectoryProperty) directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add clearMemoizedCliPath() to SentryCliProvider and call it in SentryCliExecTaskTest @before to avoid stale memoized paths from other tests interfering with CLI resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The memoizedCliPath field persisted for the lifetime of the Gradle daemon, which could return a stale CLI path after plugin upgrades or build dir cleanups. Since tasks now resolve the CLI path at execution time, the memoization is unnecessary and harmful. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SentryCliExecTask resolved the sentry-cli path inside its action by calling getSentryCliPath() at execution time, so the path was never a declared task input. Wire the existing SentryCliValueSource into a new @input cliExecutable property via cliExecutableProvider(), so the path is a config-cache-safe, lazily resolved task input. The now-unused sentryProjectDir/sentryRootDir task properties (and the duplicate getIsolatedRootProjectDir helper) are removed; the value source resolves those from the project itself. buildDirectory stays, since extraction from resources still needs it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the manual Thread { }.apply { isDaemon = true; name = ...;
start() } with the kotlin.concurrent.thread helper, which creates,
configures, and starts the daemon thread in a single call. Behavior is
unchanged: the sentry-cli org lookup still runs as a named daemon
thread so it never blocks or outlives the build.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
082b792 to
f15b88f
Compare
|
| Scenario | Mean build time |
|---|---|
Base (help base) |
1735 ms |
PR (help PR) |
1667 ms |
| Difference | ✅ -68 ms (-3.9%) |

Just testing do not review.
Summary
SentryCliValueSource,cliExecutableProvider(), and thecliExecutable@Inputproperty from all tasks — CLI path is now resolved fresh insidecomputeCommandLineArgs()at task execution timeSentryCliInfoValueSourceandSentryCliVersionValueSourcewhich spawnedsentry-cliprocesses during configuration phase; useBuildConfig.CliVersionand URL-based SaaS heuristics insteadmaybeExtractFromResources()to handle CLI version mismatches by extracting the current bundled version when a stale path from a previous plugin version is detectedFixes #613
Fixes GRADLE-70
Root Cause
The CLI path was resolved during configuration via
SentryCliValueSourceand cached by Gradle's configuration cache. This cached path became stale in two scenarios:Version mismatch on plugin upgrade: Config cache stored
sentry-cli-2.46.0.exebut the upgraded plugin shipssentry-cli-2.51.1.exe. The old recovery code inmaybeExtractFromResources()only re-extracted when paths matched exactly, sosentry-cli-2.46.0.exe != sentry-cli-2.51.1.execaused it to skip extraction.Binary deleted: The extracted CLI binary was cleaned up (OS temp cleanup,
./gradlew clean, or daemon restart) while the config cache still held the old path.Approach
Rather than patching the config cache inputs, we eliminate the problem entirely by not caching the CLI path at all. The path is resolved fresh during each task's execution phase, which is the correct time to do filesystem I/O.
This also removes the telemetry
ValueSources that executedsentry-cli infoandsentry-cli --versionduring configuration phase (~38% overhead on large projects per #1100).🤖 Generated with Claude Code