mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-01-23 02:36:05 +00:00
refactor(sync): use uuidv7 library for UUID generation
Replace custom UUID v7 implementations with the standard uuidv7 library. The custom implementation had a bug with JavaScript bitwise operators not handling 48-bit timestamps correctly.
This commit is contained in:
Johannes Millan
parent
5c673b0036
commit
c87e7ec218
5 changed files with 7 additions and 141 deletions
|
|
@ -1,90 +0,0 @@
|
|||
# Operation Log Implementation Review
|
||||
|
||||
## Overview
|
||||
|
||||
This document outlines suggestions for improving the robustness, data integrity, and performance of the Operation Log implementation (`src/app/core/persistence/operation-log/`). These findings are based on a static analysis of the current codebase.
|
||||
|
||||
## 1. Conflict Detection & Stale Operations
|
||||
|
||||
**Location:** `OperationLogSyncService.detectConflicts`
|
||||
|
||||
**Current Behavior:**
|
||||
The code currently checks if the relationship between the local frontier and the remote operation is `CONCURRENT`. If it is, it flags a conflict. If it is _not_ concurrent, it assumes the remote operation is safe to apply.
|
||||
|
||||
**Issue:**
|
||||
This logic fails to account for cases where the local state "dominates" the remote operation (Comparison result: `GREATER_THAN`) or is identical (`EQUAL`). In these scenarios, the remote operation is stale (already applied or obsolete). By treating it as "non-conflicting," the system will re-apply this old operation, effectively overwriting newer local changes with older data.
|
||||
|
||||
**Suggestion:**
|
||||
Modify `detectConflicts` to explicitly check for `GREATER_THAN` and `EQUAL`.
|
||||
|
||||
- If **Local > Remote** (Local dominates): Ignore the remote operation. It is obsolete.
|
||||
- If **Local == Remote**: Ignore the remote operation. It is a duplicate.
|
||||
- If **Local < Remote** (Remote dominates): Apply the operation (Fast-forward).
|
||||
- If **Concurrent**: Flag as conflict.
|
||||
|
||||
## 2. Compaction Consistency & Race Conditions
|
||||
|
||||
**Location:** `OperationLogEffects` and `OperationLogCompactionService`
|
||||
|
||||
**Current Behavior:**
|
||||
Compaction is triggered asynchronously via `this.triggerCompaction()` inside `OperationLogEffects`. It runs independently of the operation write lock.
|
||||
|
||||
**Issue:**
|
||||
There is a potential race condition. `OperationLogCompactionService.compact()` captures the current NgRx state and the current `lastSeq`. However, because this runs in parallel with the main thread processing new actions, it is possible for:
|
||||
|
||||
1. The State to include changes from operation `N+1`.
|
||||
2. The `lastSeq` to be recorded as `N`.
|
||||
This mismatch invalidates the snapshot invariant (State = Apply(Snapshot, Ops > lastSeq)).
|
||||
|
||||
**Suggestion:**
|
||||
Serialize the compaction process with the operation writing process.
|
||||
|
||||
- Use the `LockService` to ensure that no new operations are written to the log or applied to the store while the snapshot is being generated.
|
||||
- Ensure `lastSeq` and the `state` capture happen atomically relative to the operation stream.
|
||||
|
||||
## 3. Optimistic UI Rollback
|
||||
|
||||
**Location:** `OperationLogEffects.writeOperation`
|
||||
|
||||
**Current Behavior:**
|
||||
The code wraps the persistence logic in a `try/catch` block. However, the error handling logic is currently commented out:
|
||||
|
||||
```typescript
|
||||
// this.notifyUserAndTriggerRollback(action);
|
||||
```
|
||||
|
||||
**Issue:**
|
||||
If writing to IndexedDB fails (e.g., quota exceeded, disk error), the UI has already updated optimistically. The user sees the change, but it is not saved. If the app is reloaded, the data effectively "disappears."
|
||||
|
||||
**Suggestion:**
|
||||
Implement the `notifyUserAndTriggerRollback` method. This should:
|
||||
|
||||
1. Notify the user via a global error toast.
|
||||
2. Dispatch an undo/rollback action to the NgRx store to revert the UI state to match the persistent state.
|
||||
|
||||
## 4. Vector Clock & Frontier Management
|
||||
|
||||
**Location:** `OperationLogStoreService`
|
||||
|
||||
**Current Behavior:**
|
||||
`getCurrentVectorClock` and `getEntityFrontier` reconstruct the vector clock by merging the snapshot's clock with all subsequent operations in the log.
|
||||
|
||||
**Issue:**
|
||||
As the number of operations between snapshots grows (up to `COMPACTION_THRESHOLD` or more if compaction fails), this calculation becomes more expensive, potentially impacting sync performance.
|
||||
|
||||
**Suggestion:**
|
||||
|
||||
- Explicitly store the "Frontier Vector Clock" in the `state_cache` alongside the state snapshot.
|
||||
- Maintain an in-memory cache of the current Vector Clock in `OperationLogStoreService` that is updated on every `append`, avoiding the need to re-scan the log for every check.
|
||||
|
||||
## 5. Hard Dependency Retry Logic
|
||||
|
||||
**Location:** `OperationApplierService`
|
||||
|
||||
**Current Behavior:**
|
||||
Operations with missing hard dependencies are retried `MAX_RETRY_ATTEMPTS` (3) times.
|
||||
|
||||
**Suggestion:**
|
||||
Consider scenarios where dependencies arrive strictly out of order (e.g., during a bulk sync). A fixed retry count of 3 might be insufficient if the dependency is the 10th operation in the queue.
|
||||
|
||||
- **Improvement:** Instead of a fixed count, use a "pass" system. Retry pending operations every time a _new_ operation is successfully applied (which might resolve the dependency). Only fail permanently if no progress is made after a full cycle of processing available operations.
|
||||
|
|
@ -135,7 +135,8 @@
|
|||
"electron-window-state": "^5.0.3",
|
||||
"fs-extra": "^11.3.2",
|
||||
"hash-wasm": "^4.12.0",
|
||||
"node-fetch": "^2.7.0"
|
||||
"node-fetch": "^2.7.0",
|
||||
"uuidv7": "^1.1.0"
|
||||
},
|
||||
"devDependencies": {
|
||||
"@angular-builders/custom-webpack": "^20.0.0",
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@
|
|||
"fastify": "^5.6.2",
|
||||
"jsonwebtoken": "^9.0.2",
|
||||
"nodemailer": "^7.0.11",
|
||||
"uuidv7": "^1.1.0",
|
||||
"zod": "^4.1.13"
|
||||
},
|
||||
"devDependencies": {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify';
|
||||
import { z } from 'zod';
|
||||
import { uuidv7 } from 'uuidv7';
|
||||
import { authenticate } from '../middleware';
|
||||
import { getSyncService } from './sync.service';
|
||||
import { Logger } from '../logger';
|
||||
|
|
@ -215,7 +216,7 @@ export const syncRoutes = async (fastify: FastifyInstance): Promise<void> => {
|
|||
|
||||
// Create a SYNC_IMPORT operation
|
||||
const op = {
|
||||
id: generateOpId(),
|
||||
id: uuidv7(),
|
||||
clientId,
|
||||
actionType: 'SYNC_IMPORT',
|
||||
opType: 'SYNC_IMPORT' as const,
|
||||
|
|
@ -305,12 +306,3 @@ export const syncRoutes = async (fastify: FastifyInstance): Promise<void> => {
|
|||
},
|
||||
);
|
||||
};
|
||||
|
||||
// Simple UUID v7-like ID generator
|
||||
const generateOpId = (): string => {
|
||||
const timestamp = Date.now().toString(16).padStart(12, '0');
|
||||
const random = Array.from({ length: 20 }, () =>
|
||||
Math.floor(Math.random() * 16).toString(16),
|
||||
).join('');
|
||||
return `${timestamp}${random}`;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -1,41 +1,3 @@
|
|||
export const uuidv7 = (): string => {
|
||||
// 32-bit time_high
|
||||
const ts = Date.now();
|
||||
import { uuidv7 as uuidv7Lib } from 'uuidv7';
|
||||
|
||||
// 12-bit rand_a (in v7, some bits are time_low but JS Date.now is ms precision)
|
||||
// We can put more random bits.
|
||||
// UUID v7 format:
|
||||
// 0 1 2 3
|
||||
// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
|
||||
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|
||||
// | unix_ts_ms |
|
||||
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|
||||
// | unix_ts_ms | ver | rand_a |
|
||||
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|
||||
// |var| rand_b |
|
||||
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|
||||
// | rand_b |
|
||||
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|
||||
|
||||
const values = new Uint8Array(16);
|
||||
crypto.getRandomValues(values);
|
||||
|
||||
// 48-bit timestamp (big-endian)
|
||||
values[0] = (ts >>> 40) & 0xff;
|
||||
values[1] = (ts >>> 32) & 0xff;
|
||||
values[2] = (ts >>> 24) & 0xff;
|
||||
values[3] = (ts >>> 16) & 0xff;
|
||||
values[4] = (ts >>> 8) & 0xff;
|
||||
values[5] = ts & 0xff;
|
||||
|
||||
// Version 7 (0x7) in top 4 bits of byte 6
|
||||
values[6] = (values[6] & 0x0f) | 0x70;
|
||||
|
||||
// Variant (10) in top 2 bits of byte 8
|
||||
values[8] = (values[8] & 0x3f) | 0x80;
|
||||
|
||||
return [...values]
|
||||
.map((b) => b.toString(16).padStart(2, '0'))
|
||||
.join('')
|
||||
.replace(/^(.{8})(.{4})(.{4})(.{4})(.{12})$/, '$1-$2-$3-$4-$5');
|
||||
};
|
||||
export const uuidv7 = uuidv7Lib;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue