From bcbe28f334bd3fbc1677f760dcd24264d089f5e4 Mon Sep 17 00:00:00 2001 From: Vijay Upadya <41652029+vijayupadya@users.noreply.github.com> Date: Wed, 20 May 2026 18:56:07 -0700 Subject: [PATCH] Chronicle: Circuit breaker improvement and empty result hint (#317670) * Chronicle: Circuit breaker improvement and empty result hint * feedback changes --- .../assets/prompts/skills/chronicle/SKILL.md | 5 +- .../chronicle/common/circuitBreaker.ts | 40 ++++++++++++-- .../common/test/circuitBreaker.spec.ts | 55 +++++++++++++++++++ .../vscode-node/remoteSessionExporter.ts | 27 +++++++-- 4 files changed, 114 insertions(+), 13 deletions(-) diff --git a/extensions/copilot/assets/prompts/skills/chronicle/SKILL.md b/extensions/copilot/assets/prompts/skills/chronicle/SKILL.md index c9601da1833..b5fe96a1b03 100644 --- a/extensions/copilot/assets/prompts/skills/chronicle/SKILL.md +++ b/extensions/copilot/assets/prompts/skills/chronicle/SKILL.md @@ -29,8 +29,9 @@ When the user asks for a standup, daily summary, or "what did I do": 1. Call `copilot_sessionStoreSql` with `action: "standup"` and `description: "Generate standup"`. 2. The tool returns pre-fetched session data (sessions, turns, files, refs from the last 24 hours). -3. For any PR references in the data, check their current status (open, merged, draft) if possible. -4. Format the returned data as a standup report grouped by work stream (branch/feature): +3. If the result is empty, tell the user no sessions were found in the last 24h, suggest `/chronicle:reindex`, and stop — do not fabricate a standup. +4. For any PR references in the data, check their current status (open, merged, draft) if possible. +5. Format the returned data as a standup report grouped by work stream (branch/feature): ``` Standup for : diff --git a/extensions/copilot/src/extension/chronicle/common/circuitBreaker.ts b/extensions/copilot/src/extension/chronicle/common/circuitBreaker.ts index 1652b4385c0..07cd8b55628 100644 --- a/extensions/copilot/src/extension/chronicle/common/circuitBreaker.ts +++ b/extensions/copilot/src/extension/chronicle/common/circuitBreaker.ts @@ -26,6 +26,8 @@ export interface CircuitBreakerOptions { probeTimeoutMs: number; /** Maximum reset timeout after exponential backoff on failed probes. Defaults to resetTimeoutMs (no backoff). */ maxResetTimeoutMs?: number; + /** Optional callback fired whenever the circuit state changes. */ + onStateChange?: (from: CircuitState, to: CircuitState) => void; } const DEFAULT_OPTIONS: CircuitBreakerOptions = { @@ -59,6 +61,19 @@ export class CircuitBreaker { this.currentResetTimeoutMs = this.options.resetTimeoutMs; } + private _setState(next: CircuitState): void { + if (this.state === next) { + return; + } + const prev = this.state; + this.state = next; + try { + this.options.onStateChange?.(prev, next); + } catch { + // Best-effort: a misbehaving listener must not break the breaker. + } + } + /** * Get the current state of the circuit breaker. */ @@ -103,7 +118,16 @@ export class CircuitBreaker { this.failureCount = 0; this.probeInFlight = false; this.currentResetTimeoutMs = this.options.resetTimeoutMs; - this.state = CircuitState.CLOSED; + this._setState(CircuitState.CLOSED); + } + + /** + * Release a probe slot that was consumed by `canRequest()` but for which + * no real result is available (e.g. the caller decided not to make a + * request after all). Does not change failure count or open/closed state. + */ + cancelProbe(): void { + this.probeInFlight = false; } /** @@ -111,12 +135,18 @@ export class CircuitBreaker { */ recordFailure(): void { const wasHalfOpen = this.state === CircuitState.HALF_OPEN; - this.failureCount++; + // Clamp the failure count at the threshold once the circuit is open so + // it does not grow unboundedly across many failed probes (otherwise + // telemetry reports an ever-increasing number that suggests the breaker + // is permanently dead even though it is still probing). + if (this.failureCount < this.options.failureThreshold) { + this.failureCount++; + } this.lastFailureTime = Date.now(); this.probeInFlight = false; if (this.failureCount >= this.options.failureThreshold) { - this.state = CircuitState.OPEN; + this._setState(CircuitState.OPEN); } // Exponential backoff: double the probe interval after each failed probe @@ -137,12 +167,12 @@ export class CircuitBreaker { * Force reset the circuit breaker to closed state. */ reset(): void { - this.state = CircuitState.CLOSED; this.failureCount = 0; this.lastFailureTime = 0; this.probeInFlight = false; this.probeStartTime = 0; this.currentResetTimeoutMs = this.options.resetTimeoutMs; + this._setState(CircuitState.CLOSED); } /** @@ -153,7 +183,7 @@ export class CircuitBreaker { if (this.state === CircuitState.OPEN) { const elapsed = Date.now() - this.lastFailureTime; if (elapsed >= this.currentResetTimeoutMs) { - this.state = CircuitState.HALF_OPEN; + this._setState(CircuitState.HALF_OPEN); } } } diff --git a/extensions/copilot/src/extension/chronicle/common/test/circuitBreaker.spec.ts b/extensions/copilot/src/extension/chronicle/common/test/circuitBreaker.spec.ts index b9448c99bfe..3945f2b0e3e 100644 --- a/extensions/copilot/src/extension/chronicle/common/test/circuitBreaker.spec.ts +++ b/extensions/copilot/src/extension/chronicle/common/test/circuitBreaker.spec.ts @@ -149,4 +149,59 @@ describe('CircuitBreaker', () => { // Probe timed out, should allow another expect(cb.canRequest()).toBe(true); }); + + it('cancelProbe releases an unused probe slot without changing state', () => { + const cb = new CircuitBreaker({ failureThreshold: 1, resetTimeoutMs: 0 }); + cb.recordFailure(); + expect(cb.getState()).toBe(CircuitState.HALF_OPEN); + + expect(cb.canRequest()).toBe(true); // probe consumed + expect(cb.canRequest()).toBe(false); // second probe blocked + + cb.cancelProbe(); + expect(cb.getState()).toBe(CircuitState.HALF_OPEN); + expect(cb.getFailureCount()).toBe(1); + expect(cb.canRequest()).toBe(true); // probe available again immediately + }); + + it('clamps failure count at the threshold across repeated probe failures', () => { + const cb = new CircuitBreaker({ + failureThreshold: 3, + resetTimeoutMs: 0, + maxResetTimeoutMs: 100, + probeTimeoutMs: 5, + }); + for (let i = 0; i < 3; i++) { + cb.recordFailure(); + } + expect(cb.getFailureCount()).toBe(3); + + for (let i = 0; i < 10; i++) { + cb.canRequest(); + cb.recordFailure(); + } + expect(cb.getFailureCount()).toBe(3); + }); + + it('fires onStateChange for every transition', () => { + const transitions: Array<[CircuitState, CircuitState]> = []; + const cb = new CircuitBreaker({ + failureThreshold: 1, + resetTimeoutMs: 10, + probeTimeoutMs: 5, + onStateChange: (from, to) => transitions.push([from, to]), + }); + + cb.recordFailure(); // CLOSED → OPEN + vi.advanceTimersByTime(10); + expect(cb.getState()).toBe(CircuitState.HALF_OPEN); // OPEN → HALF_OPEN + cb.canRequest(); + cb.recordSuccess(); // HALF_OPEN → CLOSED + + expect(transitions).toEqual([ + [CircuitState.CLOSED, CircuitState.OPEN], + [CircuitState.OPEN, CircuitState.HALF_OPEN], + [CircuitState.HALF_OPEN, CircuitState.CLOSED], + ]); + }); }); diff --git a/extensions/copilot/src/extension/chronicle/vscode-node/remoteSessionExporter.ts b/extensions/copilot/src/extension/chronicle/vscode-node/remoteSessionExporter.ts index 1cbbc6b5217..bdd5b8e8544 100644 --- a/extensions/copilot/src/extension/chronicle/vscode-node/remoteSessionExporter.ts +++ b/extensions/copilot/src/extension/chronicle/vscode-node/remoteSessionExporter.ts @@ -237,6 +237,14 @@ export class RemoteSessionExporter extends Disposable implements IExtensionContr failureThreshold: 5, resetTimeoutMs: 1_000, maxResetTimeoutMs: 30_000, + onStateChange: (_from, to) => { + this._telemetryService.sendMSFTTelemetryEvent('chronicle.cloudSync', { + operation: 'circuitBreaker', + transition: to.toLowerCase(), + }, { + failureCount: this._circuitBreaker.getFailureCount(), + }); + }, }); // Register delete cloud sessions command @@ -818,7 +826,7 @@ export class RemoteSessionExporter extends Disposable implements IExtensionContr "indexingLevel": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "comment": "The indexing level for the session." }, "droppedEvents": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true, "comment": "Number of events in a failed batch." }, "reason": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "comment": "Reason session was disabled (no_consent, no_repo, init_error, create_error, policy_blocked_cached)." }, - "transition": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "comment": "Circuit breaker state transition (open, closed)." }, + "transition": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "comment": "Circuit breaker state transition (open, half_open, closed)." }, "eventsCount": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true, "comment": "Number of actually submitted events (sum of eventsBySession sizes)." }, "orphanedCount": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true, "comment": "Number of orphaned events not submitted (re-queued or dropped)." }, "batchDurationMs": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true, "comment": "Time to submit batch in ms." }, @@ -1144,6 +1152,9 @@ export class RemoteSessionExporter extends Disposable implements IExtensionContr // splice/unshift churn on each timer tick. _bufferEvents still enforces // MAX_BUFFER_SIZE so memory stays bounded. if (this._cloudClient.isRateLimited()) { + // Release the probe slot consumed by canRequest() above so we don't + // burn it on a flush we never actually attempted. + this._circuitBreaker.cancelProbe(); return; } @@ -1255,9 +1266,7 @@ export class RemoteSessionExporter extends Disposable implements IExtensionContr orphanedCount: orphanedEntries.length, batchDurationMs: Date.now() - batchStart, bufferSize: this._eventBuffer.length, - }); - - if (!this._firstCloudWriteLogged) { + }); if (!this._firstCloudWriteLogged) { this._firstCloudWriteLogged = true; this._telemetryService.sendMSFTTelemetryEvent('chronicle.cloudSync', { @@ -1270,14 +1279,20 @@ export class RemoteSessionExporter extends Disposable implements IExtensionContr this._setSyncState({ kind: 'error' }); this._telemetryService.sendMSFTTelemetryEvent('chronicle.cloudSync', { - operation: 'circuitBreaker', - transition: 'open', + operation: 'flushFailure', }, { failureCount: this._circuitBreaker.getFailureCount(), eventsCount: submittedCount, orphanedCount: orphanedEntries.length, bufferSize: this._eventBuffer.length, }); + } else { + // Nothing failed but there was also nothing to submit (eg. all + // entries were orphans, or only policy/rate-limited sessions). + // Release any probe slot consumed by canRequest() so HALF_OPEN + // can probe again on the next tick instead of waiting for the + // probe timeout. + this._circuitBreaker.cancelProbe(); } if (policyBlockedSessions > 0) {