Skip to content

fix(datastore): close races that could leave saves unsent or stall sync - #4404

Open
thisisabhash wants to merge 8 commits into
mainfrom
test/datastore-integ-flaky-waits
Open

thisisabhash wants to merge 8 commits into
mainfrom
test/datastore-integ-flaky-waits

Conversation

@thisisabhash

@thisisabhash thisisabhash commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

Issue #

None. Found by analyzing DataStore integration runs. Run 40360503947 passed, but retries hid 10 failures in 9 tests. This PR's run 40775861072 still hid failures, and those trace to DataStore itself. Its run 40956868003 hid a sync startup that never finished.

Description

Problem. DataStore integration jobs can pass while hiding failures. -test-iterations 3 -retry-tests-on-failure reruns the whole suite and counts a test as passed if it passed once. In run 40360503947 and the 6 full runs before it, retries hid at least one failure in 26 of 147 jobs. All of them were in the Base, Lazy Load and Transformer V2 suites, and every one was an async wait for a sync or subscription event that timed out. The test changes below fix the causes found in the tests. Run 40775861072, the first with those changes, still hid these:

  • DataStoreConnectionOptionalAssociations.testRemovePostFromCommentAndBlogFromPost (V2, watchOS, 2 of 3 iterations). A sync wait timed out, and then the comment had lost its post.
  • AWSDataStoreLazyLoadBlogPostComment8V2Tests.testDelete (tvOS). The create never synced, and the delete then found no sync metadata.
  • DataStoreConnectionScenario3Tests and DataStoreConnectionScenario2Tests (Base, watchOS). A sync wait timed out.
  • A Base watchOS crash, disk I/O error (code: 10) at SQLite.swift's Statement.swift:218.

Library changes 1–4 fix the races these point to. Run 40956868003, the first with them, still hid one more: AWSDataStoreLazyLoadPostComment4V2Tests.testMultiSaveWithInterruptions (Lazy Load, iOS). That test calls stop() and start() 15 times in about 150 ms while saving. The outbox never reported empty within 120 s, and 14 of its 15 post/comment pairs never reached AppSync. A unit-test version with a mock API never stalled, on the new outbox code or the old (0 of 88 and 0 of 70 runs), and on main the same test once waited 40 minutes before its saves even began. The last sync engine never finished starting up, which library changes 5 and 6 fix.

Library changes.

  1. Saves stranded, then sent out of order (AWSMutationDatabaseAdapter). getNextMutationEvent queried the MutationEvent table and, if it was empty, parked its callback. submit checked for a parked callback, then inserted the new row. Neither pair was atomic. When the outbox's query ran before the insert committed, and submit checked before the outbox parked, the row sat unsent until another save or a restart. The next save then found the parked callback and delivered its own row ahead of the stranded one. That fits each timeout above:

    • In testRemovePostFromCommentAndBlogFromPost, a comment reached the service before its post. The response had post: null, and reconciling it cleared the comment's local post ID.
    • In testDelete, the stranded create was folded into the delete, so nothing was sent.

    Now each ingest and each dequeue runs as one block on a TaskQueue:

    • A dequeue that finds the table empty parks the request, and every later ingest serves it after writing. That includes an ingest whose save reports a failure, since its row may still have been written.
    • A parked request is always served with the oldest row, so delivery stays FIFO.
    • Marking the head in process can no longer overwrite a merge into that row, or re-insert a create that a delete just folded away.
    • submit calls its completion off the queue, so callers can't hold up the outbox.
  2. Results for a replaced subscriber (AWSMutationEventPublisher). A request parked before a restart could be served after it. That handed the new subscriber an extra event, which was then sent twice. The publisher now drops results requested by a subscriber it has since replaced.

  3. Late results after a stop (OutgoingMutationQueue).

    • Processed events were deleted with Amplify.DataStore.delete, which goes through initStorageEngineAndTryStartSync. A result that arrived after stop() or clear() could start sync again or recreate the database file. The queue now deletes through its own storage adapter, as the outbox ingester already does.
    • Work begun before a stop could act on the next session. A late result could notify .processedEvent into the restarted queue and make it request a second event while one was in flight. An event delivered across a stop could be added to the suspended operation queue and sent again after the restart. A session generation, bumped on every stop, now drops that work. It replaces the .stopped state check, which missed .stopping and anything after a restart.
  4. Crash on a failed SQLite step (Statement+AnyModel, ModelSyncMetadataMigration). These looped over rows with for … in, and SQLite.swift's next() calls failableNext() with try!. The crash above most likely came from reconcile work still running when clear() deleted the database file. Both now call failableNext() themselves, as Statement+Model already does. clear() also called its completion twice when it couldn't delete the file. It now returns after the failure.

  5. Subscriptions that neither connect nor fail (AppSyncRealTimeClient, API plugin). connect() set the state to .connecting before its first try, and nothing reset it when the attempt ended without a connection. A subscribe task cancelled by an idle disconnect still ran connect(). RetryWithJitter's first Task.sleep then threw CancellationError, which isn't retried, and the client stayed .connecting with nothing running. Every later connect() returned early, so new subscriptions on that client neither connected nor failed. Rapid stop and start repeat exactly that sequence: subscribe, unsubscribe, idle disconnect. In a unit test it left the client stuck in 59 of 200 trials. Now:

    • A cancelled task doesn't claim .connecting. An attempt that ends without connecting resets the state, unless a newer attempt has taken over.
    • When retries run out, the client closes the socket, so a late handshake can't resume subscriptions. It then sends every subscription WebSocketClient.Error.connectionLost, which subscribers get as a retryable network error.
    • A start that isn't acknowledged within 15 s on a live connection fails that subscription the same way. Before, it stayed silent. The start timeout was 5 s. 15 s is longer than the 10 s AppSync gives a Lambda authorizer, so a slow start still succeeds.
    • sendRequest no longer reports a connection error as a timeout. It also fails at once on an error frame it can't parse, instead of timing out. Subscribers already receive both errors, so neither is reported twice.
  6. Sync startup that never finishes (RemoteSyncEngine). The engine acted on its subscriptions failing or finishing only in .initializingSubscriptions and .syncEngineActive. Anywhere else in startup it ignored them, and stalled in .activatingCloudSubscriptions. Subscriptions that never connected held it in .initializingSubscriptions with its outbox stopped. StorageEngine doesn't replace an engine that is still starting, so later start() and save() calls didn't help. Now:

    • Failures and completions are handled in every state from .initializingSubscriptions through .syncEngineActive. A failure goes to the retry policy and a completion ends sync, as both already did in the two states that handled them.
    • If subscriptions haven't connected 60 s after setup starts, startup fails with a retryable timeout, so the engine cleans up and restarts.
    • Cleanup cancels an initial sync still in progress. Left running, it dropped its remaining pages but still saved lastSync, so the next sync skipped those records. stop() during initial sync already had this hole; restarting on a subscription failure would have made it common. InitialSyncOperation.cancel() now sets isCancelled, which the operation already checks before reconciling a page and before saving lastSync. A query cut short by cancel() no longer reaches the errorHandler.
    • An initial sync result from an earlier startup is dropped.
    • StateMachine.notify(action:if:) applies an action only while its condition holds for the current state, checked under the state machine's lock. Subscription completions, the timeout and initial sync results use it. An action a state doesn't handle used to republish that state and run its work again.
    • A state handler that an error or stop has already replaced is skipped, and a superseded restart notifier is cancelled, so cleanup and restart run once. The handler reads the state through the new StateMachine.currentState, under the state machine's lock, because @Published delivers a new state before it stores it.

None of these change public API.

Test changes.

  1. Missed local event (DataStoreObserveQueryTests, DataStoreEndToEndTests.testCreate). The save and delete helpers wait for two observe events: the local one and the remote one. They started the observer in an unawaited Task, and observe() attaches its sink in yet another Task. When the local event came first, the count stopped at 1 and the 100 s wait timed out. The helpers and testCreate now subscribe to AWSDataStorePlugin.publisher(for:), the publisher that observe() wraps, before mutating. They cancel the subscription after the wait. This race caused the testInitialSyncWithPredicate failures (15 at the helper's wait in the sampled runs) and the testSortWithCreateUpdateDelete ones.

  2. Outbox folding (DataStoreConsecutiveUpdatesTests). Four tests mutate twice in a row, then wait for a version that exists only if the first mutation was already in flight. The outbox folds a new mutation into a pending one that isn't in process yet:

    • create + update sends one create;
    • create + delete sends nothing;
    • update + delete sends one delete;
    • and the ten updates in testSaveThenMultipleUpdate can fold into one.

    The wait for the expected version then can't succeed. A new helper, saveAndWaitUntilInFlight(_:), waits for outboxMutationEnqueued before the next mutation. That event is sent once the outbox has marked the mutation in process. testSaveThenMultipleUpdate had been skipped in the schemes since chore(datstore-v2): fix integ test Scenario 4 and 6 #2341, with no reason given. It's no longer skipped.

  3. Shared listener token (V2 DataStoreConnectionOptionalAssociations). The save helpers kept their Hub listener in a shared token property. The listener called fulfill() and only then removed self.token. But fulfill() lets the test start the next helper, which reassigns token. So the closure could remove the next helper's listener, and that helper then waited out its full 60 s. Each call now keeps its own token and removes it after the wait.

  4. Failures ended the test process. This covers every DataStore integration test base: SyncEngineIntegrationTestBase, HubEventsIntegrationTestBase, AWSDataStoreLazyLoadBaseTest, the CPK, AuthCognito, AuthIAM and MultiAuth bases, and the Stress base. They set continueAfterFailure = false. When an async test fails under that setting, XCTest runs tearDown alongside the abandoned test body, then exits the process with code 75, and xcodebuild relaunches it. All 59 failures in these suites in the sampled runs ended this way. None of V2's 10 did, because its base sets true. They all set true now. To make that safe:

    • Every base now tears down the way SyncEngineIntegrationTestBase already did (chore: stabilize flaky unit and integration tests #4340): stop, clear, wait 1 s, then Amplify.reset(). It also resets when stop or clear throws, so the next test can still configure Amplify. Before, the Hub-events and Lazy Load bases reset before the wait. All 7 Fatal error: Schema not found for … crashes (Model.swift:48) in the sampled runs were in suites on those two bases. Suites on SyncEngineIntegrationTestBase had none.
    • AWSDataStoreLazyLoadCompositePKSnapshotTests overrode setUp for the whole class to register models only. As a result, the 19 tests in the class that start DataStore skipped stop and clear. Now only the 5 snapshot tests do that, and setup(withModels:) also clears the flag as a safety net.
    • 14 Lazy Load observe tests ran a createAndWaitForSync of up to 100 s inside a Task that the test waited for only 60 s. With true, a timeout would leave that Task running into the next test. Now the Task only returns the observed model, and the test does the follow-up save and lazy-load checks after its wait. The 60 s wait also no longer covers the child's whole sync round trip. That round trip likely caused the testObserveQueryPost timeout on tvOS in run 40360503947.
    • Other tasks and loops no longer outlive their test. The Lazy Load stress test's wait for an empty outbox gives up after 120 s. testStop_whenSavingInProgress and testParallelMutations_whenWaitingForEventToProcess_noMutationLoss cancel their tasks and wait up to 30 s for them to finish.
    • In DataStoreObserveQueryTests, indexing after a count check is now guarded, and force unwraps after waits use XCTUnwrap. A failed check now fails the test instead of crashing the runner.
  5. Waiting for ready. syncStarted fires while the outbox is still making its first check for events, so a save made right after it could hit the race that library change 1 fixes. In the sampled runs, Scenario 2, which waited for syncStarted, failed its first wait 5 times out of 48. Scenario 1, which chore: stabilize flaky unit and integration tests #4340 moved to ready, failed it 0 times out of 48. These tests now wait for ready before their first save, so they don't depend on startup timing either:

    • DataStoreConsecutiveUpdatesTests;
    • the DataStoreConnectionScenario2Tests tests that wait for a sync;
    • SubscriptionEndToEndTests;
    • V2 DataStoreConnectionOptionalAssociations.

    The Lazy Load setup(withModels:) now waits for ready too, and startAndWaitForReady() returns at once after that. The stress test opts out, because it listens for the outbox status DataStore reports while starting.

  6. Expectations fulfilled twice. A second fulfill (API violation - multiple calls made to -[XCTestExpectation fulfill]) aborts the runner. These expectations can legitimately be fulfilled again, either after a sync-engine restart or by a late copy of an event, so they now set assertForOverFulfill = false:

    • the ready, syncStarted and subscriptionsEstablished waits in the bases and in DataStoreHubEventsTests;
    • the observeQuery "first isSynced snapshot" waits;
    • the CPK delete sink, which matches both the parent's delete and the cascaded child's.

    Two tests change instead:

    • testObserveToQueryPostFromDeletedComment now waits only for the delete event. Before, a late copy of the comment's create could satisfy it.
    • testParallelMutations_whenWaitingForEventToProcess_noMutationLoss fulfills once, when the last new title arrives.
  7. Smaller fixes.

    • DataStoreConnectionScenario8V2Tests now registers its listeners before saving.
    • The Lazy Load delete wait now allows 100 s, the same as create and update. It was 10 s, and testDeleteImplicitChild timed out there.
    • testStop_whenSavingInProgress now requires only the posts whose save succeeded to be in the local store. A save that stop() interrupts must fail with internalOperation and leave no post behind. Before, the test caught that failure but still expected the post, and it failed twice on watchOS in run 41114031750. A mock-API version of the test rolls back about 15 of 1,000 saves, with or without this PR's library changes.

Trade-offs.

  • With continueAfterFailure = true, a failing test runs to the end. A backend failure that persists now costs each test its whole chain of waits, not just the first. A per-test time limit in the DataStore workflows would bound that (-test-timeouts-enabled YES -default-test-execution-time-allowance, as integ_test_auth_client.yml does). That's a separate ci change.
  • A mutation whose result is dropped after a stop stays in the outbox, and the next session sends it again. That already happened when the result arrived after .stopped. A re-sent create fails its condition check, which calls the errorHandler and dispatches conditionalSaveFailed. A re-sent update or delete goes through conflict handling.
  • Observers of the internal MutationEvent model no longer get a delete event for each processed mutation. Nothing in the repo observes it.
  • Apps that call Amplify.API.subscribe now get an error in two cases where the subscription used to go silent: when the client runs out of connect retries (about 40 s with no handshake), and when a start isn't acknowledged within 15 s on a live connection. Before, a later reconnect could still have resumed such a subscription.
  • A subscription whose start is acknowledged after 5–15 s now subscribes. Before, its start timed out at 5 s, so it never reported .connected, though its data still arrived.
  • A startup whose subscriptions take longer than 60 s to connect is restarted instead of waited on. On a very slow network that can happen more than once, with the retry policy's backoff between attempts.

Not in this PR.

  • The CI failures are matched to these races by timing and side effects, not by a trace. CI runs at log level .error and keeps no xcresult or simulator logs. The Scenario 2 and 3 timeouts in particular could also come from a server error or a missed subscription event.
  • MutationEvent.reconcilePendingMutationEventsVersion and MutationEventClearState still write MutationEvent rows outside the outbox queue, so they can still race a merge into the same pending row. This predates this PR and doesn't strand saves.
  • Mutation selection sets for non-lazy models ask for the nested parent but not its foreign-key field. A response with a null parent still clears the local foreign key.
  • stop() doesn't wait for in-flight reconcile work, so clear() still deletes the file under live statements. Library change 4 turns the crash into logged errors.
  • RemoteSyncEngine.stop keeps only the first caller's completion, so an overlapping second stop() or clear() can wait forever.
  • StorageEngine.save rolls back a save, with "No SyncEngine available", when stop() completes after the save checked for a sync engine. A save that races stop() can therefore fail, though it ran before or during the stop.
  • InternalAWSPinpoint's AnalyticsEventSQLStorage has the same for … in loop over SQLite rows.
  • A subscription cancelled before AWSGraphQLSubscriptionTaskRunner.run() stores its client stays registered on that client. The client then never sees itself as idle, so it never disconnects.
  • In AppSyncRealTimeClient.onWebSocketEvent, state.value != .disconnecting || state.value != .disconnected is always true.
  • RequestRetryablePolicy doesn't cap the backoff for URL errors, so repeated startup failures wait longer and longer between restarts.
  • MutationEventClearState doesn't call its completion when a save fails, which would stall startup in .clearingStateOutgoingMutations.
  • Subscriptions that finish during startup still end sync rather than restarting it.
  • InitialSyncOperation saves lastSync once its last page is offered, not once that page's records are saved. A cleanup that cancels queued saves can still lose them. stop() and a failed initial sync for another model could already cause this.
  • A WebSocket .disconnected event cancels the start tasks of registered subscriptions. When WebSocketClient won't reconnect by itself, nothing does, and those subscriptions never connect or fail. This predates this PR. DataStore now recovers through the 60 s timeout.
  • A subscription that fails or times out stays registered on the client, so a later connection resumes it on the server for a subscriber that has stopped listening.
  • A start still pending when its socket drops keeps waiting out its 15 s timeout after a reconnect, then fails as connectionLost. The reconnect doesn't re-send it while it's still .subscribing. The 5 s timeout made that window narrower.
  • WebSocketClient.disconnect() returns early when no socket is running, which leaves auto-reconnect on. So after retries run out on handshakes that failed at once, a later network change can still reconnect and resume subscriptions that were told the connection was lost.
  • WebSocketClient's didCompleteWithError doesn't check that the task is still the current connection, as didCloseWith does.
  • The Lazy Load observe tests that create through Amplify.API.mutate depend only on subscription delivery. ready already guarantees the subscriptions are established, so there's no test-side fix for their timeouts.

Testing

  • Unit tests on macOS:

    • DataStore (xcodebuild test -scheme AWSDataStorePlugin): 616 tests, 0 failures, 21 skipped. Before the library changes: 592 tests, 0 failures.
    • API (-scheme AWSAPIPlugin): 238 tests, 0 failures.
  • Library changes 1–4 come with 12 new unit tests. Against the code on main, 10 of them fail. testSubmit_whileTheOutboxWaits_deliversOnceAndQueuesTheNextSave passes there too, because it guards behaviour that must not change. testConvertToUntypedModel_whenStepFails_throws crashes the runner at Statement.swift:218.

  • Library changes 5 and 6 come with 19 new unit tests:

    • AppSyncRealTimeClientFailureTests (7)
    • RemoteSyncEngineStartupTests (7)
    • InitialSyncCancelTests (3)
    • 2 more in StateMachineTests

    I removed each of the 17 safeguards in these changes one at a time, and each removal fails at least one of these tests. testSubscriptionsThatConnectInTime_doNotRestartSync guards against the timeout firing early, so it passes either way.

  • A unit-test version of testMultiSaveWithInterruptions, with a mock API, never stalled in 88 runs on the new outbox code or in 70 runs on the old. It isn't part of this PR.

  • xcodebuild build-for-testing (Xcode 27.1) succeeds for these 15 schemes:

    • iOS Simulator: AWSDataStorePluginIntegrationTests, AWSDataStorePluginV2Tests, AWSDataStorePluginLazyLoadTests, AWSDataStorePluginCPKTests, AWSDataStorePluginAuthCognitoTests, AWSDataStorePluginAuthIAMTests, AWSDataStorePluginMultiAuthTests, DatastoreStressTests.
    • watchOS Simulator: the …Watch variant of each of the first seven.

    After the library changes I rebuilt AWSDataStorePluginIntegrationTests for iOS Simulator and AWSDataStorePluginIntegrationTestsWatch, and after changes 1–4 also for tvOS Simulator. The other schemes compile the same plugin sources.

  • SwiftFormat 0.60.1 --swiftversion 6.0 --lint reports 0 files needing formatting across every Swift file this PR touches. SwiftLint 0.54.0 reports no errors in the changed sources; CI's config excludes AmplifyPlugins/*/Tests. Both edited schemes pass xmllint.

  • The integration tests run in this PR's DataStore workflow. I didn't run them locally because they share CI's backend. The Stress target runs only on pushes to main (stress_test.yml), so its change will first run after merge.

General Checklist

  • Added new tests to cover change, if needed
  • Build succeeds with all target using Swift Package Manager
  • All unit tests pass
  • All integration tests pass
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Documentation update for the change if required
  • PR title conforms to conventional commit style
  • New or updated tests include Given When Then inline code documentation and are named accordingly testThing_condition_expectation()
  • If breaking change, documentation/changelog update with migration instructions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@thisisabhash
thisisabhash marked this pull request as ready for review October 28, 2026 20:36
@thisisabhash
thisisabhash requested a review from a team as a code owner October 28, 2026 20:36
@thisisabhash
thisisabhash force-pushed the test/datastore-integ-flaky-waits branch from 0b5fb4c to fe4cec3 Compare October 28, 2026 21:55
@thisisabhash
thisisabhash deployed to IntegrationTest October 28, 2026 23:26 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 28, 2026 23:26 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 28, 2026 23:26 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 28, 2026 23:26 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 28, 2026 23:26 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 28, 2026 23:26 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 28, 2026 23:26 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 01:34 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 01:36 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 01:36 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 01:39 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 01:40 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 01:42 — with GitHub Actions Active
@codecov

codecov Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.33129% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.91%. Comparing base (5a9f3f0) to head (d8b2b74).

Files with missing lines Patch % Lines
.../OutgoingMutationQueue/OutgoingMutationQueue.swift 84.61% 10 Missing ⚠️
...SMutationDatabaseAdapter+MutationEventSource.swift 84.61% 8 Missing ⚠️
...utationDatabaseAdapter+MutationEventIngester.swift 93.10% 4 Missing ⚠️
...Sync/MutationEvent/AWSMutationEventPublisher.swift 88.88% 2 Missing ⚠️
...ces/AWSDataStorePlugin/Sync/RemoteSyncEngine.swift 97.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4404      +/-   ##
==========================================
+ Coverage   76.04%   78.91%   +2.86%     
==========================================
  Files        1362      233    -1129     
  Lines       57864    13277   -44587     
==========================================
- Hits        44003    10477   -33526     
+ Misses      13861     2800   -11061     
Flag Coverage Δ
API_plugin_unit_test 71.37% <100.00%> (+0.73%) ⬆️
AWSPluginsCore 77.28% <ø> (ø)
Amplify ?
Amplify_Foundation_Bridge_unit_test ?
Amplify_Foundation_unit_test ?
Amplify_Keychain_unit_test ?
Analytics_plugin_unit_test ?
Auth_plugin_unit_test ?
CloudWatchClient_unit_test ?
CognitoClient_unit_test ?
Connect_unit_test ?
DataStore_plugin_unit_test 83.04% <91.43%> (+1.12%) ⬆️
EventEnrichment_unit_test ?
Firehose_plugin_unit_test ?
Geo_plugin_unit_test ?
Kinesis_plugin_unit_test ?
Logging_plugin_unit_test ?
Predictions_plugin_unit_test ?
PushNotifications_plugin_unit_test ?
RecordCache_unit_test ?
Storage_plugin_unit_test ?
unit_tests 78.91% <92.33%> (+2.86%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 01:57 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 01:58 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 02:00 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 02:00 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 02:02 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 02:03 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 02:07 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 02:37 — with GitHub Actions Active
@thisisabhash thisisabhash changed the title test(datastore): fix races behind flaky integration tests fix(datastore): close outbox races that could leave saves unsent Oct 29, 2026
@thisisabhash
thisisabhash force-pushed the test/datastore-integ-flaky-waits branch from e991f8d to 45994db Compare October 29, 2026 21:19
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 22:53 — with GitHub Actions Active
@thisisabhash
thisisabhash deployed to IntegrationTest October 29, 2026 23:24 — with GitHub Actions Active
@thisisabhash
thisisabhash marked this pull request as draft October 30, 2026 17:01
@thisisabhash thisisabhash changed the title fix(datastore): close outbox races that could leave saves unsent fix(datastore): close races that could leave saves unsent or stall sync Oct 30, 2026
@thisisabhash
thisisabhash marked this pull request as ready for review October 30, 2026 19:05
DataStore integration jobs passed while retries hid failures: 26 of 147 jobs in
run 40360503947 and the 6 full runs before it, all of them async waits for a
sync or subscription event that timed out. Test-only changes:

- The ObserveQuery save/delete helpers and EndToEnd testCreate now subscribe
  to AWSDataStorePlugin.publisher(for:) before mutating. observe() attaches its
  sink in an unawaited Task, so the local event could be missed.
- The ConsecutiveUpdates tests wait for outboxMutationEnqueued before the second
  mutation, so the outbox can't fold that mutation into one that isn't in flight.
- The V2 OptionalAssociations helpers each keep their own listener token. With
  the shared token, a previous helper's closure could remove the next helper's
  listener.
- The Base, Hub-events and Lazy Load bases set continueAfterFailure = true.
  With false, a failed async test ran tearDown alongside the abandoned body
  and then exited the process. To make true safe:
  - The Hub-events and Lazy Load bases now stop, clear and settle before
    Amplify.reset().
  - CompositePK tests no longer skip that teardown.
  - Lazy Load observe tests run their follow-up sync waits outside the
    observing Task.
  - The stress test's outbox wait is bounded.
…bases

These follow-ups are test-only.

- The CPK, AuthCognito, AuthIAM, MultiAuth and Stress bases also set
  continueAfterFailure = true.
- Every DataStore integration base now tears down in the same order: stop,
  clear, settle 1 s, then Amplify.reset(). Reset still runs when stop or clear
  throws, so the next test can configure Amplify.
- A save made right after syncStarted can miss the outbox's first check and
  never sync. Tests that save and then wait for the sync right after starting
  now wait for ready: ConsecutiveUpdates, Scenario2, SubscriptionEndToEnd, V2
  OptionalAssociations, and every Lazy Load test that goes through
  setup(withModels:). The stress test opts out because it needs the outbox
  status reported during startup.
- A sync-engine restart or a late duplicate event can fulfill some expectations
  a second time, which aborts the runner. Those expectations now set
  assertForOverFulfill = false. testObserveToQueryPostFromDeletedComment now
  waits only for the delete event, and testParallelMutations fulfills once.
- Smaller fixes:
  - testSaveThenMultipleUpdate waits for its first update to be in flight,
    and the schemes no longer skip it.
  - The Scenario8V2 tests register their listeners before saving.
  - The EndToEnd tests cancel their tasks and wait up to 30 s for them to
    finish.
  - ObserveQuery guards indexing after a failed count check.
  - The Lazy Load delete wait now allows 100 s, the same as create and
    update.
  - The CompositePK snapshot tests register models-only themselves instead of
    for the whole class.
…anded

The outbox could leave a saved mutation unsent until the next save or a
restart. getNextMutationEvent queried the MutationEvent table and, when it
was empty, parked its callback. submit checked for a parked callback, then
inserted the new row. Neither pair was atomic. When the outbox's query ran
before the insert committed, and submit checked before the outbox parked,
the row was never delivered. The next save then found the parked callback
and delivered its own row ahead of the stranded one, so a child could reach
the service before its parent.

AWSMutationDatabaseAdapter now runs each ingest and each dequeue as one
block on a TaskQueue:
- A dequeue that finds the table empty parks the request, and every later
  ingest serves it after writing. That includes an ingest whose save reports
  a failure, since its row may still have been written.
- A parked request is always served with the oldest row, so delivery stays
  FIFO.
- Marking the head in process can no longer overwrite a merge into that row
  or re-insert a create that a delete just folded away.
- submit calls its completion off the queue, so callers can't hold up the
  outbox.

AWSMutationEventPublisher now drops results requested by a subscriber it
has since replaced. Before, a request parked before a restart could hand the
new subscriber an extra event, which was then sent twice.
…sults

OutgoingMutationQueue deleted each processed MutationEvent with
Amplify.DataStore.delete, which goes through initStorageEngineAndTryStartSync.
A result that arrived after stop() or clear() could start sync again or
recreate the database file. The queue now deletes through its own storage
adapter, as the outbox ingester already does.

Work begun before a stop could also act on the next session. A late result
could notify .processedEvent into the restarted queue and make it request a
second event while one was in flight. An event delivered across a stop could
be added to the suspended operation queue and sent again after the restart.
A session generation, bumped on every stop, now drops that work. It replaces
the check for the .stopped state, which missed .stopping and anything after
a restart.

A mutation whose result is dropped stays in the outbox, and the next session
sends it again, as it already did when the result arrived after .stopped. A
re-sent create fails its condition check, which calls the errorHandler and
dispatches conditionalSaveFailed. A re-sent update or delete goes through
conflict handling. Observers of the internal MutationEvent model no longer
get a delete event for each processed mutation.
Statement+AnyModel and ModelSyncMetadataMigration looped over rows with
for-in, and SQLite.swift's next() calls failableNext() with try!. A failed
step crashed the app instead of throwing. CI crashed there with "disk I/O
error (code: 10)" at Statement.swift:218, most likely from reconcile work
still running when clear() deleted the database file. Both now call
failableNext() themselves, as Statement+Model already does, so the error
reaches the existing error handling.

clear() also called its completion twice when it couldn't delete the
database file. It now returns after the failure.
AppSyncRealTimeClient.connect() set the state to .connecting before its
first try, and nothing reset it when the attempt ended without a
connection. A subscribe task cancelled by an idle disconnect still ran
connect(). RetryWithJitter's first Task.sleep then threw
CancellationError, which isn't retried, and the client stayed
.connecting with no attempt running. Every later connect() returned
early, so new subscriptions on that client neither connected nor failed.
Running out of retries left the client the same way.

- A cancelled task no longer claims .connecting.
- An attempt that ends without connecting resets the state, unless a
  newer attempt has taken over.
- When retries run out, the client closes the socket, so a late
  handshake can't resume subscriptions, and sends every subscription
  WebSocketClient.Error.connectionLost, which subscribers get as a
  retryable network error.
- A start that isn't acknowledged within 15 s on a live connection now
  fails that subscription the same way instead of leaving it silent.
  The start timeout was 5 s. 15 s is longer than the 10 s AppSync gives
  a Lambda authorizer, so a slow start still succeeds.
- sendRequest no longer reports a connection error as a timeout, and it
  fails at once on an error frame it can't parse instead of timing out.
  Subscribers already receive both errors, so neither is reported twice.
…startup

RemoteSyncEngine acted on its subscriptions failing or finishing only in
.initializingSubscriptions and .syncEngineActive. When that happened
during initial sync or a later startup step, it was ignored, and the
engine stalled in .activatingCloudSubscriptions. Subscriptions that never
connected or failed held the engine in .initializingSubscriptions with
its outbox stopped. StorageEngine doesn't replace an engine that is still
starting, so sync stayed down until the app stopped DataStore.

- Subscription failures and completions are handled in every state from
  .initializingSubscriptions through .syncEngineActive. A failure goes to
  the retry policy and a completion ends sync, as both already did in the
  two states that handled them.
- If subscriptions haven't connected 60 s after setup starts, startup
  fails with a retryable timeout, so the engine cleans up and restarts.
- Cleanup cancels an initial sync still in progress. Left running, it
  dropped its remaining pages but still saved lastSync, so the next sync
  skipped those records. InitialSyncOperation.cancel() now sets
  isCancelled, which the operation already checks before reconciling a
  page and before saving lastSync. A query that cancel() cuts short no
  longer reaches the errorHandler.
- An initial sync result from an earlier startup is dropped.
- StateMachine.notify(action:if:) applies an action only while the
  condition holds for the current state, checked under the state
  machine's lock. Subscription completions, the timeout and initial sync
  results use it. An action that a state doesn't handle used to
  republish that state and run its work again.
- A state handler that an error or stop has already replaced is
  skipped, and a superseded restart notifier is cancelled, so cleanup
  and restart run once. The handler reads the state through the new
  StateMachine.currentState, under the state machine's lock, because
  @published delivers a new state before it stores it.
…SavingInProgress

DataStore rolls back a save that finds no sync engine because stop()
completed after the save checked for one ("No SyncEngine available ...
rollback save"). The test caught that failure but still expected every
post in the local store, so it failed whenever a save hit that window.
It did, twice on watchOS in this PR's run 41114031750. A mock-API version
of the test rolls back about 15 of 1,000 saves, with or without this
PR's library changes.

The test now requires every post whose save succeeded to be queryable.
Each interrupted save must fail with internalOperation and leave no post
behind.

This branch was successfully deployed

2 active deployments
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.

1 participant