fix(session): prevent updateLastRoute from bypassing idle reset (#11520)#11580
Closed
VintLin wants to merge 1 commit into
Closed
fix(session): prevent updateLastRoute from bypassing idle reset (#11520)#11580VintLin wants to merge 1 commit into
VintLin wants to merge 1 commit into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+456
to
+463
| const next: SessionEntry = { | ||
| ...(existing ?? {}), | ||
| ...(metaPatch ?? {}), | ||
| ...basePatch, | ||
| sessionId: existing?.sessionId ?? crypto.randomUUID(), | ||
| // Route metadata writes must not refresh session freshness timestamps. | ||
| updatedAt: existing?.updatedAt ?? 0, | ||
| }; |
Contributor
There was a problem hiding this comment.
Stale updatedAt on new entry
updateLastRoute now forces updatedAt: existing?.updatedAt ?? 0. When the session key doesn’t exist yet, this creates a new SessionEntry with updatedAt = 0, which can cause an immediate idle reset on the next initSessionState (since the entry looks extremely stale). If updateLastRoute is called before the session is initialized for a brand-new conversation, this will likely churn session IDs/files until something else sets updatedAt.
Also appears in the same function by virtue of bypassing mergeSessionEntry, which previously ensured updatedAt was at least Date.now() for new entries.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/sessions/store.ts
Line: 456:463
Comment:
**Stale `updatedAt` on new entry**
`updateLastRoute` now forces `updatedAt: existing?.updatedAt ?? 0`. When the session key doesn’t exist yet, this creates a new `SessionEntry` with `updatedAt = 0`, which can cause an immediate idle reset on the next `initSessionState` (since the entry looks extremely stale). If `updateLastRoute` is called before the session is initialized for a brand-new conversation, this will likely churn session IDs/files until something else sets `updatedAt`.
Also appears in the same function by virtue of bypassing `mergeSessionEntry`, which previously ensured `updatedAt` was at least `Date.now()` for new entries.
How can I resolve this? If you propose a fix, please make it concise.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This PR fixes a bug where sessions configured with
idleMinuteswould never reset becauseupdateLastRoute(called during inbound message recording) would bump theupdatedAttimestamp to the current time before the session freshness check ininitSessionStateoccurred.\n\n### Changes\n- ModifiedupdateLastRouteinsrc/config/sessions/store.tsto preserve the existingupdatedAttimestamp instead of refreshing it toDate.now(). This ensures that route metadata updates (which are purely administrative) do not affect the idle timeout logic.\n- Added a regression test insrc/auto-reply/reply/session.test.tsthat simulates anupdateLastRoutecall followed byinitSessionStateon a stale session.\n\nFixes #11520Greptile Overview
Greptile Summary
This change updates
updateLastRouteso that route/delivery metadata writes no longer refresh the session’supdatedAt, preventing idle-based reset logic ininitSessionStatefrom being bypassed. It also adds a regression test that writes a stale session entry, callsupdateLastRoute, and asserts the session still resets underreset: { mode: "idle" }.Key interaction:
updatedAtis used by session freshness checks to decide whether to reuse an existing session or start a new one;updateLastRouteis invoked during inbound message recording, so its timestamp behavior directly affects idle reset behavior.Confidence Score: 3/5
updateLastRoutecan cause incorrect idle resets for newly-created session keys.updatedAtduring route metadata writes) matches the intended behavior and has a targeted regression test. However, the new logic setsupdatedAtto0whenexistingis missing, which can make newly-created entries appear immediately stale and trigger unwanted session churn whenupdateLastRouteruns before session initialization.(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!