-
Notifications
You must be signed in to change notification settings - Fork 0
Fix session list timestamp reset and missing new sessions #326
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
Changes from all commits
bfa498c
03cddfb
f0789f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ import { | |
| SESSION_PAGE_SIZE, | ||
| SESSION_MESSAGES_LIMIT, | ||
| USER_CLOSEOUT_TIMEOUT_MS, | ||
| ZERO_TURN_GRACE_MS, | ||
| } from './constants.js'; | ||
| import { INTERNAL_TOKEN } from './internal-token.js'; | ||
| import { buildTaskSystemPrompt } from './task-context.js'; | ||
|
|
@@ -689,7 +690,9 @@ async function _startChatInner( | |
|
|
||
| // Pre-register resumed sessions in EventStore so they're discoverable | ||
| // even if the query loop dies before the first assistant event. | ||
| // Preserve existing updatedAt so server restarts don't reset all timestamps. | ||
| if (options.resume) { | ||
| const existingMeta = eventStore.getSession(options.resume); | ||
| eventStore.upsertSession({ | ||
| sessionId: options.resume, | ||
| cwd, | ||
|
|
@@ -698,6 +701,7 @@ async function _startChatInner( | |
| isActive: true, | ||
| ...(worktreePath ? { wtId } : {}), | ||
| ...(options.telosTaskId ? { telosTaskId: options.telosTaskId } : {}), | ||
| ...(existingMeta ? { updatedAt: existingMeta.updatedAt } : {}), | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -1506,13 +1510,15 @@ export async function getSessions(offset = 0, limit = SESSION_PAGE_SIZE) { | |
| * Returns the same shape as getSessions() for API compatibility. | ||
| */ | ||
| export function getSessionsCached(offset = 0, limit = SESSION_PAGE_SIZE) { | ||
| const now = Date.now(); | ||
| const all = eventStore.listSessions().filter((m) => { | ||
| // Hide sessions that were never used through Mitzo (e.g. automated | ||
| // code review sessions discovered from filesystem). Active sessions | ||
| // always show regardless of turn count. | ||
| // Note: new sessions are created with is_active=1 by default (see EventStore | ||
| // schema), so a brand-new session will never be filtered out here. | ||
| if (m.numTurns === 0 && m.promptCount === 0 && !m.isActive) return false; | ||
| // always show regardless of turn count. Recently created sessions | ||
| // (< 1 hour) are kept even with no turns — they may still be starting. | ||
| if (m.numTurns === 0 && m.promptCount === 0 && !m.isActive) { | ||
|
Owner
Author
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. 🟡 missing_tests: The 1-hour grace period logic is not covered by existing tests in server/tests/session-cache.test.ts. The test suite verifies that zero-turn inactive sessions are hidden and that active sessions are shown, but no test creates a recent zero-turn inactive session to verify it's kept, nor an old one to verify it's hidden. Add two tests: one with createdAt < 1 hour ago (should show) and one with createdAt > 1 hour ago (should hide).
Owner
Author
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. 🟡 regressions: The old filter unconditionally hid zero-turn, zero-prompt, inactive sessions. The new filter keeps them visible for up to 1 hour. This means sessions discovered from the filesystem (e.g. automated code review sessions) that were created within the last hour will now appear in the session list, where previously they were always hidden. This is likely intentional but is a behavioral change — the PR title mentions 'missing new sessions', so this is presumably the fix for that. Worth a note in the PR description.
Owner
Author
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. 🔵 style: The constant |
||
| return now - m.createdAt < ZERO_TURN_GRACE_MS; | ||
| } | ||
| return true; | ||
| }); | ||
| const page = all.slice(offset, offset + limit); | ||
|
|
||
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.
🔵 missing_tests: The timestamp-preservation path (passing existing updatedAt on resume) has no direct test at the chat.ts integration level. The underlying EventStore.upsertSession respecting an explicit updatedAt is well-tested in protocol/tests/event-store.test.ts, so the risk is low, but a test in session-cache.test.ts or a chat-level test verifying that resuming a session doesn't change its updatedAt would confirm the end-to-end behavior.
[fixable]