fix: recover stale context limits after model switches - #341
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/hooks/magic-context/event-payloads.ts">
<violation number="1" location="packages/plugin/src/hooks/magic-context/event-payloads.ts:60">
P2: the finish check blacklists only the exact string "error", so every other non-empty finish (e.g. a user-cancelled/interrupted generation the host surfaces as a non-error finish) counts as a successful terminal host event. This gate drives live-model-switch bookkeeping (hook-handlers.ts:291) and ordinary scheduler pressure (event-handler.ts:519), so a cancelled turn that still carries input tokens would be treated as a valid success. Consider whitelisting the terminal success reasons ("stop","length") instead, or rejecting the known non-success finishes, to match the documented "error-free terminal host events" intent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| completedAt?: number; | ||
| } | ||
|
|
||
| export function isSuccessfulHostEvent(info: MessageUpdatedAssistantInfo): boolean { |
There was a problem hiding this comment.
P2: the finish check blacklists only the exact string "error", so every other non-empty finish (e.g. a user-cancelled/interrupted generation the host surfaces as a non-error finish) counts as a successful terminal host event. This gate drives live-model-switch bookkeeping (hook-handlers.ts:291) and ordinary scheduler pressure (event-handler.ts:519), so a cancelled turn that still carries input tokens would be treated as a valid success. Consider whitelisting the terminal success reasons ("stop","length") instead, or rejecting the known non-success finishes, to match the documented "error-free terminal host events" intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/hooks/magic-context/event-payloads.ts, line 60:
<comment>the finish check blacklists only the exact string "error", so every other non-empty finish (e.g. a user-cancelled/interrupted generation the host surfaces as a non-error finish) counts as a successful terminal host event. This gate drives live-model-switch bookkeeping (hook-handlers.ts:291) and ordinary scheduler pressure (event-handler.ts:519), so a cancelled turn that still carries input tokens would be treated as a valid success. Consider whitelisting the terminal success reasons ("stop","length") instead, or rejecting the known non-success finishes, to match the documented "error-free terminal host events" intent.</comment>
<file context>
@@ -57,6 +57,15 @@ export interface MessageUpdatedInfo {
completedAt?: number;
}
+export function isSuccessfulHostEvent(info: MessageUpdatedAssistantInfo): boolean {
+ if (info.error !== undefined && info.error !== null) return false;
+ if (info.finish === "error") return false;
</file context>
|
Retriggering the unchanged commit because the sole failure was the unrelated/flaky B9 cache-invariant E2E; plugin, Pi, Docker, and the context-limit tests passed. |
There was a problem hiding this comment.
2 issues found across 15 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/features/magic-context/migrations.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/migrations.ts:2824">
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.</violation>
</file>
<file name="packages/plugin/src/features/magic-context/storage-meta-persisted.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/storage-meta-persisted.ts:311">
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.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| { | ||
| // Temporary merge-order reservation: PR #340 owns v79, so this PR must | ||
| // remain v80 even while v79 is absent from this worktree. | ||
| version: 80, |
There was a problem hiding this comment.
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
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/migrations.ts, line 2824:
<comment>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.</comment>
<file context>
@@ -2818,6 +2818,21 @@ export const MIGRATIONS: Migration[] = [
+ {
+ // Temporary merge-order reservation: PR #340 owns v79, so this PR must
+ // remain v80 even while v79 is absent from this worktree.
+ version: 80,
+ description: "persist the original observation time for tokenless usage TTL",
+ up(db: Database): void {
</file context>
| 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.
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
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/storage-meta-persisted.ts, line 311:
<comment>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.</comment>
<file context>
@@ -297,12 +301,14 @@ function getDefaultHistorianFailureState(): PersistedHistorianFailureState {
if (
!isPersistedUsageRow(result) ||
+ result.last_usage_observed_at <= 0 ||
+ Date.now() - result.last_usage_observed_at > CONTEXT_USAGE_TTL_MS ||
(result.last_context_percentage === 0 && result.last_input_tokens === 0)
) {
</file context>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/features/magic-context/migrations.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/migrations.ts:2836">
P2: When an existing session received a tokenless response after its last usage sample, this migration records that later response time as the usage-observation time. The restored sample can then pass the TTL check and drive pressure decisions beyond its real freshness window; do not mark legacy usage fresh from a response-only timestamp unless the legacy event is known to contain usage, otherwise expire the legacy sample conservatively.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ); | ||
| db.exec(` | ||
| UPDATE session_meta | ||
| SET last_usage_observed_at = last_response_time |
There was a problem hiding this comment.
P2: When an existing session received a tokenless response after its last usage sample, this migration records that later response time as the usage-observation time. The restored sample can then pass the TTL check and drive pressure decisions beyond its real freshness window; do not mark legacy usage fresh from a response-only timestamp unless the legacy event is known to contain usage, otherwise expire the legacy sample conservatively.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/migrations.ts, line 2836:
<comment>When an existing session received a tokenless response after its last usage sample, this migration records that later response time as the usage-observation time. The restored sample can then pass the TTL check and drive pressure decisions beyond its real freshness window; do not mark legacy usage fresh from a response-only timestamp unless the legacy event is known to contain usage, otherwise expire the legacy sample conservatively.</comment>
<file context>
@@ -2831,6 +2831,13 @@ export const MIGRATIONS: Migration[] = [
);
+ db.exec(`
+ UPDATE session_meta
+ SET last_usage_observed_at = last_response_time
+ WHERE last_usage_observed_at = 0
+ AND last_input_tokens > 0
</file context>
…ion gap, MAX_SANE_LIMIT expansion, v80-without-v79 migration strand) Co-Authored-By: Alfonso <alfonso@cortexkit.io>
alfonso-magic-context
left a comment
alfonso-magic-context
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis is right, and we independently traced #331 to the same mechanism before reading your PR: an accepted 258,901-token prompt against a stale 128k catalog window after a model switch. Your event-ordering repair is solid (terminal-evidence gating, the pressure-model split, and the provenance correction in ce09be2 are all correct calls), and the behavior-based regression test is the right kind.
Three things block merge as-is, all fixable:
- The accepted-prompt floor never reaches shared geometry. It lands in the usage snapshot and
last_usage_context_limit, butresolveContextLimit,resolveTrustedContextLimit, andresolveContextWindowGeometrystill serve the stale 128k — so the scheduler, protected-tail boundaries, emergency ladder, status surfaces, and rust-mode wire keep the wrong denominator after the fix. The floor needs to propagate through the common limit/geometry chokepoints, with provider-overflow proof keeping precedence, and it should be modeled as an explicit lower bound rather than overloading a field whose readers treat it as a ceiling. - The
MAX_SANE_LIMITchange (3M →MAX_SAFE_INTEGER) is unrelated to this bug and changes geometry for >3M-window sessions that never hit #331. Please split it into its own PR so it can be evaluated on its own cache-impact merits. - The v80 migration depends on #340's v79 without containing it. Our migration runner selects only versions above the current applied max, so a database that receives #341 first records v80 and can never select a later-arriving v79. Please rebase on #340 (or land them as a stack), and drop the temporary gap allowance in the armed-replay test.
One more ask: a cross-surface regression starting from the exact reported shape (258,901 accepted over stale 128k) asserting the denominator is consistent across scheduler/status/rust paths, plus a floor-then-overflow test proving overflow still wins. Happy to re-review quickly — this is the fix #331 needs.
2 participants
Summary
detected_context_limit, so a successful sample cannot masquerade as an exact provider ceiling or force a HARD m[0] foldfinish:error, nonterminal numeric usage, fractional, negative, overflow, and unsafe-integer samplesFixes #331
Verification
finish:lengthregressions pass without changing provider geometryGreptile Summary
The PR separates usage-observation age from response timing so model-switch recovery can preserve a prompt-only lower bound without turning it into an exact provider ceiling.
last_usage_observed_atmetadata and migration v80.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD E[Assistant message update] --> V{Error-free event?} V -->|No| O[Handle overflow evidence only] V -->|Yes| T{Valid terminal usage?} T -->|Yes| U[Update live and persisted pressure] U --> A[Set usage observation time] T -->|No, tokenless| K{Known live or fresh persisted usage?} K -->|Yes| R[Refresh response timing] K -->|No| S[Leave usage and timing unchanged] A --> RReviews (9): Last reviewed commit: "fix(plugin): restore usage before tokenl..." | Re-trigger Greptile
Context used: