test: strengthen generated ID path-safety coverage#39
Closed
danReynolds wants to merge 2 commits into
Closed
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an intermittent test/core flake caused by cross-test leakage of Loon’s pending zero-delay broadcast timer after Loon.clearAll() runs in tearDown, which could cause the next test’s writes to piggyback on the prior test’s scheduled broadcast.
Changes:
- Update the core test suite
tearDownto await an extra event-loop turn afterLoon.clearAll()so the scheduled broadcast is drained before the next test begins. - Add an explanatory comment documenting why the extra await is required for test isolation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
danReynolds
force-pushed
the
claude/fix-broadcast-test-flake
branch
from
626eb9f to
9fc001e
Compare
danReynolds
changed the title
generateId drew from an alphabet that included '_', so a random ID could
contain '__' — the delimiter the store uses to split paths into segments.
Because generated IDs are used as path segments (observer value keys,
auto-generated document IDs), a '__' in an ID split it into extra
segments and misplaced the value deeper in the store than reads and
invalidations expect:
- An observable's cached value was stored below the path the broadcast
manager invalidates (clearAll/writes call delete(path, recursive:
false)), so it was never invalidated and the observer emitted a stale
value. This is the intermittent "broadcast test flake": it fired
whenever a random observer ID happened to contain '__' (~7.5% of core
suite runs at --concurrency=1, more under parallel load). Forcing an
extra segment into the observer ID turned it into 17 deterministic
failures.
- An auto-id document whose ID contained '__' was written below its
collection node and was invisible to the collection's query.
Restrict generated IDs to alphanumeric [0-9A-Za-z] via uniform rejection
sampling, so an ID can never contain the delimiter. Adds a regression
test asserting generateId never produces '__' (100k samples).
Verified: core suite 45/45 green across parallel and --concurrency=1
stress runs that previously flaked ~7.5-20%.
danReynolds
force-pushed
the
claude/fix-broadcast-test-flake
branch
from
9fc001e to
a7c4b95
Compare
…test-flake # Conflicts: # lib/utils/id.dart # test/core/id_test.dart
danReynolds
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR keeps the generated-ID delimiter safety work aligned with current
main.mainalready contains the generator fix:generateSecureIdandgenerateFastIduse a 64-character URL-safe alphabet that excludes_, so generated path segments cannot contain Loon's__path delimiter.This PR adds stronger coverage for both generators and clarifies why
_is intentionally excluded from the alphabet.Notes
I kept the 64-character alphabet instead of the original 62-character alphanumeric rejection-sampling approach because it preserves the fast 6-bit sampling path, avoids rejection overhead, and still prevents generated IDs from introducing store path boundaries.
Verification
flutter analyze lib/utils/id.dart test/core/id_test.dartflutter test test/core/id_test.dart --concurrency=1flutter test test/core --concurrency=1