diff --git a/docs/plans/2026-05-08-sync-run-service-refactor.md b/docs/plans/2026-05-08-sync-run-service-refactor.md new file mode 100644 index 0000000000..d58084ef50 --- /dev/null +++ b/docs/plans/2026-05-08-sync-run-service-refactor.md @@ -0,0 +1,163 @@ +# Sync entry-point contract: replace `withSession()` + lint check with a runner + +**Status:** Proposal +**Motivates:** follow-up to #7330 +**Date:** 2026-05-08 + +## Background + +The #7330 fixes introduced `SyncSessionValidationService` — a session-scoped +latch that records whether post-sync state validation failed. The contract is: + +1. Every top-level sync entry point wraps its work in `latch.withSession(...)`. +2. Validation sites flip `latch.setFailed()` on corruption. +3. The entry point reads `latch.hasFailed()` once before claiming `IN_SYNC`. + +Today there are four entry points: `SyncWrapperService._sync()`, +`SyncWrapperService._forceDownload()`, `WsTriggeredDownloadService._downloadOps()`, +and (after this PR) `ImmediateUploadService._performUpload()`. + +Compliance is enforced by: + +- A docstring contract on `SyncSessionValidationService`. +- `tools/check-sync-session-entry-points.js`, which counts `.withSession(` + callers in production sources and asserts the count matches an + `ALLOWED_ENTRY_POINTS` allow-list. + +## The gap this proposal closes + +The lint check enumerates `withSession()` *callers*. It catches "someone +added a withSession call without updating the allow-list." It does **not** +catch the inverse: a new sync entry point that should call `withSession()` +but doesn't. + +`ImmediateUploadService._performUpload()` is the existence proof. It +predated the latch contract, called `OperationLogSyncService.uploadPendingOps()` +(which transitively calls `validateAfterSync()`), and was not in the +allow-list — so the lint check was silent. A validation failure on a +piggybacked op flipped the latch outside any session, logged a noisy +"outside-session" error, and was reset by the next normal `sync()`'s +`withSession()` entry. Meanwhile `_performUpload` set `IN_SYNC` based purely +on `result.uploadedCount > 0`. The exact #7330 surface — corrupt state + +`IN_SYNC` checkmark — was reachable. + +The fix in this PR wraps `_performUpload` in `withSession()` and adds it to +the allow-list. That closes this specific instance. The structural weakness +remains: a future contributor who adds a fifth entry point still has to +*remember* to call `withSession()`. The lint check still won't catch them. + +## Proposal: `SyncRunService.run(ctx => ...)` + +Replace the contract-based design with a type-enforced one. + +```ts +@Injectable({ providedIn: 'root' }) +export class SyncRunService { + private _latch = inject(SyncSessionValidationService); + private _providerManager = inject(SyncProviderManager); + + async run( + label: string, + work: (ctx: SyncRunContext) => Promise, + ): Promise { + return this._latch.withSession(async () => { + const ctx: SyncRunContext = { + markFailed: () => this._latch.setFailed(), + hasFailed: () => this._latch.hasFailed(), + // ... possibly more: setStatus, etc. + }; + try { + return await work(ctx); + } finally { + if (this._latch.hasFailed()) { + this._providerManager.setSyncStatus('ERROR'); + } + // (status decision could remain in callers if some need + // UNKNOWN_OR_CHANGED or deferred-checkmark semantics) + } + }); + } +} +``` + +The validation services (`RemoteOpsProcessingService.validateAfterSync`, +`ConflictResolutionService._validateAndRepairAfterResolution`) take a +`SyncRunContext` parameter and call `ctx.markFailed()` instead of injecting +`SyncSessionValidationService` directly. The constructor of +`SyncRunContext` is private to `SyncRunService` — no other code can produce +one. + +### What this enforces at compile time + +- A new sync entry point wanting to call `processRemoteOps()` / + `validateAfterSync()` must obtain a `SyncRunContext`, which can only come + from `SyncRunService.run()`. +- A new validation site wanting to flip the latch must take a context, which + forces it to be called from inside a run. +- Latch state is no longer reachable from arbitrary services — it lives + inside the runner. + +### What this does **not** solve + +- It doesn't prevent a contributor from creating a new entry point that calls + `runService.run()` with stale logic inside. It makes new entry points safe + by default; it doesn't make their *contents* correct. +- It doesn't change the underlying semantics — same latch, same validation, + same status decisions. + +## Migration sketch + +1. Add `SyncRunService` + `SyncRunContext`. Move the `withSession()` call + inside `run()`. +2. Make `SyncSessionValidationService` package-private (or merge it into + `SyncRunService` as a private field). Public surface shrinks. +3. Migrate the four entry points one at a time: + - `SyncWrapperService._sync()` (largest — wraps `_syncBody`) + - `SyncWrapperService._forceDownload()` + - `WsTriggeredDownloadService._downloadOps()` + - `ImmediateUploadService._performUpload()` +4. Change `validateAfterSync(callerHoldsLock)` to + `validateAfterSync(ctx, callerHoldsLock)`, ditto for the conflict-resolution + validation site. Plumb `ctx` through `processRemoteOps` and the LWW retry + loops. +5. Delete or simplify `tools/check-sync-session-entry-points.js`. Keep a + smaller version that asserts no new direct callers of + `SyncSessionValidationService` outside the runner appear, if useful. +6. Adjust tests: the existing pattern `latch.setFailed()` in mocks becomes + `ctx.markFailed()` via the runner's callback; `latch._resetForTest()` + moves into the runner. + +## Risk and effort + +- **Risk:** medium. Sync paths are sensitive; `SyncWrapperService._sync()` + has substantial state-machine logic (LWW retry loops, USE_REMOTE branch, + retry-exhaustion priority). Migration touches the wrapper. +- **Effort:** ~0.5–1.5 days including tests. +- **Payoff:** prevents the class of bug found in this review (a new entry + point that forgets to wrap). Reduces conceptual surface from "audit all + call chains for `setFailed`/`hasFailed`/`withSession`" to "audit one + boundary: `SyncRunService.run()`." + +## Why not now + +- This branch is already deep in #7330 review. The latch refactor in + `b3cbdbd41` was itself a pivot from typed-return plumbing. Stacking a + second structural pivot on top adds review surface and risk. +- The immediate fix (this PR) closes the user-visible bug. The runner + refactor is value-over-time, not a now-problem. +- Doing the runner refactor as a standalone PR lets it be reviewed on its + merits without sync-correctness pressure. + +## Acceptance criteria + +- All four current entry points call `runService.run(...)`; none call + `latch.withSession()` directly. +- `SyncSessionValidationService` is no longer publicly injectable, or its + public methods are no longer called outside `SyncRunService`. +- `validateAfterSync` and the conflict-resolution validation path take a + `SyncRunContext` parameter; calling them without one is a compile error. +- The static check in `tools/` is either deleted or reduced to a one-line + assertion ("no production code outside `SyncRunService` references + `SyncSessionValidationService`"). +- Existing test coverage continues to pass; new tests assert the runner's + behavior end-to-end. diff --git a/src/app/op-log/sync/immediate-upload.service.spec.ts b/src/app/op-log/sync/immediate-upload.service.spec.ts index ff83705446..852beb02f8 100644 --- a/src/app/op-log/sync/immediate-upload.service.spec.ts +++ b/src/app/op-log/sync/immediate-upload.service.spec.ts @@ -5,6 +5,7 @@ import { OperationLogSyncService } from './operation-log-sync.service'; import { SyncProviderId } from '../sync-providers/provider.const'; import { DataInitStateService } from '../../core/data-init/data-init-state.service'; import { SyncWrapperService } from '../../imex/sync/sync-wrapper.service'; +import { SyncSessionValidationService } from './sync-session-validation.service'; import { BehaviorSubject } from 'rxjs'; import { RejectedOpInfo } from '../core/types/sync-results.types'; @@ -166,6 +167,110 @@ describe('ImmediateUploadService', () => { })); }); + // #7330 follow-up: uploadPendingOps() processes piggybacked remote ops + // through validateAfterSync(). Without an explicit withSession() wrapper + // the latch flip would either fire outside any session (silently dropped + // by the next normal sync's reset) or — worse — go unread while + // _performUpload set IN_SYNC based purely on result.uploadedCount. + describe('post-sync validation (#7330 latch)', () => { + it('reports ERROR (not IN_SYNC) when validation fails during piggybacked-op processing', fakeAsync(() => { + const latch = TestBed.inject(SyncSessionValidationService); + mockSyncService.uploadPendingOps.and.callFake(async () => { + // Simulate post-sync validation flipping the latch from inside + // uploadPendingOps -> processRemoteOps -> validateAfterSync. + latch.setFailed(); + return completedResult({ uploadedCount: 2, piggybackedOpsCount: 1 }); + }); + + service.initialize(); + service.trigger(); + tick(2100); + + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('ERROR'); + expect(mockProviderManager.setSyncStatus).not.toHaveBeenCalledWith('IN_SYNC'); + })); + + it('reports ERROR when validation fails during a clean upload (no piggyback)', fakeAsync(() => { + const latch = TestBed.inject(SyncSessionValidationService); + mockSyncService.uploadPendingOps.and.callFake(async () => { + latch.setFailed(); + return completedResult({ uploadedCount: 3 }); + }); + + service.initialize(); + service.trigger(); + tick(2100); + + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('ERROR'); + expect(mockProviderManager.setSyncStatus).not.toHaveBeenCalledWith('IN_SYNC'); + })); + + it('reports ERROR when validation fails on the LWW re-upload pass', fakeAsync(() => { + const latch = TestBed.inject(SyncSessionValidationService); + let call = 0; + mockSyncService.uploadPendingOps.and.callFake(async () => { + call += 1; + if (call === 1) { + // First pass: LWW created local-win ops; no validation failure yet. + return completedResult({ uploadedCount: 1, localWinOpsCreated: 2 }); + } + // Re-upload pass: validation fails (e.g., on piggybacked ops returned + // alongside the re-upload). + latch.setFailed(); + return completedResult({ uploadedCount: 0 }); + }); + + service.initialize(); + service.trigger(); + tick(2100); + + expect(mockSyncService.uploadPendingOps).toHaveBeenCalledTimes(2); + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('ERROR'); + expect(mockProviderManager.setSyncStatus).not.toHaveBeenCalledWith('IN_SYNC'); + })); + + it('resets the latch on each immediate-upload session', fakeAsync(() => { + const latch = TestBed.inject(SyncSessionValidationService); + // Seed stale state via the test-only helper (mirrors "a prior session + // left the latch flipped"). setFailed() outside a session would log a + // warning we don't want in test output. + latch._resetForTest(); + (latch as unknown as { _failed: boolean })._failed = true; + + mockSyncService.uploadPendingOps.and.returnValue( + Promise.resolve(completedResult({ uploadedCount: 1 })), + ); + + service.initialize(); + service.trigger(); + tick(2100); + + // After withSession's entry-reset and a clean upload, the latch is + // back to false and IN_SYNC is reported normally. + expect(latch.hasFailed()).toBe(false); + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('IN_SYNC'); + expect(mockProviderManager.setSyncStatus).not.toHaveBeenCalledWith('ERROR'); + })); + + it('reports ERROR when validation flipped the latch and the upload then threw', fakeAsync(() => { + const latch = TestBed.inject(SyncSessionValidationService); + mockSyncService.uploadPendingOps.and.callFake(async () => { + latch.setFailed(); + throw new Error('Network error after validation failure'); + }); + + service.initialize(); + service.trigger(); + tick(2100); + + // Validation failure is structural state corruption — surface it + // even though the upload itself threw. Transient errors with no + // latch flip remain silent (existing 'should NOT show checkmark when + // upload fails' test). + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('ERROR'); + })); + }); + describe('guards', () => { it('should skip upload when sync is in progress', fakeAsync(() => { // Need to re-create the mock with isSyncInProgress = true diff --git a/src/app/op-log/sync/immediate-upload.service.ts b/src/app/op-log/sync/immediate-upload.service.ts index 38a0e2ec52..442e18d160 100644 --- a/src/app/op-log/sync/immediate-upload.service.ts +++ b/src/app/op-log/sync/immediate-upload.service.ts @@ -9,6 +9,7 @@ import { OpLog } from '../../core/log'; import { DataInitStateService } from '../../core/data-init/data-init-state.service'; import { handleStorageQuotaError } from './sync-error-utils'; import { SyncWrapperService } from '../../imex/sync/sync-wrapper.service'; +import { SyncSessionValidationService } from './sync-session-validation.service'; const IMMEDIATE_UPLOAD_DEBOUNCE_MS = 2000; @@ -51,6 +52,7 @@ export class ImmediateUploadService implements OnDestroy { private _syncService = inject(OperationLogSyncService); private _dataInitStateService = inject(DataInitStateService); private _syncWrapper = inject(SyncWrapperService); + private _sessionValidation = inject(SyncSessionValidationService); private _uploadTrigger$ = new Subject(); private _subscription: Subscription | null = null; @@ -166,6 +168,18 @@ export class ImmediateUploadService implements OnDestroy { * - Handling of rejected ops * * Note: This is only called for SuperSync (file-based providers are filtered in _canUpload) + * + * ## Session boundary (#7330) + * + * uploadPendingOps() processes piggybacked remote ops, which run + * post-sync validation (`RemoteOpsProcessingService.validateAfterSync`). + * On corruption, validation flips the SyncSessionValidationService latch. + * Without an explicit `withSession()` wrapper here, the latch flip would + * either fire outside any session (logged as a contract violation and + * silently dropped by the next normal sync's reset) or — worse — go + * unread while `_performUpload` claimed `IN_SYNC` based purely on + * `result.uploadedCount`. That reproduces the exact #7330 surface on + * the immediate-upload path. */ private async _performUpload(): Promise { const provider = this._providerManager.getActiveProvider(); @@ -183,65 +197,86 @@ export class ImmediateUploadService implements OnDestroy { const syncCapableProvider = provider as unknown as import('../sync-providers/provider.interface').OperationSyncCapable; - try { - OpLog.verbose('ImmediateUploadService: Starting immediate upload...'); + return this._sessionValidation.withSession(async () => { + try { + OpLog.verbose('ImmediateUploadService: Starting immediate upload...'); - // Use sync service's uploadPendingOps which includes migration detection callback. - // This ensures SYNC_IMPORT is created when switching to a new/empty server. - const result = await this._syncService.uploadPendingOps(syncCapableProvider); - if (result.kind === 'blocked_fresh_client') { - OpLog.verbose('ImmediateUploadService: Upload blocked (fresh client)'); - return; - } + // Use sync service's uploadPendingOps which includes migration detection callback. + // This ensures SYNC_IMPORT is created when switching to a new/empty server. + const result = await this._syncService.uploadPendingOps(syncCapableProvider); + if (result.kind === 'blocked_fresh_client') { + OpLog.verbose('ImmediateUploadService: Upload blocked (fresh client)'); + return; + } - if (result.kind === 'cancelled') { - OpLog.verbose( - 'ImmediateUploadService: Upload cancelled (piggybacked SYNC_IMPORT conflict)', + if (result.kind === 'cancelled') { + OpLog.verbose( + 'ImmediateUploadService: Upload cancelled (piggybacked SYNC_IMPORT conflict)', + ); + return; + } + + // result.kind === 'completed' from here + + // If LWW local-wins created new update ops from piggybacked ops, + // do a follow-up upload to push them to the server immediately + if (result.localWinOpsCreated > 0) { + OpLog.verbose( + `ImmediateUploadService: LWW created ${result.localWinOpsCreated} local-win op(s), re-uploading`, + ); + await this._syncService.uploadPendingOps(syncCapableProvider); + } + + // Read the validation latch BEFORE any IN_SYNC / deferred-checkmark + // decision. A failure during piggybacked-op processing (or the + // re-upload above) is otherwise lost when the next normal sync + // resets the latch on entry. + if (this._sessionValidation.hasFailed()) { + OpLog.err( + 'ImmediateUploadService: Post-sync validation failed during immediate upload — reporting ERROR', + ); + this._providerManager.setSyncStatus('ERROR'); + return; + } + + // Don't show checkmark when piggybacked ops exist - there may be more + // remote ops pending. Let normal sync cycle confirm full sync state. + if (result.piggybackedOpsCount > 0) { + OpLog.verbose( + `ImmediateUploadService: Uploaded ${result.uploadedCount} ops, ` + + `processed ${result.piggybackedOpsCount} piggybacked (checkmark deferred)`, + ); + return; + } + + // Show checkmark ONLY when server confirms no pending remote ops + // (empty piggybackedOps means we're confirmed in sync) + if (result.uploadedCount > 0 || result.localWinOpsCreated > 0) { + this._providerManager.setSyncStatus('IN_SYNC'); + OpLog.verbose( + `ImmediateUploadService: Uploaded ${result.uploadedCount} ops, confirmed in sync`, + ); + } + } catch (e) { + // Check for storage quota exceeded - this requires user action + const message = e instanceof Error ? e.message : 'Unknown error'; + handleStorageQuotaError(message); + + // Validation failure is structural state corruption, not a transient + // network/throttle error — surface ERROR even though the upload + // itself threw afterward. + if (this._sessionValidation.hasFailed()) { + this._providerManager.setSyncStatus('ERROR'); + } + + // Silent failure for other errors - normal sync will pick up pending ops + OpLog.warn( + 'ImmediateUploadService: Immediate upload failed, will retry on normal sync', + e, ); - return; + // Don't emit ERROR state for non-validation failures - transient failures are expected } - - // result.kind === 'completed' from here - - // If LWW local-wins created new update ops from piggybacked ops, - // do a follow-up upload to push them to the server immediately - if (result.localWinOpsCreated > 0) { - OpLog.verbose( - `ImmediateUploadService: LWW created ${result.localWinOpsCreated} local-win op(s), re-uploading`, - ); - await this._syncService.uploadPendingOps(syncCapableProvider); - } - - // Don't show checkmark when piggybacked ops exist - there may be more - // remote ops pending. Let normal sync cycle confirm full sync state. - if (result.piggybackedOpsCount > 0) { - OpLog.verbose( - `ImmediateUploadService: Uploaded ${result.uploadedCount} ops, ` + - `processed ${result.piggybackedOpsCount} piggybacked (checkmark deferred)`, - ); - return; - } - - // Show checkmark ONLY when server confirms no pending remote ops - // (empty piggybackedOps means we're confirmed in sync) - if (result.uploadedCount > 0 || result.localWinOpsCreated > 0) { - this._providerManager.setSyncStatus('IN_SYNC'); - OpLog.verbose( - `ImmediateUploadService: Uploaded ${result.uploadedCount} ops, confirmed in sync`, - ); - } - } catch (e) { - // Check for storage quota exceeded - this requires user action - const message = e instanceof Error ? e.message : 'Unknown error'; - handleStorageQuotaError(message); - - // Silent failure for other errors - normal sync will pick up pending ops - OpLog.warn( - 'ImmediateUploadService: Immediate upload failed, will retry on normal sync', - e, - ); - // Don't emit ERROR state - transient failures are expected - } + }); } ngOnDestroy(): void { diff --git a/tools/check-sync-session-entry-points.js b/tools/check-sync-session-entry-points.js index 90c184b179..961950bf8d 100644 --- a/tools/check-sync-session-entry-points.js +++ b/tools/check-sync-session-entry-points.js @@ -2,15 +2,22 @@ /** * Static check: enumerates all production callers of * `SyncSessionValidationService.withSession()` and asserts the set is - * exactly the four known sync entry points. + * exactly the known sync entry points. * * Why brittle on purpose: the latch contract requires every top-level sync - * entry point to wrap its work in `withSession()`. Adding a 5th entry point + * entry point to wrap its work in `withSession()`. Adding a new entry point * (e.g., a new background download path) without doing so is the * maintenance hazard #7330 is most exposed to. This script fails if a new * `withSession()` caller appears, forcing the contributor to read the * service-level contract before adding to the allow-list here. * + * Known gap (see docs/plans/2026-05-08-sync-run-service-refactor.md): + * this check enumerates `withSession()` callers — it catches "added a + * withSession call without updating the list" but not the inverse, "added + * a sync entry point that *should* call withSession() but doesn't." The + * runner refactor proposed in that plan would replace this lint with a + * type-enforced contract. + * * Usage: `node tools/check-sync-session-entry-points.js` * Wired into `npm run lint` so CI catches drift without extra steps. */ @@ -30,6 +37,7 @@ const ALLOWED_ENTRY_POINTS = [ 'SyncWrapperService._sync() — top-level user-initiated sync', 'SyncWrapperService._forceDownload() — user-initiated force download', 'WsTriggeredDownloadService._downloadOps() — realtime WS-triggered download', + 'ImmediateUploadService._performUpload() — debounced post-edit upload (SuperSync)', ]; // Files that may legitimately contain `withSession(` references in production @@ -38,6 +46,7 @@ const ALLOWED_ENTRY_POINTS = [ const SCANNED_FILES = [ 'src/app/imex/sync/sync-wrapper.service.ts', 'src/app/op-log/sync/ws-triggered-download.service.ts', + 'src/app/op-log/sync/immediate-upload.service.ts', 'src/app/op-log/sync/operation-log-sync.service.ts', 'src/app/op-log/sync/conflict-resolution.service.ts', 'src/app/op-log/sync/remote-ops-processing.service.ts',