chore: sync master to dev - #220
Conversation
`killGeneration` cancelled the generation's scope and opened the next one on the very next line. DataStore drops a file from its process-global `activeFiles` set inside an `invokeOnCompletion` handler on that scope's Job (`SimpleActor.init` -> `DataStoreImpl.onComplete` -> `StorageConnection.close`), which runs when the job *completes*, not when `cancel` returns — and a write-actor child still detaching on Dispatchers.IO holds the job in COMPLETING_WAITING_CHILDREN. So the reopen raced the removal and tripped `check(!activeFiles.contains(path))`, FileStorage.kt:52, "There are multiple DataStores active for the same file". A quiet machine wins that race every time; a 4-worker CI runner does not. It went red once on release/release-v.1.49.0 (run 31421333548, attempt 1) and green on the re-run, having passed three times on PR #218 — the signature of a race, not a break. `cancelAndJoin` makes the handover deterministic. Proven rather than assumed: holding the job in Cancelling for 600ms reproduces the exact exception with the bare `cancel` and survives it with the join. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MYLZo1s6f53WJ21Psk69tu
AGP wires a module's own test fixtures onto its androidTest runtime classpath. This module is the only one that both enables testFixtures and ships an androidTest source set, so `testFixturesImplementation(libs.paparazzi.core)` put Paparazzi's JVM rendering stack — layoutlib, com.android.tools:sdk-common, protobuf-java — into an instrumented-test APK, where it collided with the protobuf-javalite that :core:core brings via firebase-perf. That reddened :core:ui:kit:checkDebugAndroidTestDuplicateClasses and, behind it, mergeDebugAndroidTestJavaResource on google/protobuf/empty.proto and then on JUnit 5's META-INF/LICENSE.md. It blocked the v1.49.0 production deploy (run 31429081872), and it had been latent since 4af7918 (2026-07-26) because nothing compiles androidTest before the deploy job. The Paparazzi plugin was never the leak: eight other modules apply it alongside androidTest sources and stayed clean. dependencyInsight on debugAndroidTestRuntimeClasspath listed app.cash.paparazzi before this change and finds nothing after. compileOnly loses nothing at runtime. The harness only ever executes on a testDebugUnitTest classpath, and all twelve modules consuming testFixtures(project(":core:ui:kit")) apply the Paparazzi plugin themselves and get junit-jupiter from the convention plugin's test bundle — set containment checked, not assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NvAARa5m1bXMKxuVYeo86t
… contradict chart_singlePoint_rendersCanvasWithoutCrash asserted that a one-point state renders ChartCanvas. Commit 9139d8c (2026-07-29) made that false in the same diff that wrote it: State.content resolves `points.size >= MIN_CHART_POINTS` to Content.Plot and anything below it to Content.Loading, so the screen composes a loading indicator and the canvas is unreachable. The test has not passed since, and only surfaced now because the smoke suite runs once per release. The canvas keeps its own below-two-points fallback — gridlines, then return — so what was lost is reachability, not the maths. Reachability is the more useful thing to pin: it is ChartContent's own claim that "there is no arrangement of fields here that can put an empty chart on screen", and CommonHandler resolves every sub-threshold dataset to EmptyReason.NO_DATA_FOR_EXERCISE, so the state this test builds cannot occur in production and hand-construction is the only way to attack the gate. Renamed to say what it now checks. Measured, not assumed: relaxing the branch to `points.isNotEmpty()` turns it red and names the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NvAARa5m1bXMKxuVYeo86t
assembleDebug does not build the androidTest variants, and the only workflow that does — ui_tests.yml — is workflow_dispatch/workflow_call only. So the instrumented source sets were compiled exactly once per release, inside android_deploy_prod.yml's ui_tests job, downstream of a guard and a build that had both already gone green. That is how a Paparazzi test-fixture leak from 2026-07-26 and a chart test broken on 2026-07-29 both reached the v1.49.0 deploy (run 31429081872) unseen: the last standalone ui_tests run was 2026-05-03. assembleDebugAndroidTest reproduces the build-graph half with no emulator. Measured, not assumed: reintroducing the fixture leak fails this step at :core:ui:kit:checkDebugAndroidTestDuplicateClasses, the exact task the deploy died on. It assembles the test APKs, it does not run them. Execution still needs a device and stays in ui_tests.yml — the chart test in that same run went red on an assertion, which is out of this step's reach by construction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NvAARa5m1bXMKxuVYeo86t
- Mode: `release` - Source branch: `dev` - versionName: `1.49.0` - versionCode: `50` <!-- changelog generated at deploy time -->
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d6d6b6ac6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # and stays in `ui_tests.yml` — a test that goes red on an assertion (as the chart test in | ||
| # that same run did) is out of this step's reach by construction. | ||
| - name: Assemble instrumented tests | ||
| run: ./gradlew assembleDebugAndroidTest --full-stacktrace |
There was a problem hiding this comment.
When matching Android-test outputs exist in the Gradle build cache restored earlier in this workflow, this invocation can succeed with tasks reported FROM-CACHE, so the new gate does not actually compile the instrumented source sets on every PR as claimed. Gradle 9.5 --help describes --no-build-cache as “Disables the Gradle build cache”; use the repository's documented --rerun-tasks --no-build-cache --continue phase-exit command so this gate supplies fresh evidence.
AGENTS.md reference: AGENTS.md:L43-L49
Useful? React with 👍 / 👎.
| # Measured, not assumed: the v1.49.0 deploy (run 31429081872) died on two of them — | ||
| # `:core:ui:kit:checkDebugAndroidTestDuplicateClasses` and | ||
| # `mergeDebugAndroidTestJavaResource`, from a Paparazzi test-fixture leak that landed | ||
| # 2026-07-26 and sat unseen for a month because the last standalone `ui_tests` run was | ||
| # 2026-05-03. Both are build-graph failures: this step alone reproduces them, and it needs |
There was a problem hiding this comment.
This run number, failure chronology, and dated account of what previously happened are release history rather than a guard needed where the command is edited. Keep only the current assemble-versus-execute constraint here and move the incident record to the commit body or the relevant documentation registry.
AGENTS.md reference: AGENTS.md:L103-L108
Useful? React with 👍 / 👎.
| * the job *completes*, not when `cancel` returns. Its `Dispatchers.IO` children keep the job in | ||
| * Cancelling for as long as they take to unwind, so opening the next generation without joining | ||
| * races that removal and trips `check(!activeFiles.contains(path))` — "There are multiple | ||
| * DataStores active for the same file" (FileStorage.kt:52). That race is what made this test flake | ||
| * red on a loaded CI runner while staying green locally. |
There was a problem hiding this comment.
The FileStorage.kt:52 reference is an explicitly prohibited line-based citation, and a repo-wide search for the distinctive activeFiles and exception text finds no documentation containing this derivation. Add the derivation to documentation first, cite it by a stable anchor, and retain only the cancelAndJoin guard and conclusion in this KDoc.
AGENTS.md reference: AGENTS.md:L115-L126
Useful? React with 👍 / 👎.
| // `checkDebugAndroidTestDuplicateClasses` and `mergeDebugAndroidTestJavaResource` both failed | ||
| // (the latter on `google/protobuf/empty.proto`, then on JUnit 5's `META-INF/LICENSE.md`). | ||
| // Measured, not assumed: `:core:ui:kit:dependencyInsight --configuration | ||
| // debugAndroidTestRuntimeClasspath --dependency app.cash.paparazzi` listed it before this | ||
| // change and finds nothing after; the eight other modules that apply the Paparazzi *plugin* |
There was a problem hiding this comment.
This block records prior failing tasks, collision artifacts, dependency-insight measurements, and what used to leak, duplicating incident history and derivation at the dependency declaration. Preserve only the guard that compileOnly keeps the JVM harness off androidTest and the consumer precondition here; first record the supporting analysis in documentation, then cite its stable anchor.
AGENTS.md reference: AGENTS.md:L103-L108
Useful? React with 👍 / 👎.
| // v3 put a SECOND, earlier gate in front of the canvas. The canvas still keeps its own | ||
| // below-two-points fallback — it draws gridlines and returns, index spacing needs n ≥ 2 | ||
| // (ChartCanvas.kt) — but the screen no longer routes a sub-threshold dataset there at | ||
| // all: `points.size >= MIN_CHART_POINTS -> Content.Plot`, else `Content.Loading` | ||
| // (ExerciseChartStore.kt). So what this pins is reachability, not the canvas's maths. |
There was a problem hiding this comment.
The account of what “v3 put” in front of the canvas and the mutation experiment are history and derivation rather than a point-of-edit guard; the renamed test and its assertions already express the current reachability contract. Record the derivation in documentation first and leave only a concise conclusion with an anchor citation if the invariant still needs explanation.
AGENTS.md reference: AGENTS.md:L103-L108
Useful? React with 👍 / 👎.
Unit Test Results1 999 tests 1 999 ✅ 11m 18s ⏱️ Results for commit 0d6d6b6. |
1 participant
Auto-generated PR to propagate master changes (version bumps, hotfixes) back to dev.