From fe54f7083f3c508d39d686c4bc05d4f5512b67c6 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 1 Jul 2026 16:19:58 +0200 Subject: [PATCH 1/5] fix: emit backup and sync trace only when mutations occur --- packages/account-tree-controller/CHANGELOG.md | 1 + .../src/AccountTreeController.ts | 2 + .../src/backup-and-sync/service/index.test.ts | 194 ++++++++---------- .../src/backup-and-sync/service/index.ts | 27 ++- .../src/backup-and-sync/syncing/group.test.ts | 11 + .../src/backup-and-sync/syncing/group.ts | 1 + .../backup-and-sync/syncing/legacy.test.ts | 11 + .../src/backup-and-sync/syncing/legacy.ts | 1 + .../backup-and-sync/syncing/metadata.test.ts | 12 ++ .../src/backup-and-sync/syncing/metadata.ts | 1 + .../src/backup-and-sync/types.ts | 20 ++ .../user-storage/network-operations.test.ts | 11 + .../user-storage/network-operations.ts | 12 +- .../src/backup-and-sync/utils/index.ts | 1 + .../backup-and-sync/utils/mutation-tracker.ts | 21 ++ 15 files changed, 211 insertions(+), 115 deletions(-) create mode 100644 packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts diff --git a/packages/account-tree-controller/CHANGELOG.md b/packages/account-tree-controller/CHANGELOG.md index f85271af32..2cb7291981 100644 --- a/packages/account-tree-controller/CHANGELOG.md +++ b/packages/account-tree-controller/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- The `Multichain Account Syncing - Full` trace is now only emitted when a backup-and-sync run actually mutates local or remote state, and the span is backdated to preserve the real sync duration ([#0000](https://github.com/MetaMask/core/pull/0000)) - Bump `@metamask/keyring-api` from `^23.1.0` to `^23.3.0` ([#9249](https://github.com/MetaMask/core/pull/9249)) - Bump `@metamask/multichain-account-service` from `^11.0.0` to `^11.1.0` ([#9264](https://github.com/MetaMask/core/pull/9264)) diff --git a/packages/account-tree-controller/src/AccountTreeController.ts b/packages/account-tree-controller/src/AccountTreeController.ts index 5640662e9f..e37b2dc152 100644 --- a/packages/account-tree-controller/src/AccountTreeController.ts +++ b/packages/account-tree-controller/src/AccountTreeController.ts @@ -22,6 +22,7 @@ import { } from './backup-and-sync/analytics'; import { BackupAndSyncService } from './backup-and-sync/service'; import type { BackupAndSyncContext } from './backup-and-sync/types'; +import { createSyncMutationTracker } from './backup-and-sync/utils'; import type { AccountGroupObject, AccountTypeOrderKey } from './group'; import { ACCOUNT_TYPE_TO_SORT_ORDER, @@ -1803,6 +1804,7 @@ export class AccountTreeController extends BaseController< controllerStateUpdateFn: this.update.bind(this), traceFn: this.#trace.bind(this), groupIdToWalletId: this.#groupIdToWalletId, + mutationTracker: createSyncMutationTracker(), }; } } diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts index ebbda948e3..338776a78c 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts @@ -3,7 +3,9 @@ import { AccountWalletType } from '@metamask/account-api'; import { BackupAndSyncService } from '.'; import type { AccountGroupObject } from '../../group'; import type { AccountWalletEntropyObject } from '../../wallet'; +import { TraceName } from '../analytics'; import { getProfileId } from '../authentication'; +import { syncWalletMetadata } from '../syncing'; import type { BackupAndSyncContext } from '../types'; // We only need to import the functions we actually spy on import { getLocalEntropyWallets } from '../utils'; @@ -20,6 +22,23 @@ const mockGetProfileId = getProfileId as jest.MockedFunction< >; const mockGetLocalEntropyWallets = getLocalEntropyWallets as jest.MockedFunction; +const mockSyncWalletMetadata = syncWalletMetadata as jest.MockedFunction< + typeof syncWalletMetadata +>; + +// Local tracker (the real factory lives in the auto-mocked `../utils`). +const createTestMutationTracker = () => { + let occurred = false; + return { + markOccurred: () => { + occurred = true; + }, + hasOccurred: () => occurred, + reset: () => { + occurred = false; + }, + }; +}; describe('BackupAndSync - Service - BackupAndSyncService', () => { let mockContext: BackupAndSyncContext; @@ -57,6 +76,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }, traceFn: jest.fn().mockImplementation((_config, fn) => fn()), groupIdToWalletId: new Map(), + mutationTracker: createTestMutationTracker(), } as unknown as BackupAndSyncContext; // Default setup - backup and sync enabled @@ -115,7 +135,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }); it('returns early when a full sync has not completed at least once', () => { - mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = false; + mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = + false; backupAndSyncService.enqueueSingleWalletSync('entropy:wallet-1'); // Should not have called any messenger functions beyond the state check expect(mockContext.messenger.call).toHaveBeenCalledTimes(1); @@ -128,7 +149,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }); it('enqueues single wallet sync when enabled and synced at least once', async () => { - mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = true; + mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = + true; // Add a mock wallet to the context so the sync can find it mockContext.controller.state.accountTree.wallets = { @@ -212,7 +234,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }); it('returns early when a full sync has not completed at least once', () => { - mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = false; + mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = + false; backupAndSyncService.enqueueSingleGroupSync('entropy:wallet-1/1'); @@ -227,7 +250,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }); it('enqueues group sync when enabled and synced at least once', async () => { - mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = true; + mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = + true; // Set up the group mapping and wallet context mockContext.groupIdToWalletId.set( @@ -325,15 +349,11 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { // Should have updated controller state to mark sync in progress and then completed expect(mockContext.controllerStateUpdateFn).toHaveBeenCalled(); - // Should have called traceFn to wrap the sync operation - expect(mockContext.traceFn).toHaveBeenCalled(); - // The key difference: full sync should call getLocalEntropyWallets expect(mockGetLocalEntropyWallets).toHaveBeenCalled(); }); - it('awaits the ongoing promise if a second call is made during sync', async () => { - // Mock some local wallets for the full sync to process + it('emits a backdated AccountSyncFull span when the sync mutates state', async () => { mockGetLocalEntropyWallets.mockReturnValue([ { id: 'entropy:wallet-1', @@ -341,17 +361,59 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Make traceFn actually async to simulate real sync work - let resolveTrace: (() => void) | undefined; - const tracePromise = new Promise((resolve) => { - resolveTrace = resolve; + // Simulate a real write happening during the sync by having a mocked + // helper report a mutation through the context. + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.markOccurred(); }); - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - fn(); - return tracePromise; + + await backupAndSyncService.performFullSync(); + + expect(mockContext.traceFn).toHaveBeenCalledTimes(1); + expect(mockContext.traceFn).toHaveBeenCalledWith( + { + name: TraceName.AccountSyncFull, + startTime: expect.any(Number), }, + expect.any(Function), ); + // The traced callback is empty (the span is backdated, work already ran). + const [, tracedCallback] = (mockContext.traceFn as jest.Mock).mock + .calls[0]; + expect(tracedCallback()).toBeUndefined(); + }); + + it('does not emit an AccountSyncFull span when the sync is a no-op', async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + + // No mocked helper reports a mutation, so the sync is a no-op. + await backupAndSyncService.performFullSync(); + + expect(mockContext.traceFn).not.toHaveBeenCalled(); + }); + + it('awaits the ongoing promise if a second call is made during sync', async () => { + // Mock some local wallets for the full sync to process + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + + // Make the sync work stay pending so the second call lands mid-sync. + let resolveSync: (() => void) | undefined; + const syncPromise = new Promise((resolve) => { + resolveSync = resolve; + }); + mockSyncWalletMetadata.mockImplementation(async () => { + await syncPromise; + }); // Start first sync const firstSyncPromise = backupAndSyncService.performFullSync(); @@ -362,8 +424,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { // Both promises should be the same reference expect(firstSyncPromise).toStrictEqual(secondSyncPromise); - // Resolve the trace to complete the sync - resolveTrace?.(); + // Resolve the sync work to complete the run + resolveSync?.(); // Both should resolve to the same value const [firstResult, secondResult] = await Promise.all([ @@ -385,15 +447,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track how many times the actual sync logic runs - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Fire multiple syncs rapidly const promises = [ backupAndSyncService.performFullSync(), @@ -409,9 +462,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await Promise.all(promises); // Should only have executed the sync logic once - expect(syncExecutionCount).toBe(1); - - // getLocalEntropyWallets should only be called once + // (getLocalEntropyWallets runs once per sync run) expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); // All promises should resolve successfully to the same value @@ -429,15 +480,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track how many times the actual sync logic runs - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Start first sync and wait for it to complete const firstSyncPromise = backupAndSyncService.performFullSync(); await firstSyncPromise; @@ -452,9 +494,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await secondSyncPromise; // Should have executed the sync logic twice (once for each call) - expect(syncExecutionCount).toBe(2); - - // getLocalEntropyWallets should be called twice (once for each sync) + // (getLocalEntropyWallets runs once per sync run) expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(2); // Both promises should resolve successfully @@ -471,15 +511,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Perform first sync const firstSyncPromise = backupAndSyncService.performFullSync(); @@ -493,7 +524,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await Promise.all([firstSyncPromise, atLeastOncePromise]); // Should only have executed once - expect(syncExecutionCount).toBe(1); + // (getLocalEntropyWallets runs once per sync run) + expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); }); }); @@ -525,22 +557,12 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - const syncPromise = backupAndSyncService.performFullSyncAtLeastOnce(); expect(syncPromise).toBeInstanceOf(Promise); await syncPromise; - expect(syncExecutionCount).toBe(1); expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); }); @@ -553,15 +575,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Fire multiple calls rapidly const promises = [ backupAndSyncService.performFullSyncAtLeastOnce(), @@ -577,7 +590,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await Promise.all(promises); // Should only have executed the sync logic once - expect(syncExecutionCount).toBe(1); expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); // All promises should resolve successfully to the same value @@ -595,15 +607,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Start first sync and wait for it to complete const firstSyncPromise = backupAndSyncService.performFullSyncAtLeastOnce(); @@ -620,7 +623,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await secondSyncPromise; // Should only have executed the sync logic once (no new sync created) - expect(syncExecutionCount).toBe(1); expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); // Both promises should resolve successfully @@ -637,22 +639,12 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Multiple sequential calls await backupAndSyncService.performFullSyncAtLeastOnce(); await backupAndSyncService.performFullSyncAtLeastOnce(); await backupAndSyncService.performFullSyncAtLeastOnce(); // Should only have executed once, regardless of how many times it's called - expect(syncExecutionCount).toBe(1); expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); }); @@ -665,15 +657,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Call performFullSyncAtLeastOnce first const atLeastOncePromise = backupAndSyncService.performFullSyncAtLeastOnce(); @@ -687,7 +670,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await Promise.all([atLeastOncePromise, fullSyncPromise]); // Should only have executed once - expect(syncExecutionCount).toBe(1); + // (getLocalEntropyWallets runs once per sync run) + expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); // Now call performFullSync again after completion const secondFullSyncPromise = backupAndSyncService.performFullSync(); @@ -698,7 +682,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await secondFullSyncPromise; // Should have executed twice now (one for each performFullSync call) - expect(syncExecutionCount).toBe(2); + expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(2); // But performFullSyncAtLeastOnce should still return the original promise const laterAtLeastOncePromise = @@ -707,7 +691,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { // And should not trigger another sync await laterAtLeastOncePromise; - expect(syncExecutionCount).toBe(2); // Still only 2 + expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(2); // Still only 2 }, 15000); // Increase timeout to 15 seconds }); }); diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.ts index 852af9daad..f5f543c67d 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.ts @@ -442,14 +442,27 @@ export class BackupAndSyncService { }); }; - // Execute the big sync function with tracing and ensure state cleanup + // Execute the big sync function and ensure state cleanup. + // The sync runs untraced so we can decide afterwards whether it did any + // real work; the span is then backdated to preserve the real duration. + const { mutationTracker } = this.#context; + mutationTracker?.reset(); + const startTime = Date.now(); try { - await this.#context.traceFn( - { - name: TraceName.AccountSyncFull, - }, - bigSyncFn, - ); + await bigSyncFn(); + + // Only emit a span when the sync actually changed something (local or + // remote). The common no-op sync (every login) is not traced. The span + // is backdated so the real sync duration is preserved. + if (mutationTracker?.hasOccurred()) { + await this.#context.traceFn( + { + name: TraceName.AccountSyncFull, + startTime, + }, + () => undefined, + ); + } } finally { // Always reset state, regardless of success or failure this.#context.controllerStateUpdateFn( diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts index 686769a399..4b19407aee 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts @@ -47,9 +47,11 @@ describe('BackupAndSync - Syncing - Group', () => { let mockContext: BackupAndSyncContext; let mockLocalGroup: AccountGroupMultichainAccountObject; let mockWallet: AccountWalletEntropyObject; + let mockMarkOccurred: jest.Mock; beforeEach(() => { mockGetLocalGroupsForEntropyWallet.mockReturnValue([]); + mockMarkOccurred = jest.fn(); mockContext = { controller: { @@ -71,6 +73,11 @@ describe('BackupAndSync - Syncing - Group', () => { call: jest.fn(), }, emitAnalyticsEventFn: jest.fn(), + mutationTracker: { + markOccurred: mockMarkOccurred, + hasOccurred: jest.fn(), + reset: jest.fn(), + }, } as unknown as BackupAndSyncContext; mockLocalGroup = { @@ -134,6 +141,7 @@ describe('BackupAndSync - Syncing - Group', () => { expect(mockContext.messenger.call).toHaveBeenCalledTimes(1); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); + expect(mockMarkOccurred).not.toHaveBeenCalled(); }); it('emits analytics events for successful creations', async () => { @@ -155,6 +163,7 @@ describe('BackupAndSync - Syncing - Group', () => { action: BackupAndSyncAnalyticsEvent.GroupAdded, profileId: 'test-profile', }); + expect(mockMarkOccurred).toHaveBeenCalledTimes(1); }); it('only emits analytics for newly created groups, not pre-existing ones', async () => { @@ -192,6 +201,8 @@ describe('BackupAndSync - Syncing - Group', () => { action: BackupAndSyncAnalyticsEvent.GroupAdded, profileId: 'test-profile', }); + // markOccurred should fire once per newly created group (1 and 2). + expect(mockMarkOccurred).toHaveBeenCalledTimes(2); }); }); diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts index 2a6c90284e..b51de74466 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts @@ -71,6 +71,7 @@ export const createMultichainAccountGroupsBatch = async ( const didGroupAlreadyExist = existingGroupIndices.has(group.groupIndex); if (!didGroupAlreadyExist) { + context.mutationTracker?.markOccurred(); context.emitAnalyticsEventFn({ action: analyticsAction, profileId, diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts index d4e10da5ae..ef32619cda 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts @@ -35,13 +35,21 @@ const mockCreateMultichainAccountGroupsBatch = describe('BackupAndSync - Syncing - Legacy', () => { let mockContext: BackupAndSyncContext; + let mockMarkOccurred: jest.Mock; beforeEach(() => { + mockMarkOccurred = jest.fn(); + mockContext = { controller: { setAccountGroupName: jest.fn(), }, emitAnalyticsEventFn: jest.fn(), + mutationTracker: { + markOccurred: mockMarkOccurred, + hasOccurred: jest.fn(), + reset: jest.fn(), + }, } as unknown as BackupAndSyncContext; }); @@ -151,6 +159,8 @@ describe('BackupAndSync - Syncing - Legacy', () => { 'Legacy Account 2', true, ); + // Two renames -> two mutations marked. + expect(mockMarkOccurred).toHaveBeenCalledTimes(2); }); it('skips legacy accounts with missing name or address', async () => { @@ -198,6 +208,7 @@ describe('BackupAndSync - Syncing - Legacy', () => { ); expect(mockContext.controller.setAccountGroupName).not.toHaveBeenCalled(); + expect(mockMarkOccurred).not.toHaveBeenCalled(); }); it('emits analytics event on completion', async () => { diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts index 616fc2f393..5cc4e76ef6 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts @@ -88,6 +88,7 @@ export const performLegacyAccountSyncing = async ( ); if (localGroup) { context.controller.setAccountGroupName(localGroup.id, n, true); + context.mutationTracker?.markOccurred(); context.emitAnalyticsEventFn({ action: BackupAndSyncAnalyticsEvent.LegacyGroupRenamed, diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts index ea97990f40..2562f2149d 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts @@ -6,13 +6,20 @@ describe('BackupAndSync - Syncing - Metadata', () => { let mockContext: BackupAndSyncContext; let mockApplyLocalUpdate: jest.Mock; let mockValidateUserStorageValue: jest.Mock; + let mockMarkOccurred: jest.Mock; beforeEach(() => { mockApplyLocalUpdate = jest.fn(); mockValidateUserStorageValue = jest.fn().mockReturnValue(true); + mockMarkOccurred = jest.fn(); mockContext = { emitAnalyticsEventFn: jest.fn(), + mutationTracker: { + markOccurred: mockMarkOccurred, + hasOccurred: jest.fn(), + reset: jest.fn(), + }, } as unknown as BackupAndSyncContext; }); @@ -33,6 +40,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(false); expect(mockApplyLocalUpdate).not.toHaveBeenCalled(); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); + expect(mockMarkOccurred).not.toHaveBeenCalled(); }); it('applies user storage value when it is more recent and valid', async () => { @@ -50,6 +58,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(false); expect(mockApplyLocalUpdate).toHaveBeenCalledWith('new'); + expect(mockMarkOccurred).toHaveBeenCalledTimes(1); expect(mockContext.emitAnalyticsEventFn).toHaveBeenCalledWith({ action: BackupAndSyncAnalyticsEvent.GroupRenamed, profileId: 'test-profile', @@ -68,6 +77,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(true); expect(mockApplyLocalUpdate).not.toHaveBeenCalled(); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); + expect(mockMarkOccurred).not.toHaveBeenCalled(); }); it('returns true when user storage value is invalid', async () => { @@ -84,6 +94,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(true); expect(mockApplyLocalUpdate).not.toHaveBeenCalled(); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); + expect(mockMarkOccurred).not.toHaveBeenCalled(); }); it('applies user storage value when no local metadata exists', async () => { @@ -97,6 +108,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(false); expect(mockApplyLocalUpdate).toHaveBeenCalledWith('remote'); + expect(mockMarkOccurred).toHaveBeenCalledTimes(1); }); it('does not emit analytics when no analytics config provided', async () => { diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts index c148de7a04..60ad206138 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts @@ -62,6 +62,7 @@ export async function compareAndSyncMetadata({ if ((isUserStorageMoreRecent || !localMetadata) && isUserStorageValueValid) { // User storage is more recent and valid, apply it locally await applyLocalUpdate(userStorageValue as T); + context.mutationTracker?.markOccurred(); // Emit analytics event if provided if (analytics) { diff --git a/packages/account-tree-controller/src/backup-and-sync/types.ts b/packages/account-tree-controller/src/backup-and-sync/types.ts index 2816be6035..64e2840c95 100644 --- a/packages/account-tree-controller/src/backup-and-sync/types.ts +++ b/packages/account-tree-controller/src/backup-and-sync/types.ts @@ -77,6 +77,20 @@ export type LegacyUserStorageSyncedAccount = Infer< typeof LegacyUserStorageSyncedAccountSchema >; +/** + * Tracks whether the current full sync run performed a real write (a local + * mutation or a remote push), so the service can gate emission of the + * full-sync trace. + */ +export type SyncMutationTracker = { + /** Records that a real write happened during the current sync run. */ + markOccurred: () => void; + /** Whether a real write has been recorded since the last reset. */ + hasOccurred: () => boolean; + /** Clears the tracker at the start of a sync run. */ + reset: () => void; +}; + export type BackupAndSyncContext = { messenger: AccountTreeControllerMessenger; controller: AccountTreeController; @@ -84,6 +98,12 @@ export type BackupAndSyncContext = { traceFn: TraceCallback; groupIdToWalletId: Map; emitAnalyticsEventFn: (event: BackupAndSyncEmitAnalyticsEventParams) => void; + /** + * Tracks whether the current sync run performed a real write (a local + * mutation or a remote push). Sync helpers call `markOccurred()`; the service + * reads it to gate emission of the full-sync trace. + */ + mutationTracker?: SyncMutationTracker; }; export type LegacyAccountSyncingContext = { diff --git a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts index e364fb2a7e..4d049a5c59 100644 --- a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts @@ -61,12 +61,20 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { let mockContext: BackupAndSyncContext; let mockWallet: AccountWalletEntropyObject; let mockGroup: AccountGroupMultichainAccountObject; + let mockMarkOccurred: jest.Mock; beforeEach(() => { + mockMarkOccurred = jest.fn(); + mockContext = { messenger: { call: jest.fn(), }, + mutationTracker: { + markOccurred: mockMarkOccurred, + hasOccurred: jest.fn(), + reset: jest.fn(), + }, } as unknown as BackupAndSyncContext; mockWallet = { @@ -204,6 +212,7 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { JSON.stringify(formattedWallet), 'test-entropy-id', ); + expect(mockMarkOccurred).toHaveBeenCalledTimes(1); }); }); @@ -436,6 +445,7 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { JSON.stringify(formattedGroup), 'test-entropy-id', ); + expect(mockMarkOccurred).toHaveBeenCalledTimes(1); }); }); @@ -466,6 +476,7 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { ], 'test-entropy-id', ); + expect(mockMarkOccurred).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts index 838f7b8e61..00a2f74b2d 100644 --- a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts +++ b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts @@ -82,12 +82,14 @@ export const pushWalletToUserStorage = async ( backupAndSyncLogger(`Pushing wallet to user storage: ${stringifiedWallet}`); - return await context.messenger.call( + const result = await context.messenger.call( 'UserStorageController:performSetStorage', `${USER_STORAGE_WALLETS_FEATURE_KEY}.${USER_STORAGE_WALLETS_FEATURE_ENTRY_KEY}`, stringifiedWallet, entropySourceId, ); + context.mutationTracker?.markOccurred(); + return result; }); }; @@ -193,12 +195,14 @@ export const pushGroupToUserStorage = async ( backupAndSyncLogger(`Pushing group to user storage: ${stringifiedGroup}`); - return await context.messenger.call( + const result = await context.messenger.call( 'UserStorageController:performSetStorage', `${USER_STORAGE_GROUPS_FEATURE_KEY}.${formattedGroup.groupIndex}`, stringifiedGroup, entropySourceId, ); + context.mutationTracker?.markOccurred(); + return result; }); }; @@ -232,12 +236,14 @@ export const pushGroupToUserStorageBatch = async ( `Pushing groups to user storage: ${entries.map(([_, value]) => value).join(', ')}`, ); - return await context.messenger.call( + const result = await context.messenger.call( 'UserStorageController:performBatchSetStorage', USER_STORAGE_GROUPS_FEATURE_KEY, entries, entropySourceId, ); + context.mutationTracker?.markOccurred(); + return result; }); }; diff --git a/packages/account-tree-controller/src/backup-and-sync/utils/index.ts b/packages/account-tree-controller/src/backup-and-sync/utils/index.ts index 7e83644ae2..f84fb0dafd 100644 --- a/packages/account-tree-controller/src/backup-and-sync/utils/index.ts +++ b/packages/account-tree-controller/src/backup-and-sync/utils/index.ts @@ -1,2 +1,3 @@ export * from './controller'; export { toErrorMessage } from './errors'; +export { createSyncMutationTracker } from './mutation-tracker'; diff --git a/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts new file mode 100644 index 0000000000..2d2c47a3e1 --- /dev/null +++ b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts @@ -0,0 +1,21 @@ +import type { SyncMutationTracker } from '../types'; + +/** + * Creates a tracker that records whether the current full sync run performed a + * real write (a local mutation or a remote push). Runs are serialized, so a + * single tracker is reset at the start of each run. + * + * @returns A fresh mutation tracker. + */ +export const createSyncMutationTracker = (): SyncMutationTracker => { + let occurred = false; + return { + markOccurred: () => { + occurred = true; + }, + hasOccurred: () => occurred, + reset: () => { + occurred = false; + }, + }; +}; From 7b05155b3864bcd57aa84b5be04b67973f6ad9bf Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 1 Jul 2026 16:20:42 +0200 Subject: [PATCH 2/5] fix: update CHANGELOG --- packages/account-tree-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/account-tree-controller/CHANGELOG.md b/packages/account-tree-controller/CHANGELOG.md index 2cb7291981..4814dedc93 100644 --- a/packages/account-tree-controller/CHANGELOG.md +++ b/packages/account-tree-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- The `Multichain Account Syncing - Full` trace is now only emitted when a backup-and-sync run actually mutates local or remote state, and the span is backdated to preserve the real sync duration ([#0000](https://github.com/MetaMask/core/pull/0000)) +- The `Multichain Account Syncing - Full` trace is now only emitted when a backup-and-sync run actually mutates local or remote state, and the span is backdated to preserve the real sync duration ([#9343](https://github.com/MetaMask/core/pull/9343)) - Bump `@metamask/keyring-api` from `^23.1.0` to `^23.3.0` ([#9249](https://github.com/MetaMask/core/pull/9249)) - Bump `@metamask/multichain-account-service` from `^11.0.0` to `^11.1.0` ([#9264](https://github.com/MetaMask/core/pull/9264)) From d55336f8107d97fd76ba442761bd80f35800e884 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 3 Jul 2026 10:28:24 +0200 Subject: [PATCH 3/5] fix: API changes and address feedbacks --- eslint-suppressions.json | 2 +- packages/account-tree-controller/CHANGELOG.md | 2 +- .../src/backup-and-sync/service/index.test.ts | 148 +++++++++++++++--- .../src/backup-and-sync/service/index.ts | 23 ++- .../src/backup-and-sync/syncing/group.test.ts | 16 +- .../src/backup-and-sync/syncing/group.ts | 2 +- .../backup-and-sync/syncing/legacy.test.ts | 12 +- .../src/backup-and-sync/syncing/legacy.ts | 2 +- .../backup-and-sync/syncing/metadata.test.ts | 18 +-- .../src/backup-and-sync/syncing/metadata.ts | 2 +- .../src/backup-and-sync/types.ts | 26 +-- .../user-storage/network-operations.test.ts | 14 +- .../user-storage/network-operations.ts | 6 +- .../utils/mutation-tracker.test.ts | 75 +++++++++ .../backup-and-sync/utils/mutation-tracker.ts | 23 ++- 15 files changed, 286 insertions(+), 85 deletions(-) create mode 100644 packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.test.ts diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 7de43a0fc6..8e35b0e375 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -37,7 +37,7 @@ "count": 1 }, "n/no-sync": { - "count": 22 + "count": 28 } }, "packages/account-tree-controller/src/backup-and-sync/service/index.ts": { diff --git a/packages/account-tree-controller/CHANGELOG.md b/packages/account-tree-controller/CHANGELOG.md index 4814dedc93..f5b66e0e10 100644 --- a/packages/account-tree-controller/CHANGELOG.md +++ b/packages/account-tree-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- The `Multichain Account Syncing - Full` trace is now only emitted when a backup-and-sync run actually mutates local or remote state, and the span is backdated to preserve the real sync duration ([#9343](https://github.com/MetaMask/core/pull/9343)) +- The `Multichain Account Syncing - Full` trace is now only emitted when a backup-and-sync run actually mutates local or remote state (rolled-back local changes do not count), including runs that fail after doing durable work, and the span is backdated to preserve the real sync duration ([#9343](https://github.com/MetaMask/core/pull/9343)) - Bump `@metamask/keyring-api` from `^23.1.0` to `^23.3.0` ([#9249](https://github.com/MetaMask/core/pull/9249)) - Bump `@metamask/multichain-account-service` from `^11.0.0` to `^11.1.0` ([#9264](https://github.com/MetaMask/core/pull/9264)) diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts index 338776a78c..60ff6f4293 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts @@ -5,10 +5,11 @@ import type { AccountGroupObject } from '../../group'; import type { AccountWalletEntropyObject } from '../../wallet'; import { TraceName } from '../analytics'; import { getProfileId } from '../authentication'; -import { syncWalletMetadata } from '../syncing'; -import type { BackupAndSyncContext } from '../types'; +import { performLegacyAccountSyncing, syncWalletMetadata } from '../syncing'; +import type { BackupAndSyncContext, SyncMutationTracker } from '../types'; +import { getAllGroupsFromUserStorage } from '../user-storage'; // We only need to import the functions we actually spy on -import { getLocalEntropyWallets } from '../utils'; +import { createStateSnapshot, getLocalEntropyWallets } from '../utils'; // Mock the sync functions and all external dependencies jest.mock('../syncing'); @@ -25,17 +26,34 @@ const mockGetLocalEntropyWallets = const mockSyncWalletMetadata = syncWalletMetadata as jest.MockedFunction< typeof syncWalletMetadata >; +const mockPerformLegacyAccountSyncing = + performLegacyAccountSyncing as jest.MockedFunction< + typeof performLegacyAccountSyncing + >; +const mockGetAllGroupsFromUserStorage = + getAllGroupsFromUserStorage as jest.MockedFunction< + typeof getAllGroupsFromUserStorage + >; +const mockCreateStateSnapshot = createStateSnapshot as jest.MockedFunction< + typeof createStateSnapshot +>; // Local tracker (the real factory lives in the auto-mocked `../utils`). -const createTestMutationTracker = () => { - let occurred = false; +const createTestMutationTracker = (): SyncMutationTracker => { + let remoteWrite = false; + let localWrite = false; return { - markOccurred: () => { - occurred = true; + setRemoteWrite: (value: boolean): void => { + remoteWrite = value; + }, + getLocalWrite: (): boolean => localWrite, + setLocalWrite: (value: boolean): void => { + localWrite = value; }, - hasOccurred: () => occurred, - reset: () => { - occurred = false; + hasOccurred: (): boolean => remoteWrite || localWrite, + reset: (): void => { + remoteWrite = false; + localWrite = false; }, }; }; @@ -85,6 +103,11 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { // Setup default mock returns mockGetLocalEntropyWallets.mockReturnValue([]); mockGetProfileId.mockResolvedValue('test-profile-id'); + // Return a truthy snapshot so the per-wallet rollback path runs (the real + // implementation always returns a snapshot object). + mockCreateStateSnapshot.mockReturnValue( + {} as ReturnType, + ); backupAndSyncService = new BackupAndSyncService(mockContext); }); @@ -135,8 +158,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }); it('returns early when a full sync has not completed at least once', () => { - mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = - false; + mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = false; backupAndSyncService.enqueueSingleWalletSync('entropy:wallet-1'); // Should not have called any messenger functions beyond the state check expect(mockContext.messenger.call).toHaveBeenCalledTimes(1); @@ -149,8 +171,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }); it('enqueues single wallet sync when enabled and synced at least once', async () => { - mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = - true; + mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = true; // Add a mock wallet to the context so the sync can find it mockContext.controller.state.accountTree.wallets = { @@ -234,8 +255,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }); it('returns early when a full sync has not completed at least once', () => { - mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = - false; + mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = false; backupAndSyncService.enqueueSingleGroupSync('entropy:wallet-1/1'); @@ -250,8 +270,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }); it('enqueues group sync when enabled and synced at least once', async () => { - mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = - true; + mockContext.controller.state.hasAccountTreeSyncingSyncedAtLeastOnce = true; // Set up the group mapping and wallet context mockContext.groupIdToWalletId.set( @@ -353,18 +372,21 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { expect(mockGetLocalEntropyWallets).toHaveBeenCalled(); }); - it('emits a backdated AccountSyncFull span when the sync mutates state', async () => { + it('emits a backdated AccountSyncFull span when the sync mutates local state', async () => { mockGetLocalEntropyWallets.mockReturnValue([ { id: 'entropy:wallet-1', metadata: { entropy: { id: 'test-entropy-id' } }, } as unknown as AccountWalletEntropyObject, ]); + // Empty remote groups makes the wallet run complete cleanly (push + skip) + // without hitting the rollback path. + mockGetAllGroupsFromUserStorage.mockResolvedValue([]); - // Simulate a real write happening during the sync by having a mocked + // Simulate a local write happening during the sync by having a mocked // helper report a mutation through the context. mockSyncWalletMetadata.mockImplementation(async (context) => { - context.mutationTracker?.markOccurred(); + context.mutationTracker?.setLocalWrite(true); }); await backupAndSyncService.performFullSync(); @@ -383,6 +405,90 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { expect(tracedCallback()).toBeUndefined(); }); + it('emits an AccountSyncFull span for a durable remote write even if the wallet is rolled back', async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + + // A remote push happens, then the wallet fails and is rolled back. Remote + // writes are durable, so the run must still emit. + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.setRemoteWrite(true); + throw new Error('boom'); + }); + + await backupAndSyncService.performFullSync(); + + expect(mockContext.traceFn).toHaveBeenCalledTimes(1); + expect(mockContext.traceFn).toHaveBeenCalledWith( + { + name: TraceName.AccountSyncFull, + startTime: expect.any(Number), + }, + expect.any(Function), + ); + }); + + it("does not emit an AccountSyncFull span when a wallet's local changes are rolled back", async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + + // A local write happens, then the wallet fails and is rolled back. The + // local change is reverted, so the run must not emit. + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.setLocalWrite(true); + throw new Error('boom'); + }); + + await backupAndSyncService.performFullSync(); + + expect(mockContext.traceFn).not.toHaveBeenCalled(); + }); + + it('emits an AccountSyncFull span when the run throws after doing durable work', async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + { + id: 'entropy:wallet-2', + metadata: { entropy: { id: 'test-entropy-id-2' } }, + } as unknown as AccountWalletEntropyObject, + ]); + mockGetAllGroupsFromUserStorage.mockResolvedValue([]); + + // Wallet 1 performs a durable remote write and completes; wallet 2's + // legacy sync then fails and aborts the whole run. + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.setRemoteWrite(true); + }); + mockPerformLegacyAccountSyncing + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error('legacy boom')); + + await expect(backupAndSyncService.performFullSync()).rejects.toThrow( + 'Legacy syncing failed', + ); + + // The span is still recorded despite the failure. + expect(mockContext.traceFn).toHaveBeenCalledTimes(1); + expect(mockContext.traceFn).toHaveBeenCalledWith( + { + name: TraceName.AccountSyncFull, + startTime: expect.any(Number), + }, + expect.any(Function), + ); + }); + it('does not emit an AccountSyncFull span when the sync is a no-op', async () => { mockGetLocalEntropyWallets.mockReturnValue([ { diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.ts index f5f543c67d..1b20c61357 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.ts @@ -359,6 +359,12 @@ export class BackupAndSyncService { // 3. Execute multichain account syncing let stateSnapshot: StateSnapshot | undefined; + // Capture the local-write flag so it can be reverted together with + // the state snapshot if this wallet is rolled back. Remote writes are + // durable and intentionally excluded. + const localWriteBeforeWallet = + this.#context.mutationTracker?.getLocalWrite() ?? false; + try { // 3.1 Wallet syncing // Create a state snapshot before processing each wallet for potential rollback @@ -416,6 +422,11 @@ export class BackupAndSyncService { ); } restoreStateFromSnapshot(this.#context, stateSnapshot); + // Revert the local-write flag too, so a rolled-back wallet does + // not keep the run marked as having changed local state. + this.#context.mutationTracker?.setLocalWrite( + localWriteBeforeWallet, + ); backupAndSyncLogger( `Rolled back state changes for wallet ${wallet.id}`, ); @@ -450,10 +461,12 @@ export class BackupAndSyncService { const startTime = Date.now(); try { await bigSyncFn(); - - // Only emit a span when the sync actually changed something (local or - // remote). The common no-op sync (every login) is not traced. The span - // is backdated so the real sync duration is preserved. + } finally { + // Emit a span whenever the run changed something (local or remote), even + // if it later threw - a failed sync that already did durable work is + // exactly the kind of anomaly we want visible. No-op syncs (the common + // per-login case) are not traced. The span is backdated so the real sync + // duration is preserved. if (mutationTracker?.hasOccurred()) { await this.#context.traceFn( { @@ -463,7 +476,7 @@ export class BackupAndSyncService { () => undefined, ); } - } finally { + // Always reset state, regardless of success or failure this.#context.controllerStateUpdateFn( (state: AccountTreeControllerState) => { diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts index 4b19407aee..2e546f2d5b 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts @@ -47,11 +47,11 @@ describe('BackupAndSync - Syncing - Group', () => { let mockContext: BackupAndSyncContext; let mockLocalGroup: AccountGroupMultichainAccountObject; let mockWallet: AccountWalletEntropyObject; - let mockMarkOccurred: jest.Mock; + let mockSetLocalWrite: jest.Mock; beforeEach(() => { mockGetLocalGroupsForEntropyWallet.mockReturnValue([]); - mockMarkOccurred = jest.fn(); + mockSetLocalWrite = jest.fn(); mockContext = { controller: { @@ -74,9 +74,7 @@ describe('BackupAndSync - Syncing - Group', () => { }, emitAnalyticsEventFn: jest.fn(), mutationTracker: { - markOccurred: mockMarkOccurred, - hasOccurred: jest.fn(), - reset: jest.fn(), + setLocalWrite: mockSetLocalWrite, }, } as unknown as BackupAndSyncContext; @@ -141,7 +139,7 @@ describe('BackupAndSync - Syncing - Group', () => { expect(mockContext.messenger.call).toHaveBeenCalledTimes(1); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); - expect(mockMarkOccurred).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('emits analytics events for successful creations', async () => { @@ -163,7 +161,7 @@ describe('BackupAndSync - Syncing - Group', () => { action: BackupAndSyncAnalyticsEvent.GroupAdded, profileId: 'test-profile', }); - expect(mockMarkOccurred).toHaveBeenCalledTimes(1); + expect(mockSetLocalWrite).toHaveBeenCalledTimes(1); }); it('only emits analytics for newly created groups, not pre-existing ones', async () => { @@ -201,8 +199,8 @@ describe('BackupAndSync - Syncing - Group', () => { action: BackupAndSyncAnalyticsEvent.GroupAdded, profileId: 'test-profile', }); - // markOccurred should fire once per newly created group (1 and 2). - expect(mockMarkOccurred).toHaveBeenCalledTimes(2); + // setLocalWrite(true) should fire once per newly created group (1 and 2). + expect(mockSetLocalWrite).toHaveBeenCalledTimes(2); }); }); diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts index b51de74466..273625027d 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts @@ -71,7 +71,7 @@ export const createMultichainAccountGroupsBatch = async ( const didGroupAlreadyExist = existingGroupIndices.has(group.groupIndex); if (!didGroupAlreadyExist) { - context.mutationTracker?.markOccurred(); + context.mutationTracker?.setLocalWrite(true); context.emitAnalyticsEventFn({ action: analyticsAction, profileId, diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts index ef32619cda..00ee21d029 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts @@ -35,10 +35,10 @@ const mockCreateMultichainAccountGroupsBatch = describe('BackupAndSync - Syncing - Legacy', () => { let mockContext: BackupAndSyncContext; - let mockMarkOccurred: jest.Mock; + let mockSetLocalWrite: jest.Mock; beforeEach(() => { - mockMarkOccurred = jest.fn(); + mockSetLocalWrite = jest.fn(); mockContext = { controller: { @@ -46,9 +46,7 @@ describe('BackupAndSync - Syncing - Legacy', () => { }, emitAnalyticsEventFn: jest.fn(), mutationTracker: { - markOccurred: mockMarkOccurred, - hasOccurred: jest.fn(), - reset: jest.fn(), + setLocalWrite: mockSetLocalWrite, }, } as unknown as BackupAndSyncContext; }); @@ -160,7 +158,7 @@ describe('BackupAndSync - Syncing - Legacy', () => { true, ); // Two renames -> two mutations marked. - expect(mockMarkOccurred).toHaveBeenCalledTimes(2); + expect(mockSetLocalWrite).toHaveBeenCalledTimes(2); }); it('skips legacy accounts with missing name or address', async () => { @@ -208,7 +206,7 @@ describe('BackupAndSync - Syncing - Legacy', () => { ); expect(mockContext.controller.setAccountGroupName).not.toHaveBeenCalled(); - expect(mockMarkOccurred).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('emits analytics event on completion', async () => { diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts index 5cc4e76ef6..b0749889d1 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts @@ -88,7 +88,7 @@ export const performLegacyAccountSyncing = async ( ); if (localGroup) { context.controller.setAccountGroupName(localGroup.id, n, true); - context.mutationTracker?.markOccurred(); + context.mutationTracker?.setLocalWrite(true); context.emitAnalyticsEventFn({ action: BackupAndSyncAnalyticsEvent.LegacyGroupRenamed, diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts index 2562f2149d..072ceefded 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts @@ -6,19 +6,17 @@ describe('BackupAndSync - Syncing - Metadata', () => { let mockContext: BackupAndSyncContext; let mockApplyLocalUpdate: jest.Mock; let mockValidateUserStorageValue: jest.Mock; - let mockMarkOccurred: jest.Mock; + let mockSetLocalWrite: jest.Mock; beforeEach(() => { mockApplyLocalUpdate = jest.fn(); mockValidateUserStorageValue = jest.fn().mockReturnValue(true); - mockMarkOccurred = jest.fn(); + mockSetLocalWrite = jest.fn(); mockContext = { emitAnalyticsEventFn: jest.fn(), mutationTracker: { - markOccurred: mockMarkOccurred, - hasOccurred: jest.fn(), - reset: jest.fn(), + setLocalWrite: mockSetLocalWrite, }, } as unknown as BackupAndSyncContext; }); @@ -40,7 +38,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(false); expect(mockApplyLocalUpdate).not.toHaveBeenCalled(); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); - expect(mockMarkOccurred).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('applies user storage value when it is more recent and valid', async () => { @@ -58,7 +56,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(false); expect(mockApplyLocalUpdate).toHaveBeenCalledWith('new'); - expect(mockMarkOccurred).toHaveBeenCalledTimes(1); + expect(mockSetLocalWrite).toHaveBeenCalledTimes(1); expect(mockContext.emitAnalyticsEventFn).toHaveBeenCalledWith({ action: BackupAndSyncAnalyticsEvent.GroupRenamed, profileId: 'test-profile', @@ -77,7 +75,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(true); expect(mockApplyLocalUpdate).not.toHaveBeenCalled(); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); - expect(mockMarkOccurred).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('returns true when user storage value is invalid', async () => { @@ -94,7 +92,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(true); expect(mockApplyLocalUpdate).not.toHaveBeenCalled(); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); - expect(mockMarkOccurred).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('applies user storage value when no local metadata exists', async () => { @@ -108,7 +106,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(false); expect(mockApplyLocalUpdate).toHaveBeenCalledWith('remote'); - expect(mockMarkOccurred).toHaveBeenCalledTimes(1); + expect(mockSetLocalWrite).toHaveBeenCalledTimes(1); }); it('does not emit analytics when no analytics config provided', async () => { diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts index 60ad206138..c3d5787666 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts @@ -62,7 +62,7 @@ export async function compareAndSyncMetadata({ if ((isUserStorageMoreRecent || !localMetadata) && isUserStorageValueValid) { // User storage is more recent and valid, apply it locally await applyLocalUpdate(userStorageValue as T); - context.mutationTracker?.markOccurred(); + context.mutationTracker?.setLocalWrite(true); // Emit analytics event if provided if (analytics) { diff --git a/packages/account-tree-controller/src/backup-and-sync/types.ts b/packages/account-tree-controller/src/backup-and-sync/types.ts index 64e2840c95..541d38a450 100644 --- a/packages/account-tree-controller/src/backup-and-sync/types.ts +++ b/packages/account-tree-controller/src/backup-and-sync/types.ts @@ -78,16 +78,24 @@ export type LegacyUserStorageSyncedAccount = Infer< >; /** - * Tracks whether the current full sync run performed a real write (a local - * mutation or a remote push), so the service can gate emission of the - * full-sync trace. + * Tracks whether the current full sync run performed a real write, so the + * service can gate emission of the full-sync trace. + * + * Writes are split by durability: + * - Remote writes (pushes to user storage) are durable and always count. + * - Local writes can be reverted by a per-wallet rollback, so the service + * reads the flag before a wallet and writes it back if that wallet rolls back. */ export type SyncMutationTracker = { - /** Records that a real write happened during the current sync run. */ - markOccurred: () => void; - /** Whether a real write has been recorded since the last reset. */ + /** Sets (or clears) the durable remote-write flag — a push to user storage. */ + setRemoteWrite: (value: boolean) => void; + /** Gets the local-write flag — a write a per-wallet rollback would revert. */ + getLocalWrite: () => boolean; + /** Sets, clears, or restores the local-write flag. */ + setLocalWrite: (value: boolean) => void; + /** Whether any write (remote or local) is currently recorded. */ hasOccurred: () => boolean; - /** Clears the tracker at the start of a sync run. */ + /** Clears all recorded writes at the start of a sync run. */ reset: () => void; }; @@ -100,8 +108,8 @@ export type BackupAndSyncContext = { emitAnalyticsEventFn: (event: BackupAndSyncEmitAnalyticsEventParams) => void; /** * Tracks whether the current sync run performed a real write (a local - * mutation or a remote push). Sync helpers call `markOccurred()`; the service - * reads it to gate emission of the full-sync trace. + * mutation or a remote push). Sync helpers set the relevant write flag; the + * service reads it to gate emission of the full-sync trace. */ mutationTracker?: SyncMutationTracker; }; diff --git a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts index 4d049a5c59..deaab84014 100644 --- a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts @@ -61,19 +61,17 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { let mockContext: BackupAndSyncContext; let mockWallet: AccountWalletEntropyObject; let mockGroup: AccountGroupMultichainAccountObject; - let mockMarkOccurred: jest.Mock; + let mockSetRemoteWrite: jest.Mock; beforeEach(() => { - mockMarkOccurred = jest.fn(); + mockSetRemoteWrite = jest.fn(); mockContext = { messenger: { call: jest.fn(), }, mutationTracker: { - markOccurred: mockMarkOccurred, - hasOccurred: jest.fn(), - reset: jest.fn(), + setRemoteWrite: mockSetRemoteWrite, }, } as unknown as BackupAndSyncContext; @@ -212,7 +210,7 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { JSON.stringify(formattedWallet), 'test-entropy-id', ); - expect(mockMarkOccurred).toHaveBeenCalledTimes(1); + expect(mockSetRemoteWrite).toHaveBeenCalledTimes(1); }); }); @@ -445,7 +443,7 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { JSON.stringify(formattedGroup), 'test-entropy-id', ); - expect(mockMarkOccurred).toHaveBeenCalledTimes(1); + expect(mockSetRemoteWrite).toHaveBeenCalledTimes(1); }); }); @@ -476,7 +474,7 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { ], 'test-entropy-id', ); - expect(mockMarkOccurred).toHaveBeenCalledTimes(1); + expect(mockSetRemoteWrite).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts index 00a2f74b2d..52cd55e9e3 100644 --- a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts +++ b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts @@ -88,7 +88,7 @@ export const pushWalletToUserStorage = async ( stringifiedWallet, entropySourceId, ); - context.mutationTracker?.markOccurred(); + context.mutationTracker?.setRemoteWrite(true); return result; }); }; @@ -201,7 +201,7 @@ export const pushGroupToUserStorage = async ( stringifiedGroup, entropySourceId, ); - context.mutationTracker?.markOccurred(); + context.mutationTracker?.setRemoteWrite(true); return result; }); }; @@ -242,7 +242,7 @@ export const pushGroupToUserStorageBatch = async ( entries, entropySourceId, ); - context.mutationTracker?.markOccurred(); + context.mutationTracker?.setRemoteWrite(true); return result; }); }; diff --git a/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.test.ts b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.test.ts new file mode 100644 index 0000000000..8cb0dc44e5 --- /dev/null +++ b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.test.ts @@ -0,0 +1,75 @@ +import { createSyncMutationTracker } from './mutation-tracker'; + +describe('BackupAndSync - Utils - createSyncMutationTracker', () => { + it('starts with no recorded writes', () => { + const tracker = createSyncMutationTracker(); + + expect(tracker.hasOccurred()).toBe(false); + expect(tracker.getLocalWrite()).toBe(false); + }); + + it('records remote writes', () => { + const tracker = createSyncMutationTracker(); + + tracker.setRemoteWrite(true); + + expect(tracker.hasOccurred()).toBe(true); + }); + + it('records local writes', () => { + const tracker = createSyncMutationTracker(); + + tracker.setLocalWrite(true); + + expect(tracker.hasOccurred()).toBe(true); + expect(tracker.getLocalWrite()).toBe(true); + }); + + it('clears all writes on reset', () => { + const tracker = createSyncMutationTracker(); + + tracker.setRemoteWrite(true); + tracker.setLocalWrite(true); + tracker.reset(); + + expect(tracker.hasOccurred()).toBe(false); + }); + + it('reverts local writes to a captured value', () => { + const tracker = createSyncMutationTracker(); + + const before = tracker.getLocalWrite(); + tracker.setLocalWrite(true); + expect(tracker.hasOccurred()).toBe(true); + + tracker.setLocalWrite(before); + + expect(tracker.hasOccurred()).toBe(false); + }); + + it('keeps durable remote writes when local writes are reverted', () => { + const tracker = createSyncMutationTracker(); + + tracker.setRemoteWrite(true); + const before = tracker.getLocalWrite(); + tracker.setLocalWrite(true); + + tracker.setLocalWrite(before); + + // Local write is gone, but the durable remote write still counts. + expect(tracker.hasOccurred()).toBe(true); + }); + + it('preserves earlier local writes captured in the value', () => { + const tracker = createSyncMutationTracker(); + + tracker.setLocalWrite(true); + const before = tracker.getLocalWrite(); + tracker.setLocalWrite(true); + + tracker.setLocalWrite(before); + + // The captured value already had a local write, so it is preserved. + expect(tracker.hasOccurred()).toBe(true); + }); +}); diff --git a/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts index 2d2c47a3e1..a2db6a9821 100644 --- a/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts +++ b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts @@ -2,20 +2,27 @@ import type { SyncMutationTracker } from '../types'; /** * Creates a tracker that records whether the current full sync run performed a - * real write (a local mutation or a remote push). Runs are serialized, so a - * single tracker is reset at the start of each run. + * real write. Remote writes (pushes to user storage) are durable; local writes + * can be reverted by a per-wallet rollback. Runs are serialized, so a single + * tracker is reset at the start of each run. * * @returns A fresh mutation tracker. */ export const createSyncMutationTracker = (): SyncMutationTracker => { - let occurred = false; + let remoteWrite = false; + let localWrite = false; return { - markOccurred: () => { - occurred = true; + setRemoteWrite: (value: boolean): void => { + remoteWrite = value; }, - hasOccurred: () => occurred, - reset: () => { - occurred = false; + getLocalWrite: (): boolean => localWrite, + setLocalWrite: (value: boolean): void => { + localWrite = value; + }, + hasOccurred: (): boolean => remoteWrite || localWrite, + reset: (): void => { + remoteWrite = false; + localWrite = false; }, }; }; From 79e5515860bbd93690932119bbf68287a9371ce5 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 3 Jul 2026 10:51:53 +0200 Subject: [PATCH 4/5] fix: address cursor feedbacks --- eslint-suppressions.json | 2 +- .../src/backup-and-sync/service/index.test.ts | 31 ++++++++++++++ .../src/backup-and-sync/service/index.ts | 41 ++++++++++++------- 3 files changed, 58 insertions(+), 16 deletions(-) diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 8e35b0e375..0b9972ca5b 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -37,7 +37,7 @@ "count": 1 }, "n/no-sync": { - "count": 28 + "count": 29 } }, "packages/account-tree-controller/src/backup-and-sync/service/index.ts": { diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts index 60ff6f4293..6ee2b19abd 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts @@ -2,6 +2,7 @@ import { AccountWalletType } from '@metamask/account-api'; import { BackupAndSyncService } from '.'; import type { AccountGroupObject } from '../../group'; +import type { AccountTreeControllerState } from '../../types'; import type { AccountWalletEntropyObject } from '../../wallet'; import { TraceName } from '../analytics'; import { getProfileId } from '../authentication'; @@ -405,6 +406,36 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { expect(tracedCallback()).toBeUndefined(); }); + it('clears the in-progress flag even when the trace emit fails', async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + mockGetAllGroupsFromUserStorage.mockResolvedValue([]); + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.setLocalWrite(true); + }); + // Tracing is best-effort: a rejected trace must not fail the sync nor + // leave the controller stuck mid-sync. + (mockContext.traceFn as jest.Mock).mockRejectedValue( + new Error('trace boom'), + ); + + expect(await backupAndSyncService.performFullSync()).toBeUndefined(); + + // Replay every state update; the in-progress flag must end up cleared. + const state = { + isAccountTreeSyncingInProgress: true, + } as AccountTreeControllerState; + for (const [updater] of (mockContext.controllerStateUpdateFn as jest.Mock) + .mock.calls) { + updater(state); + } + expect(state.isAccountTreeSyncingInProgress).toBe(false); + }); + it('emits an AccountSyncFull span for a durable remote write even if the wallet is rolled back', async () => { mockGetLocalEntropyWallets.mockReturnValue([ { diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.ts index 1b20c61357..9ff53f025d 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.ts @@ -462,27 +462,38 @@ export class BackupAndSyncService { try { await bigSyncFn(); } finally { + // Always clear the in-progress flag first, regardless of success or + // failure. Doing this before the (awaited) trace below guarantees a + // failing trace can never leave the controller stuck mid-sync and block + // later full sync attempts. + this.#context.controllerStateUpdateFn( + (state: AccountTreeControllerState) => { + state.isAccountTreeSyncingInProgress = false; + }, + ); + // Emit a span whenever the run changed something (local or remote), even // if it later threw - a failed sync that already did durable work is // exactly the kind of anomaly we want visible. No-op syncs (the common // per-login case) are not traced. The span is backdated so the real sync - // duration is preserved. + // duration is preserved. Tracing is best-effort: a trace failure must not + // affect the sync outcome or mask the sync's own error. if (mutationTracker?.hasOccurred()) { - await this.#context.traceFn( - { - name: TraceName.AccountSyncFull, - startTime, - }, - () => undefined, - ); + try { + await this.#context.traceFn( + { + name: TraceName.AccountSyncFull, + startTime, + }, + () => undefined, + ); + } catch (traceError) { + backupAndSyncLogger( + 'Failed to emit AccountSyncFull trace:', + traceError, + ); + } } - - // Always reset state, regardless of success or failure - this.#context.controllerStateUpdateFn( - (state: AccountTreeControllerState) => { - state.isAccountTreeSyncingInProgress = false; - }, - ); } } From e82188855c8680a380c5670024c4694e67e5c9c3 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 3 Jul 2026 14:04:00 +0200 Subject: [PATCH 5/5] fix: remove clutter --- packages/account-tree-controller/CHANGELOG.md | 2 +- .../src/backup-and-sync/service/index.test.ts | 28 +++++-------------- .../src/backup-and-sync/service/index.ts | 10 ------- .../src/backup-and-sync/types.ts | 5 ---- .../backup-and-sync/utils/mutation-tracker.ts | 4 +-- 5 files changed, 9 insertions(+), 40 deletions(-) diff --git a/packages/account-tree-controller/CHANGELOG.md b/packages/account-tree-controller/CHANGELOG.md index f5b66e0e10..54bcb8f868 100644 --- a/packages/account-tree-controller/CHANGELOG.md +++ b/packages/account-tree-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- The `Multichain Account Syncing - Full` trace is now only emitted when a backup-and-sync run actually mutates local or remote state (rolled-back local changes do not count), including runs that fail after doing durable work, and the span is backdated to preserve the real sync duration ([#9343](https://github.com/MetaMask/core/pull/9343)) +- The `Multichain Account Syncing - Full` trace is now only emitted when a backup-and-sync run actually mutates local or remote state (rolled-back local changes do not count) ([#9343](https://github.com/MetaMask/core/pull/9343)) - Bump `@metamask/keyring-api` from `^23.1.0` to `^23.3.0` ([#9249](https://github.com/MetaMask/core/pull/9249)) - Bump `@metamask/multichain-account-service` from `^11.0.0` to `^11.1.0` ([#9264](https://github.com/MetaMask/core/pull/9264)) diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts index 6ee2b19abd..f93f254fb2 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts @@ -7,7 +7,7 @@ import type { AccountWalletEntropyObject } from '../../wallet'; import { TraceName } from '../analytics'; import { getProfileId } from '../authentication'; import { performLegacyAccountSyncing, syncWalletMetadata } from '../syncing'; -import type { BackupAndSyncContext, SyncMutationTracker } from '../types'; +import type { BackupAndSyncContext } from '../types'; import { getAllGroupsFromUserStorage } from '../user-storage'; // We only need to import the functions we actually spy on import { createStateSnapshot, getLocalEntropyWallets } from '../utils'; @@ -39,25 +39,11 @@ const mockCreateStateSnapshot = createStateSnapshot as jest.MockedFunction< typeof createStateSnapshot >; -// Local tracker (the real factory lives in the auto-mocked `../utils`). -const createTestMutationTracker = (): SyncMutationTracker => { - let remoteWrite = false; - let localWrite = false; - return { - setRemoteWrite: (value: boolean): void => { - remoteWrite = value; - }, - getLocalWrite: (): boolean => localWrite, - setLocalWrite: (value: boolean): void => { - localWrite = value; - }, - hasOccurred: (): boolean => remoteWrite || localWrite, - reset: (): void => { - remoteWrite = false; - localWrite = false; - }, - }; -}; +// `jest.mock('../utils')` auto-mocks every export, including the tracker +// factory. Grab the real one so tests use the actual tracker behaviour rather +// than a hand-rolled duplicate. +const { createSyncMutationTracker } = + jest.requireActual('../utils'); describe('BackupAndSync - Service - BackupAndSyncService', () => { let mockContext: BackupAndSyncContext; @@ -95,7 +81,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }, traceFn: jest.fn().mockImplementation((_config, fn) => fn()), groupIdToWalletId: new Map(), - mutationTracker: createTestMutationTracker(), + mutationTracker: createSyncMutationTracker(), } as unknown as BackupAndSyncContext; // Default setup - backup and sync enabled diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.ts index 9ff53f025d..749a113c38 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.ts @@ -462,22 +462,12 @@ export class BackupAndSyncService { try { await bigSyncFn(); } finally { - // Always clear the in-progress flag first, regardless of success or - // failure. Doing this before the (awaited) trace below guarantees a - // failing trace can never leave the controller stuck mid-sync and block - // later full sync attempts. this.#context.controllerStateUpdateFn( (state: AccountTreeControllerState) => { state.isAccountTreeSyncingInProgress = false; }, ); - // Emit a span whenever the run changed something (local or remote), even - // if it later threw - a failed sync that already did durable work is - // exactly the kind of anomaly we want visible. No-op syncs (the common - // per-login case) are not traced. The span is backdated so the real sync - // duration is preserved. Tracing is best-effort: a trace failure must not - // affect the sync outcome or mask the sync's own error. if (mutationTracker?.hasOccurred()) { try { await this.#context.traceFn( diff --git a/packages/account-tree-controller/src/backup-and-sync/types.ts b/packages/account-tree-controller/src/backup-and-sync/types.ts index 541d38a450..7f6449c063 100644 --- a/packages/account-tree-controller/src/backup-and-sync/types.ts +++ b/packages/account-tree-controller/src/backup-and-sync/types.ts @@ -106,11 +106,6 @@ export type BackupAndSyncContext = { traceFn: TraceCallback; groupIdToWalletId: Map; emitAnalyticsEventFn: (event: BackupAndSyncEmitAnalyticsEventParams) => void; - /** - * Tracks whether the current sync run performed a real write (a local - * mutation or a remote push). Sync helpers set the relevant write flag; the - * service reads it to gate emission of the full-sync trace. - */ mutationTracker?: SyncMutationTracker; }; diff --git a/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts index a2db6a9821..aa0d3e74be 100644 --- a/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts +++ b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts @@ -2,9 +2,7 @@ import type { SyncMutationTracker } from '../types'; /** * Creates a tracker that records whether the current full sync run performed a - * real write. Remote writes (pushes to user storage) are durable; local writes - * can be reverted by a per-wallet rollback. Runs are serialized, so a single - * tracker is reset at the start of each run. + * real write. * * @returns A fresh mutation tracker. */