fix(sessions): preserve writes that land while responses.compact is in flight - #4680
fix(sessions): preserve writes that land while responses.compact is in flight#4680Om-singhaI wants to merge 14 commits into
Conversation
…n flight run_compaction snapshots the history, awaits the compact call, then rewrites the session from that snapshot inside the mutation lock. The snapshot goes stale during the call, so a concurrent add_items was silently deleted and a concurrent clear_session was resurrected as the compacted summary. The snapshot is now captured while the lock is held, and verified before the replacement. When the freshly read history still starts with the snapshot, items appended during the request are carried over after the compacted output. Otherwise the history diverged mid flight, so the replacement is skipped, the caches are invalidated and a warning is logged. Holding the lock across the compact call instead would serialize every add_items behind a network call that takes seconds.
![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: 8dd60cf52a
ℹ️ 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".
…cross the lock wait The prefix check compares two empty lists when the snapshot was taken on an empty session, so a clear_session during the request went unseen and the compacted output repopulated the cleared session. clear_session and pop_item now bump a generation counter under the lock, captured with the snapshot and compared before the replacement. resolved_mode was derived from the response id at entry, but the id was read again after the lock wait, where a second call may have overwritten it. The id is captured once beside the mode resolution and used from there on.
|
Both findings were right, addressed in e0566c1. The empty snapshot case: The response id: it is now captured once, next to the mode resolution, and everything after the lock wait uses that local. New test holds the lock, lets a second call overwrite the shared id, and asserts the first compact call still goes out with the id its mode was resolved from. Both tests fail on the previous commit and pass now. 61 in the file, 209 in tests/memory, lint and typecheck clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0566c11f6
ℹ️ 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".
Two overlapping compactions that both snapshot an empty session slipped the divergence check: the first replacement rewrote history without advancing the generation counter, so the second treated the first output as a concurrent tail and persisted both outputs concatenated. A successful replacement now bumps the counter, and the second call skips instead.
|
Right again, fixed in the commit on top. A successful replacement now bumps the same generation counter, since it is itself a rewrite of stored history, so the second of two overlapping compactions detects it and skips instead of absorbing the first output as a tail. New test runs two gated compactions over an empty session and asserts exactly one output persists; it fails on the previous commit with both outputs concatenated. 62 in the file, 210 in tests/memory, gates clean. |
|
I ran an independent cross-PR integration audit at exact heads The fixes are complementary rather than alternatives:
The combined contract therefore needs one ordered transaction boundary, not a mechanical conflict resolution:
That ordering preserves the non-blocking provider await and generation/CAS semantics in this PR while also preventing a compacted visible window from replacing hidden history. I can contribute a compact combined regression matrix once the maintainer chooses the landing/rebase owner; I have not opened a competing change. |
|
Thanks for running that, it matches my reading exactly. The limited history cases are #4628's territory and this PR deliberately leaves them alone: it changes what happens around the provider await, not which items the decision hook gets to see. So the conflict between the two heads is textual, not semantic, and your step list reads to me as #4628's read window fix inside the first locked block with this PR's generation and tail handling kept as is around and after the await, which is exactly how I would expect them to compose. Happy to rebase this on top of #4628 if it lands first, or the other way around, whichever ordering seratch prefers. A combined regression matrix once that is settled sounds genuinely useful. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49f34b9d7b
ℹ️ 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".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for iterating on this. There is still a correctness gap in previous_response_id mode.
Run A can capture response_id=A and then wait for _mutation_lock. If run B appends its turn before A acquires that lock, A's snapshot includes B even though responses.compact(previous_response_id=A) cannot include B. baseline_count then classifies B as part of the compacted baseline, and the replacement drops it. The prefix and generation checks cannot detect this because no mutation occurred after A's late snapshot.
Please reset the design around the ownership boundary: pair each response ID with the exact local item boundary when that response's batch is persisted, or serialize append and compaction at the owning layer. Then preserve only items after that recorded boundary. Please add a controlled-ordering regression for A response -> B append -> A snapshot/compact -> replacement. I do not think another _destructive_generation special case can make the current late snapshot authoritative.
…oundary previous_response_id compaction covers the server side history through one response only, but the replacement classified everything in the snapshot as the compacted baseline. When another run appended its turn between that response's persisted batch and the late snapshot, the prefix and generation checks saw no rewrite and the replacement dropped the newer turn. The runner now persists each response's batch through a session hook that records the exact local item count in the same locked region as the append, pairing the response id with its ownership boundary. run_compaction reads the recorded boundary under the lock and preserves every item past it, so turns appended after the batch survive no matter when the snapshot happens. Without a recorded boundary, direct calls and input mode keep the snapshot length as the boundary, since their compaction input is captured under the same lock as the snapshot. clear_session and pop_item drop recorded boundaries the same way they already invalidate the caches, so a destroyed history can never feed a stale count into a replacement. A successful replacement translates the surviving boundaries instead of clearing them, shifting each count past the rewritten prefix onto the new history, so overlapping previous_response_id compactions still preserve the turns past their own boundaries. A boundary that ended inside the rewritten prefix has no counterpart afterwards; it is kept as a tombstone and a later compaction keyed on it skips its replacement, because falling back to its snapshot would classify newer turns and the earlier summary into its baseline and drop them. Resumed pending session writes route through the same hook when the resume itself performs the append and a response id is known, so those batches record boundaries too. The new regressions drive the exact orderings from review: a turn persisted between a response's batch and its late snapshot survives that response's replacement, a second compaction lands on its translated boundary after an overlapping replacement and keeps the newer turn, and a compaction whose recorded prefix was rewritten away skips instead of dropping the newer turn and the earlier summary. Without the source change each ordering loses items.
|
You are right, once B's append lands before the snapshot there is nothing left for the checks to catch. I went with the first option you describe, fixed in 833ed99. Each response's batch now goes through a session hook that records the local item count in the same locked region as the append, so the response id and its boundary are paired at persist time. The replacement preserves everything past the recorded boundary instead of the snapshot. After a successful replacement the surviving boundaries are translated onto the rewritten history, so overlapping compactions stay sound, and a boundary whose prefix was rewritten away makes the later compaction skip instead of guessing from the snapshot. New test test_turn_persisted_before_late_snapshot_survives_replacement drives exactly your ordering, A's response persisted, B's turn appended, then A snapshots, compacts, and replaces. Two more cover the overlapping orderings. All three fail without the source change. 65 in the file, 213 in tests/memory, runner suites and gates clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 833ed99cfd
ℹ️ 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".
An id with no entry in the boundary map fell back to the snapshot length, which is only sound for direct callers that never record boundaries. The map is capped at 50 entries with the oldest evicted first, so a compaction delayed past 50 newer persisted batches lost its entry, took the fallback, and its replacement silently swallowed every newer turn into a summary covering the old response alone. clear_session and pop_item opened the same window by wiping the map while a compaction keyed on a recorded response was still pending. The session now remembers whether any boundary has ever been recorded, set in the same locked region as the registration. In previous_response_id mode an absent entry then skips exactly like a tombstone, under the same lock hold and before the billed compact call, because eviction or a wipe may have dropped a recorded entry and guessing from the snapshot would claim newer turns for that response. The flag is never reset: a compaction keyed on a response recorded before a clear must still skip after the clear. Sessions that never record boundaries keep the snapshot fallback, so the manual add_items then run_compaction workflow is untouched. The new regressions drive both orderings: a boundary evicted past the cap by newer persisted batches makes the delayed compaction skip with the newer turns intact, and a compaction keyed on a response recorded before clear_session skips instead of replacing the turn persisted after the clear. Without the source change both replace newer history with a stale summary.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c755d21891
ℹ️ 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".
| """ | ||
| async with self._mutation_lock: | ||
| await self._add_items_locked(items) | ||
| boundary = len(await self._get_all_underlying_session_items()) |
There was a problem hiding this comment.
If the new full-history read is cancelled or transiently fails after _add_items_locked() succeeds, the response batch is already durable but _add_items_for_response() propagates the failure without recording its boundary. Normal non-resumed saves have no pending-write checkpoint to reconcile this state, so the run appears to have failed while its output remains in the session, and retrying can persist the logical turn again. Compute the boundary before the append or otherwise ensure boundary bookkeeping cannot fail after the successful write.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
_add_items_for_response paired a response id with its item count by reading the full history back after the batch was appended. When that read was cancelled or failed transiently, the batch was already durable but the error still propagated, so the run treated a persisted turn as failed and a retry would persist the logical turn again. The boundary and the ever recorded flag were also left unset, and the plain save path keeps no pending write checkpoint that could reconcile the state. The hook now reads the count before the append and seeds the boundary as that count plus the batch length. The mutation lock excludes every other writer for the whole region, so the sum equals the count the read after the append used to return. Once the append succeeds the recording is pure assignment that cannot fail, leaving nothing that can raise between the durable write and its bookkeeping. When the read itself fails the append has not started, no boundary is recorded, the ever recorded flag stays untouched, and a retry begins clean. The resumed pending write path records boundaries through the same hook, so it follows the same order without further changes. The new regression persists a batch through the hook against a store that fails every read issued after an append: the call completes, the batch lands exactly once, the recorded boundary matches the count a read after the append would have produced, and the compaction keyed on the response preserves the turn persisted past it. Without the source change the injected read failure propagates out of the hook after the durable write.
…kkeeping The deferral decision filled the wrapper caches without taking the mutation lock, so a cold cache fill racing a replacement could resolve against the mid replacement store and clobber the caches the replacement had just installed under the lock. An input mode compaction would then read that torn cache as its input and replace real history with a summary that never saw it. _defer_compaction now runs its whole decision under the mutation lock, and run_compaction clears the deferred id inside its locked decision region so a deferral landing after the decision is not wiped. A failed replacement transaction leaves the store to a best effort restore, but it used to keep every recorded boundary and an unchanged generation counter, describing a history that may no longer exist. A retried compaction keyed on the same response could then slice a shorter store past its end and silently delete a batch persisted while the retry was in flight. The failure path now counts as a rewrite and drops all recorded boundaries, matching pop_item and clear_session, and as an independent guard a recorded boundary larger than the stored history is treated as corrupt state that skips the replacement and invalidates the map. The refreshed caches are built before the replacement write, so nothing that can raise sits between the durable write and its bookkeeping assignments. Cleanups that fell out of review: the None tombstone state had become behaviorally identical to an absent entry once the ever recorded flag existed, so boundary translation now drops rewritten entries outright, the map narrows to dict[str, int], and tombstones stop consuming eviction cap slots that belonged to live boundaries. The boundary hook dispatch that was duplicated across the save and resume paths moved into _session_add_items behind an optional response id. Comments now say what the generation counter and the prefix comparison each uniquely catch, the boundary contract lives in one place with short pointers elsewhere, and the sessions doc describes the shipped concurrency behavior instead of warning about the overwrite this branch removed. New regressions pin each behavior: the deferral parks at the mutation lock and a concurrent deferral survives a running compaction, a failed replacement drops its boundary state and the retried compaction skips, an oversized boundary skips instead of replacing outright, a cache build failure leaves the store untouched, and boundary translation is observed at a nonzero shift with the shifted value asserted directly.
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the substantial follow-up work. I re-reviewed the current head against the full Session persistence and compaction lifecycle.
There is still a blocking ownership gap in _add_items_for_response(): the response batch is appended durably before the subsequent full-history read computes and records its boundary. If that read is cancelled or fails transiently, the run reports failure after the batch has committed, but no response boundary exists and the normal non-resumed path has no pending checkpoint to reconcile the result. A retry can then persist the same logical turn again.
Please make the response boundary computable and owned before the durable append, or otherwise ensure that no fallible operation remains between a successful append and boundary registration.
This is now another failure in the same response-boundary abstraction after several incremental fixes. Please treat it as a complexity-reset point: collapse the design back to one response-to-batch ownership record under the existing mutation boundary instead of adding another recovery flag or special case.
|
I actually caught this a few hours before your review, when the automated pass flagged the same read, and held the push back so I could go through the whole lifecycle properly instead of sending another quick patch. Fixed in 901a543: the count is read before the append, both under the same lock hold, so registration after the durable write is plain assignment. If the pre read fails nothing was appended and a retry starts clean. The complexity reset is in 21768f2. Tombstones are gone, so it is back to one map from response id to item count: recorded with the append, translated by a successful replacement, dropped by anything destructive, including a failed replacement now. No new flags or special cases. test_persisted_batch_survives_read_failure_after_append drives your exact ordering. 74 in the file, 222 in tests/memory, gates clean, and the PR description now matches the final design. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21768f293f
ℹ️ 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".
|
The automated pass flagged one more window: a turn persisted by another run while this run's request is in flight lands below the recorded boundary, so a later replacement would drop it. I have a fix built for it that completes the ownership record the way you described, captured when the request input is read, and recording nothing when anything interleaves so the compaction skips instead of guessing. It stays one map with no new state and every ordering is pinned by a test. Before pushing another round though, I want to check the direction with you. If you would rather I collapse this to the serialized append and compaction you suggested earlier, I am happy to do that reset instead, it removes most of this machinery outright. Which would you prefer? |
When run A's request was in flight and run B persisted its turn first, A's boundary was recorded from the count at persist time, which included B's items. The server side history through A cannot contain a turn written after A's request went out, so the replacement keyed on A sliced B's turn away. The runner now captures an ownership token when it reads the session to build a run's request input: the underlying item count and the destructive generation, taken under the mutation lock before the read. The token threads through the run's persists and advances only for the run's own appends. At persist the boundary hook records the response's boundary only when both values still match, which proves the store holds exactly the history the request input was built from plus the run's own batches. Anything interleaved means nothing is recorded, and a compaction keyed on that response skips through the ever recorded gate before the billed call. The map from response id to item count stays the only bookkeeping and the session gains no new fields. The token lives inside one run, and resumed or direct callers never have one, so the snapshot fallback and input mode are untouched.
|
I’d prefer the request-input ownership record. The authority boundary is A’s session read, not A’s later append. Serializing append and compaction alone still fails Please keep this as the frozen reset contract: capture an attempt-local version or boundary atomically with the session history used for the request, carry it explicitly to the response-save path, and under the same mutation lock append A but register |
A run whose session settings set a limit reads only the newest window of stored history, and a session_input_callback can drop stored turns from the prepared input outright. Either way the request carries less than the store while the count at persist time spans all of it, so the recorded boundary claimed prefix items the server never saw and a previous_response_id replacement deleted them, with no concurrency involved. Input preparation now voids the run's ownership token at the exact sites where coverage is lost: after a windowed read that returned a different count than the token captured, and before a session input callback runs. A voided token records nothing at persist; it also marks the session as boundary managed, so a compaction keyed on the run's responses skips through the absent entry before the billed call even on a fresh wrapper over a restored store, where the snapshot fallback would otherwise have classified the whole history as covered. A limit that admits the entire store keeps recording: the windowed read returning exactly the captured count, together with the token's generation and count checks at persist, proves the request input covered every stored item. Runs with no limit and no callback are untouched.
A persist whose ownership token went stale from an interleaved write recorded nothing but also left the ever recorded gate unarmed. On a fresh wrapper where nothing was ever recorded, the first ever response could go out, have a plain add_items land another writer's item mid flight, and persist with nothing recorded and the gate still down; the compaction keyed on that response then fell through to the snapshot fallback, classified the interleaved item as covered, and deleted it in the replacement. Any persist that carries a token now arms the gate, whatever state the token is in, because a token exists only inside a runner managed run. Hookless persists still leave the gate alone, preserving the snapshot fallback for direct callers who persist before compacting and own that ordering. Two request rewriting hooks could still overstate coverage after the limit and callback guards landed. RunConfig.call_model_input_filter runs on every request and can drop stored history from it, and Handoff.input_filter or RunConfig.handoff_input_filter can rewrite the accumulated history mid run, so every request after the handoff omits turns the store keeps. Either way the count at persist time spanned the whole store while the server side history did not, and a previous_response_id replacement deleted the difference. Invoking either filter now voids the run's ownership token at the application site, unconditionally and with no comparison of the filter's output, mirroring the session_input_callback precedent: such runs record no boundaries and their compactions skip before the billed call. The streamed loop gains the interleaving regression that mirrors the non streamed ordering, run A reads, run B saves mid flight, run A saves, and the limit guard gains a regression where the truncating limit comes from the session's own session_settings through the same resolve() path as the RunConfig variant.
…ppends Nested handoff history folds the accumulated history the next requests are built from into a rendered transcript while the session keeps the original turns, so a count at persist time can no longer prove what the server saw. The traced run showed the post handoff request going out as one synthesized message wrapping the stored turns as text, the persist recording a boundary spanning the whole store, and the forced compaction keyed on that response replacing the lossless stored items with a compaction of the rendering; summarized tool items survive only as text there and a custom handoff_history_mapper may drop anything outright. Applying the nesting now voids the run's ownership token at the application site, unconditionally and with no comparison of the nested output, exactly like the handoff input filter branch: such runs record no boundaries and their compactions skip before the billed call with the store intact. Arm the ever recorded gate before the append in _add_items_for_response for every token carrying persist. The backend can commit a batch and still raise before acknowledging, and the arm sat after the append, so that persist surfaced an error with the gate still down while the store held the batch; a compaction keyed on one of the run's responses could then reach the snapshot fallback and claim items the server side history never contained. Arming first is strictly conservative: when the append never committed it costs at most a skipped compaction, and a read that fails before the append still leaves the gate untouched. Both regressions fail before the fix, the nesting run through the replaced store and the recorded boundary, the committed then failing append through the unarmed gate.
|
That matches what I built, gap for gap. The token is captured under the mutation lock right before the session read, so a write landing between capture and read can only make the token stale, never claim unseen items, and at persist the append and the conditional registration share one lock hold. A response whose token went stale records nothing and its previous_response_id compaction skips before the provider call. No lock is held across a network await. Your first interleaving point was a real hole in my draft: a stale persist on a wrapper with nothing ever recorded fell through to the snapshot fallback. Every persist that carries a token now arms the skip gate, in whatever state, so only sessions that never went through the runner keep the fallback. Closing it also led me to the same loss shape everywhere the request can shrink without the recorded coverage shrinking: a truncating session limit, session_input_callback, call_model_input_filter, handoff input filters, and handoff history nesting all void the token now, so those runs record nothing rather than a boundary the server never saw. Event gated tests cover the clean path and A read, B save, A save in both streamed and non streamed runs, plus each void site with the deletion visible when the guard is removed. 90 in the file, 238 in tests/memory, runner suites and gates clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 225ee41aa9
ℹ️ 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".
| limit=resolved_settings.limit, | ||
| wrapper=wrapper, | ||
| ) | ||
| if ownership_token is not None and len(history) != ownership_token.count: |
There was a problem hiding this comment.
When this decorator wraps a backend such as SQLiteSession(session_settings=SessionSettings(limit=2)), it exposes the inherited SessionABC.session_settings = None rather than the underlying setting, so resolved_settings.limit is None and this invalidation is bypassed even though get_items(None) returns only the limited suffix. The ownership token was captured against the full history, and its count still matches the full store at persistence time, causing the response boundary to include the omitted prefix; previous_response_id compaction can then delete history that the model never received. Proxy the wrapped session's settings or invalidate ownership whenever the returned history length differs from the captured count.
AGENTS.md reference: AGENTS.md:L145-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and your second option is the better shape. The decorator inherits SessionABC.session_settings = None instead of proxying the backend it wraps, so a limit living on the wrapped session resolved as None above the wrapper while the backend still applied it to get_items(None) and returned only the newest turns; the guard keyed on the resolved value never fired, the token had been captured over the full store, its count still matched at persist, and the replacement deleted the omitted prefix with no concurrency involved. Fixed in 7bfbb96.
The check no longer asks whether a limit was configured, or where. It sits after both read paths and voids the token whenever the history read for the request comes back a different length than the count the token captured, which covers a limit on the wrapped session, any other backend that windows its own reads, and any future setting of that shape. It replaces the resolved limit condition rather than sitting beside it, so the block stays the same size with one condition instead of two.
The comparison is against the raw returned list, before the normalization and dedupe steps that legitimately merge items with no coverage lost, and against the underlying count the token captured, so an ordinary session's unlimited read matches item for item and still records. New test test_wrapped_session_limit_records_nothing_and_compaction_skips drives your scenario, a backend carrying its own SessionSettings(limit=2) with no limit on the wrapper or the run; it fails on the previous commit with the whole store replaced by the compaction output and every prior turn gone. test_unwindowed_run_still_records_and_replaces and test_limit_covering_whole_store_still_records_and_replaces pin the other side. 91 in the file, 239 in tests/memory, runner suites and gates clean.
The compaction decorator inherits SessionABC.session_settings = None rather than proxying the session it wraps, so a limit configured on the wrapped backend, as in SQLiteSession(session_settings=SessionSettings(limit=2)), resolved as None above the wrapper while the backend still applied it to get_items(None) and returned only the newest turns. The guard that voids the run's ownership token asked whether a limit resolved at the wrapper, so it never fired: the token had been captured over the full store, its count still matched at persist time, and the response recorded a boundary spanning history its request never carried. The previous_response_id compaction keyed on that response then replaced the covered prefix and deleted turns the server side history never contained, with no limit visible anywhere above the backend and no concurrency involved. Stop asking where a limit was configured. The check now sits after both read paths and voids the token whenever the history read for the request differs in length from the count the token captured, wherever the difference came from. That covers a limit on the wrapped session, any other backend that windows its own reads, and any future setting of that shape, and it removes the resolved limit condition rather than adding a second one beside it. The comparison is against the raw returned list, before the normalization and dedupe steps that legitimately merge items with no coverage lost, and against the underlying count the token captured, so an ordinary session's unlimited read matches item for item and still records. A limit generous enough to admit the whole store matches too, and the control regressions for both pin that. The wrapped limit regression fails before the fix with the whole store replaced by the compaction output and every prior turn gone.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bfbb9689f
ℹ️ 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".
| on_response_accepted=_commit_pending_server_response, | ||
| on_response_hooks_started=_mark_response_hooks_started, | ||
| run_state=run_state, | ||
| ownership_token=session_ownership_token, |
There was a problem hiding this comment.
When a SandboxAgent capability uses the public Capability.process_context() hook to filter stored history, sandbox_runtime.prepare_agent() replaces original_input after the token was captured, but this unchanged token is still forwarded here. The following persist can therefore record a boundary covering the full session even though the model request omitted part of it, and previous_response_id compaction can silently delete that omitted prefix. Invalidate the token whenever sandbox preparation rewrites the input, as is already done for the other input-filtering hooks; the streamed path has the same stale-token forwarding.
AGENTS.md reference: AGENTS.md:L147-L149
Useful? React with 👍 / 👎.
|
Thanks @seratch for the patience across all those rounds, and for taking it the rest of the way in 4736. The generation based ownership reads cleaner than mine, and the co author credit means a lot. @FU-max-boop, thanks for writing the contract out, it made the last round much easier to get right. Leaving this open for you to close with 4736. Happy to help on the compaction paths any time. |
3 participants
Summary
This pull request fixes
OpenAIResponsesCompactionSession.run_compactiondestroying session writes that land while theresponses.compactrequest is in flight.Before this change,
run_compactionsnapshotted the session history, awaitedresponses.compact, and then cleared and rewrote the underlying session purely from that stale snapshot, so a concurrentadd_itemsduring the request was silently deleted and a concurrentclear_sessionwas undone. The runner triggersrun_compactionafter every turn save, so two overlappingRunner.runcalls on one session could hit this window in normal operation.A snapshot alone cannot say which items a
previous_response_idcompaction covers, so the session now records that explicitly. When the runner persists a response's batch, the session pairs the response id with the exact local item count at that moment. The count is computed before the append and both happen in one locked region, so no other writer can slip items in between and nothing can raise between the durable write and its bookkeeping.run_compactionreads the recorded boundary under_mutation_lockand, after the request returns, replaces only the covered prefix: every item past the recorded boundary survives, including turns other runs appended before the snapshot was taken. A successful replacement translates the surviving boundaries onto the rewritten history, so overlapping compactions still preserve the turns past their own shifted boundaries, and it drops the boundaries that ended inside the rewritten prefix. Once any boundary has been recorded on the session, a compaction keyed on a response without an entry skips with a warning instead of guessing from the snapshot. The snapshot length remains the boundary only for sessions that never record boundaries, meaning directrun_compactioncallers and input mode, whose compaction input is captured under the same lock as the snapshot.Destructive concurrent operations stay conservative.
clear_sessionandpop_itembump a generation counter and drop every recorded boundary, a history that diverged from the snapshot skips the replacement with a warning, a failed replacement transaction drops the recorded boundaries and counts as a rewrite because its restore is best effort, and a recorded boundary larger than the stored history is treated as corrupt state and skips instead of slicing past the end. The deferral decision and its cache fill run under the mutation lock as well, so they cannot interleave with a replacement. Insrc/agents/run_internal/session_persistence.py, the save path and the resumed pending write path now append through one shared helper that dispatches to the session's boundary hook whenever a response id is known, so both paths record boundaries the same way.Test plan
uv run pytest tests/memory/test_openai_responses_compaction_session.py: 74 passed. The concurrency regressions drive deterministic interleavings gated on events rather than timing: a turn persisted between a response's batch and a late snapshot survives that response's replacement, boundaries translate at a nonzero shift and the shifted value is asserted directly, a compaction whose recorded prefix was rewritten away skips without a billed call, a failed replacement drops its boundary state so a retried compaction cannot slice a shorter store past its end, the deferral decision parks at the mutation lock instead of filling caches mid replacement, and a cache build failure surfaces before the store is rewritten. tests/memory, tests/test_agent_runner.py, and tests/test_agent_runner_streamed.py all pass, andruff format --check,ruff check,mypy src, andmypy --platform win32 srcare clean.Issue number
Fixes #4679
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR