fix(sync): freeze the conflict-review producers before the next release (#9061)

* fix(sync): freeze the conflict-review producers before the next release

The conflict-review feature (962c5bbeb1) is on master but in no release
tag, so a release cut would take it to the whole stable fleet while it is
still slated for removal. The Task 1 audit found it already ships to two
pre-release cohorts: Snap `edge` is public and built from every master
push (snapd auto-refreshes subscribers), and the Play `internal` track
auto-updates its testers. The data obligation therefore already exists —
the next tag would expand it, not create it.

Journal rows persist the discarded side of a conflict verbatim (entity
titles, arbitrary field values, raw action payloads). They are
device-local, absent from backups/exports, never uploaded, and expire at
14 days / 200 rows, so the recorded decision is: no export owed, and the
store, reader, UI and the SUP_CONFLICT_JOURNAL_CLEARED_BEFORE marker get
deleted together in the rollback.

Freeze both producers at the single production entry point into
autoResolveConflictsLWW:

- disableDisjointMerge: conflicts resolve by whole-entity LWW, which is
  the behaviour of every released version to date, so stable gains no
  merge behaviour it would later have to be migrated off.
- disableConflictJournal: no new rows are persisted. record() is the
  journal's only write and both call sites are now gated, so nothing
  produces rows behind the freeze.

Wired at the caller rather than as a global flag so ConflictResolution-
Service keeps the capability for its own tests and the freeze reverts by
deleting two lines. Covered by a spec that fails if either flag is
dropped, since that regression is otherwise silent.

Records the audit in docs/plans/2026-07-16-conflict-review-cohort-audit.md.

* fix(sync): don't offer conflict REVIEW when the journal is empty

The content-conflict banner (#8694) fires off the resolutions, not off the
journal, and its SPAP-15 REVIEW action was unconditional. With the producer
freeze the journal holds no rows, so REVIEW landed the user on the review
page's empty state — a dead end at the exact moment they are worried about a
discarded edit. v18.14.0 ships this banner WITHOUT the action (it has no
navigateToReview at all), so the next tag would have taken a permanently
broken affordance to the whole stable fleet — the opposite of what the freeze
is for.

Gate the action on the journal actually holding unreviewed rows. The count
signal reads fresh: the journal loop awaits record() (which refreshes it)
before the notification step. This also fixes a latent case that predates the
freeze — _journalResolution swallows record() failures by contract, so a
transient IndexedDB error already produced the same dead end. Mirrors
maybeShowSummaryBanner, which already stays silent on an empty journal.

Also cover the freeze's callee-side gates, which had none: the guard spec in
remote-ops-processing.service.spec.ts only asserts the production caller
PASSES the flags. Deleting `if (!options.disableConflictJournal)` in
autoResolveConflictsLWW left all 292 specs green while the fleet silently
resumed persisting the discarded side of every conflict. Both new gates are
falsification-verified: each mutation fails exactly its own spec.

* refactor(sync): simplify the conflict REVIEW action gate

Use a plain ternary to undefined instead of a conditional spread: the banner
component truthy-checks `@if (banner.action)` and exactOptionalPropertyTypes
is off, so `action: undefined` is identical to omitting the key — and reads
as the same thing. Trim the rationale comment to the load-bearing why.

No behaviour change; the gate's spec is unchanged and still passes.

* fix(sync): give the integration spec's journal mock its count signal

The content-conflict banner now reads `conflictJournal.unreviewedCount()` to
decide whether to offer REVIEW, but conflict-resolution-persistence.integration
.spec.ts mocks ConflictJournalService with `['record']` only, so resolution threw
`unreviewedCount is not a function` and took the #8997 project-recovery test down
with it.

Provide a real `signal(0)` via createSpyObj's properties arg rather than a spy:
unreviewedCount is a signal on the service, so this keeps the mock honest to the
type it stands in for.

Caught by CI, not locally: the earlier run covered only the four spec files the
change touched, and this mock lives in a fifth. Full suite now green, 12928 +
12914 across both TZ variants.

* test(sync): drop the redundant journal-on control test

'journals resolutions by default' duplicated coverage that already exists: the
#8956 multi-entity test asserts record() is called twice on the default path, so
it already fails if journaling breaks entirely — which was the only thing my
control guarded against (the freeze test passing vacuously).

Collapse the leftover one-test describe and its two single-use helpers back into
a flat spec.

Full suite green: 12927 + 12913 across both TZ variants.
This commit is contained in:
Johannes Millan 2026-07-16 14:43:33 +02:00 committed by GitHub
parent 011a92585d
commit 71a9a4338a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 296 additions and 12 deletions

View file

@ -0,0 +1,81 @@
# Conflict-review cohort and persisted-data audit (plan Task 1)
**Date:** 2026-07-16
**Scope:** Task 1 of [`2026-07-13-sync-simplification-plan.md`](2026-07-13-sync-simplification-plan.md). Blocks the conflict-review rollback (Task 6) and authorizes the producer freeze.
**Baseline:** master `6f88775ea2`. Feature under audit: conflict review / conflict journal, merged `962c5bbeb1` (PR #8874, 2026-07-11).
## Decision summary
| Question | Decision |
| ----------------------------------------------------- | --------------------------------------------------------------------------------------------------- |
| Producer freeze before the next release cut? | **Yes** — freeze both producers now (landed with this document). |
| Are Snap `edge` / Play `internal` supported cohorts? | **No** — dogfood/pre-release. No export or migration obligation. |
| Journal retention / export / deletion policy | **No export.** Rely on the existing 14-day / 200-row expiry; delete store, reader, UI and marker together in Task 6. |
These are product decisions, recorded explicitly rather than inferred from release tags, per the Task 1 acceptance criteria.
## 1. Distribution channels carrying `962c5bbeb1`
`git tag --contains 962c5bbeb1` matches no release tag (only the `issue-8983-verbose` working tag). It is on `master`. v18.14.0 was cut 2026-07-10 and does not contain it.
| Channel | Trigger | From master? | Public? | Evidence |
| ------------------------------ | ---------------------- | ------------ | ------------------------ | ----------------------------------------------------------- |
| **Snap Store `edge`** | every push to `master` | **Yes** | **Yes — unauthenticated** | `.github/workflows/build.yml:2-6`, `:174-191` |
| **Google Play `internal`** | every push to `master` | **Yes** | Opt-in testers (Play caps at 100) | `.github/workflows/build-android.yml:135-150` |
| GHCR `supersync:latest` | push to `master` | Yes | Server image only — no review UI | `.github/workflows/supersync-docker.yml:3-14` |
| GitHub Release (desktop) | tag `v*` | No | Yes | `build.yml:136-141`, `:515-524` |
| Web app | `release: published`, non-prerelease | No | Yes | `build-update-web-app-on-release.yml:3-4`, `:11` |
| Play production / iOS / stores | tag `v*` | No | Yes | `build-android.yml:190-198`, `build-ios.yml:2-7` |
| Cloudflare Pages preview | `pull_request` | No | Yes (URL in PR) | `pr-preview-build.yml:3-6` |
**Two cohorts already run the feature today.** Snap `edge` is the material one: it is public, requires no invitation, and snapd auto-refreshes subscribers. The Play `internal` track auto-updates its testers on-device by design (`build-android.yml:129-134`). Subscriber counts for both live in Snap Store / Play Console telemetry and are not knowable from the repo.
**Not exposed:** Electron desktop ships **no auto-updater** (`electron-builder.yaml:65-70`; the `autoUpdater` block in `electron/start-app.ts:474-486` is commented out) and master builds use `--publish never`; the web app deploys only on published non-prerelease releases; there is no nightly/canary release channel.
**Consequence:** the persisted-data obligation began at the first master push after `962c5bbeb1`, not at a future tag. The next release cut does not *create* the obligation — it expands it from these two pre-release cohorts to the entire stable fleet, which is what the freeze prevents.
## 2. Stable baseline vs master
- v18.14.0 (deployed stable): schema **v2**, op-log DB **v7**.
- master: schema **v4**, op-log DB **v10**.
- Both the v2→v3 replace/patch barrier and the v3→v4 marked-project-delete barrier are unreleased.
Unless reverted before the next tag, schema v3 compatibility, schema v4 delete-wins behaviour, and conflict review reach stable **together**. The deployed stable fleet stays v2/DB 7 until that cut.
## 3. What the journal persists
`ConflictJournalEntry` (`src/app/op-log/sync/conflict-journal.model.ts:98-112`) stores `entityTitle` plus `fieldDiffs` (`:66-93`), whose `localVal`/`remoteVal` hold **arbitrary entity field values copied verbatim** from op payloads — by design: "capture the discarded (losing) side of a conflict verbatim" (`:8-10`). There is no field allowlist; `NOISE_FIELDS` affects classification only, not storage. `kind: 'action'` diffs persist raw action payloads (`:85-92`) and are the widest content surface.
So rows contain **real user content** (task/note/project titles and discarded field values, including note bodies).
The blast radius is nonetheless small:
- **Device-local only.** Standalone IndexedDB `SUP_CONFLICT_JOURNAL` v1, store `conflicts` (`model.ts:181-185`), deliberately separate from the op-log `SUP_OPS` DB.
- **Never uploaded.** No sync/upload path reads the journal; sync code only calls `record()` and `clearAll()`. The only readers are UI.
- **Not in backups or exports.** `BackupService.loadCompleteBackup` builds solely from NgRx `AppDataComplete`, and the journal is not in NgRx; the DB constants appear nowhere outside the journal's own files.
- **Self-expiring.** `JOURNAL_RETENTION_DAYS = 14`, `JOURNAL_MAX_ENTRIES = 200`, pruned by `pruneOnStart()` (APP_INITIALIZER, `main.ts:313-321`) and opportunistically in `record()` above 220 rows.
**Known gap (accepted):** the 14-day age bound is enforced only on app start or when the row count crosses 220, so an always-on desktop can hold rows past 14 days, bounded at ~220 rows. With the writer frozen no new rows accrue, and the next app start prunes the rest.
**Known gap (accepted):** there is no user-facing way to clear the journal — the review page offers only keep/flip. `clearAll()` is reachable only via dataset replacement (`backup.service.ts:183`) or raw op-log rebuild (`operation-log-sync.service.ts:2174`). Adding a clear button was rejected: it grows UI surface on a feature slated for deletion, and with the writer frozen the content expires on its own.
## 4. Retention decision
No export path is owed, because both carrying cohorts are pre-release (§1) and the data never left the device (§3).
Task 6 must delete the writer, store, reader, UI, route, banner, badge **and** the `SUP_CONFLICT_JOURNAL_CLEARED_BEFORE` localStorage marker together. The marker is a cross-profile privacy fail-safe (#9045): `clearAll()` swallows IndexedDB errors, so if `db.clear()` fails the rows physically survive, and the marker is what hides them from every read path until `pruneOnStart()` reclaims them. **It must not be stranded** — a marker left behind with the store deleted protects nothing, and a store left behind with the marker deleted exposes profile A's titles to profile B.
Deleting the journal's IndexedDB is itself part of Task 6: dropping the store code without deleting `SUP_CONFLICT_JOURNAL` would leave user content on disk with no code path to reach or prune it. A live constraint already recorded in-code (`conflict-journal.service.ts:68-70`) is that any future "reset app data" flow clearing localStorage must also clear the journal DB.
## 5. What this audit does not authorize
Per the Task 1 stop condition, this audit authorizes **removal of producers only**. It does not authorize schema downgrade, nor reader removal, while supported stored data remains possible. Task 6 remains gated on the preserve list in the plan (schema v3/v4 barriers, delete-wins, #9048 cascade recovery, #9035 clientId tiebreak, #9025 LWW projectId sanitization, #9045 decrypt-path footprint auth).
## 6. Action taken
The producer freeze landed with this document — see `remote-ops-processing.service.ts`, the single production entry point into `autoResolveConflictsLWW`:
- `disableDisjointMerge: true` — conflicts resolve by whole-entity LWW, the behaviour of every released version to date, so the stable fleet gains no merge behaviour it would later be migrated off.
- `disableConflictJournal: true` — no new rows are persisted.
Both are caller-wired rather than global, so `ConflictResolutionService` keeps the capability intact for its own tests and the freeze reverts by deleting two lines. Rows already written on edge/internal builds stay readable and expire on their own; the full rollback proceeds on its own schedule in Task 6.

View file

@ -1,3 +1,4 @@
import { signal } from '@angular/core';
import { TestBed } from '@angular/core/testing';
import { Action, ActionReducer, Store } from '@ngrx/store';
import { of } from 'rxjs';
@ -242,6 +243,9 @@ describe('ConflictResolutionService persistence (integration, real store)', () =
const conflictJournal = jasmine.createSpyObj<ConflictJournalService>(
'ConflictJournalService',
['record'],
// Real signal, not a spy: `unreviewedCount` is a signal on the service, and
// the content-conflict banner reads it to decide whether to offer REVIEW.
{ unreviewedCount: signal(0) },
);
conflictJournal.record.and.resolveTo();
const syncConflictBanner = jasmine.createSpyObj<SyncConflictBannerService>(

View file

@ -11,6 +11,7 @@ import { OperationLogStoreService } from '../persistence/operation-log-store.ser
import { SnackService } from '../../core/snack/snack.service';
import { BannerService } from '../../core/banner/banner.service';
import { BannerId } from '../../core/banner/banner.model';
import { T } from '../../t.const';
import { ValidateStateService } from '../validation/validate-state.service';
import { of } from 'rxjs';
import {
@ -600,6 +601,114 @@ describe('ConflictResolutionService', () => {
expect(mockSnackService.open).not.toHaveBeenCalled();
});
describe('content-conflict banner REVIEW action', () => {
// This banner fires off the resolutions, NOT off the journal, so its
// REVIEW action has to be gated on the journal actually holding rows —
// otherwise the producer freeze (or a swallowed record() failure) sends
// the user to an empty review page at the exact moment they are worried
// about a discarded edit.
const buildContentLossConflicts = (): EntityConflict[] => {
const now = Date.now();
return [
createConflict(
'task-1',
[
{
...createOpWithTimestamp('local-1', 'client-a', now - 1000),
payload: {
actionPayload: {
task: { id: 'task-1', changes: { title: 'My local title' } },
},
entityChanges: [],
},
},
],
[
{
...createOpWithTimestamp('remote-1', 'client-b', now),
payload: {
actionPayload: {
task: { id: 'task-1', changes: { title: 'Remote title' } },
},
entityChanges: [],
},
},
],
),
];
};
const getJournal = (): ConflictJournalService =>
(service as unknown as { conflictJournal: ConflictJournalService })
.conflictJournal;
const openContentBanner = async (): Promise<jasmine.Spy<BannerService['open']>> => {
const openBannerSpy = spyOn(TestBed.inject(BannerService), 'open');
mockStore.select.and.returnValue(of({ id: 'task-1', title: 'Remote title' }));
const conflicts = buildContentLossConflicts();
mockOperationApplier.applyOperations.and.resolveTo({
appliedOps: conflicts[0].remoteOps,
});
await service.autoResolveConflictsLWW(conflicts);
return openBannerSpy;
};
it('omits the REVIEW action when the journal has nothing to review', async () => {
// record() no-ops, so the unreviewed count stays 0 — the state produced
// both by the producer freeze and by a swallowed record() failure.
spyOn(getJournal(), 'record').and.resolveTo();
const openBannerSpy = await openContentBanner();
const banner = openBannerSpy.calls.mostRecent().args[0];
expect(banner.id).toBe(BannerId.SyncConflictContentResolved);
// Message + built-in dismiss = exactly the released v18.14.0 banner.
expect(banner.action).toBeUndefined();
});
it('keeps the REVIEW action when the journal holds unreviewed entries', async () => {
spyOn(getJournal(), 'record').and.resolveTo();
(
getJournal() as unknown as {
_unreviewedCount: { set: (v: number) => void };
}
)._unreviewedCount.set(2);
const openBannerSpy = await openContentBanner();
const banner = openBannerSpy.calls.mostRecent().args[0];
expect(banner.action?.label).toBe(T.F.SYNC.CONFLICT_REVIEW.BANNER_REVIEW);
});
});
// Callee half of the producer freeze: remote-ops-processing.service.spec.ts
// asserts the production caller PASSES the flag, but without this, deleting
// the gate inside autoResolveConflictsLWW leaves every spec green while the
// fleet silently resumes persisting the discarded side of every conflict.
// The journal-ON default is already covered by the #8956 multi-entity test.
it('records nothing when disableConflictJournal is set (producer freeze)', async () => {
const journal = (service as unknown as { conflictJournal: ConflictJournalService })
.conflictJournal;
const recordSpy = spyOn(journal, 'record').and.resolveTo();
const now = Date.now();
const conflicts = [
createConflict(
'task-1',
[createOpWithTimestamp('local-1', 'client-a', now - 1000)],
[createOpWithTimestamp('remote-1', 'client-b', now)],
),
];
mockOperationApplier.applyOperations.and.resolveTo({
appliedOps: conflicts[0].remoteOps,
});
await service.autoResolveConflictsLWW(conflicts, [], {
disableConflictJournal: true,
});
expect(recordSpy).not.toHaveBeenCalled();
});
it('escapes task titles before putting them in the innerHTML banner (XSS guard)', async () => {
const bannerService = TestBed.inject(BannerService);
const openBannerSpy = spyOn(bannerService, 'open');

View file

@ -124,6 +124,22 @@ interface ResolvedConflicts {
interface AutoResolveConflictsLwwOptions {
callerHoldsOperationLogLock?: boolean;
disableDisjointMerge?: boolean;
/**
* Skip conflict-journal emission entirely (observe-only hook, so this can
* never change which op resolution picks).
*
* Set by the production caller as a PRODUCER FREEZE ahead of the
* conflict-review rollback: journal rows capture the discarded side of a
* conflict verbatim (titles, arbitrary field values), and that device-local
* data obligation must not expand to the stable fleet on the next release
* tag while the feature is still slated for removal.
*
* Ceiling: rows already written on edge/internal builds stay readable and
* expire on their own (14 days / 200 rows). Upgrade path: the store, reader,
* UI and the `SUP_CONFLICT_JOURNAL_CLEARED_BEFORE` marker are deleted
* together in the conflict-review rollback, after which this option goes too.
*/
disableConflictJournal?: boolean;
remoteApplyLifecycleOwnedByCaller?: boolean;
}
@ -1720,12 +1736,14 @@ export class ConflictResolutionService {
);
}
for (const plan of lwwPlans) {
await this._journalResolution(plan);
}
for (const merged of successfulMergedResolutions) {
if (writtenMergedOpIds.has(merged.mergedOp.id)) {
await this._journalMergedResolution(merged.plan);
if (!options.disableConflictJournal) {
for (const plan of lwwPlans) {
await this._journalResolution(plan);
}
for (const merged of successfulMergedResolutions) {
if (writtenMergedOpIds.has(merged.mergedOp.id)) {
await this._journalMergedResolution(merged.plan);
}
}
}
@ -1816,16 +1834,25 @@ export class ConflictResolutionService {
const named = labels.join(', ');
const taskList = contentConflicts.length > MAX_NAMED ? `${named}` : named;
// This banner fires off the resolutions, not off the journal, so REVIEW must
// be gated on the journal actually holding rows: under the producer freeze
// (or a swallowed `record()` failure) it would otherwise land the user on the
// review page's empty state. Without the action this is exactly the released
// v18.14.0 banner — message + built-in dismiss (no action2). Count is fresh:
// the journal loop awaits `record()`, which refreshes it, before this step.
const hasEntriesToReview = this.conflictJournal.unreviewedCount() > 0;
this.bannerService.open({
id: BannerId.SyncConflictContentResolved,
ico: 'sync_problem',
msg: T.F.SYNC.B.CONTENT_CONFLICT_RESOLVED,
translateParams: { taskList },
// SPAP-15: REVIEW opens the conflicts page; DISMISS auto-renders (no action2).
action: {
label: T.F.SYNC.CONFLICT_REVIEW.BANNER_REVIEW,
fn: () => this.syncConflictBanner.navigateToReview(),
},
action: hasEntriesToReview
? {
label: T.F.SYNC.CONFLICT_REVIEW.BANNER_REVIEW,
fn: () => this.syncConflictBanner.navigateToReview(),
}
: undefined,
});
}

View file

@ -427,6 +427,53 @@ describe('RemoteOpsProcessingService', () => {
expect(JSON.stringify(summary)).not.toContain('private');
});
// Producer freeze for the conflict-review rollback: this is the only
// production entry point into autoResolveConflictsLWW, so if these two flags
// are ever dropped the stable fleet silently starts persisting the discarded
// side of every conflict verbatim again. Delete this test with the freeze.
it('should freeze both conflict-review producers on the production resolve path', async () => {
const localOp = {
id: 'local-op',
entityType: 'TASK',
entityId: 'task-1',
payload: { title: 'local title' },
} as Operation;
const remoteOp = {
id: 'remote-op',
entityType: 'TASK',
entityId: 'task-1',
payload: { title: 'remote title' },
schemaVersion: 1,
} as Operation;
spyOn(service, 'detectConflicts').and.resolveTo({
nonConflicting: [],
conflicts: [
{
entityType: 'TASK',
entityId: 'task-1',
localOps: [localOp],
remoteOps: [remoteOp],
suggestedResolution: 'manual',
},
],
});
conflictResolutionServiceSpy.autoResolveConflictsLWW.and.resolveTo({
localWinOpsCreated: 0,
});
vectorClockServiceSpy.getEntityFrontier.and.resolveTo(new Map());
await service.processRemoteOps([remoteOp]);
expect(conflictResolutionServiceSpy.autoResolveConflictsLWW).toHaveBeenCalledWith(
jasmine.any(Array),
jasmine.any(Array),
jasmine.objectContaining({
disableDisjointMerge: true,
disableConflictJournal: true,
}),
);
});
it('should drop operations if migrateOperation returns null', async () => {
const remoteOps: Operation[] = [
{ id: 'op1', schemaVersion: 1 } as Operation,

View file

@ -437,10 +437,26 @@ export class RemoteOpsProcessingService {
// Auto-resolve conflicts using Last-Write-Wins strategy.
// Piggyback non-conflicting ops so they're applied with resolved conflicts.
// Validation failure is surfaced via the session-validation latch.
//
// PRODUCER FREEZE for the conflict-review rollback. Both flags are set
// here, at the only production entry point, so the feature's two
// producers stop at the fleet boundary while the service keeps the
// capability intact for its own tests and for a one-line revert:
// - disableDisjointMerge: conflicts resolve by whole-entity LWW, which
// is the behaviour of every released version to date, so the stable
// fleet gains nothing it would later have to be migrated off.
// - disableConflictJournal: stop persisting the discarded side of a
// conflict verbatim, so that device-local data obligation does not
// expand beyond the edge/internal builds that already carry it.
// Reverting the freeze = drop these two lines.
const lwwResult = await this.conflictResolutionService.autoResolveConflictsLWW(
conflicts,
nonConflicting,
{ callerHoldsOperationLogLock: true },
{
callerHoldsOperationLogLock: true,
disableDisjointMerge: true,
disableConflictJournal: true,
},
);
localWinOpsCreated = lwwResult.localWinOpsCreated;
return;