From 6d5c39f1b5ea926ec3897fc8ca9d3a68c0c15895 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sat, 31 Oct 2026 14:49:08 +0000 Subject: [PATCH 1/2] fix(cli): Repair DB schema when telemetry is off getDatabase() only wrapped the connection with the schema auto-repair and read-only handling proxy when telemetry was on, because that proxy lived in telemetry.ts next to the tracing. With SENTRY_CLI_NO_TELEMETRY=1, and on every `sentry __complete` run, a missing table or column threw instead of being repaired, which showed up as silently empty completions. Move the repair and read-only handling into db/resilient.ts, which does not load @sentry/node, and always wrap the connection with it. Telemetry now only adds query spans on top. tryRepairAndRetry() takes the connection to repair instead of lazily requiring db/index.js. Fixes #1768 Co-Authored-By: BYK <126780+BYK@users.noreply.github.com> --- packages/cli/src/lib/db/index.ts | 14 +- packages/cli/src/lib/db/resilient.ts | 296 ++++++++++++++++++++ packages/cli/src/lib/db/schema.ts | 13 +- packages/cli/src/lib/telemetry.ts | 311 ++------------------- packages/cli/test/lib/db/resilient.test.ts | 153 ++++++++++ packages/cli/test/lib/telemetry.test.ts | 104 ------- 6 files changed, 480 insertions(+), 411 deletions(-) create mode 100644 packages/cli/src/lib/db/resilient.ts create mode 100644 packages/cli/test/lib/db/resilient.test.ts diff --git a/packages/cli/src/lib/db/index.ts b/packages/cli/src/lib/db/index.ts index 929946cde..c1aa4a6b7 100644 --- a/packages/cli/src/lib/db/index.ts +++ b/packages/cli/src/lib/db/index.ts @@ -16,6 +16,7 @@ const _require = createRequire(import.meta.url); const log = logger.withTag("db"); import { migrateFromJson } from "./migration.js"; +import { createResilientDatabase } from "./resilient.js"; import { initSchema, runMigrations } from "./schema.js"; import { Database } from "./sqlite.js"; @@ -38,9 +39,9 @@ const CACHE_TTL_MS = 7 * 24 * 60 * 60 * 1000; /** Probability of running cleanup on write operations */ const CLEANUP_PROBABILITY = 0.1; -/** Traced database wrapper (returned by getDatabase) */ +/** Wrapped database (returned by getDatabase) */ let db: Database | null = null; -/** Raw database without tracing (used for repair operations) */ +/** Raw, unwrapped database (used for repair operations) */ let rawDb: Database | null = null; let dbOpenedPath: string | null = null; /** @@ -223,11 +224,12 @@ export function getDatabase(): Database { migrateFromJson(rawDb); } - // Wrap with tracing proxy for automatic query instrumentation. - // Lazy-require telemetry to avoid top-level import of @sentry/node (~100ms). - // Shell completions set SENTRY_CLI_NO_TELEMETRY=1 to skip this entirely. + // Both wrappers repair schema errors and skip read-only writes; the traced + // one also adds query spans. Lazy-require telemetry to avoid a top-level + // import of @sentry/node (~100ms). Shell completions set + // SENTRY_CLI_NO_TELEMETRY=1 to skip it. if (getEnv().SENTRY_CLI_NO_TELEMETRY === "1") { - db = rawDb; + db = createResilientDatabase(rawDb); } else { const { createTracedDatabase } = _require("../telemetry.js") as { createTracedDatabase: (d: Database) => Database; diff --git a/packages/cli/src/lib/db/resilient.ts b/packages/cli/src/lib/db/resilient.ts new file mode 100644 index 000000000..27e940ed0 --- /dev/null +++ b/packages/cli/src/lib/db/resilient.ts @@ -0,0 +1,296 @@ +/** + * Self-healing wrapper for the CLI's SQLite connection. + * + * Wraps `query()` and the statement execution methods so that: + * - schema errors (missing table/column) trigger {@link tryRepairAndRetry} + * - writes to a read-only database are skipped with a one-time warning + * + * This module must stay free of `@sentry/node` so it also applies when + * telemetry is off (including the shell-completion fast path). Telemetry adds + * query spans by passing a {@link QueryRunner} (see `createTracedDatabase` in + * `telemetry.ts`). + */ + +import { chmodSync, statSync } from "node:fs"; +import { createRequire } from "node:module"; +import { dirname } from "node:path"; +import { getRealUsername } from "../utils.js"; +import { isReadonlyError, tryRepairAndRetry } from "./schema.js"; +import type { Database } from "./sqlite.js"; + +const _require = createRequire(import.meta.url); + +/** + * Runs one statement execution. Telemetry passes a runner that wraps the call + * in a span; the default just calls it. + */ +export type QueryRunner = (sql: string, execute: () => T) => T; + +const runDirectly: QueryRunner = (_sql, execute) => execute(); + +/** Intentional no-op used as a self-replacement target for one-shot functions. */ +// intentional noop +const noop = (): void => {}; + +/** Resolves the database path, falling back to a default if the import fails. */ +function resolveDbPath(): string { + // oxlint-disable-next-line sentry-cli/no-silent-catch -- grandfathered silent catch — see #1531; drain by adding log.debug()/log.warn() or re-throwing. + try { + // Lazy require: db/index.ts imports this module. + const { getDbPath } = _require("./index.js") as { + getDbPath: () => string; + }; + return getDbPath(); + } catch { + return "~/.sentry/cli.db"; + } +} + +function warnReadonlyDatabase(): void { + warnReadonlyDatabaseOnce = noop; + + const dbPath = resolveDbPath(); + process.stderr.write( + `\nWarning: Sentry CLI local database is read-only. Caching and preferences won't persist.\n` + + ` Path: ${dbPath}\n` + + " Fix: sentry cli fix\n\n", + ); +} + +/** + * Print a one-time warning to stderr when the local database is read-only. + * Replaces itself with a noop after the first call so subsequent invocations + * are free. Assigned via `let` so the binding can be swapped. + */ +let warnReadonlyDatabaseOnce: () => void = warnReadonlyDatabase; + +/** Whether we already attempted a permission repair this process. */ +let repairAttempted = false; + +/** + * Chmod a path, ignoring ENOENT (file doesn't exist yet). + * Re-throws any other error so permission failures aren't silently masked. + */ +function chmodIfExists(filePath: string, mode: number): void { + try { + chmodSync(filePath, mode); + } catch (error: unknown) { + if ( + error instanceof Error && + (error as NodeJS.ErrnoException).code === "ENOENT" + ) { + return; + } + throw error; + } +} + +/** + * Check whether the file at `filePath` is owned by root (uid 0). + * Returns false if the file doesn't exist, can't be stat'd, or if running on + * Windows where `fs.stat().uid` always returns 0 regardless of ownership. + */ +function isOwnedByRoot(filePath: string): boolean { + // Windows fs.stat() always reports uid 0 — skip the check entirely. + if (process.platform === "win32") { + return false; + } + // oxlint-disable-next-line sentry-cli/no-silent-catch -- grandfathered silent catch — see #1531; drain by adding log.debug()/log.warn() or re-throwing. + try { + return statSync(filePath).uid === 0; + } catch { + return false; + } +} + +/** + * Attempt to repair database file permissions so future commands can write. + * + * SQLite caches the readonly state at connection open time, so even after a + * successful chmod the *current* connection remains readonly. This function + * repairs permissions for the NEXT command and prints a differentiated message. + * If the repair fails (e.g., file owned by another user) we fall through to + * {@link warnReadonlyDatabaseOnce} which tells the user to run `sentry cli fix`. + * + * Only tries once per process via the `repairAttempted` guard. + */ +function tryRepairReadonly(): boolean { + if (repairAttempted) { + return false; + } + repairAttempted = true; + + const dbPath = resolveDbPath(); + const configDir = dirname(dbPath); + + // If the config dir or DB file is root-owned, chmod won't help. + // Emit an actionable message telling the user to run sudo chown. + if (isOwnedByRoot(configDir) || isOwnedByRoot(dbPath)) { + const username = getRealUsername(); + // Disable the generic warning — we're emitting a better one here. + warnReadonlyDatabaseOnce = noop; + process.stderr.write( + "\nWarning: Sentry CLI config directory is owned by root.\n" + + ` Path: ${configDir}\n` + + ` Fix: sudo chown -R ${username} "${configDir}"\n` + + " Or: sudo sentry cli fix\n\n", + ); + return false; + } + + // oxlint-disable-next-line sentry-cli/no-silent-catch -- grandfathered silent catch — see #1531; drain by adding log.debug()/log.warn() or re-throwing. + try { + // Repair config directory (needs rwx for WAL/SHM creation) + chmodSync(configDir, 0o700); + + // Repair database file and journal files + chmodSync(dbPath, 0o600); + chmodIfExists(`${dbPath}-wal`, 0o600); + chmodIfExists(`${dbPath}-shm`, 0o600); + + // Disable the fallback warning — repair succeeded + warnReadonlyDatabaseOnce = noop; + + process.stderr.write( + "\nNote: Database permissions were auto-repaired. Caching will resume on next command.\n\n", + ); + return true; + } catch { + // chmod failed — fall through so warnReadonlyDatabaseOnce fires + return false; + } +} + +/** + * Reset all readonly-related state (for testing). + * @internal + */ +export function resetReadonlyWarning(): void { + repairAttempted = false; + warnReadonlyDatabaseOnce = warnReadonlyDatabase; +} + +/** Methods on SQLite Statement that execute queries and should be wrapped */ +const WRAPPED_STATEMENT_METHODS = ["get", "run", "all", "values"] as const; + +/** + * Handle a readonly database error by attempting auto-repair and returning a + * type-appropriate no-op value. Returns `undefined` for run/get (void / no-row) + * and `[]` for all/values (empty result set). + * + * First tries to repair file permissions via {@link tryRepairReadonly}. If that + * fails (or was already attempted), falls back to a one-shot warning directing + * the user to `sentry cli fix`. + */ +function handleReadonlyError(method: string | symbol): unknown { + if (!tryRepairReadonly()) { + warnReadonlyDatabaseOnce(); + } + if (method === "all" || method === "values") { + return []; + } + return; +} + +/** + * Wrap a SQLite Statement so get/run/all/values repair schema errors, skip + * read-only writes, and run through `runQuery`. + */ +function createResilientStatement( + db: Database, + stmt: T, + sql: string, + runQuery: QueryRunner, +): T { + return new Proxy(stmt as object, { + get(target, prop) { + const value = Reflect.get(target, prop); + + // Non-function properties pass through directly + if (typeof value !== "function") { + return value; + } + + // Other methods get bound to preserve 'this' context for native methods + if ( + !WRAPPED_STATEMENT_METHODS.includes( + prop as (typeof WRAPPED_STATEMENT_METHODS)[number], + ) + ) { + return value.bind(target); + } + + return (...args: unknown[]) => + runQuery(sql, () => { + const execute = () => + (value as (...a: unknown[]) => unknown).apply(target, args); + + try { + return execute(); + } catch (error) { + // Attempt auto-repair for schema errors + const repairResult = tryRepairAndRetry(db, execute, error); + if (repairResult.attempted) { + return repairResult.result; + } + + // Handle readonly database gracefully: warn once, skip the write. + // The CLI still works — reads succeed, only caching/persistence is lost. + if (isReadonlyError(error)) { + return handleReadonlyError(prop); + } + + // Re-throw if repair didn't help or wasn't applicable + throw error; + } + }); + }, + }) as T; +} + +/** + * Wrap a SQLite Database so queries repair schema errors and skip read-only + * writes. + * + * @param db - The raw SQLite Database to wrap; schema repairs run on it + * @param runQuery - Runs each statement execution, e.g. inside a span + * @returns A proxied Database + */ +export function createResilientDatabase( + db: Database, + runQuery: QueryRunner = runDirectly, +): Database { + const originalQuery = db.query.bind(db); + + return new Proxy(db as object, { + get(target, prop) { + if (prop === "query") { + return (sql: string) => { + // Try to prepare the statement, with auto-repair on schema errors + const prepareStatement = () => originalQuery(sql); + + let stmt: ReturnType; + try { + stmt = prepareStatement(); + } catch (error) { + // Attempt auto-repair for schema errors during statement preparation + const repairResult = tryRepairAndRetry(db, prepareStatement, error); + if (repairResult.attempted) { + stmt = repairResult.result; + } else { + throw error; + } + } + + return createResilientStatement(db, stmt, sql, runQuery); + }; + } + const value = Reflect.get(target, prop); + // Bind methods to preserve 'this' context for native methods with private fields + if (typeof value === "function") { + return value.bind(target); + } + return value; + }, + }) as Database; +} diff --git a/packages/cli/src/lib/db/schema.ts b/packages/cli/src/lib/db/schema.ts index 4b4632acf..8176067e3 100644 --- a/packages/cli/src/lib/db/schema.ts +++ b/packages/cli/src/lib/db/schema.ts @@ -11,7 +11,6 @@ * - Migration checks */ -import { createRequire } from "node:module"; import { uuidv7 } from "uuidv7"; import { getEnv } from "../env.js"; import { stringifyUnknown } from "../errors.js"; @@ -19,8 +18,6 @@ import { logger } from "../logger.js"; import { storedCredentialIdentity } from "./identity-hash.js"; import type { Database } from "./sqlite.js"; -const _require = createRequire(import.meta.url); - export const CURRENT_SCHEMA_VERSION = 20; /** Environment variable to disable auto-repair */ @@ -759,6 +756,7 @@ export type RepairAttemptResult = * 2. Runs repairSchema() to fix missing tables/columns * 3. Retries the original operation * + * @param db - The raw (unwrapped) connection to repair * @param operation - The failed operation to retry after repair * @param error - The error that triggered the repair attempt * @returns An object indicating whether repair was attempted and the result. @@ -767,6 +765,7 @@ export type RepairAttemptResult = * like stmt.get() that legitimately return undefined). */ export function tryRepairAndRetry( + db: Database, operation: () => T, error: unknown, ): RepairAttemptResult { @@ -789,13 +788,7 @@ export function tryRepairAndRetry( let repairSucceeded = false; // oxlint-disable-next-line sentry-cli/no-silent-catch -- grandfathered silent catch — see #1531; drain by adding log.debug()/log.warn() or re-throwing. try { - // Dynamic imports to avoid circular dependencies with db/index.js - const { getRawDatabase } = _require("./index.js") as { - getRawDatabase: () => Database; - }; - - const rawDb = getRawDatabase(); - const { fixed } = repairSchema(rawDb); + const { fixed } = repairSchema(db); if (fixed.length > 0) { logger.info(`Auto-repaired database: ${fixed.join(", ")}`); diff --git a/packages/cli/src/lib/telemetry.ts b/packages/cli/src/lib/telemetry.ts index f4a878f7b..91ebdb1f2 100644 --- a/packages/cli/src/lib/telemetry.ts +++ b/packages/cli/src/lib/telemetry.ts @@ -9,7 +9,6 @@ * No PII is collected. Opt-out via SENTRY_CLI_NO_TELEMETRY=1 environment variable. */ -import { chmodSync, statSync } from "node:fs"; import { createRequire } from "node:module"; // oxlint-disable-next-line sentry-cli/no-namespace-import -- Sentry SDK recommends namespace import import * as Sentry from "@sentry/node"; @@ -26,7 +25,8 @@ import { } from "./constants.js"; import { getCustomCaCerts } from "./custom-ca.js"; import { getOnOffPreference } from "./db/defaults.js"; -import { isReadonlyError, tryRepairAndRetry } from "./db/schema.js"; +import { createResilientDatabase } from "./db/resilient.js"; +import type { Database } from "./db/sqlite.js"; import { type AgentInfo, detectAgent, @@ -43,7 +43,6 @@ import { attachSentryReporter, logger } from "./logger.js"; import { getSentryBaseUrl, isSentrySaasUrl } from "./sentry-urls.js"; import { redactTelemetryEnvelope } from "./telemetry/credential-redaction.js"; import { makeCompressedTransport } from "./telemetry/zstd-transport.js"; -import { getRealUsername } from "./utils.js"; export type { Span } from "@sentry/core"; @@ -1168,262 +1167,11 @@ export function withDbSpan(operation: string, fn: () => T): T { ); } -/** Intentional no-op used as a self-replacement target for one-shot functions. */ -// intentional noop -const noop = (): void => {}; - -/** Resolves the database path, falling back to a default if the import fails. */ -function resolveDbPath(): string { - // oxlint-disable-next-line sentry-cli/no-silent-catch -- grandfathered silent catch — see #1531; drain by adding log.debug()/log.warn() or re-throwing. - try { - const { getDbPath } = _require("./db/index.js") as { - getDbPath: () => string; - }; - return getDbPath(); - } catch { - return "~/.sentry/cli.db"; - } -} - -/** - * Print a one-time warning to stderr when the local database is read-only. - * Replaces itself with a noop after the first call so subsequent invocations - * are free. Assigned via `let` so the binding can be swapped. - * - * Uses lazy require for db/index.js to avoid a circular dependency - * (db/index.ts imports createTracedDatabase from this module). - */ -let warnReadonlyDatabaseOnce = (): void => { - warnReadonlyDatabaseOnce = noop; - - const dbPath = resolveDbPath(); - process.stderr.write( - `\nWarning: Sentry CLI local database is read-only. Caching and preferences won't persist.\n` + - ` Path: ${dbPath}\n` + - " Fix: sentry cli fix\n\n", - ); -}; - -/** Whether we already attempted a permission repair this process. */ -let repairAttempted = false; - -/** - * Attempt to repair database file permissions so future commands can write. - * - * SQLite caches the readonly state at connection open time, so even after a - * successful chmod the *current* connection remains readonly. This function - * repairs permissions for the NEXT command and prints a differentiated message. - * If the repair fails (e.g., file owned by another user) we fall through to - * {@link warnReadonlyDatabaseOnce} which tells the user to run `sentry cli fix`. - * - * Replaces itself with a noop after the first call via the `repairAttempted` - * guard so we only try once per process. - */ -/** - * Chmod a path, ignoring ENOENT (file doesn't exist yet). - * Re-throws any other error so permission failures aren't silently masked. - */ -function chmodIfExists(filePath: string, mode: number): void { - try { - chmodSync(filePath, mode); - } catch (error: unknown) { - if ( - error instanceof Error && - (error as NodeJS.ErrnoException).code === "ENOENT" - ) { - return; - } - throw error; - } -} - -/** - * Check whether the file at `filePath` is owned by root (uid 0). - * Returns false if the file doesn't exist, can't be stat'd, or if running on - * Windows where `fs.stat().uid` always returns 0 regardless of ownership. - */ -function isOwnedByRoot(filePath: string): boolean { - // Windows fs.stat() always reports uid 0 — skip the check entirely. - if (process.platform === "win32") { - return false; - } - // oxlint-disable-next-line sentry-cli/no-silent-catch -- grandfathered silent catch — see #1531; drain by adding log.debug()/log.warn() or re-throwing. - try { - return statSync(filePath).uid === 0; - } catch { - return false; - } -} - -function tryRepairReadonly(): boolean { - if (repairAttempted) { - return false; - } - repairAttempted = true; - - const dbPath = resolveDbPath(); - const { dirname } = _require("node:path") as { - dirname: (p: string) => string; - }; - const configDir = dirname(dbPath); - - // If the config dir or DB file is root-owned, chmod won't help. - // Emit an actionable message telling the user to run sudo chown. - if (isOwnedByRoot(configDir) || isOwnedByRoot(dbPath)) { - const username = getRealUsername(); - // Disable the generic warning — we're emitting a better one here. - warnReadonlyDatabaseOnce = noop; - process.stderr.write( - "\nWarning: Sentry CLI config directory is owned by root.\n" + - ` Path: ${configDir}\n` + - ` Fix: sudo chown -R ${username} "${configDir}"\n` + - " Or: sudo sentry cli fix\n\n", - ); - return false; - } - - // oxlint-disable-next-line sentry-cli/no-silent-catch -- grandfathered silent catch — see #1531; drain by adding log.debug()/log.warn() or re-throwing. - try { - // Repair config directory (needs rwx for WAL/SHM creation) - chmodSync(configDir, 0o700); - - // Repair database file and journal files - chmodSync(dbPath, 0o600); - chmodIfExists(`${dbPath}-wal`, 0o600); - chmodIfExists(`${dbPath}-shm`, 0o600); - - // Disable the fallback warning — repair succeeded - warnReadonlyDatabaseOnce = noop; - - process.stderr.write( - "\nNote: Database permissions were auto-repaired. Caching will resume on next command.\n\n", - ); - return true; - } catch { - // chmod failed — fall through so warnReadonlyDatabaseOnce fires - return false; - } -} - /** - * Reset all readonly-related state (for testing). - * @internal - */ -export function resetReadonlyWarning(): void { - repairAttempted = false; - warnReadonlyDatabaseOnce = (): void => { - warnReadonlyDatabaseOnce = noop; - - const dbPath = resolveDbPath(); - process.stderr.write( - `\nWarning: Sentry CLI local database is read-only. Caching and preferences won't persist.\n` + - ` Path: ${dbPath}\n` + - " Fix: sentry cli fix\n\n", - ); - }; -} - -/** Methods on SQLite Statement that execute queries and should be traced */ -const TRACED_STATEMENT_METHODS = ["get", "run", "all", "values"] as const; - -/** - * Handle a readonly database error by attempting auto-repair and returning a - * type-appropriate no-op value. Returns `undefined` for run/get (void / no-row) - * and `[]` for all/values (empty result set). - * - * First tries to repair file permissions via {@link tryRepairReadonly}. If that - * fails (or was already attempted), falls back to a one-shot warning directing - * the user to `sentry cli fix`. - */ -function handleReadonlyError(method: string | symbol): unknown { - if (!tryRepairReadonly()) { - warnReadonlyDatabaseOnce(); - } - if (method === "all" || method === "values") { - return []; - } - return; -} - -/** - * Wrap a SQLite Statement to automatically trace query execution. - * - * Intercepts get/run/all/values methods and wraps them with Sentry spans - * that include the SQL query as both the span name and db.statement attribute. + * Wrap a SQLite Database to trace every query, on top of the schema + * auto-repair and read-only handling from {@link createResilientDatabase}. * - * @param stmt - The SQLite Statement to wrap - * @param sql - The SQL query string (parameterized) - * @returns A proxied Statement with automatic tracing - * - * @internal Used by createTracedDatabase - */ -function createTracedStatement(stmt: T, sql: string): T { - return new Proxy(stmt as object, { - get(target, prop) { - const value = Reflect.get(target, prop); - - // Non-function properties pass through directly - if (typeof value !== "function") { - return value; - } - - // Non-traced methods get bound to preserve 'this' context for native methods - if ( - !TRACED_STATEMENT_METHODS.includes( - prop as (typeof TRACED_STATEMENT_METHODS)[number], - ) - ) { - return value.bind(target); - } - - // Traced methods get wrapped with Sentry span and auto-repair - return (...args: unknown[]) => - Sentry.startSpan( - { - name: sql, - op: "db", - attributes: { - "db.system": "sqlite", - "db.statement": sql, - }, - onlyIfParent: true, - }, - () => { - const execute = () => - (value as (...a: unknown[]) => unknown).apply(target, args); - - try { - return execute(); - } catch (error) { - // Attempt auto-repair for schema errors - const repairResult = tryRepairAndRetry(execute, error); - if (repairResult.attempted) { - return repairResult.result; - } - - // Handle readonly database gracefully: warn once, skip the write. - // The CLI still works — reads succeed, only caching/persistence is lost. - if (isReadonlyError(error)) { - return handleReadonlyError(prop); - } - - // Re-throw if repair didn't help or wasn't applicable - throw error; - } - }, - ); - }, - }) as T; -} - -/** Minimal interface for a database with a query method */ -type QueryableDatabase = { query: (sql: string) => unknown }; - -/** - * Wrap a SQLite Database to automatically trace all queries. - * - * Intercepts the query() method and wraps returned Statements with - * createTracedStatement, which traces get/run/all/values calls. + * Each get/run/all/values call gets a span named after the SQL query. * * @param db - The SQLite Database to wrap * @returns A proxied Database with automatic query tracing @@ -1437,40 +1185,21 @@ type QueryableDatabase = { query: (sql: string) => unknown }; * tracedDb.query("SELECT * FROM users WHERE id = ?").get(1); * ``` */ -export function createTracedDatabase(db: T): T { - const originalQuery = db.query.bind(db) as (sql: string) => unknown; - - return new Proxy(db as object, { - get(target, prop) { - if (prop === "query") { - return (sql: string) => { - // Try to prepare the statement, with auto-repair on schema errors - const prepareStatement = () => originalQuery(sql); - - let stmt: unknown; - try { - stmt = prepareStatement(); - } catch (error) { - // Attempt auto-repair for schema errors during statement preparation - const repairResult = tryRepairAndRetry(prepareStatement, error); - if (repairResult.attempted) { - stmt = repairResult.result; - } else { - throw error; - } - } - - return createTracedStatement(stmt, sql); - }; - } - const value = Reflect.get(target, prop); - // Bind methods to preserve 'this' context for native methods with private fields - if (typeof value === "function") { - return value.bind(target); - } - return value; - }, - }) as T; +export function createTracedDatabase(db: Database): Database { + return createResilientDatabase(db, (sql, execute) => + Sentry.startSpan( + { + name: sql, + op: "db", + attributes: { + "db.system": "sqlite", + "db.statement": sql, + }, + onlyIfParent: true, + }, + execute, + ), + ); } /** diff --git a/packages/cli/test/lib/db/resilient.test.ts b/packages/cli/test/lib/db/resilient.test.ts new file mode 100644 index 000000000..921679f89 --- /dev/null +++ b/packages/cli/test/lib/db/resilient.test.ts @@ -0,0 +1,153 @@ +/** + * Tests for the schema auto-repair and read-only handling wrapper around + * the CLI database. + */ + +import { chmodSync, mkdirSync, rmSync } from "node:fs"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { getDatabase, getRawDatabase } from "../../../src/lib/db/index.js"; +import { + createResilientDatabase, + resetReadonlyWarning, +} from "../../../src/lib/db/resilient.js"; +import { + generatePreMigrationTableDDL, + hasColumn, +} from "../../../src/lib/db/schema.js"; +import { Database } from "../../../src/lib/db/sqlite.js"; +import { useTestConfigDir } from "../../helpers.js"; + +describe("getDatabase with telemetry off", () => { + useTestConfigDir("resilient-test-"); + + // Regression: with telemetry off (including every `sentry __complete` run), + // getDatabase() used to return the raw connection, so schema errors threw + // instead of being repaired. + beforeEach(() => { + expect(process.env.SENTRY_CLI_NO_TELEMETRY).toBe("1"); + }); + + test("repairs a missing table and retries the query", () => { + getDatabase(); + getRawDatabase().exec("DROP TABLE completion_cache"); + + const rows = getDatabase().query("SELECT * FROM completion_cache").all(); + + expect(rows).toEqual([]); + }); + + test("repairs a missing column and retries the query", () => { + getDatabase(); + const raw = getRawDatabase(); + raw.exec("DROP TABLE dsn_cache"); + raw.exec(generatePreMigrationTableDDL("dsn_cache")); + expect(hasColumn(raw, "dsn_cache", "fingerprint")).toBe(false); + + const rows = getDatabase().query("SELECT fingerprint FROM dsn_cache").all(); + + expect(rows).toEqual([]); + expect(hasColumn(raw, "dsn_cache", "fingerprint")).toBe(true); + }); +}); + +describe("createResilientDatabase: readonly database", () => { + let tmpDir: string; + let dbPath: string; + + beforeEach(() => { + resetReadonlyWarning(); + + tmpDir = `${import.meta.dirname}/tmp-readonly-${Date.now()}`; + mkdirSync(tmpDir, { recursive: true }); + dbPath = `${tmpDir}/test.db`; + + const setupDb = new Database(dbPath); + setupDb.exec("CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)"); + setupDb.exec("INSERT INTO test (id, name) VALUES (1, 'Alice')"); + setupDb.close(); + + chmodSync(dbPath, 0o444); + }); + + afterEach(() => { + try { + chmodSync(dbPath, 0o644); + } catch { + // May already be cleaned up + } + rmSync(tmpDir, { recursive: true, force: true }); + }); + + test("does not throw on write to readonly database", () => { + // bun:sqlite opens the file successfully — the error only surfaces on write + const db = new Database(dbPath); + const wrappedDb = createResilientDatabase(db); + + expect(() => { + wrappedDb + .query("INSERT INTO test (id, name) VALUES (?, ?)") + .run(2, "Bob"); + }).not.toThrow(); + + db.close(); + }); + + test("reads still work on readonly database", () => { + const db = new Database(dbPath); + const wrappedDb = createResilientDatabase(db); + + const row = wrappedDb.query("SELECT * FROM test WHERE id = ?").get(1) as { + id: number; + name: string; + }; + expect(row).toEqual({ id: 1, name: "Alice" }); + + db.close(); + }); + + test("all() and values() return empty arrays on readonly write", () => { + const db = new Database(dbPath); + const wrappedDb = createResilientDatabase(db); + + const allResult = wrappedDb + .query("INSERT INTO test (id, name) VALUES (?, ?)") + .all(2, "Bob"); + expect(allResult).toEqual([]); + + // values() is bun:sqlite-specific; skip on Node.js + const stmt = wrappedDb.query("INSERT INTO test (id, name) VALUES (?, ?)"); + if (typeof stmt.values === "function") { + const valuesResult = stmt.values(3, "Charlie"); + expect(valuesResult).toEqual([]); + } + + db.close(); + }); + + test("shows readonly warning when auto-repair fails", () => { + // Mock chmodSync to always throw, simulating a file owned by another user. + // This makes tryRepairReadonly fail and fall through to warnReadonlyDatabaseOnce. + const fs = require("node:fs"); + const chmodSpy = vi.spyOn(fs, "chmodSync").mockImplementation(() => { + throw Object.assign(new Error("EPERM: operation not permitted"), { + code: "EPERM", + }); + }); + + const db = new Database(dbPath); + const wrappedDb = createResilientDatabase(db); + + const stderrSpy = vi.spyOn(process.stderr, "write"); + + wrappedDb.query("INSERT INTO test (id, name) VALUES (?, ?)").run(2, "Bob"); + + const output = stderrSpy.mock.calls.map((c) => String(c[0])).join(""); + // When repair fails, the warning message should appear instead + expect(output).toContain("read-only"); + expect(output).toContain("sentry cli fix"); + + stderrSpy.mockRestore(); + chmodSpy.mockRestore(); + db.close(); + }); +}); diff --git a/packages/cli/test/lib/telemetry.test.ts b/packages/cli/test/lib/telemetry.test.ts index 113c91066..678af2977 100644 --- a/packages/cli/test/lib/telemetry.test.ts +++ b/packages/cli/test/lib/telemetry.test.ts @@ -4,7 +4,6 @@ * Tests for withTelemetry wrapper and opt-out behavior. */ -import { chmodSync, mkdirSync, rmSync } from "node:fs"; import { setTimeout as sleep } from "node:timers/promises"; // oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking import * as Sentry from "@sentry/node"; @@ -34,7 +33,6 @@ import { isEpipeError, isUserApiError, recordApiErrorOnSpan, - resetReadonlyWarning, setArgsContext, setCommandSpanName, setFlagContext, @@ -1177,108 +1175,6 @@ describe("createTracedDatabase", () => { db.close(); }); - - describe("readonly database handling", () => { - let tmpDir: string; - let dbPath: string; - - beforeEach(() => { - resetReadonlyWarning(); - - tmpDir = `${import.meta.dirname}/tmp-readonly-${Date.now()}`; - mkdirSync(tmpDir, { recursive: true }); - dbPath = `${tmpDir}/test.db`; - - const setupDb = new Database(dbPath); - setupDb.exec("CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)"); - setupDb.exec("INSERT INTO test (id, name) VALUES (1, 'Alice')"); - setupDb.close(); - - chmodSync(dbPath, 0o444); - }); - - afterEach(() => { - try { - chmodSync(dbPath, 0o644); - } catch { - // May already be cleaned up - } - rmSync(tmpDir, { recursive: true, force: true }); - }); - - test("does not throw on write to readonly database", () => { - // bun:sqlite opens the file successfully — the error only surfaces on write - const db = new Database(dbPath); - const tracedDb = createTracedDatabase(db); - - expect(() => { - tracedDb - .query("INSERT INTO test (id, name) VALUES (?, ?)") - .run(2, "Bob"); - }).not.toThrow(); - - db.close(); - }); - - test("reads still work on readonly database", () => { - const db = new Database(dbPath); - const tracedDb = createTracedDatabase(db); - - const row = tracedDb.query("SELECT * FROM test WHERE id = ?").get(1) as { - id: number; - name: string; - }; - expect(row).toEqual({ id: 1, name: "Alice" }); - - db.close(); - }); - - test("all() and values() return empty arrays on readonly write", () => { - const db = new Database(dbPath); - const tracedDb = createTracedDatabase(db); - - const allResult = tracedDb - .query("INSERT INTO test (id, name) VALUES (?, ?)") - .all(2, "Bob"); - expect(allResult).toEqual([]); - - // values() is bun:sqlite-specific; skip on Node.js - const stmt = tracedDb.query("INSERT INTO test (id, name) VALUES (?, ?)"); - if (typeof stmt.values === "function") { - const valuesResult = stmt.values(3, "Charlie"); - expect(valuesResult).toEqual([]); - } - - db.close(); - }); - - test("shows readonly warning when auto-repair fails", () => { - // Mock chmodSync to always throw, simulating a file owned by another user. - // This makes tryRepairReadonly fail and fall through to warnReadonlyDatabaseOnce. - const fs = require("node:fs"); - const chmodSpy = vi.spyOn(fs, "chmodSync").mockImplementation(() => { - throw Object.assign(new Error("EPERM: operation not permitted"), { - code: "EPERM", - }); - }); - - const db = new Database(dbPath); - const tracedDb = createTracedDatabase(db); - - const stderrSpy = vi.spyOn(process.stderr, "write"); - - tracedDb.query("INSERT INTO test (id, name) VALUES (?, ?)").run(2, "Bob"); - - const output = stderrSpy.mock.calls.map((c) => String(c[0])).join(""); - // When repair fails, the warning message should appear instead - expect(output).toContain("read-only"); - expect(output).toContain("sentry cli fix"); - - stderrSpy.mockRestore(); - chmodSpy.mockRestore(); - db.close(); - }); - }); }); describe("getSentryTracePropagationTargets", () => { From c61fc1fa9b5b5dbccba0c4e1488e1d1288c8089f Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sat, 31 Oct 2026 14:57:25 +0000 Subject: [PATCH 2/2] fix(cli): Keep read-only DB handling quiet during completion Now that `sentry __complete` uses the resilient wrapper, a read-only cli.db made each TAB press try a permission repair and print a multi-line warning to stderr. Completion now skips read-only writes silently; the next normal command still repairs and warns. Co-Authored-By: BYK --- packages/cli/src/lib/complete.ts | 3 ++ packages/cli/src/lib/db/resilient.ts | 21 +++++++++- packages/cli/test/lib/db/resilient.test.ts | 45 ++++++++++++++++++++++ 3 files changed, 67 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/lib/complete.ts b/packages/cli/src/lib/complete.ts index 43ddd7934..663fde930 100644 --- a/packages/cli/src/lib/complete.ts +++ b/packages/cli/src/lib/complete.ts @@ -37,6 +37,7 @@ import { queueCompletionTelemetry } from "./db/completion-telemetry.js"; import { getProjectAliases } from "./db/project-aliases.js"; import { getCachedProjectsForOrg } from "./db/project-cache.js"; import { getCachedOrganizations } from "./db/regions.js"; +import { silenceReadonlyHandling } from "./db/resilient.js"; import { getEnv } from "./env.js"; import { ValidationError } from "./errors.js"; import { fuzzyMatch } from "./fuzzy.js"; @@ -78,6 +79,8 @@ type Completion = { export async function runCompletion(args: string[]): Promise { // Keeps the database modules from requiring telemetry and `@sentry/node` getEnv().SENTRY_CLI_NO_TELEMETRY = "1"; + // A read-only cli.db must not print a warning on every TAB press + silenceReadonlyHandling(); await handleComplete(args); } diff --git a/packages/cli/src/lib/db/resilient.ts b/packages/cli/src/lib/db/resilient.ts index 27e940ed0..dc018bab3 100644 --- a/packages/cli/src/lib/db/resilient.ts +++ b/packages/cli/src/lib/db/resilient.ts @@ -67,6 +67,22 @@ let warnReadonlyDatabaseOnce: () => void = warnReadonlyDatabase; /** Whether we already attempted a permission repair this process. */ let repairAttempted = false; +/** + * Whether read-only writes are skipped without touching permissions or + * writing to stderr. Set by {@link silenceReadonlyHandling}. + */ +let silent = false; + +/** + * Skip read-only writes silently for the rest of this process: no permission + * repair and no stderr warning. Used by shell completion, where each TAB press + * is a new process and any stderr output would land in the user's prompt. The + * next normal command still repairs and warns. + */ +export function silenceReadonlyHandling(): void { + silent = true; +} + /** * Chmod a path, ignoring ENOENT (file doesn't exist yet). * Re-throws any other error so permission failures aren't silently masked. @@ -167,6 +183,7 @@ function tryRepairReadonly(): boolean { */ export function resetReadonlyWarning(): void { repairAttempted = false; + silent = false; warnReadonlyDatabaseOnce = warnReadonlyDatabase; } @@ -180,10 +197,10 @@ const WRAPPED_STATEMENT_METHODS = ["get", "run", "all", "values"] as const; * * First tries to repair file permissions via {@link tryRepairReadonly}. If that * fails (or was already attempted), falls back to a one-shot warning directing - * the user to `sentry cli fix`. + * the user to `sentry cli fix`. Does neither after {@link silenceReadonlyHandling}. */ function handleReadonlyError(method: string | symbol): unknown { - if (!tryRepairReadonly()) { + if (!(silent || tryRepairReadonly())) { warnReadonlyDatabaseOnce(); } if (method === "all" || method === "values") { diff --git a/packages/cli/test/lib/db/resilient.test.ts b/packages/cli/test/lib/db/resilient.test.ts index 921679f89..dcca01b7a 100644 --- a/packages/cli/test/lib/db/resilient.test.ts +++ b/packages/cli/test/lib/db/resilient.test.ts @@ -9,6 +9,7 @@ import { getDatabase, getRawDatabase } from "../../../src/lib/db/index.js"; import { createResilientDatabase, resetReadonlyWarning, + silenceReadonlyHandling, } from "../../../src/lib/db/resilient.js"; import { generatePreMigrationTableDDL, @@ -151,3 +152,47 @@ describe("createResilientDatabase: readonly database", () => { db.close(); }); }); + +describe("createResilientDatabase: silenced read-only handling", () => { + /** A database whose writes always fail as read-only, independent of OS permissions. */ + function readonlyDatabase(): Database { + const stmt = { + run: () => { + throw new Error("attempt to write a readonly database"); + }, + }; + return { query: () => stmt } as unknown as Database; + } + + function stderrDuringWrite(): string { + const stderrSpy = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); + try { + createResilientDatabase(readonlyDatabase()) + .query("INSERT INTO test VALUES (1)") + .run(); + return stderrSpy.mock.calls.map((c) => String(c[0])).join(""); + } finally { + stderrSpy.mockRestore(); + } + } + + beforeEach(() => { + resetReadonlyWarning(); + }); + + afterEach(() => { + resetReadonlyWarning(); + }); + + test("writes to stderr by default", () => { + expect(stderrDuringWrite()).not.toBe(""); + }); + + test("skips the write without stderr output once silenced", () => { + silenceReadonlyHandling(); + + expect(stderrDuringWrite()).toBe(""); + }); +});