Skip to content

Latest commit

 

History

History
248 lines (190 loc) · 8.78 KB

File metadata and controls

248 lines (190 loc) · 8.78 KB

Transaction Boundaries in SessionService

Current State

✅ IMPLEMENTED: The SessionService.markAsRead() method now uses database transactions to ensure atomicity of critical operations. All strategy executions are wrapped in db.transaction(), ensuring that multi-step workflows either succeed completely or roll back entirely.

Operations in markAsRead()

Critical Operations (Should Be Transactional)

These operations modify core data and should succeed or fail together:

  1. Status Transition (ensureReadingStatus())

    • Creates/updates reading session
    • Changes book status to "reading"
  2. Progress Creation (create100PercentProgress())

    • Creates 100% progress log
    • Triggers auto-completion to "read" status
    • Archives the reading session
  3. Direct Status Update (updateStatus())

    • Direct status change for books without pages
    • Session state updates

Best-Effort Operations (Should Be Outside Transaction)

These operations can fail without compromising data integrity:

  1. Rating Update (updateBookRating())

    • Syncs to external services (Calibre)
    • Updates books.rating field
    • Failures logged but don't block main operation
  2. Review Update (updateSessionReview())

    • Updates session.review field
    • Failures logged but don't block main operation

Implementation Details

Repository Layer Updates

All repository methods now accept an optional tx?: any parameter:

// BaseRepository
async findById(id: number, tx?: any): Promise<T | undefined> {
  const database = tx || this.getDatabase();
  return database.select().from(this.tableSchema).where(eq(this.tableSchema.id, id)).get();
}

async create(data: InsertT, tx?: any): Promise<T> {
  const database = tx || this.getDatabase();
  const result = database.insert(this.tableSchema).values(data).returning().all();
  return result[0];
}

async update(id: number, data: Partial<InsertT>, tx?: any): Promise<T | undefined> {
  const database = tx || this.getDatabase();
  const result = database.update(this.tableSchema).set(data).where(eq(this.tableSchema.id, id)).returning().all();
  return result[0];
}

Service Layer Updates

Service methods now propagate transaction context:

async ensureReadingStatus(bookId: number, tx?: any): Promise<ReadingSession> {
  const activeSession = await sessionRepository.findActiveByBookId(bookId, tx);
  if (activeSession?.status === "reading") {
    return activeSession;
  }
  const result = await this.updateStatus(bookId, { status: "reading" }, tx);
  return result.session;
}

async create100PercentProgress(bookId: number, totalPages: number, completedDate?: Date, tx?: any): Promise<void> {
  await progressService.logProgress(bookId, {
    currentPage: totalPages,
    currentPercentage: 100,
    notes: "Marked as read",
    progressDate: completedDate,
  }, tx);
}

Transaction Wrapper in markAsRead

The main markAsRead() method now wraps strategy execution in a transaction:

async markAsRead(params: MarkAsReadParams): Promise<MarkAsReadResult> {
  const db = getDatabase();

  // CRITICAL SECTION: Execute strategy within transaction
  let sessionId: number | undefined;
  let progressCreated: boolean;

  try {
    const result = await db.transaction((tx) => {
      const strategyContext: MarkAsReadStrategyContext = {
        // ... context setup ...
        tx, // Pass transaction context
        ensureReadingStatus: this.ensureReadingStatus.bind(this),
        create100PercentProgress: this.create100PercentProgress.bind(this),
        updateStatus: this.updateStatus.bind(this),
        // ...
      };
      return strategy(strategyContext);
    });

    sessionId = result.sessionId;
    progressCreated = result.progressCreated;
  } catch (error) {
    logger.error({ err: error }, "Transaction failed, rolling back");
    throw error;
  }

  // POST-TRANSACTION: Best-effort operations
  // streak updates, cache invalidation, rating/review updates
  // ...
}

Circular Dependency Fix

The auto-completion logic in ProgressService.logProgress() previously created a new SessionService instance, creating a circular dependency. This has been fixed by using direct repository calls:

// OLD (circular dependency)
const sessionService = new SessionService();
await sessionService.updateStatus(bookId, { status: "read", completedDate: requestedDate });

// NEW (direct repository call)
await sessionRepository.update(activeSession.id, {
  status: "read",
  completedDate: requestedDate,
  isActive: false,
} as any, tx);

Risk Assessment

Risk Level: LOW (reduced from MEDIUM)

Improvements from Transaction Implementation:

  1. ✅ All critical operations are now atomic - they either all succeed or all roll back
  2. ✅ No more partial state from failures during multi-step workflows
  3. ✅ Database failures during strategy execution trigger automatic rollback
  4. ✅ Best-effort operations (rating, review, streak, cache) remain outside transaction

Remaining Acceptable Risks:

  1. Rating/review updates are best-effort (documented behavior)

    • Failures are logged but don't block the main operation
    • Users can retry these operations independently
  2. External service failures (Calibre sync)

    • Handled outside transaction
    • Won't cause rollback of book completion

Protected Scenarios:

  1. Progress creation failure - entire operation rolls back, book state unchanged
  2. Auto-completion failure - progress creation also rolled back
  3. Session update failure - entire status transition rolled back

Implementation Timeline

Phase 1: Documentation ✅ COMPLETED

  • ✅ Document transaction boundaries
  • ✅ Identify critical vs best-effort operations
  • ✅ Create technical reference

Phase 2: Repository Refactoring ✅ COMPLETED

Actual effort: ~4 hours

  1. ✅ Add transaction parameter to BaseRepository methods (create, update, findById)
  2. ✅ Update SessionRepository methods (findActiveByBookId, getNextSessionNumber, findLatestByBookId, hasCompletedReads, findAllByBookId)
  3. ✅ Update ProgressRepository methods (findLatestBySessionId, findBySessionId, hasProgressForSession)
  4. ✅ BookRepository.findById() now accepts tx parameter (via BaseRepository)

Phase 3: Service Layer Transactions ✅ COMPLETED

Actual effort: ~3 hours

  1. ✅ Add tx parameter to updateStatus() and propagate to repository calls
  2. ✅ Add tx parameter to ensureReadingStatus()
  3. ✅ Add tx parameter to create100PercentProgress()
  4. ✅ Add tx parameter to progressService.logProgress()
  5. ✅ Fix circular dependency in auto-completion (use direct repository call)

Phase 4: Strategy Pattern Integration ✅ COMPLETED

Actual effort: ~2 hours

  1. ✅ Add tx to MarkAsReadStrategyContext interface
  2. ✅ Update all 4 strategy methods to use tx from context
  3. ✅ Wrap strategy execution in db.transaction() in markAsRead
  4. ✅ Move streak/cache updates outside transaction

Phase 5: Testing & Documentation ✅ COMPLETED

Actual effort: ~1 hour

  1. ✅ Run existing test suite - all 1971 tests passing
  2. ✅ Verify backward compatibility
  3. ✅ Update TRANSACTION_BOUNDARIES.md

Total Implementation Time

~10 hours (significantly less than estimated 18-26 hours due to well-structured strategy pattern)

Related References

  • Example transaction usage: lib/services/book.service.ts:updateTotalPages()
  • Drizzle transaction docs: https://orm.drizzle.team/docs/transactions
  • Database context: lib/db/context.ts
  • Repository pattern: lib/repositories/base.repository.ts

Testing Status

Existing Tests ✅ PASSING

  • All 1971 existing tests pass with transaction implementation
  • No regressions detected
  • Backward compatibility maintained

Transaction Tests ✅ COMPLETED

File: __tests__/services/session/mark-as-read.transactions.test.ts

Test Results: 10 passing tests

  • ✅ Transaction Rollback Scenarios (3 tests)

    • ✅ Session update failure during status change rolls back
    • ✅ Session creation failure rolls back (ManualSessionUpdateStrategy)
    • ✅ GetNextSessionNumber failure prevents session creation
  • ✅ Best-Effort Operations (6 tests)

    • ✅ Rating update failures don't prevent book completion
    • ✅ Review update failures don't prevent session completion
    • ✅ Combined rating+review failures still complete successfully
    • ✅ Rating failures don't prevent progress creation
    • ✅ Review failures don't prevent session archiving
    • ✅ Core operation succeeds independently of best-effort failures
  • ✅ Transaction Atomicity Guarantees (2 tests)

    • ✅ DirectStatusChangeStrategy status updates are atomic
    • ✅ ManualSessionUpdateStrategy session creation is atomic

Last Updated: 2025-12-30 Status: ✅ Implemented and tested Risk Level: LOW (reduced from MEDIUM) Next Review: Monitor production for any edge cases