Skip to content

Compare isModifiedAfter against the given time#187727

Merged
auto-submit[bot] merged 2 commits into
flutter:masterfrom
passsy:fix-test-asset-rebuild-race
Jun 13, 2026
Merged

Compare isModifiedAfter against the given time#187727
auto-submit[bot] merged 2 commits into
flutter:masterfrom
passsy:fix-test-asset-rebuild-race

Conversation

@passsy

@passsy passsy commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Fixes #187725, regression was introduced in #187488

DevFSFileContent.isModifiedAfter returned true whenever the cached _fileStat was null, ignoring the time argument entirely. _fileStat is the sync baseline written only by markClean() the entries of a freshly built AssetBundle are never marked clean, so _fileStat is always null for them and isModifiedAfter always reported them as modified.

Pre-launch Checklist

@github-actions github-actions Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 9, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates DevFSFileContent.isModifiedAfter to determine modification status solely based on the file's current modification time, removing the fallback check on the cached _fileStat. A corresponding unit test is added to verify this behavior. Feedback suggests simplifying the conditional block in isModifiedAfter by returning the boolean expression directly.

Comment thread packages/flutter_tools/lib/src/devfs.dart Outdated
DevFSFileContent.isModifiedAfter returned true whenever the cached
_fileStat was null, ignoring the time argument entirely. _fileStat is the
sync baseline written only by markClean(); the entries of a freshly built
AssetBundle are never marked clean, so _fileStat is always null for them
and isModifiedAfter always reported them as modified.

flutter test's _needsRebuild() relies on isModifiedAfter to decide
whether build/unit_test_assets is up to date. With the broken short
circuit it always rebuilt the directory (delete + non-atomic rewrite) on
every invocation. When several flutter test invocations run in the same
project directory, one invocation's delete window overlaps another's
asset reads, surfacing as a transient

    Exception: Asset 'shaders/ink_sparkle.frag' not found

that fails an unrelated test.

isModifiedAfter is handed an explicit reference time and must not consult
the sync baseline. Determine modification purely from the file's current
modification time, matching the other DevFSContent implementations.

Regression from flutter#187488, which made the stat accessors pure. Before that
the mutating _stat() populated _fileStat as a side effect of reads during
the asset build, so _needsRebuild happened to work. Fixes
flutter#187725.
@passsy passsy force-pushed the fix-test-asset-rebuild-race branch from ca2ee71 to 33213e6 Compare June 9, 2026 10:46
@kevmoo

kevmoo commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request simplifies the logic of DevFSFileContent.isModifiedAfter to rely solely on the file's current modification time instead of falling back to the cached _fileStat. This prevents newly created DevFSFileContent instances from being incorrectly reported as modified when the file is older than the comparison time. A unit test has been added to verify this behavior. There are no review comments, so no additional feedback is provided.

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 9, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 9, 2026
@auto-submit

auto-submit Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/flutter/187727, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a member of flutter-hackers before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@kevmoo kevmoo requested a review from harryterkelsen June 9, 2026 18:57

@harryterkelsen harryterkelsen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@harryterkelsen harryterkelsen added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 9, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 9, 2026
@auto-submit

auto-submit Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

auto label is removed for flutter/flutter/187727, Failed to enqueue flutter/flutter/187727 with HTTP 400: Pull request Required status check "Merge Queue Guard" is expected..

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 9, 2026
@auto-submit

auto-submit Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

auto label is removed for flutter/flutter/187727, Failed to enqueue flutter/flutter/187727 with HTTP 400: Pull request Required status check "Merge Queue Guard" is expected..

@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 9, 2026
@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 12, 2026
@jmagman jmagman added the CICD Run CI/CD label Jun 12, 2026
@auto-submit auto-submit Bot added this pull request to the merge queue Jun 12, 2026
Merged via the queue into flutter:master with commit 3c38ccb Jun 13, 2026
173 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 13, 2026
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Jun 15, 2026
Roll Flutter from b7cb925419e6 to 5827d5fd2b8d (35 revisions)

flutter/flutter@b7cb925...5827d5f

2026-06-15 engine-flutter-autoroll@skia.org Roll Skia from 7128af60575a to c8d9f80f13e4 (1 revision) (flutter/flutter#188015)
2026-06-15 jason-simmons@users.noreply.github.com In the APNG decoder, validate the chunk data length before calling GetChunkSize to avoid potential overflow in the chunk size calculation (flutter/flutter#187949)
2026-06-15 engine-flutter-autoroll@skia.org Roll Skia from 6b4ac3bfb39d to 7128af60575a (1 revision) (flutter/flutter#188011)
2026-06-15 engine-flutter-autoroll@skia.org Roll Skia from 0a3b8549cbf0 to 6b4ac3bfb39d (7 revisions) (flutter/flutter#188007)
2026-06-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[a11y] Map some framework semantics roles to android classes.  (#185217)" (flutter/flutter#188008)
2026-06-15 chris@bracken.jp [ios] Filter UIScene events to those relating to Flutter VC scene (flutter/flutter#187987)
2026-06-15 jhy03261997@gmail.com [a11y] Map some framework semantics roles to android classes.  (flutter/flutter#185217)
2026-06-15 engine-flutter-autoroll@skia.org Roll Skia from f46928e7f50c to 0a3b8549cbf0 (1 revision) (flutter/flutter#188004)
2026-06-14 stuartmorgan@google.com Rework docs for flutter/packages changelogs (flutter/flutter#187666)
2026-06-14 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from nvzMQAmuRSzo7-wAP... to TbB86Po_HDe1dvXvT... (flutter/flutter#187997)
2026-06-14 engine-flutter-autoroll@skia.org Roll Skia from 4e2c9b5e4dad to f46928e7f50c (1 revision) (flutter/flutter#187996)
2026-06-14 engine-flutter-autoroll@skia.org Roll Skia from c52667607242 to 4e2c9b5e4dad (1 revision) (flutter/flutter#187990)
2026-06-14 737941+loic-sharma@users.noreply.github.com Improve RenderTargetCache docs (flutter/flutter#187893)
2026-06-13 brackenavaron@gmail.com [Test cross_imports] Check cross imports in flutter_test/** (flutter/flutter#187587)
2026-06-13 matt.kosarek@canonical.com Fixing corrupted window size OnEmptyFrameGenerated due to transpsed width/height (flutter/flutter#187954)
2026-06-13 engine-flutter-autoroll@skia.org Roll Skia from 42355271a335 to c52667607242 (2 revisions) (flutter/flutter#187979)
2026-06-13 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from A3eaUn9mQ_EkSNxVI... to nvzMQAmuRSzo7-wAP... (flutter/flutter#187975)
2026-06-13 engine-flutter-autoroll@skia.org Roll Skia from 9ef46390c2d1 to 42355271a335 (1 revision) (flutter/flutter#187974)
2026-06-13 bdero@google.com [Flutter GPU] Make ShaderLibrary.fromAsset asynchronous (flutter/flutter#187716)
2026-06-13 engine-flutter-autoroll@skia.org Roll Skia from 8c89bf2b0ee3 to 9ef46390c2d1 (6 revisions) (flutter/flutter#187968)
2026-06-12 bdero@google.com [Flutter GPU] Add surface API for framework presentation (flutter/flutter#187358)
2026-06-12 bkonyi@google.com [gen_l10n] Exclude inherited keys from untranslated-messages-file (flutter/flutter#187950)
2026-06-12 31859944+LongCatIsLooong@users.noreply.github.com Update `MediaQueryData` docs for devicePixelRatio overriding (flutter/flutter#187542)
2026-06-12 bdero@google.com [Impeller] Fix dirty-range race in DeviceBufferGLES uploads (flutter/flutter#187932)
2026-06-12 pascal@phntm.xyz Compare isModifiedAfter against the given time (flutter/flutter#187727)
2026-06-12 planetmarshall@users.noreply.github.com Enable unit tests for compilation of compute shaders on non-metal backends (flutter/flutter#179683)
2026-06-12 matt.boetger@gmail.com [flutter_tools] Add doctor validator warning for multiple adb installations (flutter/flutter#186031)
2026-06-12 matt.boetger@gmail.com Optimize SHA hash calculation of generated APK (flutter/flutter#187184)
2026-06-12 mu7ammadkamel@hotmail.com Scope widget inspector overlay to the selected widget's modal route (flutter/flutter#186784)
2026-06-12 30870216+gaaclarke@users.noreply.github.com Switches Windows to OpenGLESSDF (flutter/flutter#187877)
2026-06-12 matt.boetger@gmail.com [integration_test] Update README to support modern Kotlin-based setups by default (flutter/flutter#186080)
2026-06-12 jason-simmons@users.noreply.github.com Convert the PNG signature constant in APNGImageGenerator to a std::array and use it in the APNG tests (flutter/flutter#187930)
2026-06-12 matt.boetger@gmail.com Correct backoff retry time cap unit and add regression tests (flutter/flutter#187250)
2026-06-12 chingjun@google.com Fix std::vector out-of-bounds access in Flutter Android JNI and Delegate (flutter/flutter#187218)
2026-06-12 matt.boetger@gmail.com [Android] Adding 30-second timeouts to adb stopApp and uninstallApp (flutter/flutter#187876)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622
...
via-guy pushed a commit to via-guy/flutter that referenced this pull request Jun 26, 2026
Fixes flutter#187725, regression was introduced in flutter#187488

`DevFSFileContent.isModifiedAfter` returned `true` whenever the cached
`_fileStat` was `null`, ignoring the time argument entirely. `_fileStat`
is the sync baseline written only by `markClean()` the entries of a
freshly built `AssetBundle` are never marked clean, so `_fileStat` is
always `null` for them and `isModifiedAfter` always reported them as
modified.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

Co-authored-by: Kevin Moore <kevmoo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression: flutter test rebuilds build/unit_test_assets on every run, breaking concurrent invocations

5 participants