diff --git a/.changeset/fs-drain-probe-the-write.md b/.changeset/fs-drain-probe-the-write.md new file mode 100644 index 00000000..9a861071 --- /dev/null +++ b/.changeset/fs-drain-probe-the-write.md @@ -0,0 +1,9 @@ +--- +"evlog": patch +--- + +The file system drain now detects an unwritable directory reliably. + +The previous check probed with `mkdir({ recursive: true })`, which is a no-op on a directory that already exists and therefore succeeds even when that directory is read-only. A deployment whose log directory already existed still threw on every batch. The probe also ran per call rather than per resolved state, so concurrent batches could each warn. + +The write itself is now the check: once an append fails with `EROFS`, `EACCES` or `EPERM`, the drain is disabled for that directory and warns once. Batches already in flight when that happens still attempt their own append. Any other failure, including a full disk, is reported by the drain as before. diff --git a/.changeset/fs-drain-readonly-directory.md b/.changeset/fs-drain-readonly-directory.md index 8649c576..4096b198 100644 --- a/.changeset/fs-drain-readonly-directory.md +++ b/.changeset/fs-drain-readonly-directory.md @@ -11,4 +11,4 @@ The file system drain disables itself when its directory is not writable. const drain = process.env.VERCEL !== '1' ? createFsDrain() : undefined ``` -The drain now probes the directory once, and on `EROFS`, `EACCES` or `EPERM` warns a single time and stops, the same way it already bows out of the Edge runtime. Attach it unconditionally. Any other failure — a full disk, a genuine bug — still propagates. +The drain now disables itself once it observes an `EROFS`, `EACCES` or `EPERM` write failure for that directory, warning a single time, the same way it already disables itself in the Edge runtime. Attach it unconditionally. Any other failure — a full disk, a genuine bug — still propagates. diff --git a/packages/evlog/src/adapters/fs.ts b/packages/evlog/src/adapters/fs.ts index 0459485c..cab80673 100644 --- a/packages/evlog/src/adapters/fs.ts +++ b/packages/evlog/src/adapters/fs.ts @@ -26,7 +26,7 @@ const FS_FIELDS: ConfigField[] = [ ] const gitignoreWritten = new Set() -const writableDirs = new Map() +const unwritableDirs = new Set() let warnedFsEdgeRuntime = false function isEdgeRuntime(): boolean { @@ -36,7 +36,7 @@ function isEdgeRuntime(): boolean { function warnFsEdgeRuntimeOnce(): void { if (warnedFsEdgeRuntime) return warnedFsEdgeRuntime = true - console.warn('[evlog/fs] File system drain is not available on the Edge runtime. Use evlog/memory or a HTTP adapter instead.') + console.warn('[evlog/fs] File system drain is not available on the Edge runtime. Use evlog/memory or an HTTP adapter instead.') } /** Read-only or permission-denied, as opposed to a full disk or a real bug. */ @@ -46,27 +46,15 @@ function isUnwritableError(error: unknown): boolean { } /** - * Whether `dir` can be created and written to, probed once per directory. + * Record that `dir` cannot be written to, and say so once. * - * Serverless hosts mount everything outside the temp directory read-only, so a - * drain attached there would otherwise throw on every batch for the lifetime of - * the deployment. Anything other than a permission failure still propagates. + * Synchronous on purpose: concurrent batches that all fail their first write + * reach this together, and the check-then-add pair cannot interleave. */ -async function isDirWritable(dir: string): Promise { - const cached = writableDirs.get(dir) - if (cached !== undefined) return cached - - try { - await mkdir(dir, { recursive: true }) - } catch (error) { - if (!isUnwritableError(error)) throw error - writableDirs.set(dir, false) - console.warn(`[evlog/fs] "${dir}" is not writable, so the file system drain is disabled. This is expected on a serverless host, where only the temp directory is writable and does not outlive the instance — send events to a HTTP adapter instead, or set the drain's \`dir\` to a writable path.`) - return false - } - - writableDirs.set(dir, true) - return true +function markUnwritable(dir: string): void { + if (unwritableDirs.has(dir)) return + unwritableDirs.add(dir) + console.warn(`[evlog/fs] "${dir}" is not writable, so the file system drain is disabled. This is expected on a serverless host, where only the temp directory is writable and does not outlive the instance — send events to an HTTP adapter instead, or set the drain's \`dir\` to a writable path.`) } async function ensureGitignore(dir: string): Promise { @@ -176,6 +164,12 @@ export async function writeBatchToFs(events: WideEvent[], config: FsConfig): Pro * pretty: true, * })) * ``` + * + * @remarks + * A write that fails with `EROFS`, `EACCES` or `EPERM` marks the configured + * directory unavailable and disables the drain for the rest of the process, + * after warning once. Attaching it on a host without a writable directory is + * therefore safe, though the events go nowhere. */ export function createFsDrain(overrides?: Partial) { return defineDrain({ @@ -187,7 +181,7 @@ export function createFsDrain(overrides?: Partial) { } const resolved = await resolveAdapterConfig('fs', FS_FIELDS, overrides) const dir = resolved.dir ?? '.evlog/logs' - if (!await isDirWritable(dir)) return null + if (unwritableDirs.has(dir)) return null return { dir, pretty: resolved.pretty ?? false, @@ -195,7 +189,17 @@ export function createFsDrain(overrides?: Partial) { maxSizePerFile: resolved.maxSizePerFile, } }, - send: writeBatchToFs, + // The write itself is the probe. `mkdir` is a no-op on a directory that + // already exists, so it succeeds on a read-only one and proves nothing; + // only the append tells you whether this host will take the events. + send: async (events, config) => { + try { + await writeBatchToFs(events, config) + } catch (error) { + if (!isUnwritableError(error)) throw error + markUnwritable(config.dir) + } + }, }) } diff --git a/packages/evlog/test/adapters/fs.test.ts b/packages/evlog/test/adapters/fs.test.ts index 163a5cca..1f222f90 100644 --- a/packages/evlog/test/adapters/fs.test.ts +++ b/packages/evlog/test/adapters/fs.test.ts @@ -339,11 +339,10 @@ describe('fs adapter', () => { it('warns once and disables itself when the directory is read-only', async () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - const readOnly = Object.assign(new Error('EROFS'), { code: 'EROFS' }) try { vi.resetModules() - mockedMkdir.mockRejectedValue(readOnly) + mockedAppendFile.mockRejectedValue(Object.assign(new Error('EROFS'), { code: 'EROFS' })) const { createFsDrain: createFsDrainFresh } = await import('../../src/adapters/fs') const drain = createFsDrainFresh({ dir: '/var/task/.evlog/logs' }) @@ -352,23 +351,67 @@ describe('fs adapter', () => { expect(warnSpy).toHaveBeenCalledTimes(1) expect(warnSpy.mock.calls[0]?.[0]).toContain('not writable') - expect(mockedAppendFile).not.toHaveBeenCalled() - // Probed once, then answered from cache. - expect(mockedMkdir).toHaveBeenCalledTimes(1) + // The first batch attempts the write; the second never reaches it. + expect(mockedAppendFile).toHaveBeenCalledTimes(1) + } finally { + warnSpy.mockRestore() + } + }) + + it('disables itself when the directory exists but rejects the write', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + try { + vi.resetModules() + // `mkdir` is a no-op on an existing directory, so it succeeds even when + // that directory is read-only. Only the append reveals it. + mockedMkdir.mockResolvedValue(undefined) + mockedAppendFile.mockRejectedValue(Object.assign(new Error('EACCES'), { code: 'EACCES' })) + const { createFsDrain: createFsDrainFresh } = await import('../../src/adapters/fs') + const drain = createFsDrainFresh({ dir: '/var/task/.evlog/logs' }) + + await drain(createDrainContext()) + await drain(createDrainContext()) + + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(mockedAppendFile).toHaveBeenCalledTimes(1) + } finally { + warnSpy.mockRestore() + } + }) + + it('warns once when concurrent batches all fail their first write', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + try { + vi.resetModules() + mockedAppendFile.mockRejectedValue(Object.assign(new Error('EROFS'), { code: 'EROFS' })) + const { createFsDrain: createFsDrainFresh } = await import('../../src/adapters/fs') + const drain = createFsDrainFresh({ dir: '/var/task/.evlog/logs' }) + + await Promise.all([ + drain(createDrainContext({ action: 'a' })), + drain(createDrainContext({ action: 'b' })), + drain(createDrainContext({ action: 'c' })), + ]) + + expect(warnSpy).toHaveBeenCalledTimes(1) } finally { warnSpy.mockRestore() } }) - it('propagates a write failure that is not a permission problem', async () => { + it('reports a write failure that is not a permission problem', async () => { vi.resetModules() - mockedMkdir.mockRejectedValue(Object.assign(new Error('ENOSPC'), { code: 'ENOSPC' })) + mockedAppendFile.mockRejectedValue(Object.assign(new Error('ENOSPC'), { code: 'ENOSPC' })) const { createFsDrain: createFsDrainFresh } = await import('../../src/adapters/fs') const drain = createFsDrainFresh({ dir: '.evlog/logs' }) const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) try { - await expect(drain(createDrainContext())).rejects.toThrow('ENOSPC') + await drain(createDrainContext()) + expect(errorSpy).toHaveBeenCalled() + expect(String(errorSpy.mock.calls[0]?.[1])).toContain('ENOSPC') } finally { errorSpy.mockRestore() }