-
Notifications
You must be signed in to change notification settings - Fork 49.5k
fix(hindsight): flush buffered turns on session end #36219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
aggarwaldev
wants to merge
1
commit into
NousResearch:main
Choose a base branch
from
aggarwaldev:fix/hindsight-flush-on-session-end
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,14 @@ | ||
| """Tests for the on_session_switch hook and session_id propagation. | ||
| """Tests for the on_session_switch hook, on_session_end hook, and session_id propagation. | ||
|
|
||
| Covers #6672: memory providers must be notified when AIAgent.session_id | ||
| rotates mid-process (via /resume, /branch, /reset, /new, or context | ||
| compression). Without the notification, providers that cache per-session | ||
| state in initialize() (Hindsight, and any plugin that stores session_id | ||
| for scoped writes) keep writing into the old session's record. | ||
|
|
||
| Also covers: Hindsight on_session_end must flush any buffered turns when | ||
| a session is torn down (CLI exit, gateway idle expiry), preventing silent | ||
| data loss for users with retain_every_n_turns > 1. | ||
| """ | ||
|
|
||
|
|
||
|
|
@@ -325,3 +329,82 @@ def test_hindsight_preserves_parent_across_empty_parent_arg(): | |
| provider._parent_session_id = "original-parent" | ||
| provider.on_session_switch("new-sid") # no parent passed | ||
| assert provider._parent_session_id == "original-parent" | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Hindsight on_session_end β flush buffered turns on session teardown | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_hindsight_on_session_end_flushes_buffered_turns(): | ||
| """Buffered turns must be flushed via the writer queue on session end. | ||
|
|
||
| Users with retain_every_n_turns > 1 accumulate turns in _session_turns | ||
| until the modulo boundary is hit. If the session ends before that | ||
| boundary (e.g. 5 turns with retain_every_n_turns=12), on_session_end | ||
| must enqueue a retain for whatever is buffered. | ||
| """ | ||
| provider = _make_hindsight_provider() | ||
| provider._auto_retain = True | ||
| assert len(provider._session_turns) == 2 # pre-seeded by helper | ||
|
|
||
| provider.on_session_end([]) | ||
|
|
||
| # Buffer must be cleared after flush. | ||
| assert provider._session_turns == [] | ||
| assert provider._turn_counter == 0 | ||
| assert provider._turn_index == 0 | ||
| # A flush closure must have been enqueued on the writer queue. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only proves that a closure was queued. Execute it against a fake client and assert the payload for an append-mode session that already retained a prior batch; otherwise the duplicate-retain regression is not covered. |
||
| assert not provider._retain_queue.empty() | ||
|
|
||
|
|
||
| def test_hindsight_on_session_end_noop_when_buffer_empty(): | ||
| """No flush should be enqueued when there are no buffered turns.""" | ||
| provider = _make_hindsight_provider() | ||
| provider._auto_retain = True | ||
| provider._session_turns = [] | ||
| provider._turn_counter = 0 | ||
|
|
||
| provider.on_session_end([]) | ||
|
|
||
| assert provider._retain_queue.empty() | ||
|
|
||
|
|
||
| def test_hindsight_on_session_end_noop_when_auto_retain_disabled(): | ||
| """Providers with auto_retain=False must not flush on session end.""" | ||
| provider = _make_hindsight_provider() | ||
| provider._auto_retain = False | ||
| assert len(provider._session_turns) == 2 # pre-seeded | ||
|
|
||
| provider.on_session_end([]) | ||
|
|
||
| # Turns should NOT be cleared β auto_retain is off, nothing happened. | ||
| assert len(provider._session_turns) == 2 | ||
| assert provider._retain_queue.empty() | ||
|
|
||
|
|
||
| def test_hindsight_on_session_end_noop_when_shutting_down(): | ||
| """If shutdown has already fired, on_session_end must not enqueue.""" | ||
| provider = _make_hindsight_provider() | ||
| provider._auto_retain = True | ||
| provider._shutting_down.set() # simulate shutdown already in progress | ||
|
|
||
| provider.on_session_end([]) | ||
|
|
||
| # Buffer untouched β we can't enqueue after shutdown. | ||
| assert len(provider._session_turns) == 2 | ||
| assert provider._retain_queue.empty() | ||
|
|
||
|
|
||
| def test_hindsight_on_session_end_then_shutdown_no_double_flush(): | ||
| """on_session_end clears the buffer, so a subsequent shutdown() | ||
| draining the writer queue must not re-flush the same turns.""" | ||
| provider = _make_hindsight_provider() | ||
| provider._auto_retain = True | ||
|
|
||
| provider.on_session_end([]) | ||
| queue_size_after_end = provider._retain_queue.qsize() | ||
|
|
||
| # Simulate what shutdown() does β it should find an empty buffer. | ||
| assert provider._session_turns == [] | ||
| assert queue_size_after_end == 1 # exactly one flush from on_session_end | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current main now tracks
_last_retained_turn_countfor append-capable APIs (sync_turn()only submits the unretained suffix). When this session already crossed a retain boundary, flushing all_session_turnshere withupdate_mode="append"duplicates the earlier turns. Select the unretained delta for append mode and retain the full buffer only for legacy overwrite mode.