From 2b28b51a73fe98d8fabca28c747dad47bc5c799d Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:20:39 -0500 Subject: [PATCH] refactor: move status history file I/O behind storage (#1193) --- .../service-filesystem-boundary.json | 8 +- docs/testing/critical-path-coverage.json | 1 + .../status-history-repository.test.ts | 91 ++++++ .../__tests__/status-history-service.test.ts | 3 +- server/src/services/status-history-service.ts | 282 ++++++------------ server/src/storage/index.ts | 1 + .../src/storage/status-history-repository.ts | 71 +++++ 7 files changed, 257 insertions(+), 200 deletions(-) create mode 100644 server/src/__tests__/status-history-repository.test.ts create mode 100644 server/src/storage/status-history-repository.ts diff --git a/docs/architecture/service-filesystem-boundary.json b/docs/architecture/service-filesystem-boundary.json index 22851160..dc56b64a 100644 --- a/docs/architecture/service-filesystem-boundary.json +++ b/docs/architecture/service-filesystem-boundary.json @@ -1,6 +1,6 @@ { "schemaVersion": 1, - "maximumEntries": 51, + "maximumEntries": 50, "entries": [ { "path": "server/src/services/agent-health-service.ts", @@ -230,12 +230,6 @@ "owner": "#1189", "rationale": "Remaining process I/O migration is tracked in issue #1189." }, - { - "path": "server/src/services/status-history-service.ts", - "category": "authoritative-persistence", - "owner": "#1186", - "rationale": "Coordination-state storage migration is tracked in issue #1186." - }, { "path": "server/src/services/task-identity-diagnostics.ts", "category": "authoritative-persistence", diff --git a/docs/testing/critical-path-coverage.json b/docs/testing/critical-path-coverage.json index b6dec2e3..b8bea26d 100644 --- a/docs/testing/critical-path-coverage.json +++ b/docs/testing/critical-path-coverage.json @@ -55,6 +55,7 @@ "src/__tests__/schemas.test.ts", "src/__tests__/security-legacy-runtime.test.ts", "src/__tests__/shared-api-permissions.test.ts", + "src/__tests__/status-history-repository.test.ts", "src/__tests__/sqlite-journal-ownership-policy.test.ts", "src/__tests__/sqlite-maintenance-bootstrap.test.ts", "src/__tests__/sqlite-portability-service.test.ts", diff --git a/server/src/__tests__/status-history-repository.test.ts b/server/src/__tests__/status-history-repository.test.ts new file mode 100644 index 00000000..0c0db5a8 --- /dev/null +++ b/server/src/__tests__/status-history-repository.test.ts @@ -0,0 +1,91 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises'; +import path from 'node:path'; +import type { StatusHistoryEntry } from '../services/status-history-service.js'; +import { FileStatusHistoryStore } from '../storage/status-history-repository.js'; + +function entry(id: string): StatusHistoryEntry { + return { + id, + timestamp: '2026-08-23T20:00:00.000Z', + previousStatus: 'idle', + newStatus: 'working', + }; +} + +describe('FileStatusHistoryStore', () => { + let root: string; + let historyFile: string; + let store: FileStatusHistoryStore; + + beforeEach(async () => { + root = await mkdtemp(path.join(process.cwd(), '.veritas-status-history-store-')); + historyFile = path.join(root, 'state', 'status-history.json'); + store = new FileStatusHistoryStore(historyFile); + }); + + afterEach(async () => { + await rm(root, { recursive: true, force: true }); + }); + + it('atomically updates, reads, and clears status history', async () => { + await expect(store.read()).resolves.toEqual([]); + await store.update((entries) => [entry('status_1'), ...entries]); + await store.update((entries) => [entry('status_2'), ...entries]); + + await expect(store.read()).resolves.toEqual([entry('status_2'), entry('status_1')]); + await store.clear(); + await expect(store.read()).resolves.toEqual([]); + await expect(readFile(historyFile, 'utf8')).resolves.toBe('[]'); + }); + + it('serializes concurrent read-modify-write updates', async () => { + await Promise.all([ + store.update((entries) => [entry('status_alpha'), ...entries]), + store.update((entries) => [entry('status_beta'), ...entries]), + ]); + + const ids = (await store.read()).map(({ id }) => id); + expect(ids).toHaveLength(2); + expect(ids).toEqual(expect.arrayContaining(['status_alpha', 'status_beta'])); + }); + + it('treats malformed and non-array legacy content as empty history', async () => { + await mkdir(path.dirname(historyFile), { recursive: true }); + await writeFile(historyFile, '{broken', 'utf8'); + await expect(store.read()).resolves.toEqual([]); + + await writeFile(historyFile, '{}', 'utf8'); + await expect(store.read()).resolves.toEqual([]); + }); + + it('rejects symbolic links and non-file history paths', async () => { + await mkdir(path.dirname(historyFile), { recursive: true }); + const target = path.join(root, 'outside.json'); + await writeFile(target, '[]', 'utf8'); + await symlink(target, historyFile); + await expect(store.read()).rejects.toThrow(/symbolic link/i); + + await rm(historyFile); + await mkdir(historyFile); + await expect(store.read()).rejects.toThrow(/bounded regular file/i); + }); + + it('rejects symbolic-link parent directories and oversized writes', async () => { + const realDirectory = path.join(root, 'real-state'); + const linkedDirectory = path.join(root, 'linked-state'); + await mkdir(realDirectory); + await symlink(realDirectory, linkedDirectory, 'dir'); + const linkedStore = new FileStatusHistoryStore(path.join(linkedDirectory, 'history.json')); + await expect(linkedStore.update(() => [entry('unsafe')])).rejects.toThrow(/regular directory/i); + + await expect( + store.update(() => [ + { + ...entry('oversized'), + taskTitle: 'x'.repeat(16 * 1024 * 1024), + }, + ]) + ).rejects.toThrow(/16 MiB/i); + }); +}); diff --git a/server/src/__tests__/status-history-service.test.ts b/server/src/__tests__/status-history-service.test.ts index de6e2c88..84868827 100644 --- a/server/src/__tests__/status-history-service.test.ts +++ b/server/src/__tests__/status-history-service.test.ts @@ -5,7 +5,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import fs from 'fs/promises'; import path from 'path'; -import os from 'os'; import { StatusHistoryService } from '../services/status-history-service.js'; describe('StatusHistoryService', () => { @@ -15,7 +14,7 @@ describe('StatusHistoryService', () => { beforeEach(async () => { const suffix = Math.random().toString(36).substring(7); - testDir = path.join(os.tmpdir(), `veritas-test-status-history-${suffix}`); + testDir = path.join(process.cwd(), `.veritas-test-status-history-${suffix}`); const configDir = path.join(testDir, '.veritas-kanban'); await fs.mkdir(configDir, { recursive: true }); historyFile = path.join(configDir, 'status-history.json'); diff --git a/server/src/services/status-history-service.ts b/server/src/services/status-history-service.ts index 15781272..f0f41321 100644 --- a/server/src/services/status-history-service.ts +++ b/server/src/services/status-history-service.ts @@ -1,13 +1,13 @@ -import { readFile, writeFile, mkdir } from 'fs/promises'; -import { fileExists } from '../storage/fs-helpers.js'; -import { dirname, join } from 'path'; +import { join } from 'node:path'; import { createLogger } from '../lib/logger.js'; -import { withFileLock } from './file-lock.js'; import type { StatusHistoryRepository } from '../storage/interfaces.js'; +import { FileStatusHistoryStore } from '../storage/status-history-repository.js'; import { SqliteDatabase, type SqliteConnectionOptions } from '../storage/sqlite/database.js'; import { SqliteStatusHistoryRepository } from '../storage/sqlite/status-history-repository.js'; import { getRuntimeDir } from '../utils/paths.js'; + const log = createLogger('status-history-service'); +const MAX_ENTRIES = 5000; export type AgentStatusState = 'idle' | 'working' | 'thinking' | 'sub-agent' | 'error'; @@ -19,15 +19,15 @@ export interface StatusHistoryEntry { taskId?: string; taskTitle?: string; subAgentCount?: number; - durationMs?: number; // How long the previous status lasted + durationMs?: number; } export interface DailySummary { - date: string; // YYYY-MM-DD - activeMs: number; // Total time in working/thinking/sub-agent states - idleMs: number; // Total time in idle state - errorMs: number; // Total time in error state - transitions: number; // Number of status changes + date: string; + activeMs: number; + idleMs: number; + errorMs: number; + transitions: number; periods: StatusPeriod[]; } @@ -43,78 +43,50 @@ export interface StatusPeriod { export interface StatusHistoryServiceOptions { historyFile?: string; storageType?: 'file' | 'sqlite'; + repository?: StatusHistoryRepository; sqliteDatabase?: SqliteDatabase; sqliteConnectionOptions?: SqliteConnectionOptions; now?: () => Date; } export class StatusHistoryService { - private historyFile: string; - private readonly MAX_ENTRIES = 5000; // Keep more entries for historical analysis private lastEntry: StatusHistoryEntry | null = null; - private initPromise: Promise; - private repository: StatusHistoryRepository | null = null; + private readonly repository: StatusHistoryRepository | null; + private readonly fileStore: FileStatusHistoryStore | null; private sqliteDatabase: SqliteDatabase | null = null; private ownsSqliteDatabase = false; private readonly now: () => Date; constructor(options: StatusHistoryServiceOptions = {}) { this.now = options.now ?? (() => new Date()); - this.historyFile = options.historyFile || join(getRuntimeDir(), 'status-history.json'); + if (options.repository) { + this.repository = options.repository; + this.fileStore = null; + return; + } + const storageType = options.storageType ?? (process.env.VERITAS_STORAGE === 'sqlite' ? 'sqlite' : 'file'); - if (storageType === 'sqlite') { this.sqliteDatabase = options.sqliteDatabase ?? new SqliteDatabase(options.sqliteConnectionOptions); this.ownsSqliteDatabase = !options.sqliteDatabase; this.sqliteDatabase.open(); this.repository = new SqliteStatusHistoryRepository(this.sqliteDatabase); - this.initPromise = Promise.resolve(); - } else { - this.initPromise = this.ensureDir(); + this.fileStore = null; + return; } + + this.repository = null; + this.fileStore = new FileStatusHistoryStore( + options.historyFile ?? join(getRuntimeDir(), 'status-history.json') + ); } - private async ensureDir(): Promise { - await mkdir(dirname(this.historyFile), { recursive: true }); - } - - private async getLastEntry(): Promise { - if (this.lastEntry) { - return this.lastEntry; - } - - try { - const entries = await this.getHistory(1); - this.lastEntry = entries[0] ?? null; - return this.lastEntry; - } catch { - // Intentionally silent: history file may not exist on first run - this.lastEntry = null; - return null; - } - } - - async getHistory(limit: number = 100, offset: number = 0): Promise { - if (this.repository) { - return this.repository.getHistory(limit, offset); - } - - await this.initPromise; - - if (!(await fileExists(this.historyFile))) { - return []; - } - - try { - const content = await readFile(this.historyFile, 'utf-8'); - const entries: StatusHistoryEntry[] = JSON.parse(content); - return entries.slice(offset, offset + limit); - } catch { - // Intentionally silent: corrupted file — return empty list - return []; - } + async getHistory(limit = 100, offset = 0): Promise { + if (this.repository) return this.repository.getHistory(limit, offset); + const entries = await this.getFileStore().read(); + return entries.slice(offset, offset + limit); } async logStatusChange( @@ -132,75 +104,37 @@ export class StatusHistoryService { taskTitle, subAgentCount ); - - log.info( - `[StatusHistory] ${previousStatus} → ${newStatus}${taskId ? ` (task: ${taskId})` : ''}` - ); - + this.logChange(previousStatus, newStatus, taskId); return entry; } - await this.initPromise; - const now = this.now(); const timestamp = now.toISOString(); - - // Calculate duration of previous status - let durationMs: number | undefined; - const previousEntry = await this.getLastEntry(); - if (previousEntry) { - const lastTime = new Date(previousEntry.timestamp).getTime(); - durationMs = now.getTime() - lastTime; - } - + const previousEntry = this.lastEntry ?? (await this.getHistory(1))[0]; const entry: StatusHistoryEntry = { - id: `status_${now.getTime()}_${Math.random().toString(36).substr(2, 9)}`, + id: `status_${now.getTime()}_${Math.random().toString(36).slice(2, 11)}`, timestamp, previousStatus, newStatus, taskId, taskTitle, subAgentCount, - durationMs, + durationMs: previousEntry + ? now.getTime() - new Date(previousEntry.timestamp).getTime() + : undefined, }; - await withFileLock(this.historyFile, async () => { - let entries: StatusHistoryEntry[] = []; - - if (await fileExists(this.historyFile)) { - try { - const content = await readFile(this.historyFile, 'utf-8'); - entries = JSON.parse(content); - } catch { - // Intentionally silent: corrupted history file — start fresh - entries = []; - } - } - - // Prepend new entry and limit to MAX_ENTRIES - entries = [entry, ...entries].slice(0, this.MAX_ENTRIES); - - await writeFile(this.historyFile, JSON.stringify(entries, null, 2), 'utf-8'); - }); - + await this.getFileStore().update((entries) => [entry, ...entries].slice(0, MAX_ENTRIES)); this.lastEntry = entry; - - log.info( - `[StatusHistory] ${previousStatus} → ${newStatus}${taskId ? ` (task: ${taskId})` : ''}` - ); - + this.logChange(previousStatus, newStatus, taskId); return entry; } async getHistoryByDateRange(startDate: string, endDate: string): Promise { - if (this.repository) { - return this.repository.getHistoryByDateRange(startDate, endDate); - } - - const entries = await this.getHistory(this.MAX_ENTRIES); + if (this.repository) return this.repository.getHistoryByDateRange(startDate, endDate); + const entries = await this.getHistory(MAX_ENTRIES); const start = new Date(startDate).getTime(); const end = new Date(endDate).getTime(); - return entries.filter((entry) => { const entryTime = new Date(entry.timestamp).getTime(); return entryTime >= start && entryTime <= end; @@ -208,91 +142,53 @@ export class StatusHistoryService { } async getDailySummary(date?: string): Promise { - if (this.repository) { - return this.repository.getDailySummary(date); - } - + if (this.repository) return this.repository.getDailySummary(date); const targetDate = date || this.now().toISOString().split('T')[0]; const startOfDay = new Date(`${targetDate}T00:00:00.000Z`); const endOfDay = new Date(`${targetDate}T23:59:59.999Z`); - const entries = await this.getHistoryByDateRange( startOfDay.toISOString(), endOfDay.toISOString() ); - - // Reverse to process chronologically const chronological = [...entries].reverse(); - let activeMs = 0; let idleMs = 0; let errorMs = 0; const periods: StatusPeriod[] = []; - // Process each transition - for (let i = 0; i < chronological.length; i++) { - const entry = chronological[i]; - const nextEntry = chronological[i + 1]; - - // Calculate how long this status lasted - let endTime: Date; - if (nextEntry) { - endTime = new Date(nextEntry.timestamp); - } else { - // Last entry - use current time or end of day if analyzing past dates - const now = this.now(); - endTime = now < endOfDay ? now : endOfDay; - } - + for (let index = 0; index < chronological.length; index++) { + const entry = chronological[index]; + const nextEntry = chronological[index + 1]; + const endTime = nextEntry ? new Date(nextEntry.timestamp) : this.getOpenPeriodEnd(endOfDay); const startTime = new Date(entry.timestamp); const durationMs = endTime.getTime() - startTime.getTime(); + if (durationMs <= 0) continue; - // Only count positive durations within the day - if (durationMs > 0) { - // Categorize the time - if (entry.newStatus === 'idle') { - idleMs += durationMs; - } else if (entry.newStatus === 'error') { - errorMs += durationMs; - } else { - activeMs += durationMs; - } + if (entry.newStatus === 'idle') idleMs += durationMs; + else if (entry.newStatus === 'error') errorMs += durationMs; + else activeMs += durationMs; - // Add to periods - periods.push({ - status: entry.newStatus, - startTime: entry.timestamp, - endTime: endTime.toISOString(), - durationMs, - taskId: entry.taskId, - taskTitle: entry.taskTitle, - }); - } + periods.push({ + status: entry.newStatus, + startTime: entry.timestamp, + endTime: endTime.toISOString(), + durationMs, + taskId: entry.taskId, + taskTitle: entry.taskTitle, + }); } - // If no entries for the day, check the last status from before this day if (chronological.length === 0) { - const allEntries = await this.getHistory(this.MAX_ENTRIES); - const beforeDay = allEntries.filter( - (e) => new Date(e.timestamp).getTime() < startOfDay.getTime() + const lastBeforeDay = (await this.getHistory(MAX_ENTRIES)).find( + (entry) => new Date(entry.timestamp).getTime() < startOfDay.getTime() ); - - if (beforeDay.length > 0) { - // The most recent entry before this day determines the starting status - const lastBeforeDay = beforeDay[0]; - const now = this.now(); - const effectiveEnd = now < endOfDay ? now : endOfDay; + if (lastBeforeDay) { + const effectiveEnd = this.getOpenPeriodEnd(endOfDay); const durationMs = effectiveEnd.getTime() - startOfDay.getTime(); - if (durationMs > 0) { - if (lastBeforeDay.newStatus === 'idle') { - idleMs = durationMs; - } else if (lastBeforeDay.newStatus === 'error') { - errorMs = durationMs; - } else { - activeMs = durationMs; - } - + if (lastBeforeDay.newStatus === 'idle') idleMs = durationMs; + else if (lastBeforeDay.newStatus === 'error') errorMs = durationMs; + else activeMs = durationMs; periods.push({ status: lastBeforeDay.newStatus, startTime: startOfDay.toISOString(), @@ -316,44 +212,48 @@ export class StatusHistoryService { } async getWeeklySummary(): Promise { - if (this.repository) { - return this.repository.getWeeklySummary(); - } - + if (this.repository) return this.repository.getWeeklySummary(); const summaries: DailySummary[] = []; const today = this.now(); - - for (let i = 0; i < 7; i++) { + for (let offset = 0; offset < 7; offset++) { const date = new Date(today); - date.setDate(date.getDate() - i); - const dateStr = date.toISOString().split('T')[0]; - summaries.push(await this.getDailySummary(dateStr)); + date.setDate(date.getDate() - offset); + summaries.push(await this.getDailySummary(date.toISOString().split('T')[0])); } - return summaries; } async clearHistory(): Promise { - if (this.repository) { - await this.repository.clearHistory(); - this.lastEntry = null; - return; - } - - await this.initPromise; - await writeFile(this.historyFile, '[]', 'utf-8'); + if (this.repository) await this.repository.clearHistory(); + else await this.getFileStore().clear(); this.lastEntry = null; } dispose(): void { - if (this.ownsSqliteDatabase) { - this.sqliteDatabase?.close(); - } + if (this.ownsSqliteDatabase) this.sqliteDatabase?.close(); this.sqliteDatabase = null; - this.repository = null; this.lastEntry = null; } + + private getFileStore(): FileStatusHistoryStore { + if (!this.fileStore) throw new Error('File status history store is unavailable'); + return this.fileStore; + } + + private getOpenPeriodEnd(endOfDay: Date): Date { + const now = this.now(); + return now < endOfDay ? now : endOfDay; + } + + private logChange( + previousStatus: AgentStatusState, + newStatus: AgentStatusState, + taskId?: string + ): void { + log.info( + `[StatusHistory] ${previousStatus} → ${newStatus}${taskId ? ` (task: ${taskId})` : ''}` + ); + } } -// Singleton instance export const statusHistoryService = new StatusHistoryService(); diff --git a/server/src/storage/index.ts b/server/src/storage/index.ts index 389976b0..f5ace63c 100644 --- a/server/src/storage/index.ts +++ b/server/src/storage/index.ts @@ -40,6 +40,7 @@ export type { } from './interfaces.js'; export { LocalWorkspaceFileRepository } from './workspace-file-repository.js'; export { FileProgressRepository, type ProgressRepository } from './progress-repository.js'; +export { FileStatusHistoryStore } from './status-history-repository.js'; export { FileWorkspaceExecutionTrustRepository, InMemoryWorkspaceExecutionTrustRepository, diff --git a/server/src/storage/status-history-repository.ts b/server/src/storage/status-history-repository.ts new file mode 100644 index 00000000..b512c30d --- /dev/null +++ b/server/src/storage/status-history-repository.ts @@ -0,0 +1,71 @@ +import { lstat, mkdir, open } from 'node:fs/promises'; +import path from 'node:path'; +import type { StatusHistoryEntry } from '../services/status-history-service.js'; +import { withFileLock } from '../services/file-lock.js'; +import { ensureWithinBase } from '../utils/sanitize.js'; +import { atomicWriteFile } from './fs-helpers.js'; + +const MAX_HISTORY_BYTES = 16 * 1024 * 1024; + +export class FileStatusHistoryStore { + private readonly historyFile: string; + + constructor(historyFile: string) { + this.historyFile = path.resolve(historyFile); + ensureWithinBase(path.dirname(this.historyFile), this.historyFile); + } + + async read(): Promise { + let handle: Awaited> | undefined; + try { + handle = await open(this.historyFile, 'r'); + const [pathStats, stats] = await Promise.all([lstat(this.historyFile), handle.stat()]); + if ( + pathStats.isSymbolicLink() || + pathStats.dev !== stats.dev || + pathStats.ino !== stats.ino + ) { + throw new Error('Status history must not be a symbolic link or changed file'); + } + if (!stats.isFile() || stats.size > MAX_HISTORY_BYTES) { + throw new Error('Status history must be a bounded regular file'); + } + const parsed: unknown = JSON.parse(await handle.readFile({ encoding: 'utf8' })); + return Array.isArray(parsed) ? (parsed as StatusHistoryEntry[]) : []; + } catch (error) { + const errorCode = (error as NodeJS.ErrnoException).code; + if (errorCode === 'ENOENT' || error instanceof SyntaxError) return []; + throw error; + } finally { + await handle?.close(); + } + } + + async update( + updater: (entries: StatusHistoryEntry[]) => StatusHistoryEntry[] + ): Promise { + await this.prepareParent(); + return withFileLock(this.historyFile, async () => { + const entries = updater(await this.read()); + const content = JSON.stringify(entries, null, 2); + if (Buffer.byteLength(content, 'utf8') > MAX_HISTORY_BYTES) { + throw new Error('Status history exceeds the 16 MiB storage limit'); + } + await atomicWriteFile(this.historyFile, content, 'utf8'); + return entries; + }); + } + + async clear(): Promise { + await this.update(() => []); + } + + private async prepareParent(): Promise { + const parent = path.dirname(this.historyFile); + await mkdir(parent, { recursive: true, mode: 0o700 }); + const stats = await lstat(parent); + if (!stats.isDirectory() || stats.isSymbolicLink()) { + throw new Error('Status history path must use a regular directory'); + } + } +}