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 <agent@opencode.local>
This commit is contained in:
parent
4258621def
commit
39de98dc95
6 changed files with 143 additions and 21 deletions
|
|
@ -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 path from "node:path"
|
||||||
import { fileURLToPath } from "node:url"
|
import { fileURLToPath } from "node:url"
|
||||||
|
import os from "node:os"
|
||||||
import { spawnSync } from "node:child_process"
|
import { spawnSync } from "node:child_process"
|
||||||
import { loadBrand } from "./src/config.ts"
|
import { loadBrand } from "./src/config.ts"
|
||||||
import { loadTemplate, buildSvg, computeHash } from "./src/render.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 svg = buildSvg(templateSvg, brand, { template, title, subtitle, slots, out }, __dirname)
|
||||||
const tmpSvg = path.join(__dirname, ".tmp-render.svg")
|
const tmpSvg = path.join(os.tmpdir(), `draw-image-${process.pid}.svg`)
|
||||||
writeFileSync(tmpSvg, svg)
|
|
||||||
|
|
||||||
const r = spawnSync("node", [path.join(__dirname, "render.mjs"), tmpSvg, outResolved], {
|
try {
|
||||||
encoding: "utf-8",
|
writeFileSync(tmpSvg, svg)
|
||||||
})
|
|
||||||
if (r.status !== 0) {
|
|
||||||
process.stderr.write(r.stderr || r.stdout || "render failed\n")
|
|
||||||
process.exit(1)
|
|
||||||
}
|
|
||||||
|
|
||||||
const meta = {
|
const r = spawnSync("node", [path.join(__dirname, "render.mjs"), tmpSvg, outResolved], {
|
||||||
hash,
|
encoding: "utf-8",
|
||||||
generatedAt: new Date().toISOString(),
|
})
|
||||||
size: 1024,
|
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()
|
main()
|
||||||
59
.opencode/draw-image/tests/cleanup.test.ts
Normal file
59
.opencode/draw-image/tests/cleanup.test.ts
Normal file
|
|
@ -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([])
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
@ -3,11 +3,12 @@ import { existsSync, readFileSync, rmSync, mkdirSync } from "node:fs"
|
||||||
import path from "node:path"
|
import path from "node:path"
|
||||||
import { fileURLToPath } from "node:url"
|
import { fileURLToPath } from "node:url"
|
||||||
import { spawnSync } from "node:child_process"
|
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 __dirname = path.dirname(fileURLToPath(import.meta.url))
|
||||||
const DRAW_IMAGE_DIR = path.resolve(__dirname, "..")
|
const DRAW_IMAGE_DIR = path.resolve(__dirname, "..")
|
||||||
const TMP = "/tmp/draw-image-optional-e2e"
|
const TMP = "/tmp/draw-image-optional-e2e"
|
||||||
const TMP_SVG = path.join(DRAW_IMAGE_DIR, ".tmp-render.svg")
|
|
||||||
|
|
||||||
beforeAll(() => {
|
beforeAll(() => {
|
||||||
if (existsSync(TMP)) rmSync(TMP, { recursive: true, force: true })
|
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)
|
expect(r.status, `stderr: ${r.stderr}`).toBe(0)
|
||||||
assertPng(out)
|
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('x="780" y="780"')
|
||||||
expect(svg).not.toContain('y="930"')
|
expect(svg).not.toContain('y="930"')
|
||||||
expect(svg).not.toContain("{{subtitle}}")
|
expect(svg).not.toContain("{{subtitle}}")
|
||||||
|
|
|
||||||
|
|
@ -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 (два процесса всё равно пишут в один путь), нужен уникальный путь.
|
||||||
|
|
@ -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`.
|
||||||
|
|
@ -67,7 +67,7 @@ opencode-config/
|
||||||
│ │ ├── tsconfig.json # ES2022 Bundler, strict, noEmit
|
│ │ ├── tsconfig.json # ES2022 Bundler, strict, noEmit
|
||||||
│ │ ├── vitest.config.ts # node env, tests/**/*.test.ts
|
│ │ ├── vitest.config.ts # node env, tests/**/*.test.ts
|
||||||
│ │ ├── render.mjs # SVG string → PNG via sharp (fontFiles: Geist TTF bundle)
|
│ │ ├── render.mjs # SVG string → PNG via sharp (fontFiles: Geist TTF bundle)
|
||||||
│ │ ├── cli.ts # CLI entry: render <template> --title --slots --out
|
│ │ ├── cli.ts # CLI entry: render <template> --title --slots --out; temp SVG в os.tmpdir()/draw-image-${pid}.svg + finally rmSync (race-safe для параллельных рендеров) — PR#178
|
||||||
│ │ ├── brand.json # default palette slaid098 (base/surface/fg/muted/accent/line)
|
│ │ ├── brand.json # default palette slaid098 (base/surface/fg/muted/accent/line)
|
||||||
│ │ ├── templates/cover.svg # 1024×1024 cover template (slots: icon + sub-icon + badge, {{title}}/{{subtitle}})
|
│ │ ├── templates/cover.svg # 1024×1024 cover template (slots: icon + sub-icon + badge, {{title}}/{{subtitle}})
|
||||||
│ │ ├── fonts/ # Geist Sans TTF (Regular + Bold) bundled for sharp fontFiles
|
│ │ ├── fonts/ # Geist Sans TTF (Regular + Bold) bundled for sharp fontFiles
|
||||||
|
|
@ -81,7 +81,7 @@ opencode-config/
|
||||||
│ │ │ ├── recolor.ts # recolorSvg (stroke/fill → target) + stripSvgWrapper + extractRootAttrs
|
│ │ │ ├── recolor.ts # recolorSvg (stroke/fill → target) + stripSvgWrapper + extractRootAttrs
|
||||||
│ │ │ ├── resolve.ts # resolveSlotContent (lucide/brand-logo/file lookup) + renderSlotSvg
|
│ │ │ ├── resolve.ts # resolveSlotContent (lucide/brand-logo/file lookup) + renderSlotSvg
|
||||||
│ │ │ └── render.ts # buildSvg (assemble final SVG) + computeHash (sha256 idempotency)
|
│ │ │ └── render.ts # buildSvg (assemble final SVG) + computeHash (sha256 idempotency)
|
||||||
│ │ └── tests/ # vitest: 48 tests (unit + integration + e2e)
|
│ │ └── tests/ # vitest: 51 tests (unit + integration + e2e) — PR#178 (+3 cleanup)
|
||||||
│ │ ├── config.test.ts
|
│ │ ├── config.test.ts
|
||||||
│ │ ├── slot-parser.test.ts
|
│ │ ├── slot-parser.test.ts
|
||||||
│ │ ├── fit.test.ts
|
│ │ ├── fit.test.ts
|
||||||
|
|
@ -95,7 +95,8 @@ opencode-config/
|
||||||
│ │ ├── e2e.bad-input.test.ts
|
│ │ ├── e2e.bad-input.test.ts
|
||||||
│ │ ├── render.optional.test.ts # unit: empty badge → no rect, empty subtitle → no text (PR#144)
|
│ │ ├── render.optional.test.ts # unit: empty badge → no rect, empty subtitle → no text (PR#144)
|
||||||
│ │ ├── render.optional.integration.test.ts # integration: PNG valid with/without subtitle+badge (PR#144)
|
│ │ ├── render.optional.integration.test.ts # integration: PNG valid with/without subtitle+badge (PR#144)
|
||||||
│ │ └── e2e.optional.test.ts # e2e: title-only CLI → exit 0, clean SVG (PR#144)
|
│ │ ├── e2e.optional.test.ts # e2e: title-only CLI → exit 0, clean SVG (PR#144); хелпер переписан — buildSvg напрямую вместо чтения leftover .tmp-render.svg (PR#178)
|
||||||
|
│ │ └── cleanup.test.ts # unit: temp SVG cleanup — no .tmp-render.svg leftover, PID-based path under os.tmpdir, no stray .svg (PR#178)
|
||||||
│ ├── telegram/ # Telegram Bot API CLI-проект (по паттерну draw-image: plugin-tool + CLI, без runtime-deps) — PR#174
|
│ ├── telegram/ # Telegram Bot API CLI-проект (по паттерну draw-image: plugin-tool + CLI, без runtime-deps) — PR#174
|
||||||
│ │ ├── package.json # "type": "module", без runtime-deps; devDeps: @types/node, typescript, vitest; script test: vitest run
|
│ │ ├── package.json # "type": "module", без runtime-deps; devDeps: @types/node, typescript, vitest; script test: vitest run
|
||||||
│ │ ├── tsconfig.json # ES2022 + DOM (для типов fetch/Blob/FormData/File), moduleResolution: Bundler, allowImportingTsExtensions: true
|
│ │ ├── tsconfig.json # ES2022 + DOM (для типов fetch/Blob/FormData/File), moduleResolution: Bundler, allowImportingTsExtensions: true
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue