fix(sessions): reset compaction state after pop - #4868
Conversation
![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: 11e8d339ad
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11e8d339ad
ℹ️ 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".
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40443083c2
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b0179c6d6
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 086b8455e7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 086b8455e7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 708cdd92bd
ℹ️ 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".
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
CI-equivalent validation update on head 2a9d945: the prior Actions failure on 4044308 was Pyright-only (await_args optional access in tests). I added explicit await_args is not None guards and replicated the workflow environment locally with uv sync --all-extras --all-packages --group dev. Fresh validation: uv run mypy src -> 308 source files, 0 issues; uv run pyright --project pyrightconfig.json --threads 4 -> 0 errors / 0 warnings; compaction-session tests 62/62 PASS; all tests/memory 210/210 PASS; Ruff check/format and git diff --check PASS. No production behavior changed in this last commit. @codex review
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff5e264d08
ℹ️ 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".
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The new clear_session() exception path treats the underlying history as ambiguous by dropping the cached history and advancing _history_generation, but it leaves the retained Responses chain intact. That seems inconsistent with the pop_item() cancellation path immediately above, which calls _invalidate_response_chain() because a destructive mutation may have settled before cancellation is re-raised. The same can happen for clear_session(): if the underlying clear commits and then raises/cancels, _response_id still points at the pre-clear history and a later previous_response_id compaction can use that stale chain. I would invalidate the response chain in this ambiguous clear failure path as well, and add the same settle-before-cancel regression used for pop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dabfa1365c
ℹ️ 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".
|
Review reconciliation update for the |
|
@codex review Please review the current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd3b0ee1ce
ℹ️ 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".
|
@codex review Please review current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65905d5b15
ℹ️ 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".
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Exact-head follow-up on 26fa2adf40d64330a0a41c893d180e5a47df03c4: the latest P1/P2 deferral ownership findings are addressed with deterministic RED→GREEN regressions. Please review this exact head, especially the separation between destructive invalidation and ordinary response/history advancement in _defer_compaction(), and whether establishing deferred response ownership before candidate loading closes the stale older-compaction window without weakening final replacement serialization.
Validation: 73/73 focused compaction-session tests, 221/221 tests/memory, Ruff check/format PASS, targeted mypy 0 issues, targeted Pyright 0 errors/0 warnings, git diff --check PASS.
@codex review
|
Codex Review: Didn't find any major issues. Hooray! 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". |
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Exact-head follow-up on 74139eb4ba7c11952becb6a24a537f6a518b5240: fixed the remaining concurrent forced-compaction retry resurrection race with a dedicated compaction-publication generation. Deterministic regression is RED on 26fa2adf and GREEN here. Fresh exact-head validation: focused compaction session 74/74 PASS; all tests/memory 222/222 PASS; Ruff check/format PASS; mypy 0 issues; Pyright 0 errors/0 warnings; git diff --check PASS. @codex review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74139eb4ba
ℹ️ 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".
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Exact-head follow-up on ee829d0653b0b39f9e4b992b972a70edd0ca6c90: adversarial audit found and fixed one additional ownership failure not raised in prior reviews. A forced compaction previously consumed _deferred_response_id before responses.compact(); if the remote request then raised, no compaction was published but the deferred retry was lost. The fix restores the consumed marker only when destructive invalidation has not occurred, no newer compaction has published, and no newer deferral has appeared.
Deterministic regression test_forced_compaction_failure_restores_deferred_retry is RED on 74139eb4 (None after RuntimeError) and GREEN here. Fresh exact-head validation: 75/75 focused compaction-session tests, 223/223 tests/memory, Ruff check/format PASS, mypy 0 issues, Pyright 0 errors/0 warnings, git diff --check PASS.
Please review the exception/cancellation ownership semantics on this exact head. @codex review
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Re-reviewed exact ee829d0. The new exception/cancellation restoration is sound, but one deferred-retry ownership gap remains on this head. The stale-result restoration branch still requires response_chain_generation == self._response_chain_generation. Ordinary response-ID advancement changes that generation without destructive invalidation. If a forced compaction consumes a deferred marker, a concurrent newer response advances the ID and declines compaction, and the older remote result is then rejected, the marker is not restored even though no compaction succeeded. The retry obligation can therefore still disappear. Please base that restoration decision on destructive invalidation plus compaction-publication generation rather than ordinary response-ID generation, and add a deterministic regression for this interleaving.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee829d0653
ℹ️ 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".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
The stale response chain after pop is worth fixing, but this implementation has grown beyond that boundary. There is still a lost-deferred-work ordering: A consumes the deferred marker and waits for compact; B records a newer response but declines compaction; A then returns as stale without restoring the marker, although no replacement completed. Please reset the design around the serialized mutation boundary in #4736 and add pop/clear response-chain invalidation there. I would not add another generation exception to the current four-counter design. Keep the regression focused on the surviving history and deferred work.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d654fee28
ℹ️ 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".
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Exact-head review request after reconciling with current main / upstream #4736. Head: e172f74bc732982cbc723cb1bc6ece15913cf5d0. The implementation now preserves upstream mutation serialization and Runner _mutation_generation handoff, while adding only destructive Responses-chain invalidation, old-response rejection behind destructive mutation, stale deferred-publication rejection, and synchronous deferred retry restoration on compact API failure. Fresh exact-head local validation: 68/68 focused, 218/218 tests/memory, Ruff check/format PASS, mypy 0 issues, Pyright 0 errors/0 warnings, diff check against current main PASS. @codex review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e172f74bc7
ℹ️ 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".
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Exact-head follow-up after addressing the new stale-cache P1 on 141d1670ac9a1c1aad101031a53158fff8c21d55. RED on e172f74b: _defer_compaction() returned without a marker after destructive pop but left pre-pop _session_items published; a later input compaction could reuse deleted history. GREEN here: deferred history loading is non-publishing, destructive invalidation aborts publication, ordinary append drift retries against current history, and the tightened regression verifies a later input compaction sees input=[]. Validation: 69/69 focused, 219/219 memory, Ruff/format PASS, mypy 0, Pyright 0/0, diff check PASS. @codex review
|
Codex Review: Didn't find any major issues. Swish! 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". |
|
@seratch Reworked this PR around the serialized boundary from #4736 as requested in your review. Current exact head is |
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Exact-head follow-up on 2a18ce3745dedda99b07512b6e57fa6c766fe46d. The branch remains on the serialized #4736 architecture requested in the prior CHANGES_REQUESTED review: no new ownership counter was added. Adversarial testing found one residual gap specific to manual compaction calls without Runner's generation handoff: an old response_id request queued behind an append could compact the stale server chain after the append completed. The fix snapshots the existing _mutation_generation before waiting for _mutation_lock and skips manual compaction if history changed while queued. Deterministic RED on 141d1670: responses.compact() was awaited once; GREEN here: it is not called and [old, new] survives. Exact-head validation: 70/70 focused, 220/220 memory, Ruff/format PASS, mypy 0, Pyright 0/0, diff-check PASS. Please re-review this exact head, including whether this closes the remaining manual-call ordering gap while preserving #4736's single serialized ownership model. @codex review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a18ce3745
ℹ️ 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".
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Exact-head follow-up on 7e6734873ceff00d071bc29e3a9fe13c20c2f000, integrated with current main cb808dfaee4f5f6bc256abc4f6f021a4693c397c. Addressed both latest Codex findings without adding a new ownership counter: stale manual calls are rejected only when they resolve to previous_response_id, and that rejection invalidates the retained response chain; queued explicit input mode remains eligible and reads post-append history under the existing serialized lock. Deterministic RED→GREEN coverage added for both. Exact-head validation: 72/72 focused, 222/222 memory, Ruff/format PASS, mypy 0, Pyright 0/0, diff-check PASS. Please re-review this exact head, especially the narrowed manual guard and fail-closed chain invalidation. @codex review
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review Please review the current head |
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
3 participants
Summary
Fixes #4867.
Reconciled onto current
mainafter upstream #4736 (2bbe535a) changed compaction ownership. This PR preserves upstream's full_mutation_lockserialization and Runner_mutation_generationhandoff, and adds destructive Responses-chain invalidation for pop/clear plus narrowly-scoped stale manual response-chain rejection.Current behavior:
pop_item()invalidates_response_id,_deferred_response_id, and_last_unstored_response_id;CancelledErrorfrom pop is treated as an ambiguous destructive outcome and invalidates retained chain state before re-raising;run_compaction(response_id=...)call that began before destructive mutation cannot reclaim an old chain after waiting on_mutation_lock;_mutation_generationbefore waiting for the lock; if history changes while queued, only compactions that resolve toprevious_response_idare rejected, and the retained response chain is invalidated before return;inputmode remains eligible after a queued append and reads the post-append history under the serialized lock;clear_session()and clear failures/cancellation invalidate retained response-chain state conservatively;_defer_compaction()does not publish candidate/session caches from an unlocked history read;responses.compact()fails or is cancelled after consuming a deferred retry, the marker is restored synchronously while upstream's mutation lock is still owned, then the original exception/cancellation is re-raised.Upstream #4736 serializes compaction against wrapper mutations and carries a Runner generation from history read through append/compaction. The older PR implementation's response/history/deferred/compaction generation machinery for overlapping writes was removed during conflict reconciliation rather than maintaining two competing ownership models. The manual-call guard reuses the same
_mutation_generation; it does not introduce another ownership counter.Implementation remains confined to the compaction-session class and its tests; GitHub reports 2 changed files.
Regression coverage
Deterministic rebased regressions cover:
responses.compact();[old, new]after the append;These run alongside upstream #4736's mutation-serialization and Runner generation-handoff regressions.
Validation
Current exact head:
7e6734873ceff00d071bc29e3a9fe13c20c2f000.Current
mainintegrated:cb808dfaee4f5f6bc256abc4f6f021a4693c397c.Fresh Windows validation on the exact integrated tree:
tests/memory/test_openai_responses_compaction_session.py: 72 passed;tests/memory: 222 passed;git diff --checkagainst current main: PASS.Contributor-side validation is not represented as upstream CI PASS unless GitHub Actions actually runs successfully.