fix(sync): wrap ImmediateUploadService._performUpload in withSession()

Fourth #7330 entry point. uploadPendingOps() processes piggybacked
remote ops, which run 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, 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.

Wrap _performUpload's body in latch.withSession(...) and read
hasFailed() before any IN_SYNC / deferred-checkmark decision. On
failure (including a thrown upload after the latch flipped) emit
ERROR; transient errors with no latch flip remain silent.

Allow-list: ImmediateUploadService._performUpload added to
tools/check-sync-session-entry-points.js (now 4 entry points).

Tests: 5 new specs cover failure during piggybacked-op processing,
clean upload, LWW re-upload pass, latch reset between sessions, and
upload-throws-after-latch-flip.

Plan: docs/plans/2026-05-08-sync-run-service-refactor.md proposes a
type-enforced runner to replace the contract+lint pair. Deferred —
this PR closes the user-visible bug; the runner refactor is
value-over-time.

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<T>(
+    label: string,
+    work: (ctx: SyncRunContext) => Promise<T>,
+  ): Promise<T> {
+    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.

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

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<void>();
   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<void> {
     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 {

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',