Skip to content

feat(debug): add capture trace on error admin option#663

Merged
mcowger merged 2 commits into
mainfrom
feat/admin-capture-trace-on-error
Jul 4, 2026
Merged

feat(debug): add capture trace on error admin option#663
mcowger merged 2 commits into
mainfrom
feat/admin-capture-trace-on-error

Conversation

@mcowger

@mcowger mcowger commented Jul 4, 2026

Copy link
Copy Markdown
Owner

User description

Summary

Adds a persisted admin toggle, Capture Trace on Error, that forces debug trace capture/persistence for a request whenever it writes to the inference error log or triggers a provider cooldown — even when global/per-key debug tracing is otherwise disabled.

Behavior

  • When enabled, DebugManager buffers a trace for every request in memory, but only persists it if the request:
    • writes an entry to the inference error log (UsageStorageService.saveError), or
    • triggers a provider cooldown (CooldownManager.markProviderFailure, including stall cooldowns).
  • Successful requests are discarded on flush as before, so this does not increase steady-state DB writes.
  • The setting is persisted in the systemSettings table (debug.captureOnError), loaded into DebugManager at startup, and mirrored live on PATCH.
  • Surfaced as a switch on both the Config page (persisted admin settings) and the Debug page (alongside the existing global/per-key tracing controls).

Backend changes

  • services/request-context.ts — added requestId to the async-local request context.
  • services/debug-manager.tscaptureOnError flag, broadened isCaptureEnabled(), forcePersist flag on DebugLogRecord, markForcePersist().
  • services/usage-storage.tssaveError() triggers force-persist.
  • services/cooldown-manager.tsmarkProviderFailure() triggers force-persist via request context.
  • db/config-repository.ts / routes/management/config.ts — persisted setting + GET/PATCH /v0/management/config/capture-trace-on-error.
  • index.ts — loads the persisted setting into DebugManager at startup.

Frontend changes

  • lib/api.ts — client methods for the new endpoint.
  • pages/Config.tsx / pages/Debug.tsx — toggle UI.

Testing

  • bun run typecheck — passes across all workspaces.
  • bun run test:force-all — 206 test files, 2306 tests passed.

Description

  • Add persisted admin toggle capture-trace-on-error to force trace persistence on errors or cooldowns

  • Integrate forcePersist flag in DebugManager triggered by UsageStorageService and CooldownManager

  • Add requestId to async-local request context for trace correlation

  • Add API endpoints and frontend UI on Config and Debug pages for the new setting


Add a persisted admin toggle that forces debug trace capture and
persistence for a request whenever it writes to the inference error
log or triggers a provider cooldown, even when global/per-key debug
tracing is disabled. Surfaced on both the Config and Debug pages.
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Memory Growth

When captureOnError is enabled, isCaptureEnabled() returns true for every request, causing startLog to buffer a DebugLogRecord in pendingLogs for all requests. If a request completes successfully and flush is never called for it (e.g., due to an unhandled exception, client disconnect before the flush call site, or a code path that skips flush), the record remains in the map indefinitely. Over time under sustained traffic this leads to unbounded memory growth. Successful requests rely entirely on flush being invoked to delete their entries, so any missed flush path becomes a leak.

isCaptureEnabled(): boolean {
  // Capture-on-error mode buffers every request so a trace exists to persist
  // if the request later errors or triggers a cooldown.
  return this.captureOnError || this.isEnabledForKey(getCurrentKeyName() ?? null);
}
Async-Local Mutation

setCurrentRequestId mutates the existing RequestContext store object in place. If the same context object is reused across nested run calls or shared by reference elsewhere, mutating it could cause requestId to bleed into sibling or parent scopes, leading to the wrong trace being force-persisted on error. Using storage.enterWith({ ...store, requestId }) or otherwise creating a new context object would avoid any cross-scope contamination.

export function setCurrentRequestId(requestId: string): void {
  const store = storage.getStore();
  if (store) store.requestId = requestId;
}

Comment on lines +53 to +56
export function setCurrentRequestId(requestId: string): void {
const store = storage.getStore();
if (store) store.requestId = requestId;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Mutating the shared RequestContext store object directly is unsafe, as other consumers may hold references to it. Create a shallow copy with the new requestId and re-enter the AsyncLocalStorage context using storage.enterWith() to ensure the update is isolated to the current execution chain. [possible issue, importance: 8]

Suggested change
export function setCurrentRequestId(requestId: string): void {
const store = storage.getStore();
if (store) store.requestId = requestId;
}
export function setCurrentRequestId(requestId: string): void {
const store = storage.getStore();
if (store) {
storage.enterWith({ ...store, requestId });
}
}

Comment on lines 96 to 100
isCaptureEnabled(): boolean {
return this.isEnabledForKey(getCurrentKeyName() ?? null);
// Capture-on-error mode buffers every request so a trace exists to persist
// if the request later errors or triggers a cooldown.
return this.captureOnError || this.isEnabledForKey(getCurrentKeyName() ?? null);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: When captureOnError is enabled, isCaptureEnabled() returns true for every request, causing all requests to be buffered in pendingLogs until flush. For high-throughput deployments this can lead to unbounded memory growth if many requests succeed and are discarded. Consider adding a cap on the number of pending logs or a TTL-based eviction for buffered traces in capture-on-error mode. [general, importance: 7]

Suggested change
isCaptureEnabled(): boolean {
return this.isEnabledForKey(getCurrentKeyName() ?? null);
// Capture-on-error mode buffers every request so a trace exists to persist
// if the request later errors or triggers a cooldown.
return this.captureOnError || this.isEnabledForKey(getCurrentKeyName() ?? null);
}
isCaptureEnabled(): boolean {
// Capture-on-error mode buffers every request so a trace exists to persist
// if the request later errors or triggers a cooldown.
if (this.captureOnError) {
// Guard against unbounded memory growth in high-throughput scenarios.
if (this.pendingLogs.size >= this.maxPendingCaptureOnErrorLogs) {
logger.warn('Capture-on-error pending log buffer full; skipping capture');
return false;
}
return true;
}
return this.isEnabledForKey(getCurrentKeyName() ?? null);
}

Replace the async-local store via enterWith instead of mutating the
existing object in place, so other holders of a reference to the
prior store aren't affected. Addresses PR review feedback.
@mcowger mcowger merged commit 72e63e3 into main Jul 4, 2026
2 checks passed
@mcowger mcowger deleted the feat/admin-capture-trace-on-error branch July 4, 2026 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant