From da740d4b004c33dec53a3b5028a86ab6ab227066 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Tue, 27 Oct 2026 02:53:08 +0100 Subject: [PATCH] fix: stop logging settings.json contents from the git-fetch tick, and log an unreadable file once (release review) The settings readers for git fetch and keep-warm no longer repeat V8's JSON.parse message, which quotes the text around the fault and could print voiceSettings.apiKey. The 15 s scheduler tick now logs an unreadable settings.json once, and again only after a successful read in between. Stale comments that said settings.json is rewritten in place now say writes are atomic. Co-Authored-By: Claude Opus 5.5 --- src/git-fetch-scheduler.ts | 21 +++++++++++---- src/hooks-config.ts | 24 +++++++++++++---- test/git-fetch-scheduler.test.ts | 45 ++++++++++++++++++++++++++++++-- test/keep-warm-launch.test.ts | 4 +-- 4 files changed, 80 insertions(+), 14 deletions(-) diff --git a/src/git-fetch-scheduler.ts b/src/git-fetch-scheduler.ts index 5f8b1dd2..a6d00578 100644 --- a/src/git-fetch-scheduler.ts +++ b/src/git-fetch-scheduler.ts @@ -45,8 +45,10 @@ * `DEFAULT_GIT_TIMEOUT_MS`): both settings are per device and never reach the server, so the server * cannot know what a device chose. * - A failure is reported, not retried harder: the folder keeps its schedule and gets its next run at its - * own interval. A settings read that fails skips this tick only, because settings.json is rewritten in - * place and a kept fallback would hold the feature off until the next settings change. + * own interval. A settings read that fails skips this tick only, rather than deciding from defaults. + * settings.json is replaced atomically, so such a failure is a file that does not parse (a hand edit) or + * a real read error, which lasts until someone fixes it: it is logged once, and again only after a read + * has succeeded in between, not every 15 s tick. * * @module git-fetch-scheduler */ @@ -130,6 +132,8 @@ export class GitFetchScheduler { private readonly folderOf = new Map(); private running = false; private stopped = false; + /** The last settings read failed and was logged; cleared by the next read that succeeds. */ + private settingsUnreadable = false; constructor(ports: GitFetchPorts) { this.ports = ports; @@ -144,11 +148,18 @@ export class GitFetchScheduler { try { settings = await this.ports.readSettings(); } catch (err) { - // A read can land mid-write. Skipping this tick costs one interval; deciding from a fallback would - // hold the feature off until the next settings change. - this.ports.log?.(`git fetch: settings unreadable, tick skipped: ${getErrorMessage(err)}`); + // Writes are atomic, so this is a real read error or a file that does not parse (a hand edit), and it + // stays that way until it is fixed: logged once, not on every tick. The reader never puts the file's + // text in its error (readGitFetchSettings), since settings.json can hold an API key. + if (!this.settingsUnreadable) { + this.settingsUnreadable = true; + this.ports.log?.( + `git fetch: settings unreadable, ticks skipped until it reads again: ${getErrorMessage(err)}` + ); + } return 0; } + this.settingsUnreadable = false; // Pass 1 — who is on what, before anything is decided. Every session open on a folder is recorded here, // not only the one that would get picked, so `remove()` can still see who is open on it when the session diff --git a/src/hooks-config.ts b/src/hooks-config.ts index 28559ca5..3c995e13 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -1171,16 +1171,17 @@ export async function readKeepWarmSettingsOrThrow(): Promise { if ((err as NodeJS.ErrnoException).code === 'ENOENT') return keepWarmSettingsFrom({}); throw err; } - return keepWarmSettingsFrom(JSON.parse(text) as Record); + return keepWarmSettingsFrom(parseSettingsText(text)); } /** * The Git fetch interval's app-wide default (`gitFetchEnabled`, absent = OFF) and its cap * (`gitFetchIntervalSeconds`, seconds between runs). Read FRESH on every tick by the scheduler, so a live * toggle takes effect on the next tick and turning it off actually stops the fetching. A missing file is - * the defaults — OFF — but any other read or parse failure rejects, for the same reason as - * {@link readKeepWarmSettingsOrThrow}: settings.json is rewritten in place, so a read can land mid-write, - * and a fallback kept from it would hold the feature off until the next settings change. + * the defaults (OFF), but any other read or parse failure rejects and the scheduler skips that tick rather + * than deciding from defaults. settings.json is replaced atomically (a temp file renamed into place, see + * settings-writer.ts), so such a failure is a real read error or a file that does not parse (a hand edit), + * never a write in progress, and the scheduler logs it once rather than every tick. */ export async function readGitFetchSettings(): Promise { let text: string; @@ -1190,7 +1191,20 @@ export async function readGitFetchSettings(): Promise { if ((err as NodeJS.ErrnoException).code === 'ENOENT') return gitFetchSettingsFrom({}); throw err; } - return gitFetchSettingsFrom(JSON.parse(text) as Record); + return gitFetchSettingsFrom(parseSettingsText(text)); +} + +/** + * settings.json's text for the two readers above, which reject a file that does not parse. The parser's + * own message is never repeated, as in settings-writer.ts: V8 quotes the text around the fault, settings.json + * can hold an API key (`voiceSettings.apiKey`), and both callers log the error they get. + */ +function parseSettingsText(text: string): Record { + try { + return JSON.parse(text) as Record; + } catch { + throw new Error(`${SETTINGS_PATH} is not valid JSON`); + } } function keepWarmSettingsFrom(settings: Record): KeepWarmSettings { diff --git a/test/git-fetch-scheduler.test.ts b/test/git-fetch-scheduler.test.ts index e38d9e1f..cd936c3c 100644 --- a/test/git-fetch-scheduler.test.ts +++ b/test/git-fetch-scheduler.test.ts @@ -11,6 +11,8 @@ * * Port: N/A. */ +import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { dirname } from 'node:path'; import { describe, expect, it, vi } from 'vitest'; import { GitFetchScheduler, @@ -29,6 +31,8 @@ import { } from '../src/git-fetch.js'; import { DEFAULT_GIT_TIMEOUT_MS, MAX_REPOS } from '../src/git-workspace-status.js'; import type { PathProbeKind } from '../src/utils/bounded-path-probe.js'; +import { readGitFetchSettings, readKeepWarmSettingsOrThrow } from '../src/hooks-config.js'; +import { SETTINGS_PATH } from '../src/web/route-helpers.js'; const SETTINGS: GitFetchSettings = { enabled: true, intervalSeconds: 300 }; const VIEW = (id: string, extra: Partial = {}): GitFetchSessionView => ({ @@ -109,7 +113,7 @@ function harness( sessionIds: () => sessions.filter((s) => !gone.has(s.id)).map((s) => s.id), getSession: (id) => (gone.has(id) ? undefined : sessions.find((s) => s.id === id)), readSettings: async () => { - if (h.settings === null) throw new Error('settings.json is mid-write'); + if (h.settings === null) throw new Error('settings.json is not valid JSON'); return h.settings; }, dockerWorkspaces: async () => { @@ -155,7 +159,7 @@ describe('a quiet tick', () => { expect(h.calls.length).toBe(1); }); - it('skips this tick only when a settings read fails, because settings.json is rewritten in place', async () => { + it('skips this tick only when a settings read fails, rather than deciding from defaults', async () => { const h = harness([VIEW('a')]); h.settings = null as unknown as GitFetchSettings; const logged = vi.fn(); @@ -168,6 +172,43 @@ describe('a quiet tick', () => { // Nothing was recorded for the failed tick, so the folder is still due at once. expect(await h.scheduler.tick(1000)).toBe(1); }); + + /** + * The tick runs every 15 s whether or not the feature is on, so an unreadable settings.json would be logged + * every 15 s, and V8's parse message quotes the text around the fault: a hand edit with single quotes put + * the API key it held into the log on every tick. + */ + it('logs an unreadable settings.json once until it reads again, and never with the text of the file', async () => { + const secret = 'sk-leak42'; + const broken = `{"voiceSettings":{"apiKey":'${secret}'}}`; + await mkdir(dirname(SETTINGS_PATH), { recursive: true }); + await writeFile(SETTINGS_PATH, broken); + try { + // The real readers: what they reject with is what gets logged. + await expect(readGitFetchSettings()).rejects.toThrow(/not valid JSON/); + await expect(readGitFetchSettings()).rejects.not.toThrow(secret); + await expect(readKeepWarmSettingsOrThrow()).rejects.not.toThrow(secret); + + const h = harness([VIEW('a')]); + h.ports.readSettings = readGitFetchSettings; + const logged = vi.fn(); + h.ports.log = logged; + for (const now of [0, 15_000, 30_000]) expect(await h.scheduler.tick(now)).toBe(0); + expect(h.calls).toEqual([]); + expect(logged).toHaveBeenCalledTimes(1); + + // A read that succeeds in between re-arms the warning for the next failure. + await writeFile(SETTINGS_PATH, JSON.stringify({ gitFetchEnabled: true })); + expect(await h.scheduler.tick(45_000)).toBe(1); + await writeFile(SETTINGS_PATH, broken); + expect(await h.scheduler.tick(400_000)).toBe(0); + expect(await h.scheduler.tick(415_000)).toBe(0); + expect(logged).toHaveBeenCalledTimes(2); + expect(JSON.stringify(logged.mock.calls)).not.toContain(secret); + } finally { + await rm(SETTINGS_PATH, { force: true }); + } + }); }); describe('the bounded probe', () => { diff --git a/test/keep-warm-launch.test.ts b/test/keep-warm-launch.test.ts index 03eb819b..1c08aa6c 100644 --- a/test/keep-warm-launch.test.ts +++ b/test/keep-warm-launch.test.ts @@ -159,8 +159,8 @@ describe('readKeepWarmSettings', () => { /** * The service keeps what it reads until the next keep-warm settings change, so its reader must - * never hand back a fallback for a file it could not read: settings.json is rewritten in place - * by several routes, and defaults kept from a mid-write read would hold keep-warm off. + * never hand back a fallback for a file it could not read: defaults kept from a file that does + * not parse (a hand edit; writes are atomic) would hold keep-warm off after it is fixed. */ it('the service reader rejects an unreadable file instead of falling back, and the server wires it', async () => { expect(await readKeepWarmSettingsOrThrow()).toEqual({ keepWarmDefault: false, maxHours: 2 });