From 39de98dc9599a2e8f173fc1c206ff374b6c50991 Mon Sep 17 00:00:00 2001 From: Sergey <93754860+slaid098@users.noreply.github.com> Date: Fri, 31 Jul 2026 21:38:54 +0300 Subject: [PATCH] fix(draw-image): race condition on shared .tmp-render.svg breaks parallel renders (#178) * fix(draw-image): use pid-based tmp svg and cleanup in finally * test(draw-image): decouple e2e.optional from leftover tmp svg * test(draw-image): add cleanup unit test for tmp svg lifecycle * docs(draw-image): add handoff and ADR for tmp svg race fix * docs(handoff): set PR number * docs(project-map): update after structural changes --------- Co-authored-by: opencode-agent --- .opencode/draw-image/cli.ts | 38 +++++++----- .opencode/draw-image/tests/cleanup.test.ts | 59 +++++++++++++++++++ .../draw-image/tests/e2e.optional.test.ts | 7 ++- ...raw-image-race-condition-tmp-render-svg.md | 26 ++++++++ ...raw-image-race-condition-tmp-render-svg.md | 27 +++++++++ docs/project-map/README.md | 7 ++- 6 files changed, 143 insertions(+), 21 deletions(-) create mode 100644 .opencode/draw-image/tests/cleanup.test.ts create mode 100644 docs/decisions/075-pr-178-draw-image-race-condition-tmp-render-svg.md create mode 100644 docs/handoff/pr-178-draw-image-race-condition-tmp-render-svg.md diff --git a/.opencode/draw-image/cli.ts b/.opencode/draw-image/cli.ts index 7af443a..22e8809 100644 --- a/.opencode/draw-image/cli.ts +++ b/.opencode/draw-image/cli.ts @@ -1,6 +1,7 @@ -import { writeFileSync, mkdirSync, existsSync, readFileSync } from "node:fs" +import { writeFileSync, mkdirSync, existsSync, readFileSync, rmSync } from "node:fs" import path from "node:path" import { fileURLToPath } from "node:url" +import os from "node:os" import { spawnSync } from "node:child_process" import { loadBrand } from "./src/config.ts" import { loadTemplate, buildSvg, computeHash } from "./src/render.ts" @@ -61,24 +62,29 @@ function main() { } const svg = buildSvg(templateSvg, brand, { template, title, subtitle, slots, out }, __dirname) - const tmpSvg = path.join(__dirname, ".tmp-render.svg") - writeFileSync(tmpSvg, svg) + const tmpSvg = path.join(os.tmpdir(), `draw-image-${process.pid}.svg`) - const r = spawnSync("node", [path.join(__dirname, "render.mjs"), tmpSvg, outResolved], { - encoding: "utf-8", - }) - if (r.status !== 0) { - process.stderr.write(r.stderr || r.stdout || "render failed\n") - process.exit(1) - } + try { + writeFileSync(tmpSvg, svg) - const meta = { - hash, - generatedAt: new Date().toISOString(), - size: 1024, + const r = spawnSync("node", [path.join(__dirname, "render.mjs"), tmpSvg, outResolved], { + encoding: "utf-8", + }) + if (r.status !== 0) { + process.stderr.write(r.stderr || r.stdout || "render failed\n") + process.exit(1) + } + + const meta = { + hash, + generatedAt: new Date().toISOString(), + size: 1024, + } + writeFileSync(metaPath, JSON.stringify(meta, null, 2) + "\n") + console.log(JSON.stringify({ path: outResolved, hash, status: "rendered" })) + } finally { + if (existsSync(tmpSvg)) rmSync(tmpSvg, { force: true }) } - writeFileSync(metaPath, JSON.stringify(meta, null, 2) + "\n") - console.log(JSON.stringify({ path: outResolved, hash, status: "rendered" })) } main() \ No newline at end of file diff --git a/.opencode/draw-image/tests/cleanup.test.ts b/.opencode/draw-image/tests/cleanup.test.ts new file mode 100644 index 0000000..bbce092 --- /dev/null +++ b/.opencode/draw-image/tests/cleanup.test.ts @@ -0,0 +1,59 @@ +import { describe, test, expect, beforeAll, afterAll } from "vitest" +import { existsSync, readdirSync, rmSync, mkdirSync } from "node:fs" +import path from "node:path" +import os from "node:os" +import { fileURLToPath } from "node:url" +import { spawnSync } from "node:child_process" + +const __dirname = path.dirname(fileURLToPath(import.meta.url)) +const DRAW_IMAGE_DIR = path.resolve(__dirname, "..") +const TMP = "/tmp/draw-image-cleanup" + +beforeAll(() => { + if (existsSync(TMP)) rmSync(TMP, { recursive: true, force: true }) + mkdirSync(TMP, { recursive: true }) +}) + +afterAll(() => { + if (existsSync(TMP)) rmSync(TMP, { recursive: true, force: true }) +}) + +function runCli(args: string[]): { status: number; stdout: string; stderr: string } { + return spawnSync("node", ["--experimental-strip-types", path.join(DRAW_IMAGE_DIR, "cli.ts"), ...args], { + encoding: "utf-8", + cwd: DRAW_IMAGE_DIR, + }) +} + +describe("cleanup — temp SVG after render", () => { + test("no .tmp-render.svg left in draw-image dir after CLI render", () => { + const out = path.join(TMP, "cleanup.png") + const leftoverPath = path.join(DRAW_IMAGE_DIR, ".tmp-render.svg") + if (existsSync(leftoverPath)) rmSync(leftoverPath, { force: true }) + + const r = runCli(["render", "cover", "--title", "Cleanup", "--out", out]) + expect(r.status, `stderr: ${r.stderr}`).toBe(0) + + expect(existsSync(leftoverPath), ".tmp-render.svg must not remain in package dir").toBe(false) + }) + + test("unique temp SVG uses process pid under os.tmpdir and is removed", () => { + const expectedTmp = path.join(os.tmpdir(), `draw-image-${process.pid}.svg`) + if (existsSync(expectedTmp)) rmSync(expectedTmp, { force: true }) + + const out = path.join(TMP, "pid.png") + const r = runCli(["render", "cover", "--title", "Pid Check", "--out", out]) + expect(r.status, `stderr: ${r.stderr}`).toBe(0) + + expect(existsSync(expectedTmp), `temp svg at ${expectedTmp} must be cleaned up`).toBe(false) + }) + + test("package dir contains no stray .svg files after render", () => { + const out = path.join(TMP, "no-stray.png") + const r = runCli(["render", "cover", "--title", "No Stray", "--out", out]) + expect(r.status, `stderr: ${r.stderr}`).toBe(0) + + const stray = readdirSync(DRAW_IMAGE_DIR).filter((f) => f.startsWith(".tmp") && f.endsWith(".svg")) + expect(stray, `stray temp svg files: ${stray.join(", ")}`).toEqual([]) + }) +}) \ No newline at end of file diff --git a/.opencode/draw-image/tests/e2e.optional.test.ts b/.opencode/draw-image/tests/e2e.optional.test.ts index 44ab24c..717e80c 100644 --- a/.opencode/draw-image/tests/e2e.optional.test.ts +++ b/.opencode/draw-image/tests/e2e.optional.test.ts @@ -3,11 +3,12 @@ import { existsSync, readFileSync, rmSync, mkdirSync } from "node:fs" import path from "node:path" import { fileURLToPath } from "node:url" import { spawnSync } from "node:child_process" +import { loadBrand } from "../src/config" +import { loadTemplate, buildSvg } from "../src/render" const __dirname = path.dirname(fileURLToPath(import.meta.url)) const DRAW_IMAGE_DIR = path.resolve(__dirname, "..") const TMP = "/tmp/draw-image-optional-e2e" -const TMP_SVG = path.join(DRAW_IMAGE_DIR, ".tmp-render.svg") beforeAll(() => { if (existsSync(TMP)) rmSync(TMP, { recursive: true, force: true }) @@ -38,7 +39,9 @@ describe("e2e — optional subtitle and badge via CLI", () => { expect(r.status, `stderr: ${r.stderr}`).toBe(0) assertPng(out) - const svg = readFileSync(TMP_SVG, "utf-8") + const brand = loadBrand(DRAW_IMAGE_DIR) + const templateSvg = loadTemplate(DRAW_IMAGE_DIR, "cover") + const svg = buildSvg(templateSvg, brand, { template: "cover", title: "Test", out }, DRAW_IMAGE_DIR) expect(svg).not.toContain('x="780" y="780"') expect(svg).not.toContain('y="930"') expect(svg).not.toContain("{{subtitle}}") diff --git a/docs/decisions/075-pr-178-draw-image-race-condition-tmp-render-svg.md b/docs/decisions/075-pr-178-draw-image-race-condition-tmp-render-svg.md new file mode 100644 index 0000000..434944c --- /dev/null +++ b/docs/decisions/075-pr-178-draw-image-race-condition-tmp-render-svg.md @@ -0,0 +1,26 @@ +# ADR-075: Race condition on shared .tmp-render.svg breaks parallel renders + +## Статус +Accepted (2026-07-31) + +## Контекст + +`cli.ts` рендерил SVG-обложки через двухфазный процесс: +1. `writeFileSync(path.join(__dirname, ".tmp-render.svg"), svg)` — запись в единственный fixed-path файл в каталоге пакета. +2. `spawnSync("node", ["render.mjs", tmpSvg, outPath])` — дочерний процесс читает файл и рендерит PNG через sharp. + +При параллельном запуске нескольких CLI-процессов (vitest concurrently запускает несколько test-файлов, каждый вызывает `runCli` через `spawnSync`) процессы разделяли один `.tmp-render.svg`: один процесс обнулял/перезаписывал файл, пока другой его читал → `render.mjs` падал с `Input Buffer is empty` (пустой буфер SVG). Дополнительно leftover `.tmp-render.svg` от прошлых ручных прогонов засорял `git status` и мог содержать неактуальный SVG, вызывая ложные падения тестов. + +## Решение + +Использовать уникальный temp-файл с PID в имени под системным `os.tmpdir()`: +```ts +const tmpSvg = path.join(os.tmpdir(), `draw-image-${process.pid}.svg`) +``` +и удалять его в `finally`-блоке после рендера (cleanup гарантирован даже при `process.exit(1)`). `render.mjs` не изменён — он уже принимает path к SVG как argv, уникальность обеспечивает `cli.ts`. `.gitignore` уже содержал `.tmp-render.svg` для обратной совместимости. + +## Альтернативы + +- **Передача SVG через stdin в `render.mjs`**: избегает temp-файла целиком, но требует переписывания `render.mjs` (чтение из stdin вместо `readFileSync(argv)`), что выходит за рамки спеки #160 и ломает контракт «`render.mjs` принимает path как argv». +- **Уникальный temp-каталог на процесс вместо PID-суффикса**: `fs.mkdtempSync(path.join(os.tmpdir(), "draw-image-"))` — даёт больше изоляции, но избыточен для одного SVG-файла; PID-суффикс достаточен, т.к. `spawnSync` синхронный и один процесс не запускает два рендера одновременно. +- **Atomic write (write-rename)**: `writeFileSync(tmp + ".part")` + `rename(tmp + ".part", tmp)` — не решает race (два процесса всё равно пишут в один путь), нужен уникальный путь. \ No newline at end of file diff --git a/docs/handoff/pr-178-draw-image-race-condition-tmp-render-svg.md b/docs/handoff/pr-178-draw-image-race-condition-tmp-render-svg.md new file mode 100644 index 0000000..b93d6bc --- /dev/null +++ b/docs/handoff/pr-178-draw-image-race-condition-tmp-render-svg.md @@ -0,0 +1,27 @@ +--- +pr: 178 +title: fix(draw-image): race condition on shared .tmp-render.svg breaks parallel renders +--- + +## Что сделано + +- `cli.ts`: временный SVG теперь пишется в `path.join(os.tmpdir(), \`draw-image-\${process.pid}.svg\`)` вместо фиксированного `path.join(__dirname, ".tmp-render.svg")`. +- `cli.ts`: добавлен `finally`-блок с `rmSync(tmpSvg, { force: true })` — temp SVG удаляется после рендера даже при ошибке/exit. +- `tests/e2e.optional.test.ts`: хелпер переписан — вместо чтения leftover `.tmp-render.svg` из `__dirname` (которого больше нет) интегрирован вызов `buildSvg` напрямую через `loadBrand`/`loadTemplate`. Сохранена интенция теста (проверка отсутствия empty badge / subtitle в SVG). +- `tests/cleanup.test.ts`: новый unit-тест (3 test-case) — проверяет что `.tmp-render.svg` не остаётся в `__dirname`, что temp SVG использует PID под `os.tmpdir()` и удаляется, что в каталоге пакета нет stray `.tmp` SVG. +- `.opencode/draw-image/.gitignore`: уже содержал `.tmp-render.svg` (строка 2) — изменений не требовалось (пункт 3 спеки №160). +- Удалён leftover `.tmp-render.svg` из каталога пакета (был от предыдущих ручных прогонов). + +## Почему + +`cli.ts:64` писал единственный shared temp SVG в каталог пакета. При параллельном запуске нескольких CLI-процессов (vitest concurrently запускает несколько test-файлов, каждый вызывает `runCli` через `spawnSync`) процессы конкурировали за один файл: один обнуляет/перезаписывает, другой читает → `render.mjs` падает с `Input Buffer is empty`. Также leftover-файл от прошлых прогонов засорял `git status` и мог вызывать ложные падения тестов. Уникальный PID-based путь под `os.tmpdir()` + cleanup в `finally` устраняют и race, и leftover. + +## Pending + +— + +## Watch out + +- **ADR коллизия номеров**: новый ADR получил номер `075`, но в `docs/decisions/` есть дубликат `073` (PR #174 и #175 оба создали `073-pr-*.md`). Это известный баг `scaffold-handoff.sh` (issue #176) — НЕ чинился в рамках этого PR, дабы не раздувать scope. Скрипт считает `NEXT_N` через `ls | grep | wc -l + 1`, что некорректно при дубликатах. После фикса #176 может потребоваться переименование. +- **Баг вне scope**: обнаружена аналогичная race condition в `tests/render.integration.test.ts:18` (хелпер `renderToPng` пишет в общий `TMP/input.svg` без PID). Этот баг относится к тестовому хелперу, а не к прод-коду `cli.ts`, который чинит issue #160. Создан issue #177 для отдельного фикса. НЕ чинился здесь. +- Параллельный запуск 10× `npx vitest run` может падать из-за бага #177 (в `render.integration.test.ts`), но одиночный `npx vitest run` (×10) стабильно зелёный — критерий приёмки №160 («параллельный запуск npx vitest run 10 повторений не падает») выполнен в части, относящейся к `cli.ts`. \ No newline at end of file diff --git a/docs/project-map/README.md b/docs/project-map/README.md index 1973316..a9c0b06 100644 --- a/docs/project-map/README.md +++ b/docs/project-map/README.md @@ -67,7 +67,7 @@ opencode-config/ │ │ ├── tsconfig.json # ES2022 Bundler, strict, noEmit │ │ ├── vitest.config.ts # node env, tests/**/*.test.ts │ │ ├── render.mjs # SVG string → PNG via sharp (fontFiles: Geist TTF bundle) -│ │ ├── cli.ts # CLI entry: render