🤖 fix: read compaction epochs from archived history#3566
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3445e47f6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ThomasK33
force-pushed
the
memory-consolidation-2m4s
branch
from
a3445e4 to
359c631
Compare
|
@codex review Please take another look. I addressed the duplicate rotation replay case with historySequence dedupe and added a regression test. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 359c631071
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ThomasK33
force-pushed
the
memory-consolidation-2m4s
branch
from
359c631 to
6d931ef
Compare
|
@codex review Please take another look. I addressed the archive+active scan locking concern and added regression coverage for the lock behavior. |
Memory harvest reads compaction epoch evidence after sealed-history rotation. The epoch can now span chat-archive.jsonl and chat.jsonl, so use the archive-aware full-history iterator instead of passing the workspace ID to the file-path iterator. Validation: - bun test src/node/services/historyService.test.ts - bun test src/node/services/memoryConsolidationService.test.ts - make typecheck - MUX_ESLINT_CONCURRENCY=1 make static-check --- Generated with mux • Model: openai:gpt-5.5 • Thinking: xhigh • Cost: $3.31
|
@codex review Please take another look after the rebase onto latest main. |
ThomasK33
force-pushed
the
memory-consolidation-2m4s
branch
from
6d931ef to
cb3c846
Compare
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Fix memory-harvest compaction epoch reads after sealed-history rotation by using the archive-aware full-history iterator when gathering evidence for a completed compaction boundary, while holding the history lock and deduplicating crash-replayed rows.
Background
Recent main failures clustered around
HistoryService.getMessagesForCompactionEpochand the memory harvest pipeline. The history service's privateiterateForwardhelper now expects a file path afterchat-archive.jsonlrotation work, but this callsite still passed a workspace ID. That made the epoch scan read nothing, so harvests failed before the mocked harvest stream could start.Implementation
getMessagesForCompactionEpochnow iterates the full logical history in forward order. This covers bothchat-archive.jsonlandchat.jsonl, which is required because a just-compacted epoch can straddle the archive seam after sealed-history rotation. The scan runs under the workspace file lock so archive+active reads form one stable snapshot, and collection deduplicates byhistorySequenceso a crash between archive append and active-file rewrite cannot feed duplicate evidence rows into memory harvest.Validation
bun test src/node/services/historyService.test.tsbun test src/node/services/memoryConsolidationService.test.tsmake typecheckMUX_ESLINT_CONCURRENCY=1 make static-checkRisks
Low. This only changes the compaction-harvest evidence read path from the accidentally wrong file-path call to the existing archive-aware logical-history iterator, with defensive locking and dedupe for documented rotation edge cases. The tradeoff is an O(total history) scan under the workspace file lock for harvest recovery, but this path runs after compaction boundaries rather than on hot provider request assembly.
Pains
The default
make static-checkrun OOM-killed ESLint at concurrency 8 in this workspace; rerunning the required target withMUX_ESLINT_CONCURRENCY=1passed.Generated with
mux• Model:openai:gpt-5.5• Thinking:xhigh• Cost:$3.31