Skip to content

[DNM] [TEST] fix(config-cache): Defer CLI path resolution to execution phase#1224

Open
runningcode wants to merge 17 commits into
mainfrom
no/fix-config-cache-cli-path
Open

[DNM] [TEST] fix(config-cache): Defer CLI path resolution to execution phase#1224
runningcode wants to merge 17 commits into
mainfrom
no/fix-config-cache-cli-path

Conversation

@runningcode
Copy link
Copy Markdown
Contributor

@runningcode runningcode commented May 22, 2026

Just testing do not review.

Summary

  • Defer sentry-cli path resolution from Gradle's configuration phase to execution phase, fixing recurring build failures when the configuration cache stores a stale CLI binary path
  • Remove SentryCliValueSource, cliExecutableProvider(), and the cliExecutable @Input property from all tasks — CLI path is now resolved fresh inside computeCommandLineArgs() at task execution time
  • Remove SentryCliInfoValueSource and SentryCliVersionValueSource which spawned sentry-cli processes during configuration phase; use BuildConfig.CliVersion and URL-based SaaS heuristics instead
  • Improve maybeExtractFromResources() to handle CLI version mismatches by extracting the current bundled version when a stale path from a previous plugin version is detected

Fixes #613
Fixes GRADLE-70

Root Cause

The CLI path was resolved during configuration via SentryCliValueSource and cached by Gradle's configuration cache. This cached path became stale in two scenarios:

  1. Version mismatch on plugin upgrade: Config cache stored sentry-cli-2.46.0.exe but the upgraded plugin ships sentry-cli-2.51.1.exe. The old recovery code in maybeExtractFromResources() only re-extracted when paths matched exactly, so sentry-cli-2.46.0.exe != sentry-cli-2.51.1.exe caused it to skip extraction.

  2. 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 executed sentry-cli info and sentry-cli --version during configuration phase (~38% overhead on large projects per #1100).

🤖 Generated with Claude Code

@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 22, 2026

GRADLE-70

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 22, 2026

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

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 #skip-changelog to the PR description or adding a skip-changelog label.

Generated by 🚫 dangerJS against f15b88f

@runningcode runningcode marked this pull request as draft May 22, 2026 12:59
@runningcode runningcode changed the title fix(config-cache): Defer CLI path resolution to execution phase [DNM] [TEST] fix(config-cache): Defer CLI path resolution to execution phase May 22, 2026
Comment thread plugin-build/src/main/kotlin/io/sentry/android/gradle/SentryCliProvider.kt Outdated
@runningcode runningcode reopened this May 26, 2026
@get:Internal abstract val sentryTelemetryService: Property<SentryTelemetryService>

private val buildDirectory: Provider<File> = project.layout.buildDirectory.asFile
@get:Internal abstract val sentryProjectDir: DirectoryProperty
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we don't actually want the contents of these directories as inputs

@runningcode runningcode marked this pull request as ready for review May 26, 2026 11:33
@runningcode runningcode reopened this May 26, 2026
@runningcode runningcode requested a review from 0xadam-brown as a code owner May 26, 2026 14:02
@runningcode runningcode force-pushed the no/fix-config-cache-cli-path branch from d4734af to 1a2f01e Compare May 28, 2026 11:21
Comment thread plugin-build/src/main/kotlin/io/sentry/android/gradle/SentryCliProvider.kt Outdated
Comment thread plugin-build/src/main/kotlin/io/sentry/android/gradle/util/SentryCliExec.kt Outdated
Comment thread plugin-build/src/main/kotlin/io/sentry/android/gradle/SentryCliProvider.kt Outdated
Comment thread plugin-build/src/main/kotlin/io/sentry/android/gradle/SentryCliProvider.kt Outdated
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ 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.

Comment thread plugin-build/src/main/kotlin/io/sentry/android/gradle/SentryCliProvider.kt Outdated
@runningcode runningcode force-pushed the no/fix-config-cache-cli-path branch from b0fe22e to 73d9f21 Compare May 29, 2026 07:35
Comment on lines 275 to 276
}
buildEvents.onOperationCompletion(sentryTelemetryProvider)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

runningcode and others added 17 commits May 29, 2026 14:35
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>
@runningcode runningcode force-pushed the no/fix-config-cache-cli-path branch from 082b792 to f15b88f Compare May 29, 2026 13:21
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

help configuration benchmark (configuration cache disabled)

Mean of 5 builds after 2 warm-ups.

Scenario Mean build time
Base (help base) 1735 ms
PR (help PR) 1667 ms
Difference ✅ -68 ms (-3.9%)

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.

Build failures regarding sentry-cli7059257849644749494.exe on macOS

1 participant