feat: [ENG-2493] FileTaskHistoryStore: Prune + Compaction

[bao-byterover]

No description provided.

[github-actions]

Claude finished @bao-byterover's task in 2m 36s —— View job

---

PR Review: feat: [ENG-2493] FileTaskHistoryStore: Prune + Compaction

• Gather context and read changed files
• Analyze implementation for correctness, security, and code quality
• Post review with findings

---

Summary

Score: 4 / 5

Solid implementation of prune + compaction for the two-tier JSONL store. The atomicity design (tombstone-first, data-unlink-second; tmp→rename for index rewrites; appendFile for small batches) is correct. Test coverage is thorough — 30+ cases spanning age/count prune, compaction triggers, concurrency, and the waitForPruneToSettle polling harness.

A few issues worth addressing before this pattern is copied elsewhere:

---

Issues

issue (correctness): getById ignores tombstones — can return deleted entries on partial unlink failure

getById reads the data file directly without consulting the index. If tombstoneAndUnlink appends the tombstone successfully but the subsequent rm fails (ENOSPC, EPERM, race), the data file still exists. getById would return the entry while list() correctly hides it — the detail panel would show a record the list doesn't include.

src/server/infra/storage/file-task-history-store.ts — getById method

// Before reading the data file, check the index:
const dedup = await this.readIndexDedup()
const line = dedup.get(taskId)
if (line === undefined || '_deleted' in line) return undefined

Fix this →

---

issue (performance): O(n²) dead.includes() in count-prune phase

prune() builds survivors with a linear scan per entry against the dead array. At maxEntries = 1000 with ~500 age-pruned entries, this is O(500 × 500) = 250k iterations. Use a Set instead.

src/server/infra/storage/file-task-history-store.ts — prune() method, ~line 310

// Replace:
const survivors = liveEntries.filter((line) => !dead.includes(line.taskId))

// With:
const deadSet = new Set(dead)
const survivors = liveEntries.filter((line) => !deadSet.has(line.taskId))

Fix this →

---

nitpick (docs): Module-level header and tombstoneAndUnlink comment are stale

The file header still says "Prune (M2.03) … land in subsequent tickets" — but M2.03 just landed. Similarly, tombstoneAndUnlink has "M2.03 (compaction) is the right place to add chunking" — M2.03 is now here and chunking wasn't added. Either update the comment with the new rationale or file a follow-up ticket and reference it.

---

nitpick (reliability): tombstoneAndUnlink loses POSIX atomicity for large prune batches

The existing comment correctly notes the ~50-id limit (PIPE_BUF ≈ 4 KB, ~80 bytes/tombstone). An age prune run can send hundreds of taskIds to tombstoneAndUnlink in one call (maxEntries = 1000 with all entries past cutoff). At that scale a concurrent appendFile from save() can interleave bytes, corrupting the index line.

For now the risk is low (prune runs in a dedup'd background macrotask; save() appends are typically interleaved at line granularity not byte granularity on Linux). But since this is the M2.03 milestone that was supposed to address chunking, it's worth at least a TODO comment or a tracked ticket.

---

nitpick (test reliability): waitForPruneToSettle is a time-based heuristic

async function waitForPruneToSettle(): Promise<void> {
  let prev = await countIndexLines()
  for (let i = 0; i < 50; i++) {
    await new Promise((r) => { setTimeout(r, 4) })
    const cur = await countIndexLines()
    if (cur === prev) return
    prev = cur
  }
}

200 ms max budget with a 4 ms poll can be flaky on loaded CI runners. A more robust pattern would be to expose a flushPrune() test-only method that awaits the current in-flight pass, or drain via setImmediate/queueMicrotask rather than wall-clock polling.

---