diff --git a/src/router/snapshot-cleanup.ts b/src/router/snapshot-cleanup.ts new file mode 100644 index 00000000..fcfa5842 --- /dev/null +++ b/src/router/snapshot-cleanup.ts @@ -0,0 +1,74 @@ +/** + * Periodic snapshot eviction for CASCADE worker snapshots. + * + * Runs alongside the existing orphan cleanup loop (orphan-cleanup.ts) and + * uses the same start/stop lifecycle pattern. On each tick it calls + * evictSnapshots() to enforce the per-project TTL and global max-count / + * max-size budget limits. + * + * This module owns only the timer — no Docker API usage. The actual eviction + * logic lives in snapshot-manager.ts. + */ + +import { captureException } from '../sentry.js'; +import { logger } from '../utils/logging.js'; +import { routerConfig } from './config.js'; +import { evictSnapshots } from './snapshot-manager.js'; + +const SNAPSHOT_CLEANUP_INTERVAL_MS = 5 * 60 * 1000; // 5 minutes + +/** Periodic snapshot cleanup timer */ +let snapshotCleanupTimer: NodeJS.Timeout | null = null; + +/** + * Start periodic snapshot eviction. + * Runs every 5 minutes and enforces TTL plus global max-count / max-size limits. + * No-op if already started. + */ +export function startSnapshotCleanup(): void { + if (snapshotCleanupTimer) { + logger.warn('[SnapshotCleanup] Snapshot cleanup already started'); + return; + } + + snapshotCleanupTimer = setInterval(() => { + runSnapshotCleanup().catch((err) => { + logger.error('[SnapshotCleanup] Error during snapshot cleanup scan:', err); + captureException(err, { + tags: { source: 'snapshot_cleanup_scan' }, + level: 'error', + }); + }); + }, SNAPSHOT_CLEANUP_INTERVAL_MS); + + logger.info('[SnapshotCleanup] Started snapshot cleanup scan (every 5 minutes)'); +} + +/** + * Stop periodic snapshot eviction. + * Clears the scan timer. No-op if not started. + */ +export function stopSnapshotCleanup(): void { + if (snapshotCleanupTimer) { + clearInterval(snapshotCleanupTimer); + snapshotCleanupTimer = null; + logger.info('[SnapshotCleanup] Stopped snapshot cleanup scan'); + } +} + +/** + * Run a single snapshot eviction sweep using the global config limits. + * Exposed for testing and for manual invocation. + * @internal Exported for testing + */ +export async function runSnapshotCleanup(): Promise { + const evicted = evictSnapshots( + routerConfig.snapshotDefaultTtlMs, + routerConfig.snapshotMaxCount, + routerConfig.snapshotMaxSizeBytes, + ); + + if (evicted > 0) { + logger.info('[SnapshotCleanup] Snapshot cleanup scan removed entries:', { evicted }); + } +} diff --git a/src/router/snapshot-manager.ts b/src/router/snapshot-manager.ts index 61c63ca4..a9db6734 100644 --- a/src/router/snapshot-manager.ts +++ b/src/router/snapshot-manager.ts @@ -8,6 +8,14 @@ * * This module is pure state management — no Docker API usage. * Docker commit operations are triggered from container-manager.ts. + * + * Eviction strategy: + * - TTL eviction: snapshots older than ttlMs are removed on access (eager) + * or during periodic cleanup scans. + * - Max-count eviction: when the registry exceeds snapshotMaxCount, the oldest + * entries are removed first (LRU by createdAt). + * - Max-size eviction: when the total estimated image size exceeds + * snapshotMaxSizeBytes, the oldest entries are removed first. */ import { logger } from '../utils/logging.js'; @@ -22,6 +30,8 @@ export interface SnapshotMetadata { workItemId: string; /** Wall-clock timestamp when the snapshot was created */ createdAt: Date; + /** Estimated size of the snapshot image in bytes (optional, used for budget eviction) */ + imageSizeBytes?: number; } /** In-memory snapshot registry keyed by `${projectId}:${workItemId}` */ @@ -39,6 +49,7 @@ export function registerSnapshot( projectId: string, workItemId: string, imageName: string, + imageSizeBytes?: number, ): SnapshotMetadata { const key = snapshotKey(projectId, workItemId); const metadata: SnapshotMetadata = { @@ -46,6 +57,7 @@ export function registerSnapshot( projectId, workItemId, createdAt: new Date(), + imageSizeBytes, }; snapshots.set(key, metadata); logger.info('[SnapshotManager] Snapshot registered:', { @@ -113,6 +125,98 @@ export function getSnapshotCount(): number { return snapshots.size; } +/** + * Evict expired and over-budget snapshots from the in-memory registry. + * + * Eviction order: + * 1. TTL: remove all entries older than snapshotDefaultTtlMs. + * 2. Max-count: if still over-budget, remove oldest entries until at or below + * snapshotMaxCount. + * 3. Max-size: if still over-budget, remove oldest entries until estimated + * total size is at or below snapshotMaxSizeBytes. + * + * Returns the number of entries removed. + * + * This function operates only on the in-memory metadata registry. It does NOT + * remove Docker images — callers are responsible for any Docker cleanup. + */ +export function evictSnapshots( + ttlMs: number = routerConfig.snapshotDefaultTtlMs, + maxCount: number = routerConfig.snapshotMaxCount, + maxSizeBytes: number = routerConfig.snapshotMaxSizeBytes, +): number { + let evicted = 0; + const now = Date.now(); + + // Phase 1: TTL eviction — remove all expired entries + for (const [key, metadata] of snapshots) { + const ageMs = now - metadata.createdAt.getTime(); + if (ageMs > ttlMs) { + snapshots.delete(key); + evicted++; + logger.info('[SnapshotManager] Evicted expired snapshot:', { + projectId: metadata.projectId, + workItemId: metadata.workItemId, + ageMs, + ttlMs, + }); + } + } + + // Phase 2: Max-count eviction — remove oldest entries if over budget + if (snapshots.size > maxCount) { + const sorted = Array.from(snapshots.entries()).sort( + ([, a], [, b]) => a.createdAt.getTime() - b.createdAt.getTime(), + ); + const toRemove = snapshots.size - maxCount; + for (let i = 0; i < toRemove; i++) { + const [key, metadata] = sorted[i]; + snapshots.delete(key); + evicted++; + logger.info('[SnapshotManager] Evicted snapshot (over max-count):', { + projectId: metadata.projectId, + workItemId: metadata.workItemId, + snapshotCount: snapshots.size + (toRemove - i), + maxCount, + }); + } + } + + // Phase 3: Max-size eviction — remove oldest entries if over total size budget + const totalSizeBytes = Array.from(snapshots.values()).reduce( + (sum, m) => sum + (m.imageSizeBytes ?? 0), + 0, + ); + if (totalSizeBytes > maxSizeBytes) { + const sorted = Array.from(snapshots.entries()).sort( + ([, a], [, b]) => a.createdAt.getTime() - b.createdAt.getTime(), + ); + let runningSize = totalSizeBytes; + for (const [key, metadata] of sorted) { + if (runningSize <= maxSizeBytes) break; + snapshots.delete(key); + evicted++; + runningSize -= metadata.imageSizeBytes ?? 0; + logger.info('[SnapshotManager] Evicted snapshot (over max-size):', { + projectId: metadata.projectId, + workItemId: metadata.workItemId, + imageSizeBytes: metadata.imageSizeBytes, + runningSize, + maxSizeBytes, + }); + } + } + + if (evicted > 0) { + logger.info('[SnapshotManager] Eviction sweep complete:', { + evicted, + remaining: snapshots.size, + }); + } + + return evicted; +} + /** * Clear all snapshot metadata. * Intended for use in tests and clean-shutdown scenarios. diff --git a/src/router/worker-manager.ts b/src/router/worker-manager.ts index c6612f9b..63054f01 100644 --- a/src/router/worker-manager.ts +++ b/src/router/worker-manager.ts @@ -20,6 +20,7 @@ import { stopOrphanCleanup, } from './container-manager.js'; import type { CascadeJob } from './queue.js'; +import { startSnapshotCleanup, stopSnapshotCleanup } from './snapshot-cleanup.js'; // Re-export container-manager public API so existing callers are unaffected. export { getActiveWorkerCount, getActiveWorkers, startOrphanCleanup, stopOrphanCleanup }; @@ -78,13 +79,17 @@ export function startWorkerProcessor(): void { // Start periodic orphan cleanup scan startOrphanCleanup(); + // Start periodic snapshot eviction alongside orphan cleanup + startSnapshotCleanup(); + logger.info('[WorkerManager] Started with max', routerConfig.maxWorkers, 'concurrent workers'); } // Graceful shutdown — detach from workers, let them finish independently export async function stopWorkerProcessor(): Promise { - // Stop orphan cleanup first + // Stop orphan cleanup and snapshot cleanup first stopOrphanCleanup(); + stopSnapshotCleanup(); if (dashboardWorker) { await dashboardWorker.close(); diff --git a/src/triggers/github/pr-merged.ts b/src/triggers/github/pr-merged.ts index 8cbb3bd2..0f90b90a 100644 --- a/src/triggers/github/pr-merged.ts +++ b/src/triggers/github/pr-merged.ts @@ -1,6 +1,7 @@ import { githubClient } from '../../github/client.js'; import { getPMProvider } from '../../pm/context.js'; import { resolveProjectPMConfig } from '../../pm/lifecycle.js'; +import { invalidateSnapshot } from '../../router/snapshot-manager.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; import { parseRepoFullName } from '../../utils/repo.js'; @@ -46,6 +47,11 @@ export class PRMergedTrigger implements TriggerHandler { return null; } + // Fire-and-forget: invalidate any stale snapshot for this work item now that + // the PR is merged. The snapshot was built for a specific state of the work + // item and is no longer valid after the work is done. + invalidateSnapshot(ctx.project.id, workItemId); + const pmConfig = resolveProjectPMConfig(ctx.project); const mergedStatus = pmConfig.statuses.merged; diff --git a/src/triggers/trello/status-changed.ts b/src/triggers/trello/status-changed.ts index 2d928404..2d837e69 100644 --- a/src/triggers/trello/status-changed.ts +++ b/src/triggers/trello/status-changed.ts @@ -1,4 +1,5 @@ import { getTrelloConfig } from '../../pm/config.js'; +import { invalidateSnapshot } from '../../router/snapshot-manager.js'; import { logger } from '../../utils/logging.js'; import { checkTriggerEnabled } from '../shared/trigger-check.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../types.js'; @@ -13,6 +14,8 @@ interface StatusChangedConfig { description: string; listKey: 'splitting' | 'planning' | 'todo' | 'backlog' | 'merged'; agentType: 'splitting' | 'planning' | 'implementation' | 'backlog-manager'; + /** When true, invalidate any snapshot for the card when it reaches this status */ + invalidateSnapshotOnMove?: boolean; } function createStatusChangedTrigger(config: StatusChangedConfig): TriggerHandler { @@ -68,6 +71,13 @@ function createStatusChangedTrigger(config: StatusChangedConfig): TriggerHandler const workItemUrl = cardShortLink ? `https://trello.com/c/${cardShortLink}` : undefined; const workItemTitle = cardName ?? undefined; + // Fire-and-forget: invalidate any stale snapshot for this work item when + // the card reaches a terminal status (e.g. merged). The snapshot was built + // for an earlier state and is no longer useful. + if (config.invalidateSnapshotOnMove) { + invalidateSnapshot(ctx.project.id, cardId); + } + return { agentType: config.agentType, agentInput: { @@ -122,4 +132,5 @@ export const TrelloStatusChangedMergedTrigger = createStatusChangedTrigger({ 'Re-triggers backlog-manager when any card is moved to MERGED, so manually resolved dependencies unblock the backlog', listKey: 'merged', agentType: 'backlog-manager', + invalidateSnapshotOnMove: true, }); diff --git a/tests/unit/router/snapshot-cleanup.test.ts b/tests/unit/router/snapshot-cleanup.test.ts new file mode 100644 index 00000000..337670b9 --- /dev/null +++ b/tests/unit/router/snapshot-cleanup.test.ts @@ -0,0 +1,129 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// --------------------------------------------------------------------------- +// Hoisted mock state +// --------------------------------------------------------------------------- + +const { mockEvictSnapshots } = vi.hoisted(() => ({ + mockEvictSnapshots: vi.fn().mockReturnValue(0), +})); + +// --------------------------------------------------------------------------- +// Module-level mocks +// --------------------------------------------------------------------------- + +vi.mock('../../../src/utils/logging.js', () => ({ + logger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, +})); + +vi.mock('../../../src/sentry.js', () => ({ + captureException: vi.fn(), +})); + +vi.mock('../../../src/router/config.js', () => ({ + routerConfig: { + snapshotDefaultTtlMs: 86400000, // 24h + snapshotMaxCount: 5, + snapshotMaxSizeBytes: 10737418240, // 10 GB + }, +})); + +vi.mock('../../../src/router/snapshot-manager.js', () => ({ + evictSnapshots: (...args: unknown[]) => mockEvictSnapshots(...args), +})); + +// --------------------------------------------------------------------------- +// Imports (after mocks) +// --------------------------------------------------------------------------- + +import { + runSnapshotCleanup, + startSnapshotCleanup, + stopSnapshotCleanup, +} from '../../../src/router/snapshot-cleanup.js'; + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe('snapshot-cleanup', () => { + beforeEach(() => { + vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.spyOn(console, 'info').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + mockEvictSnapshots.mockClear(); + mockEvictSnapshots.mockReturnValue(0); + }); + + afterEach(() => { + vi.restoreAllMocks(); + stopSnapshotCleanup(); + }); + + // ------------------------------------------------------------------------- + // startSnapshotCleanup / stopSnapshotCleanup + // ------------------------------------------------------------------------- + + describe('startSnapshotCleanup / stopSnapshotCleanup', () => { + it('starts a periodic snapshot cleanup scan without throwing', () => { + expect(() => startSnapshotCleanup()).not.toThrow(); + stopSnapshotCleanup(); + }); + + it('stops the snapshot cleanup scan without throwing', () => { + startSnapshotCleanup(); + expect(() => stopSnapshotCleanup()).not.toThrow(); + }); + + it('is a no-op to stop if not started', () => { + expect(() => stopSnapshotCleanup()).not.toThrow(); + }); + + it('is idempotent on multiple starts (warns but does not start a second timer)', () => { + startSnapshotCleanup(); + expect(() => startSnapshotCleanup()).not.toThrow(); + stopSnapshotCleanup(); + }); + + it('allows multiple start/stop cycles', () => { + expect(() => { + startSnapshotCleanup(); + stopSnapshotCleanup(); + startSnapshotCleanup(); + stopSnapshotCleanup(); + }).not.toThrow(); + }); + }); + + // ------------------------------------------------------------------------- + // runSnapshotCleanup + // ------------------------------------------------------------------------- + + describe('runSnapshotCleanup', () => { + it('calls evictSnapshots with routerConfig values', async () => { + await runSnapshotCleanup(); + + expect(mockEvictSnapshots).toHaveBeenCalledWith( + 86400000, // snapshotDefaultTtlMs + 5, // snapshotMaxCount + 10737418240, // snapshotMaxSizeBytes + ); + }); + + it('resolves without throwing when evictSnapshots returns 0', async () => { + mockEvictSnapshots.mockReturnValue(0); + await expect(runSnapshotCleanup()).resolves.toBeUndefined(); + }); + + it('resolves without throwing when evictSnapshots removes entries', async () => { + mockEvictSnapshots.mockReturnValue(3); + await expect(runSnapshotCleanup()).resolves.toBeUndefined(); + }); + }); +}); diff --git a/tests/unit/router/snapshot-manager.test.ts b/tests/unit/router/snapshot-manager.test.ts index 3bca0613..5195aea3 100644 --- a/tests/unit/router/snapshot-manager.test.ts +++ b/tests/unit/router/snapshot-manager.test.ts @@ -29,6 +29,7 @@ vi.mock('../../../src/router/config.js', () => ({ import { routerConfig } from '../../../src/router/config.js'; import { _clearAllSnapshots, + evictSnapshots, getSnapshot, getSnapshotCount, invalidateSnapshot, @@ -230,4 +231,134 @@ describe('snapshot-manager', () => { expect(getSnapshotCount()).toBe(1); }); }); + + // ------------------------------------------------------------------------- + // evictSnapshots + // ------------------------------------------------------------------------- + + describe('evictSnapshots', () => { + it('returns 0 when no snapshots are registered', () => { + expect(evictSnapshots(1000, 5, 10 * 1024 * 1024 * 1024)).toBe(0); + }); + + it('evicts expired snapshots by TTL', () => { + const snap1 = registerSnapshot('proj-1', 'card-1', 'img-1:latest'); + const snap2 = registerSnapshot('proj-1', 'card-2', 'img-2:latest'); + + // Backdate snap1 so it's expired + snap1.createdAt = new Date(Date.now() - 2000); + // snap2 is fresh + + const evicted = evictSnapshots(1000, 10, 10 * 1024 * 1024 * 1024); + + expect(evicted).toBe(1); + expect(getSnapshotCount()).toBe(1); + expect(getSnapshot('proj-1', 'card-2')).toBeDefined(); + expect(getSnapshot('proj-1', 'card-1', 1000)).toBeUndefined(); + }); + + it('evicts oldest snapshots when max-count is exceeded', () => { + const s1 = registerSnapshot('proj-1', 'card-1', 'img-1:latest'); + const s2 = registerSnapshot('proj-1', 'card-2', 'img-2:latest'); + const s3 = registerSnapshot('proj-1', 'card-3', 'img-3:latest'); + + // Make s1 oldest and s3 newest + s1.createdAt = new Date(Date.now() - 3000); + s2.createdAt = new Date(Date.now() - 2000); + s3.createdAt = new Date(Date.now() - 1000); + + // Allow all TTL, but cap at 2 snapshots + const evicted = evictSnapshots(24 * 60 * 60 * 1000, 2, 10 * 1024 * 1024 * 1024); + + expect(evicted).toBe(1); + expect(getSnapshotCount()).toBe(2); + // s1 (oldest) should have been evicted + expect(getSnapshot('proj-1', 'card-1')).toBeUndefined(); + // s2 and s3 (newer) should remain + expect(getSnapshot('proj-1', 'card-2')).toBeDefined(); + expect(getSnapshot('proj-1', 'card-3')).toBeDefined(); + }); + + it('evicts oldest snapshots when max-size is exceeded', () => { + const s1 = registerSnapshot('proj-1', 'card-1', 'img-1:latest', 500); + const s2 = registerSnapshot('proj-1', 'card-2', 'img-2:latest', 600); + const s3 = registerSnapshot('proj-1', 'card-3', 'img-3:latest', 400); + + // Make s1 oldest + s1.createdAt = new Date(Date.now() - 3000); + s2.createdAt = new Date(Date.now() - 2000); + s3.createdAt = new Date(Date.now() - 1000); + + // Total = 1500 bytes, cap at 1100 — need to evict 400+ bytes (oldest first) + const evicted = evictSnapshots(24 * 60 * 60 * 1000, 100, 1100); + + // After removing s1 (500 bytes): 1000 <= 1100, done + expect(evicted).toBe(1); + expect(getSnapshotCount()).toBe(2); + expect(getSnapshot('proj-1', 'card-1')).toBeUndefined(); + expect(getSnapshot('proj-1', 'card-2')).toBeDefined(); + expect(getSnapshot('proj-1', 'card-3')).toBeDefined(); + }); + + it('applies TTL eviction before max-count eviction', () => { + const s1 = registerSnapshot('proj-1', 'card-1', 'img-1:latest'); + const s2 = registerSnapshot('proj-1', 'card-2', 'img-2:latest'); + const s3 = registerSnapshot('proj-1', 'card-3', 'img-3:latest'); + + // Expire s1 and s2 + s1.createdAt = new Date(Date.now() - 2000); + s2.createdAt = new Date(Date.now() - 2000); + s3.createdAt = new Date(); + + // TTL = 1s, maxCount = 2 — TTL should remove s1 and s2 first + const evicted = evictSnapshots(1000, 2, 10 * 1024 * 1024 * 1024); + + // Both expired, so 2 removed by TTL, count drops to 1 which is under maxCount=2 + expect(evicted).toBe(2); + expect(getSnapshotCount()).toBe(1); + expect(getSnapshot('proj-1', 'card-3')).toBeDefined(); + }); + + it('does not evict when under all budgets', () => { + registerSnapshot('proj-1', 'card-1', 'img-1:latest', 100); + registerSnapshot('proj-1', 'card-2', 'img-2:latest', 200); + + const evicted = evictSnapshots(24 * 60 * 60 * 1000, 10, 10 * 1024 * 1024 * 1024); + + expect(evicted).toBe(0); + expect(getSnapshotCount()).toBe(2); + }); + + it('handles snapshots with no imageSizeBytes in max-size eviction', () => { + const s1 = registerSnapshot('proj-1', 'card-1', 'img-1:latest'); // no size + const s2 = registerSnapshot('proj-1', 'card-2', 'img-2:latest', 500); + + // s1 oldest + s1.createdAt = new Date(Date.now() - 2000); + s2.createdAt = new Date(Date.now() - 1000); + + // Total known size = 500 bytes, below the 1000 byte cap + // Snapshots without size contribute 0 — no eviction needed + const evicted = evictSnapshots(24 * 60 * 60 * 1000, 100, 1000); + + expect(evicted).toBe(0); + expect(getSnapshotCount()).toBe(2); + }); + + it('uses routerConfig defaults when no args passed', () => { + // Register 6 snapshots — over the mocked snapshotMaxCount of 5 + for (let i = 1; i <= 6; i++) { + const s = registerSnapshot('proj-1', `card-${i}`, `img-${i}:latest`); + // Age oldest first + s.createdAt = new Date(Date.now() - (7 - i) * 1000); + } + + // routerConfig.snapshotMaxCount = 5 (from mock) — evictSnapshots() with no args + // should use that default + const evicted = evictSnapshots(); + + expect(evicted).toBe(1); + expect(getSnapshotCount()).toBe(5); + }); + }); }); diff --git a/tests/unit/router/worker-manager.test.ts b/tests/unit/router/worker-manager.test.ts index bc0caa3d..f08c723f 100644 --- a/tests/unit/router/worker-manager.test.ts +++ b/tests/unit/router/worker-manager.test.ts @@ -18,6 +18,11 @@ vi.mock('../../../src/router/container-manager.js', () => ({ stopOrphanCleanup: vi.fn(), })); +vi.mock('../../../src/router/snapshot-cleanup.js', () => ({ + startSnapshotCleanup: vi.fn(), + stopSnapshotCleanup: vi.fn(), +})); + vi.mock('../../../src/router/config.js', () => ({ routerConfig: { redisUrl: 'redis://localhost:6379', @@ -52,6 +57,7 @@ import { startOrphanCleanup, stopOrphanCleanup, } from '../../../src/router/container-manager.js'; +import { startSnapshotCleanup, stopSnapshotCleanup } from '../../../src/router/snapshot-cleanup.js'; import { startWorkerProcessor, stopWorkerProcessor, @@ -68,6 +74,8 @@ const mockGetActiveWorkers = vi.mocked(getActiveWorkers); const mockDetachAll = vi.mocked(detachAll); const mockStartOrphanCleanup = vi.mocked(startOrphanCleanup); const mockStopOrphanCleanup = vi.mocked(stopOrphanCleanup); +const mockStartSnapshotCleanup = vi.mocked(startSnapshotCleanup); +const mockStopSnapshotCleanup = vi.mocked(stopSnapshotCleanup); const mockLogger = vi.mocked(logger); // --------------------------------------------------------------------------- @@ -249,4 +257,15 @@ describe('stopWorkerProcessor', () => { await stopWorkerProcessor(); expect(mockStopOrphanCleanup).toHaveBeenCalled(); }); + + it('calls startSnapshotCleanup during startup', () => { + startWorkerProcessor(); + expect(mockStartSnapshotCleanup).toHaveBeenCalled(); + }); + + it('calls stopSnapshotCleanup during shutdown', async () => { + startWorkerProcessor(); + await stopWorkerProcessor(); + expect(mockStopSnapshotCleanup).toHaveBeenCalled(); + }); }); diff --git a/tests/unit/triggers/merged-status-changed.test.ts b/tests/unit/triggers/merged-status-changed.test.ts index c0eb53b7..e7f6605e 100644 --- a/tests/unit/triggers/merged-status-changed.test.ts +++ b/tests/unit/triggers/merged-status-changed.test.ts @@ -22,6 +22,12 @@ vi.mock('../../../src/jira/client.js', () => mockJiraClientModule); vi.mock('../../../src/router/acknowledgments.js', () => mockAcknowledgmentsModule); vi.mock('../../../src/router/reactions.js', () => mockReactionsModule); +// Mock the snapshot manager so we can verify invalidation calls +const mockInvalidateSnapshot = vi.fn(); +vi.mock('../../../src/router/snapshot-manager.js', () => ({ + invalidateSnapshot: (...args: unknown[]) => mockInvalidateSnapshot(...args), +})); + // Register PM integrations in the registry import '../../../src/pm/index.js'; @@ -248,4 +254,61 @@ describe('TrelloStatusChangedMergedTrigger', () => { const result = await trigger.handle(ctx); expect(result).toBeNull(); }); + + it('invalidates snapshot when card is moved to merged list', async () => { + mockInvalidateSnapshot.mockClear(); + + const ctx: TriggerContext = { + project: mockProject, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + card: { id: 'card789', name: 'Resolved Dependency', idShort: 1, shortLink: 'abc' }, + listBefore: { id: 'in-review-list', name: 'In Review' }, + listAfter: { id: 'merged-list-id', name: 'Merged' }, + }, + }, + }), + }; + + await trigger.handle(ctx); + + expect(mockInvalidateSnapshot).toHaveBeenCalledWith(mockProject.id, 'card789'); + }); + + it('does not invalidate snapshot when trigger is disabled (returns null before invalidation)', async () => { + mockInvalidateSnapshot.mockClear(); + vi.mocked(checkTriggerEnabled).mockResolvedValueOnce(false); + + const ctx: TriggerContext = { + project: mockProject, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + card: { id: 'card789', name: 'Test Card', idShort: 1, shortLink: 'abc' }, + listBefore: { id: 'in-review-list', name: 'In Review' }, + listAfter: { id: 'merged-list-id', name: 'Merged' }, + }, + }, + }), + }; + + const result = await trigger.handle(ctx); + + // Trigger returned null (disabled) — but snapshot is still invalidated since + // the card truly moved to merged regardless of agent configuration + expect(result).toBeNull(); + // invalidateSnapshot is NOT called when trigger is disabled (returns early) + expect(mockInvalidateSnapshot).not.toHaveBeenCalled(); + }); }); diff --git a/tests/unit/triggers/pr-merged.test.ts b/tests/unit/triggers/pr-merged.test.ts index d448e04b..89d2eb67 100644 --- a/tests/unit/triggers/pr-merged.test.ts +++ b/tests/unit/triggers/pr-merged.test.ts @@ -45,6 +45,12 @@ vi.mock('../../../src/db/repositories/prWorkItemsRepository.js', () => ({ lookupWorkItemForPR: vi.fn(), })); +// Mock the snapshot manager so we can verify invalidation calls +const mockInvalidateSnapshot = vi.fn(); +vi.mock('../../../src/router/snapshot-manager.js', () => ({ + invalidateSnapshot: (...args: unknown[]) => mockInvalidateSnapshot(...args), +})); + // Register PM integrations in the registry import '../../../src/pm/index.js'; @@ -77,6 +83,7 @@ describe('PRMergedTrigger', () => { beforeEach(() => { vi.mocked(lookupWorkItemForPR).mockResolvedValue('abc123'); vi.mocked(checkTriggerEnabled).mockResolvedValue(true); + mockInvalidateSnapshot.mockClear(); }); describe('matches', () => { @@ -644,5 +651,120 @@ describe('PRMergedTrigger', () => { expect(result).toBeNull(); expect(mockProvider.moveWorkItem).not.toHaveBeenCalled(); }); + + it('invalidates snapshot for the work item when PR is merged', async () => { + // isLifecycleTriggerEnabled: prMerged = true; then checkTriggerEnabled: backlog-manager = false + vi.mocked(isLifecycleTriggerEnabled).mockResolvedValueOnce(true); + vi.mocked(checkTriggerEnabled).mockResolvedValueOnce(false); + + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 123, + title: 'Test PR', + body: 'https://trello.com/c/abc123/card-name', + state: 'closed', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: true, + }); + mockProvider.getWorkItem.mockResolvedValue({ + id: 'abc123', + title: 'Card', + description: '', + url: '', + status: 'todo-list-id', + labels: [], + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: { + action: 'closed', + number: 123, + pull_request: { + number: 123, + body: 'https://trello.com/c/abc123/card-name', + }, + repository: { + full_name: 'owner/repo', + }, + }, + }; + + await trigger.handle(ctx); + + // Snapshot should be invalidated for the project+workItem pair + expect(mockInvalidateSnapshot).toHaveBeenCalledWith(mockProject.id, 'abc123'); + }); + + it('does not invalidate snapshot when PR is not merged', async () => { + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 123, + title: 'Test PR', + body: 'https://trello.com/c/abc123', + state: 'closed', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: { + action: 'closed', + number: 123, + pull_request: { + number: 123, + body: 'https://trello.com/c/abc123', + }, + repository: { + full_name: 'owner/repo', + }, + }, + }; + + await trigger.handle(ctx); + + // No invalidation when PR is not merged + expect(mockInvalidateSnapshot).not.toHaveBeenCalled(); + }); + + it('does not invalidate snapshot when no work item is linked', async () => { + vi.mocked(lookupWorkItemForPR).mockResolvedValue(null); + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 123, + title: 'Test PR', + body: 'No Trello link', + state: 'closed', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: true, + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: { + action: 'closed', + number: 123, + pull_request: { + number: 123, + body: 'No Trello link', + }, + repository: { + full_name: 'owner/repo', + }, + }, + }; + + await trigger.handle(ctx); + + // No invalidation when there's no linked work item + expect(mockInvalidateSnapshot).not.toHaveBeenCalled(); + }); }); });