Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions devlog/2026-09-16_compaction-fork-recovery/REQ.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# REQ - Reconcile compaction restart and custom-storage fork recovery

- Task ID: `2026-09-16_compaction-fork-recovery`
- Home Repo: `opencode-acp`
- Created: 2026-09-16
- Status: InProgress
- Priority: P1
- Owner: ranxianglei
- References: https://github.com/ranxianglei/opencode-acp/issues/407 (related: #395 OpenCode V2 migration, #404 concurrency)

## 1. Background & Problem Statement

- **Context**: Two pre-existing state-recovery paths in `ensureSessionInitialized` (`lib/state/state.ts`) can restore stale or incomplete ACP state after a process restart or a session fork.
- **Current behavior (symptom)**:
1. **Compaction-restart gap**: init sets `state.lastCompaction = findLastCompactionTimestamp(messages)` from the _current_ history _before_ loading persisted state, then restores persisted nudge anchors/baselines, message refs, and tool-cache state unconditionally. If native compaction completed after the last persist (restart in between), the restored transient fields are stale — and `updatePerTurnState` cannot reset them because `state.lastCompaction` already equals the current boundary, so its `>` comparison never fires.
2. **Fork recovery gaps**: (a) the parent state is loaded via `loadSessionState(parentSessionId, logger)` without the child's resolved `storageDir`, so with a custom `storagePath` the parent file is never found; (b) `mapForkIds` (`lib/state/rebuild.ts`) consumes parent `byRef` keys verbatim — legacy pre-1.1.0 four-digit refs (`m0001`) never match the fork's five-digit refs (`m00001`), so parent-to-fork translation yields an empty map and inherited blocks are lost whenever the copied history no longer contains replayable compress inputs.
- **Expected behavior**: A restart immediately after native compaction resets stale transient refs/nudges/tool cache while preserving compression blocks and stats, and persists the corrected boundary. Fork recovery finds the parent state under the configured `storagePath` and normalizes legacy parent refs before translation.
- **Impact**: Stale refs/nudge baselines after restart-after-compaction; forked sessions with custom storage or legacy parent state silently lose inherited compression blocks → context overflow.

## 2. Reproduction

- **Environment**: Node 22/24, any OS.
- **Minimal reproduction steps** (encoded as unit/E2E tests in `tests/restart-compaction-fork-recovery.test.ts`):
1. Run a session until one compress block exists; populate nudge/message-ref/tool-cache state; persist.
2. Replace history with a compaction summary message (newer timestamp) and re-initialize the same session ID (simulated restart) → observed: stale `lastPerMessageNudgeTokens`, stale `byRef`/`byRawId`, stale anchors retained.
3. Build a parent with a compress block under a custom `storageDir`; fork it with an input-less copied compress part and `config.storagePath` set → observed: 0 blocks restored (parent file not found at default location).
4. Same fork flow with parent `messageIds` rewritten to four-digit refs → observed: 0 blocks restored (ref mismatch in `mapForkIds`).
- **Relevant configuration**: `storagePath` (custom storage), legacy state files written by pre-1.1.0 versions.

## 3. Constraints & Non-Goals

- **Constraints**:
- Backward compatibility: persisted state format unchanged (no new required fields); legacy 4-digit refs must keep working; own-session load-time migration in `state.ts` untouched.
- Reset semantics must match the live-compaction path: `resetOnCompaction` preserves `prune.messages` (blocks) and stats by design (Bug 2 patch).
- No `as any` / type-assertion hacks in `lib/` changes.
- No version bump on this feature branch (release branches only).
- **Non-Goals**: migrating/moving existing default-location files when `storagePath` changes (explicitly out of scope per existing warn-once behavior); fixing the dead `_persistedToolParameters` persistence field (observed, reported separately if warranted); concurrency work (#404) and V2 migration (#395).

## 4. Acceptance Criteria (must be testable)

- **Correctness**:
- [x] Restart after a newer native compaction resets message refs, nudge anchors/baselines, and tool cache; preserves compression blocks and stats; persists the corrected `lastCompaction` boundary.
- [x] Restart _without_ a newer compaction still restores persisted transient state (no over-resetting).
- [x] Fork recovery loads parent state from the resolved `storagePath` directory and persists the fork state there.
- [x] Fork recovery normalizes legacy 4-digit parent `byRef`/`byRawId` before translation (unit + full-init paths).
- [x] All regression tests verified to FAIL against unfixed code (stash lib changes → 4/5 fail, negative control passes; restore → 5/5 pass).
- **Performance / Stability**:
- [x] Full suite green: 1268 tests, 0 failures; `npm run typecheck` and `npm run build` pass.
50 changes: 50 additions & 0 deletions devlog/2026-09-16_compaction-fork-recovery/WORKLOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# WORKLOG - Reconcile compaction restart and custom-storage fork recovery

- Task ID: `2026-09-16_compaction-fork-recovery`
- Home Repo: `opencode-acp`
- Status: InProgress
- Updated: 2026-09-16 12:30

## 1. Summary

- **What was done** (1–3 sentences): Added a post-load compaction-boundary reconciliation in `ensureSessionInitialized` that resets stale transient state when the current history is newer than the persisted boundary; made fork recovery load parent state from the resolved `storageDir`; normalized legacy 4-digit parent refs before fork ID translation. Added 5 regression tests covering all three failure modes plus a no-regression control.
- **Why** (1–3 sentences): A restart between native compaction and the next transform left stale message refs, nudge baselines, and tool-cache entries because `updatePerTurnState`'s reset trigger compares against an already-current `lastCompaction`. Forks with custom `storagePath` or pre-1.1.0 parent state silently lost inherited compression blocks because the parent file was looked up at the default location and legacy refs never matched.
- **Behavior / compatibility changes**: Yes — restart-after-compaction now resets transient fields (parity with the live-compaction reset path) while preserving blocks/stats; persisted state format unchanged; own-session legacy-ref migration untouched.
- **Risk level**: Low — changes are confined to init/recovery paths, guarded by boundary comparison (`>`), and reuse the existing `resetOnCompaction` semantics.

## 2. Change Log

### Commits

| Commit | Description |
| ----------- | ------------------------------------------------------------------------------------------------------------------------------------------- |
| `419045b` | fix: reconcile compaction restart and custom-storage fork recovery (lib + tests + REQ) |
| `f30da03` | docs: worklog for compaction restart / fork recovery fix |
| (follow-up) | review fixes: default-dir parent fallback during storagePath transition + transition test; test config type-conformance; assertion comments |

### Key Files

- `lib/state/state.ts` — reconciliation block after `_persistedLastCompaction` merge in `ensureSessionInitialized`; `loadSessionState(parentSessionId, logger, state.storageDir)` in the fork branch **plus a default-location fallback** when the custom-dir load misses (storagePath-transition scenario, found in code review).
- `lib/state/rebuild.ts` — new `normalizeParentMessageIds()` helper (4→5 digit, byRef rebuilt from byRawId); used by `mapForkIds`.
- `tests/restart-compaction-fork-recovery.test.ts` — 6 tests: restart-after-compaction reset, negative control, custom-storage fork e2e, storagePath-transition fallback, legacy-ref unit, legacy-ref full-init e2e.

## 3. Design & Implementation Notes

- **Why reconcile at init instead of relying on `updatePerTurnState`**: `state.lastCompaction = findLastCompactionTimestamp(messages)` runs _before_ the persisted load, so by the time stale fields are restored, the "newer than persisted" signal only exists as a comparison between current history and `_persistedLastCompaction`. The reconciliation uses exactly that comparison; `Math.max` keeps the merged value when the persisted boundary is newer (no spurious reset).
- **Reset scope**: `resetOnCompaction` (lib/state/utils.ts) clears tool cache, all nudge anchors/baselines, and message refs; it deliberately preserves `prune.messages` and stats (Bug 2 patch comment). This matches the issue's required semantics and gives parity with the live-compaction path. The freshly seeded `turnNudgeAnchors` (from `collectTurnNudgeAnchors`) are wiped too — the inject pipeline re-derives them per turn, identical to the live path's one-turn behavior.
- **Fork storageDir**: passing `state.storageDir` (possibly `undefined`) to the parent load is backward compatible — `getStorageDir(override)` falls back to the default directory, so default-storage forks behave exactly as before.
- **Legacy ref normalization**: mirrors the own-session migration loop in `state.ts` (`parseMessageRef`/`formatMessageRef`). `byRef` is rebuilt from `byRawId` (authoritative direction) with defensive carry-over of byRef-only entries. Non-matching keys pass through unchanged, so malformed refs behave exactly as before (skipped during translation).
- **Observation (out of scope)**: `_persistedToolParameters` is written by `saveSessionState` but never read back anywhere in `lib/` — the tool cache is re-derived each turn via `syncToolCache`. Dead persistence data; noted for a future cleanup issue if desired.

## 4. Dual-Agent Review (AGENTS.md §5.3 + §5.6)

Two independent agent reviews on the PR branch:

1. **Test review** — APPROVE. Fixed on-branch: `buildConfig()` now type-conformant to `PluginConfig` (added `logLevel`, top-level `allowSubAgents`, `qualityGate`, `messageFilters`; removed mis-nested `experimental.allowSubAgents` — inherited gap from `rebuild.test.ts`); comment pinning why the `toolParameters.size === 0` assertion exists (transient-by-design cache); persisted-side-effect reload assertion added to the legacy-ref e2e test.
2. **Code review** — APPROVE. One minor finding fixed on-branch: parent-state lookup now falls back to the default storage location when the custom-dir load misses, preserving master's behavior in the "just configured storagePath" transition (child state still at default location) where recovery would otherwise degrade from transfer to replay. New regression test added for this path and verified to fail without the fallback.

## 5. Verification

- Regression validity (per AGENTS.md §5.7.3 lesson): with lib fixes stashed, all bug-repro tests FAIL while the negative control passes; transition test verified to fail without the default-dir fallback. With fixes: 6/6 pass.
- Full suite: `npm test` → 1269 tests, 0 failures.
- `npm run typecheck` clean; `npm run build` clean; Prettier applied to changed files.
39 changes: 36 additions & 3 deletions lib/state/rebuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
*/
import type { GCConfig, PluginConfig } from "../config"
import type { Logger } from "../logger"
import { assignMessageRefs } from "../message-ids"
import { assignMessageRefs, formatMessageRef, parseMessageRef } from "../message-ids"
import {
buildSearchContext,
resolveAnchorMessageId,
Expand All @@ -37,7 +37,7 @@ import {
} from "../compress/state"
import { countTokens } from "../token-utils"
import { createHash } from "node:crypto"
import type { PersistedSessionState } from "./persistence"
import type { PersistedMessageIds, PersistedSessionState } from "./persistence"
import { createPruneMessagesState } from "./utils"
import type {
BoundaryReference,
Expand Down Expand Up @@ -152,13 +152,46 @@ interface ForkIdMap {
tools: Map<string, string>
}

/**
* [Issue #407] Normalize legacy 4-digit parent refs (m0001) to the current
* 5-digit format (m00001). Fork-assigned refs are always 5-digit
* (formatMessageRef), so unnormalized pre-1.1.0 parent aliases would never
* match during parent-to-fork translation and inherited compression blocks
* would be silently lost. Mirrors the own-session migration applied in
* state.ts on state load. byRef is rebuilt from byRawId (the authoritative
* direction); any byRef-only entries are carried over defensively.
*/
function normalizeParentMessageIds(
messageIds?: PersistedMessageIds,
): PersistedMessageIds | undefined {
if (!messageIds) return undefined
const migrate = (ref: string): string => {
const parsed = parseMessageRef(ref)
return parsed !== null ? formatMessageRef(parsed) : ref
}
const byRawId: Record<string, string> = {}
const byRef: Record<string, string> = {}
for (const [rawId, ref] of Object.entries(messageIds.byRawId || {})) {
const normalized = migrate(ref)
byRawId[rawId] = normalized
byRef[normalized] = rawId
}
for (const [ref, rawId] of Object.entries(messageIds.byRef || {})) {
const normalized = migrate(ref)
if (byRef[normalized] === undefined) {
byRef[normalized] = rawId
}
}
return { byRef, byRawId, nextRef: messageIds.nextRef || 1 }
}

function mapForkIds(
state: SessionState,
parent: PersistedSessionState,
parentMessages: WithParts[],
forkMessages: WithParts[],
): ForkIdMap | null {
const parentRefs = parent.messageIds?.byRef
const parentRefs = normalizeParentMessageIds(parent.messageIds)?.byRef
if (!parentRefs) return null

const parentById = new Map(parentMessages.map((message) => [message.info.id, message]))
Expand Down
32 changes: 31 additions & 1 deletion lib/state/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,15 @@ export async function ensureSessionInitialized(
let restored = 0
if (parentSessionId) {
try {
const parent = await loadSessionState(parentSessionId, logger)
// [Issue #407] Load the parent state from the same resolved storage
// directory as the child — with a custom `storagePath`, the parent
// file lives there too, not in the default location. During the
// storagePath transition (files still at the default location,
// warned about above) the parent may be found there instead.
let parent = await loadSessionState(parentSessionId, logger, state.storageDir)
if (!parent && state.storageDir) {
parent = await loadSessionState(parentSessionId, logger)
}
const response = parent
? await client.session.messages({ path: { id: parentSessionId } })
: undefined
Expand Down Expand Up @@ -442,6 +450,28 @@ export async function ensureSessionInitialized(
if (persistedAny._persistedLastCompaction !== undefined) {
state.lastCompaction = Math.max(state.lastCompaction, persistedAny._persistedLastCompaction)
}
// [Issue #407] Reconcile against the persisted compaction boundary. If the
// current history contains a newer completed compaction than the persisted
// state recorded (process restarted between the native compaction and the
// next transform), the transient fields restored above — message refs,
// nudge anchors/baselines, tool cache — are stale: their underlying
// messages were replaced by the compaction summary. updatePerTurnState
// cannot catch this because state.lastCompaction was already set to the
// current boundary before loading (findLastCompactionTimestamp above).
// resetOnCompaction preserves prune.messages (compression blocks) and
// stats by design; the final save below persists the corrected state so
// the next restart starts clean.
const persistedBoundary =
typeof persistedAny._persistedLastCompaction === "number"
? persistedAny._persistedLastCompaction
: 0
if (state.lastCompaction > persistedBoundary) {
resetOnCompaction(state)
logger.info("Restarted after native compaction - reset stale transient state", {
timestamp: state.lastCompaction,
persistedBoundary,
})
}
if (typeof persisted.modelContextLimit === "number" && persisted.modelContextLimit > 0) {
state.modelContextLimit = persisted.modelContextLimit
// Restore the identity pair together with the limit (persisted as a
Expand Down
Loading
Loading