-
Notifications
You must be signed in to change notification settings - Fork 111
fix: recover stale context limits after model switches #341
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 4 commits
fdc66ff
7eff875
ce09be2
2d610d5
f4f6b55
6a5e30d
af22bd4
8549d11
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 |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /// <reference types="bun-types" /> | ||
|
|
||
| import { describe, expect, test } from "bun:test"; | ||
| import { Database } from "../../shared/sqlite"; | ||
| import { closeQuietly } from "../../shared/sqlite-helpers"; | ||
| import { LATEST_MIGRATION_VERSION, runMigrations } from "./migrations"; | ||
| import { initializeDatabase, LATEST_SUPPORTED_VERSION } from "./storage-db"; | ||
|
|
||
| function seedAppliedVersion(db: Database, version: number): void { | ||
| db.exec(` | ||
| CREATE TABLE schema_migrations ( | ||
| version INTEGER PRIMARY KEY, | ||
| description TEXT NOT NULL, | ||
| applied_at INTEGER NOT NULL | ||
| ); | ||
| `); | ||
| const insert = db.prepare( | ||
| "INSERT INTO schema_migrations (version, description, applied_at) VALUES (?, ?, ?)", | ||
| ); | ||
| for (let current = 1; current <= version; current += 1) { | ||
| insert.run(current, `seed v${current}`, Date.now()); | ||
| } | ||
| } | ||
|
|
||
| function columnNames(db: Database, table: string): string[] { | ||
| return (db.prepare(`PRAGMA table_info(${table})`).all() as Array<{ name: string }>).map( | ||
| (column) => column.name, | ||
| ); | ||
| } | ||
|
|
||
| describe("migration v80: tokenless usage observation timestamp", () => { | ||
| test("fresh databases include the timestamp and align the schema fence", () => { | ||
| const db = new Database(":memory:"); | ||
| try { | ||
| initializeDatabase(db); | ||
| runMigrations(db); | ||
|
|
||
| expect(columnNames(db, "session_meta")).toContain("last_usage_observed_at"); | ||
| expect(LATEST_SUPPORTED_VERSION).toBe(80); | ||
| expect(LATEST_SUPPORTED_VERSION).toBe(LATEST_MIGRATION_VERSION); | ||
| } finally { | ||
| closeQuietly(db); | ||
| } | ||
| }); | ||
|
|
||
| test("replaying from v79 adds the timestamp once with a fail-closed default", () => { | ||
| const db = new Database(":memory:"); | ||
| try { | ||
| seedAppliedVersion(db, 79); | ||
| db.exec(` | ||
| CREATE TABLE session_meta ( | ||
| session_id TEXT PRIMARY KEY, | ||
| last_context_percentage REAL DEFAULT 0, | ||
| last_input_tokens INTEGER DEFAULT 0, | ||
| last_response_time INTEGER | ||
| ); | ||
| INSERT INTO session_meta ( | ||
| session_id, last_context_percentage, last_input_tokens, last_response_time | ||
| ) VALUES ('ses-legacy', 50, 50000, 123); | ||
| `); | ||
|
|
||
| runMigrations(db); | ||
| runMigrations(db); | ||
|
|
||
| expect( | ||
| db | ||
| .prepare("SELECT last_usage_observed_at FROM session_meta WHERE session_id = ?") | ||
| .get("ses-legacy"), | ||
| ).toEqual({ last_usage_observed_at: 0 }); | ||
| expect( | ||
| db | ||
| .prepare("SELECT COUNT(*) AS count FROM schema_migrations WHERE version = 80") | ||
| .get(), | ||
| ).toEqual({ count: 1 }); | ||
| } finally { | ||
| closeQuietly(db); | ||
| } | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,8 @@ import { stableStringify } from "../../shared/stable-json"; | |
| import { ensureSessionMetaRow } from "./storage-meta-shared"; | ||
| import type { ContextUsage } from "./types"; | ||
|
|
||
| export const CONTEXT_USAGE_TTL_MS = 60 * 60 * 1_000; | ||
|
|
||
| const emergencyRecoveryArmedSessions = new Set<string>(); | ||
| const emergencyRecoveryArmedAtBySession = new Map<string, number>(); | ||
| const providerOverflowReconfirmedSessions = new Set<string>(); | ||
|
|
@@ -38,6 +40,7 @@ interface PersistedUsageRow { | |
| last_response_time: number; | ||
| last_observed_model_key: string | null; | ||
| last_usage_context_limit: number | null; | ||
| last_usage_observed_at: number; | ||
| } | ||
|
|
||
| interface PersistedReasoningWatermarkRow { | ||
|
|
@@ -198,7 +201,8 @@ function isPersistedUsageRow(row: unknown): row is PersistedUsageRow { | |
| typeof r.last_input_tokens === "number" && | ||
| typeof r.last_response_time === "number" && | ||
| (typeof r.last_observed_model_key === "string" || r.last_observed_model_key === null) && | ||
| (typeof r.last_usage_context_limit === "number" || r.last_usage_context_limit === null) | ||
| (typeof r.last_usage_context_limit === "number" || r.last_usage_context_limit === null) && | ||
| typeof r.last_usage_observed_at === "number" | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -297,12 +301,14 @@ function getDefaultHistorianFailureState(): PersistedHistorianFailureState { | |
| export function loadPersistedUsage(db: Database, sessionId: string): PersistedUsageState | null { | ||
| const result = db | ||
| .prepare( | ||
| "SELECT last_context_percentage, last_input_tokens, last_response_time, last_observed_model_key, last_usage_context_limit FROM session_meta WHERE session_id = ?", | ||
| "SELECT last_context_percentage, last_input_tokens, last_response_time, last_observed_model_key, last_usage_context_limit, last_usage_observed_at FROM session_meta WHERE session_id = ?", | ||
| ) | ||
| .get(sessionId); | ||
|
|
||
| if ( | ||
| !isPersistedUsageRow(result) || | ||
| result.last_usage_observed_at <= 0 || | ||
| Date.now() - result.last_usage_observed_at > CONTEXT_USAGE_TTL_MS || | ||
|
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. P2: After 60 minutes of tokenless responses (no successful usage sample), loadPersistedUsage returns null even when the session is live. This drops the persisted lower-bound pressure, the lastUsageContextLimit used by event-resolvers, and the lastObservedModelKey that transform.ts uses to detect a model change and clear stale per-model state. Confirm an hour-long tokenless stretch is an acceptable reason to discard the lower bound the PR is meant to keep in persisted metadata; if so, consider documenting it, or refresh last_usage_observed_at on tokenless responses so only truly stale bounds expire. Prompt for AI agents |
||
| (result.last_context_percentage === 0 && result.last_input_tokens === 0) | ||
| ) { | ||
| return null; | ||
|
|
@@ -313,7 +319,7 @@ export function loadPersistedUsage(db: Database, sessionId: string): PersistedUs | |
| percentage: result.last_context_percentage, | ||
| inputTokens: result.last_input_tokens, | ||
| }, | ||
| updatedAt: result.last_response_time || Date.now(), | ||
| updatedAt: result.last_usage_observed_at, | ||
| lastObservedModelKey: result.last_observed_model_key, | ||
| lastUsageContextLimit: | ||
| typeof result.last_usage_context_limit === "number" | ||
|
|
||
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.
P1: When this PR is applied before PR #340, v80 becomes the high-water mark and the later v79 migration is skipped permanently. Land v79 first, or change migration selection to support out-of-order pending versions.
Prompt for AI agents