fix(compaction): persist first-turn manual /compact via canonical primitives#545
Conversation
…mitives The previous attempt at this fix (b82fd65c00, stripped from #542 per cohort verdict A — see #541 Step 10) replaced updateSessionStoreEntry with bare updateSessionStore + raw object spread. Codex auto-review surfaced two empirically-valid P2 invariant losses against that shape: 1. Lost mergeSessionEntry semantics. Raw {...storedEntry, ...updates} spread bypasses the canonical merge primitive at src/config/sessions/types.ts:510: - resolveMergedUpdatedAt monotonically clamps updatedAt to >= max(existing, patch, Date.now()) — protects against backward time-travel from concurrent compaction races - sessionStartedAt rolls to the new-session epoch when sessionId changes (existing.sessionId !== sessionId) — load-bearing during /compact since sessionId does roll - stale-modelProvider scrub when model is patched without modelProvider 2. Lost activeSessionKey preserve-from-prune. Old path updateSessionStoreEntry -> persistResolvedSessionEntry -> saveSessionStoreUnlocked({ activeSessionKey: ... }) protected the active session from enforce-mode maintenance / disk-budget cleanup running inside the same lock window. Bare updateSessionStore(storePath, mutator) without opts drops it. Fix mirrors the canonical pattern at recordSessionMetaFromInbound (src/config/sessions/store.ts:706-741): - in-memory: sessionStore[normalizedKey] = mergeSessionEntry(entry, updates) - on-disk: updateSessionStore(storePath, (store) => { const resolved = resolveSessionStoreEntry({ store, sessionKey }); const storedEntry = resolved.existing ?? entry; // resolve-then-merge-or-create store[resolved.normalizedKey] = mergeSessionEntry(storedEntry, updates); ... }, { activeSessionKey: normalizeStoreSessionKey(sessionKey) }) Resolve-then-merge-or-create (storedEntry ?? entry) handles the original bug shape from b82fd65c00: first-turn manual /compact has no on-disk entry yet because the persist runs before any other write; the legacy updateSessionStoreEntry returned null and dropped the count. Falling back to the active in-memory entry merge-or-creates correctly. Test coverage (src/auto-reply/reply/session-updates.compaction.test.ts): - First-turn /compact with empty on-disk store: count persists from active in-memory entry (the original bug shape). - sessionId rollover during compaction: mergeSessionEntry rolls sessionStartedAt to the resolved updatedAt (verified against pre-rollover value). - Multi-compaction merge: counts accumulate; sessionStartedAt stays stable when sessionId is unchanged. Tests for monotonic-updatedAt under concurrent-compaction races and activeSessionKey-preserve-from-enforce-mode-prune are queued for follow-up coverage — they require concurrent-write simulation and enforce-mode trigger staging that's beyond this PR's scope. The current fix uses the same canonical primitives those scenarios depend on, so the structural protection is in place even though the targeted tests are deferred. Refs #541 Step 10 (cohort verdict A: strip-and-relane) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4282753fbb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, | ||
| // activeSessionKey opt protects this entry from enforce-mode pruning / | ||
| // disk-budget cleanup that runs inside the same lock window. | ||
| { activeSessionKey: normalizeStoreSessionKey(sessionKey) }, |
There was a problem hiding this comment.
Pass trimmed normalized key to active-session preservation
updateSessionStore only preserves entries whose key exactly matches opts.activeSessionKey during prune/cap maintenance, but this call passes normalizeStoreSessionKey(sessionKey), which lowercases without trimming. If sessionKey includes leading/trailing whitespace (a case resolveSessionStoreEntry explicitly normalizes), the persisted entry key becomes trimmed while activeSessionKey remains space-padded, so the active-session preserve guard can miss and evict the just-updated session in the same lock window. The previous updateSessionStoreEntry path used resolved.normalizedKey, so this safety guarantee regressed here.
Useful? React with 👍 / 👎.
|
🌊 ronan-seat APPROVE-equivalent (PR-author can't formally approve own PR per gh constraint). Quality byte-walk on PR #545: Both lost-protection-restorations land cleanly: ✓ In-memory: ✓ On-disk: ✓ Imports correctly updated: ✓ Inline comments document BOTH the resolve-then-merge-or-create rationale AND the activeSessionKey-opt protection — fixing the documentation surface that the original ✓ Ancestor checks both pass: v29 SHA Test coverage well-shaped: ✓ Test 1 (first-turn-no-on-disk-entry): real-fs scenario via ✓ Test 2 (sessionId-rollover): relational assertions for sessionStartedAt rollover (not exact-time-equality). Handles timing-realism: in-memory + on-disk merges are separate ✓ Test 3 (multi-compaction merge): explicit Honest queued-test acknowledgment: PR body + test file both explicitly note monotonic- ✓ Commit attribution clean: Verdict: APPROVE-equivalent. Cohort-substrate cycle on the 12th-costume cure works end-to-end:
That's the cohort-substrate cycle as designed. 🐾 |
|
🌻 quality byte-walk + cohort cosign on the canonical-primitives fix. Ancestor + diff
Production fix (
Tests (
Two non-blocking notes
Verdict: APPROVE. Fix is canonical, tests are targeted, lost-protection invariants are restored. 🐾 |
…y opt Per Codex P2 review on PR #545 commit 4282753: resolveSessionStoreEntry computes normalizedKey = normalizeStoreSessionKey(sessionKey.trim()), but the activeSessionKey opt was passing normalizeStoreSessionKey(sessionKey) without the trim — would mismatch any whitespace-padded sessionKey and silently miss the active-session preserve guard during enforce-mode prune / disk-budget cleanup. The legacy updateSessionStoreEntry path used resolved.normalizedKey, so this preservation property regressed in the canonical-primitives port. Same kind of edge as the broader fix this PR exists for — Codex caught what cohort byte-walks would have likely found on subsequent passes. Defense-in-depth working as designed. Note: recordSessionMetaFromInbound (store.ts:740) has the same shape and may need the same trim — queued as separate audit, not folded into this PR's scope. Refs #545 codex-review Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b8a8c06
into
frond-scribe/20260429/v3-cohort-fixes
Summary
Narrow follow-up PR for the compaction-count-persistence fix that was stripped from #542 per cohort verdict (A) (see #541 Step 10 for the full story). Reapplies the fix using the canonical primitives Codex auto-review identified as load-bearing.
The original attempt (
b82fd65c00) replacedupdateSessionStoreEntrywith bareupdateSessionStore+ raw object spread, which dropped two structural protections:mergeSessionEntrysemantics (src/config/sessions/types.ts:510): monotonicupdatedAtclamp +sessionStartedAtrollover on sessionId change + stale-modelProvider scrubactiveSessionKeypreserve-from-prune (src/config/sessions/store.ts:541-543): protects active session from enforce-mode pruning during the same lock windowThis PR mirrors the canonical pattern at
recordSessionMetaFromInbound(src/config/sessions/store.ts:706-741).Test plan
pnpm tsgopassespnpm checkpasses (import-cycles, lint, format)pnpm test src/auto-reply/reply/session-updates.{,lifecycle.,compaction.}test.ts— 5/5 passNew regression coverage at
src/auto-reply/reply/session-updates.compaction.test.ts:updateSessionStoreEntryreturnednulland dropped the count when no on-disk entry existed yet (manual /compact lands before any other persist). The resolve-then-merge-or-create shape fixes it correctly.sessionIdrollover during compaction:mergeSessionEntryrollssessionStartedAtto the resolvedupdatedAt. Verified against pre-rollover value (raw spread would have left it stale).sessionStartedAtstays stable when sessionId is unchanged.Test coverage queued for follow-up (require concurrent-write / enforce-mode-trigger staging beyond this PR's scope; the canonical primitives provide the structural protection regardless):
updatedAtunder concurrent-compaction-write races (would simulate concurrentincrementCompactionCountinvocations and assert diskupdatedAtnever moves backward)activeSessionKeypreserve-from-enforce-mode-prune (would set up enforce-mode disk-budget cleanup that fires inside the lock window and assert the active entry survives)Cohort context
git rebase -i 55df7162c0withGIT_SEQUENCE_EDITOR='sed -i "/^pick c5792eb976/d"'per cohort verdict (A)1500308473165123615), 🌊 (msg1500308822084943972)rebase.classifyspan-emission wiring memo (memo-before-wire) #413 keypath-normalization: a real bug fix benefits from its own narrow PR with targeted test-coverage description, not buried inside a port-PRRefs #541 Step 10
🤖 Generated with Claude Code