[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
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
082b792 to
f15b88f
Compare
|
| Scenario | Mean build time |
|---|---|
Base (help base) |
1870 ms |
PR (help PR) |
1818 ms |
| Difference | ✅ -52 ms (-2.8%) |
f15b88f to
dcbe1a1
Compare
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>
dcbe1a1 to
98cbe06
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 98cbe06. Configure here.
The org lookup was wired to started(), which never fires (the service is registered via onOperationCompletion, which only delivers finished events), so the org was never attached. Move the sentry-cli lookup into a ValueSource resolved during task execution and attach it on the first tracked task. This keeps the external process out of the configuration phase so the configuration cache stays valid, while still attaching the org before the build transaction is sent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| return | ||
| } | ||
| orgAttached = true | ||
| orgProvider?.orNull?.let { org -> | ||
| hub.configureScope { scope -> | ||
| if (scope.user?.id == null) { | ||
| scope.user = User().also { it.id = org } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: The SentryTelemetryService has a race condition. The attachDefaultOrg method lacks synchronization, allowing concurrent tasks in parallel builds to cause redundant operations and inconsistent state.
Severity: MEDIUM
Suggested Fix
Ensure thread-safety in SentryTelemetryService. Synchronize the access to the orgAttached flag and the subsequent scope configuration within the attachDefaultOrg method. This can be achieved by using a synchronized block or leveraging thread-safe constructs like AtomicBoolean.
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/telemetry/SentryTelemetryService.kt#L209-L221
Potential issue: The `SentryTelemetryService`, a Gradle `BuildService`, is not
thread-safe. The `attachDefaultOrg()` method, called during task execution, contains a
check-then-act race condition on the `orgAttached` flag. In parallel builds, multiple
tasks can call this method concurrently, pass the `orgAttached == false` check, and
proceed to configure the Sentry hub's scope simultaneously. This leads to a data race
and can result in redundant operations and inconsistent telemetry state, as the service
is shared across all tasks without serialization constraints.
Read the sentry-cli info output only after waitFor(timeout) returns, so a hung process is killed by the timeout instead of blocking the read indefinitely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| } catch (t: Throwable) { | ||
| logger.info { "Failed to fetch default org from sentry-cli: ${t.message}" } | ||
| null | ||
| } | ||
| } | ||
|
|
||
| companion object { | ||
| private val ORG_REGEX = Regex("""(?m)Default Organization: (.*)$""") | ||
| private const val CLI_INFO_TIMEOUT_SECONDS = 5L | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: The SentryOrgValueSource spawns a sentry-cli process during Gradle's configuration phase on every build, causing a performance overhead that the PR intended to remove.
Severity: MEDIUM
Suggested Fix
Refactor SentryOrgValueSource.obtain() to avoid spawning an external process. The organization information should be obtained through a different mechanism that is compatible with Gradle's configuration cache, or the operation should be deferred to task execution time without using a ValueSource that triggers this behavior.
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/SentryCliProvider.kt#L224-L274
Potential issue: The `SentryOrgValueSource.obtain()` method is called by Gradle on every
build to check the configuration cache fingerprint. This method spawns a `sentry-cli
info` external process to retrieve organization information. As a result, this
subprocess is executed during the configuration phase of every build, even when the
configuration is cached. This negates the performance improvements intended by the pull
request, which aimed to eliminate such processes from running during the configuration
phase.

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