From ddabc614dd08c6b47ee609c0b8c6af27956f3566 Mon Sep 17 00:00:00 2001 From: kosako Date: Sun, 5 Jul 2026 20:43:01 +0900 Subject: [PATCH 1/2] =?UTF-8?q?receiver:=20graceful=20shutdown=E3=83=BB/he?= =?UTF-8?q?althz=E3=83=BB=E5=AE=9F=E5=8A=B9=E8=A8=AD=E5=AE=9A=E3=82=B5?= =?UTF-8?q?=E3=83=9E=E3=83=AA=E3=82=92=E8=BF=BD=E5=8A=A0=E3=81=99=E3=82=8B?= =?UTF-8?q?=20(#99)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit アーキテクチャレビュー F-2 / F-3 の運用ハードニング。 - SIGTERM / SIGINT で graceful shutdown: server.close() で新規接続を 止めて in-flight リクエストの完了を待ち、store.close() 後に exit 0。 ハング対策に 10 秒の drain deadline(unref なので正常終了は遅延しない) - GET /healthz を追加: store 疎通(count)込みで 200、draining 中と store 異常時は 503。認証不要(liveness しか返さない)・rate limit 対象外(route table の rateLimit: false 宣言。厳しい制限設定でも ヘルスチェックがフラップしない)。404/405 を含む他のパスは従来通り rate limit の対象 - 数値系設定の不正値(非数値・上限系の 0/負/小数)は fallback 時に 「ignored invalid setting <名前>」を起動ログに警告し、実効値を limits: / rate limit: のサマリ行で出力(secret 値は出さない) Closes #99 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LRdDjwxbAAaTMwwnzipkau --- README.md | 5 ++ server/receive.js | 143 ++++++++++++++++++++++++++++++++---------- test/receiver.test.js | 69 ++++++++++++++++++++ 3 files changed, 185 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 8bc739b..a428ba6 100644 --- a/README.md +++ b/README.md @@ -182,6 +182,8 @@ node server/receive.js - inbox UI から `.patchloop-feedback.json` を選択して import できます - `GET /feedback.json` で raw JSON を返します（`?projectId=` / `?demoId=` / `?status=` で絞り込み可） - `GET /screenshots/:file` で保存済み screenshot を返します +- `GET /healthz` で死活監視ができます（store 疎通込みで 200 / 異常・シャットダウン中は 503。認証不要・rate limit 対象外） +- SIGTERM / SIGINT で graceful shutdown します（新規接続を止め、処理中のリクエスト完了と store の close を待ってから終了。10 秒で強制終了） ### ストレージ @@ -236,6 +238,8 @@ cp server/receiver.config.example.json server/receiver.config.json 別の場所の設定ファイルを使う場合は `PATCHLOOP_RECEIVER_CONFIG=/path/to/receiver.config.json node server/receive.js` で指定できます。 +数値系の設定に不正な値（非数値や、上限系での 0・負・小数）を与えた場合は、その値を無視してデフォルト（env が不正なら config の値）に戻し、起動ログに `ignored invalid setting ...` の警告を出します。実効値は起動時の `limits:` / `rate limit:` サマリ行で確認できます。 + 環境変数を指定した場合は設定ファイルより優先されます。たとえば一時的に Slack 転送を試す場合: ```sh @@ -272,6 +276,7 @@ receiver はデフォルトでローカルプロトタイプ前提（`127.0.0.1` - **`ALLOWED_ORIGINS` にデモページの origin を列挙する**: widget からの投稿を想定した origin に絞ります - **`PUBLIC_BASE_URL` を `https://` の公開 URL にする**: Slack / GitHub に載せる screenshot link の到達性に加え、セッション cookie の `Secure` 属性がこの URL のスキームで決まります - **secrets（`RECEIVER_TOKEN` / `GITHUB_TOKEN` / `SLACK_WEBHOOK_URL` など）は env 注入を推奨**: env は config ファイルより優先されます。config ファイル（`receiver.config.json`）に書く場合は git 管理外・ファイル権限の管理下に置いてください +- **死活監視は `GET /healthz` に張る**: 認証不要・rate limit 対象外で、store 疎通込みの 200 / 503 を返します。systemd / ALB からの停止は SIGTERM で graceful shutdown します（in-flight 完了を待つため、停止タイムアウトは 10 秒より長めに） ```sh RECEIVER_TOKEN="" \ diff --git a/server/receive.js b/server/receive.js index cb8f489..31ad090 100644 --- a/server/receive.js +++ b/server/receive.js @@ -13,32 +13,32 @@ const CONFIG_PATH = process.env.PATCHLOOP_RECEIVER_CONFIG || path.join(__dirname const config = loadConfig(CONFIG_PATH); const configDir = path.dirname(CONFIG_PATH); -const PORT = numberSetting(process.env.PORT, numberSetting(config.port, 4000)); +const PORT = numberSetting(process.env.PORT, numberSetting(config.port, 4000, "port (config)"), "PORT (env)"); const HOST = process.env.HOST || config.host || "127.0.0.1"; const LEGACY_STORE_PATH = process.env.FEEDBACK_STORE_PATH || pathFromConfig(config.feedbackStorePath, path.join(__dirname, "feedback.json")); const DB_PATH = process.env.FEEDBACK_DB_PATH || pathFromConfig(config.feedbackDbPath, path.join(__dirname, "feedback.db")); -const MAX_BODY_BYTES = numberSetting(process.env.MAX_BODY_BYTES, numberSetting(config.maxBodyBytes, 3_000_000)); +const MAX_BODY_BYTES = numberSetting(process.env.MAX_BODY_BYTES, numberSetting(config.maxBodyBytes, 3_000_000, "maxBodyBytes (config)"), "MAX_BODY_BYTES (env)"); const SCREENSHOT_DIR = process.env.SCREENSHOT_DIR || pathFromConfig(config.screenshotDir, path.join(__dirname, "screenshots")); -const SCREENSHOT_MAX_BYTES = numberSetting(process.env.SCREENSHOT_MAX_BYTES, numberSetting(config.screenshotMaxBytes, 1_500_000)); +const SCREENSHOT_MAX_BYTES = numberSetting(process.env.SCREENSHOT_MAX_BYTES, numberSetting(config.screenshotMaxBytes, 1_500_000, "screenshotMaxBytes (config)"), "SCREENSHOT_MAX_BYTES (env)"); // Defense-in-depth shape limits on accepted payloads. MAX_BODY_BYTES already // caps the raw request, but without these a single in-budget request could // still smuggle an oversized string (e.g. a multi-MB comment copied verbatim // into a GitHub issue body), a huge array, a deeply nested object, or an import // bundle with an unbounded number of items. Lenient defaults keep local / // zero-config runs working; tune via env or config. -const MAX_IMPORT_ITEMS = positiveIntSetting(process.env.MAX_IMPORT_ITEMS, positiveIntSetting(config.maxImportItems, 500)); -const MAX_FIELD_LENGTH = positiveIntSetting(process.env.MAX_FIELD_LENGTH, positiveIntSetting(config.maxFieldLength, 20_000)); -const MAX_ARRAY_LENGTH = positiveIntSetting(process.env.MAX_ARRAY_LENGTH, positiveIntSetting(config.maxArrayLength, 1_000)); -const MAX_OBJECT_DEPTH = positiveIntSetting(process.env.MAX_OBJECT_DEPTH, positiveIntSetting(config.maxObjectDepth, 32)); +const MAX_IMPORT_ITEMS = positiveIntSetting(process.env.MAX_IMPORT_ITEMS, positiveIntSetting(config.maxImportItems, 500, "maxImportItems (config)"), "MAX_IMPORT_ITEMS (env)"); +const MAX_FIELD_LENGTH = positiveIntSetting(process.env.MAX_FIELD_LENGTH, positiveIntSetting(config.maxFieldLength, 20_000, "maxFieldLength (config)"), "MAX_FIELD_LENGTH (env)"); +const MAX_ARRAY_LENGTH = positiveIntSetting(process.env.MAX_ARRAY_LENGTH, positiveIntSetting(config.maxArrayLength, 1_000, "maxArrayLength (config)"), "MAX_ARRAY_LENGTH (env)"); +const MAX_OBJECT_DEPTH = positiveIntSetting(process.env.MAX_OBJECT_DEPTH, positiveIntSetting(config.maxObjectDepth, 32, "maxObjectDepth (config)"), "MAX_OBJECT_DEPTH (env)"); // Resource limits (DoS / disk exhaustion). A public receiver accepts unauth'd // POST /feedback, so without these an attacker can spam requests until the // process or disk is exhausted. All are tunable; lenient defaults stay on so // local / zero-config runs are unaffected. -const RATE_LIMIT_WINDOW_MS = positiveIntSetting(process.env.RATE_LIMIT_WINDOW_MS, positiveIntSetting(config.rateLimitWindowMs, 60_000)); -const RATE_LIMIT_MAX = positiveIntSetting(process.env.RATE_LIMIT_MAX, positiveIntSetting(config.rateLimitMax, 120)); -const RATE_LIMIT_MAX_CLIENTS = positiveIntSetting(process.env.RATE_LIMIT_MAX_CLIENTS, positiveIntSetting(config.rateLimitMaxClients, 10_000)); -const MAX_FEEDBACK_COUNT = positiveIntSetting(process.env.MAX_FEEDBACK_COUNT, positiveIntSetting(config.maxFeedbackCount, 100_000)); -const SCREENSHOT_DISK_MAX_BYTES = positiveIntSetting(process.env.SCREENSHOT_DISK_MAX_BYTES, positiveIntSetting(config.screenshotDiskMaxBytes, 500_000_000)); +const RATE_LIMIT_WINDOW_MS = positiveIntSetting(process.env.RATE_LIMIT_WINDOW_MS, positiveIntSetting(config.rateLimitWindowMs, 60_000, "rateLimitWindowMs (config)"), "RATE_LIMIT_WINDOW_MS (env)"); +const RATE_LIMIT_MAX = positiveIntSetting(process.env.RATE_LIMIT_MAX, positiveIntSetting(config.rateLimitMax, 120, "rateLimitMax (config)"), "RATE_LIMIT_MAX (env)"); +const RATE_LIMIT_MAX_CLIENTS = positiveIntSetting(process.env.RATE_LIMIT_MAX_CLIENTS, positiveIntSetting(config.rateLimitMaxClients, 10_000, "rateLimitMaxClients (config)"), "RATE_LIMIT_MAX_CLIENTS (env)"); +const MAX_FEEDBACK_COUNT = positiveIntSetting(process.env.MAX_FEEDBACK_COUNT, positiveIntSetting(config.maxFeedbackCount, 100_000, "maxFeedbackCount (config)"), "MAX_FEEDBACK_COUNT (env)"); +const SCREENSHOT_DISK_MAX_BYTES = positiveIntSetting(process.env.SCREENSHOT_DISK_MAX_BYTES, positiveIntSetting(config.screenshotDiskMaxBytes, 500_000_000, "screenshotDiskMaxBytes (config)"), "SCREENSHOT_DISK_MAX_BYTES (env)"); // Behind a reverse proxy the socket address is the proxy's; trust X-Forwarded-For // only when explicitly enabled, so a direct client can't spoof its rate-limit // identity by sending the header. @@ -47,7 +47,7 @@ const WIDGET_DIST_PATH = path.join(__dirname, "..", "dist", "patchloop-widget.js const STATIC_DIR = path.join(__dirname, "static"); const PUBLIC_BASE_URL = trimTrailingSlash(process.env.PUBLIC_BASE_URL || config.publicBaseUrl || `http://${HOST}:${PORT}`); const SLACK_WEBHOOK_URL = process.env.SLACK_WEBHOOK_URL || config.slackWebhookUrl || ""; -const SLACK_TIMEOUT_MS = numberSetting(process.env.SLACK_TIMEOUT_MS, numberSetting(config.slackTimeoutMs, 5000)); +const SLACK_TIMEOUT_MS = numberSetting(process.env.SLACK_TIMEOUT_MS, numberSetting(config.slackTimeoutMs, 5000, "slackTimeoutMs (config)"), "SLACK_TIMEOUT_MS (env)"); const SLACK_IMAGE_MODE = normalizeSlackImageMode(process.env.SLACK_IMAGE_MODE || config.slackImageMode || "auto"); const SLACK_BOT_TOKEN = process.env.SLACK_BOT_TOKEN || config.slackBotToken || ""; const SLACK_UPLOAD_CHANNEL_ID = process.env.SLACK_UPLOAD_CHANNEL_ID || config.slackUploadChannelId || ""; @@ -56,7 +56,7 @@ const GITHUB_REPO = normalizeGitHubRepo(process.env.GITHUB_REPO || config.github const GITHUB_LABELS = normalizeStringList(process.env.GITHUB_LABELS || config.githubLabels); const GITHUB_ASSIGNEES = normalizeStringList(process.env.GITHUB_ASSIGNEES || config.githubAssignees); const GITHUB_API_BASE = trimTrailingSlash(process.env.GITHUB_API_BASE || config.githubApiBase || "https://api.github.com"); -const GITHUB_TIMEOUT_MS = numberSetting(process.env.GITHUB_TIMEOUT_MS, numberSetting(config.githubTimeoutMs, 8000)); +const GITHUB_TIMEOUT_MS = numberSetting(process.env.GITHUB_TIMEOUT_MS, numberSetting(config.githubTimeoutMs, 8000, "githubTimeoutMs (config)"), "GITHUB_TIMEOUT_MS (env)"); const GITHUB_CONFIGURED = Boolean(GITHUB_TOKEN && GITHUB_REPO); // Optional shared token guarding the management (import, status, delete, // github-issue) and read (inbox, feedback.json, screenshots) endpoints. Unset = @@ -220,6 +220,7 @@ function redirect(res, location) { const ROUTE_AUTH_KINDS = new Set(["none", "protected", "page"]); const ROUTES = [ { method: "POST", pattern: /^\/feedback$/, auth: "none", cors: true, handler: handlePostFeedback }, + { method: "GET", pattern: /^\/healthz$/, auth: "none", rateLimit: false, handler: handleGetHealthz }, { method: "GET", pattern: /^\/login$/, auth: "none", handler: handleGetLogin }, { method: "POST", pattern: /^\/login$/, auth: "none", handler: handlePostLogin }, { method: "POST", pattern: /^\/logout$/, auth: "none", handler: handlePostLogout }, @@ -266,29 +267,43 @@ const server = http.createServer((req, res) => { return; } - const retryAfter = rateLimitRetryAfter(clientIp(req)); - if (retryAfter > 0) { - respondJson(res, 429, { ok: false, error: "Too Many Requests" }, { "Retry-After": String(retryAfter) }); - return; - } - const allowedMethods = new Set(); + let matched = null; + let match = null; for (const route of ROUTES) { - const match = route.pattern.exec(pathname); - if (!match) continue; + const result = route.pattern.exec(pathname); + if (!result) continue; if (route.method !== req.method) { allowedMethods.add(route.method); continue; } - if (route.auth !== "none" && !isAuthorizedRequest(req)) { - if (route.auth === "page") { + matched = route; + match = result; + break; + } + + // Health probes poll continuously and must not be throttled into flapping a + // load balancer (routes opt out with rateLimit: false). Everything else — + // including unmatched paths, so scanning floods still count — draws from the + // per-client budget. + if (!matched || matched.rateLimit !== false) { + const retryAfter = rateLimitRetryAfter(clientIp(req)); + if (retryAfter > 0) { + respondJson(res, 429, { ok: false, error: "Too Many Requests" }, { "Retry-After": String(retryAfter) }); + return; + } + } + + if (matched) { + if (matched.auth !== "none" && !isAuthorizedRequest(req)) { + if (matched.auth === "page") { redirect(res, "/login"); } else { respondJson(res, 401, { ok: false, error: "Unauthorized" }); } return; } - route.handler(req, res, match); + matched.handler(req, res, match); return; } @@ -302,6 +317,51 @@ const server = http.createServer((req, res) => { res.end("Not Found"); }); +// Liveness for load balancers / monitoring: 200 while serving, 503 once the +// process is draining or the store fails a trivial query. Unauthenticated — +// probes cannot carry credentials and the response reveals only liveness. +async function handleGetHealthz(req, res) { + if (shuttingDown) { + respondJson(res, 503, { ok: false, status: "shutting-down" }); + return; + } + try { + await store.count(); + respondJson(res, 200, { ok: true }); + } catch (error) { + // Details go to the log, not the (unauthenticated) response. + console.warn(`[PatchLoop receiver] healthz store check failed: ${error.message}`); + respondJson(res, 503, { ok: false, status: "store-unavailable" }); + } +} + +let shuttingDown = false; +const SHUTDOWN_TIMEOUT_MS = 10_000; + +// SIGTERM (systemd / docker stop) and SIGINT (Ctrl-C) drain instead of dying +// mid-write: stop accepting connections, let in-flight requests finish, then +// close the store so no sqlite write is cut off. If a request hangs past the +// drain deadline, exit anyway (the timer is unref'd, so it never delays a +// clean exit). +function shutdown(signal) { + if (shuttingDown) return; + shuttingDown = true; + console.log(`[PatchLoop receiver] ${signal} received, draining connections`); + setTimeout(() => { + console.warn(`[PatchLoop receiver] drain deadline (${SHUTDOWN_TIMEOUT_MS}ms) exceeded, exiting`); + process.exit(1); + }, SHUTDOWN_TIMEOUT_MS).unref(); + server.close(async () => { + try { + await store.close(); + } catch (error) { + console.warn(`[PatchLoop receiver] store close failed: ${error.message}`); + } + console.log("[PatchLoop receiver] shutdown complete"); + process.exit(0); + }); +} + // Storage is initialized (and the legacy JSON store migrated) before the // server accepts requests, so no handler can run against an unready store. async function start() { @@ -321,11 +381,16 @@ async function start() { } else { console.warn("[PatchLoop receiver] CORS: every origin may POST /feedback (*) — set ALLOWED_ORIGINS / allowedOrigins for public deploys"); } + console.log(`[PatchLoop receiver] limits: body=${MAX_BODY_BYTES}B screenshot=${SCREENSHOT_MAX_BYTES}B disk=${SCREENSHOT_DISK_MAX_BYTES}B count=${MAX_FEEDBACK_COUNT} importItems=${MAX_IMPORT_ITEMS} fieldLength=${MAX_FIELD_LENGTH} arrayLength=${MAX_ARRAY_LENGTH} objectDepth=${MAX_OBJECT_DEPTH}`); + console.log(`[PatchLoop receiver] rate limit: ${RATE_LIMIT_MAX} req / ${RATE_LIMIT_WINDOW_MS}ms per client (max ${RATE_LIMIT_MAX_CLIENTS} clients, trustProxy=${TRUST_PROXY})`); console.log(`[PatchLoop receiver] Slack webhook: ${SLACK_WEBHOOK_URL ? "enabled" : "disabled"}`); console.log(`[PatchLoop receiver] Slack image mode: ${SLACK_IMAGE_MODE}`); console.log(`[PatchLoop receiver] Slack file upload: ${SLACK_BOT_TOKEN && SLACK_UPLOAD_CHANNEL_ID ? "enabled" : "disabled"}`); console.log(`[PatchLoop receiver] GitHub issues: ${GITHUB_CONFIGURED ? `enabled (${GITHUB_REPO})` : "disabled"}`); + process.on("SIGTERM", () => shutdown("SIGTERM")); + process.on("SIGINT", () => shutdown("SIGINT")); + server.listen(PORT, HOST, () => { console.log(`[PatchLoop receiver] listening on http://${HOST}:${PORT}`); }); @@ -356,20 +421,34 @@ function setCorsHeaders(req, res) { res.setHeader("Access-Control-Allow-Headers", "Content-Type"); } +function warnIgnoredSetting(label, value, fallback) { + console.warn(`[PatchLoop receiver] ignored invalid setting ${label}: ${JSON.stringify(String(value))} — using ${fallback}`); +} + // Number("") is 0 and Number("abc") is NaN; both would silently disable or -// corrupt a limit, so blank and non-numeric settings fall back instead. -function numberSetting(value, fallback) { +// corrupt a limit, so blank settings are treated as unset and non-numeric ones +// fall back with a startup warning (#99: a misconfig must be visible). +function numberSetting(value, fallback, label) { if (value === undefined || value === null || String(value).trim() === "") return fallback; const number = Number(value); - return Number.isFinite(number) ? number : fallback; + if (!Number.isFinite(number)) { + warnIgnoredSetting(label, value, fallback); + return fallback; + } + return number; } // For limits where 0, a negative, or a fractional value is a misconfiguration // (it would reject nearly every request): such values fall back to the lenient -// default instead of silently bricking the receiver. -function positiveIntSetting(value, fallback) { - const number = numberSetting(value, fallback); - return Number.isInteger(number) && number > 0 ? number : fallback; +// default instead of silently bricking the receiver, and warn at startup. +function positiveIntSetting(value, fallback, label) { + if (value === undefined || value === null || String(value).trim() === "") return fallback; + const number = Number(value); + if (!Number.isInteger(number) || number <= 0) { + warnIgnoredSetting(label, value, fallback); + return fallback; + } + return number; } function boolSetting(value) { diff --git a/test/receiver.test.js b/test/receiver.test.js index 993823b..980b5c8 100644 --- a/test/receiver.test.js +++ b/test/receiver.test.js @@ -1177,6 +1177,73 @@ test("without an allowlist, ingest CORS stays open and startup warns", async (t) assert.equal(inbox.headers.get("access-control-allow-origin"), null); }); +test("GET /healthz reports liveness and is exempt from rate limiting", async (t) => { + const receiver = await startReceiver(t, { RATE_LIMIT_MAX: "1" }); + + // Repeated probes (a load balancer polls continuously) all succeed and do + // not consume the per-client budget… + for (let i = 0; i < 3; i++) { + const health = await fetch(`${receiver.baseUrl}/healthz`); + assert.equal(health.status, 200); + assert.deepEqual(await health.json(), { ok: true }); + } + // …so a real request still gets the full budget (1 allowed, then 429). + assert.equal((await fetch(`${receiver.baseUrl}/feedback.json`)).status, 200); + assert.equal((await fetch(`${receiver.baseUrl}/feedback.json`)).status, 429); +}); + +test("SIGTERM drains: an in-flight request completes and the process exits cleanly", async (t) => { + const receiver = await startReceiver(t); + const body = JSON.stringify(feedbackPayload("pl_drain_1")); + const head = [ + "POST /feedback HTTP/1.1", + "Host: 127.0.0.1", + "Content-Type: application/json", + `Content-Length: ${Buffer.byteLength(body)}`, + "Connection: close", + "", "" + ].join("\r\n"); + + // Start a request but hold back the tail of the body so it is still in + // flight when the signal arrives. + const socket = net.connect(receiver.port, "127.0.0.1"); + await new Promise((resolve, reject) => { + socket.once("connect", resolve); + socket.once("error", reject); + }); + const exited = new Promise((resolve) => receiver.child.once("exit", (code, signal) => resolve({ code, signal }))); + socket.write(head + body.slice(0, 50)); + await new Promise((resolve) => setTimeout(resolve, 50)); + receiver.child.kill("SIGTERM"); + await new Promise((resolve) => setTimeout(resolve, 100)); + socket.write(body.slice(50)); + + const response = await new Promise((resolve, reject) => { + let raw = ""; + socket.on("data", (chunk) => { raw += chunk; }); + socket.once("end", () => resolve(raw)); + socket.once("error", reject); + }); + assert.match(response, /^HTTP\/1\.1 201/); + + // Graceful exit: code 0 (not killed by the signal), store closed last. + const exit = await exited; + assert.deepEqual(exit, { code: 0, signal: null }); + const stored = await readStoredFeedback(receiver.dbPath); + assert.equal(stored.length, 1); + assert.equal(stored[0].id, "pl_drain_1"); +}); + +test("invalid settings warn at startup and the effective values are logged", async (t) => { + const receiver = await startReceiver(t, { MAX_IMPORT_ITEMS: "0", RATE_LIMIT_MAX: "abc" }); + + assert.match(receiver.logs, /ignored invalid setting MAX_IMPORT_ITEMS \(env\): "0" — using 500/); + assert.match(receiver.logs, /ignored invalid setting RATE_LIMIT_MAX \(env\): "abc" — using 120/); + // The one-block effective summary shows what the server actually runs with. + assert.match(receiver.logs, /limits: body=3000000B .*importItems=500/); + assert.match(receiver.logs, /rate limit: 120 req \/ 60000ms per client/); +}); + async function startReceiver(t, extraEnv = {}) { const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "patchloop-receiver-test-")); const port = await getFreePort(); @@ -1216,7 +1283,9 @@ async function startReceiver(t, extraEnv = {}) { return { baseUrl, + child, logs, + port, screenshotDir, storePath, dbPath, From fa64807fed359a22f74431a2abd3d14604a039cc Mon Sep 17 00:00:00 2001 From: kosako Date: Sun, 5 Jul 2026 20:49:11 +0900 Subject: [PATCH 2/2] =?UTF-8?q?receiver:=20=E3=82=B7=E3=82=B0=E3=83=8A?= =?UTF-8?q?=E3=83=AB=E3=83=8F=E3=83=B3=E3=83=89=E3=83=A9=E3=82=92=E8=B5=B7?= =?UTF-8?q?=E5=8B=95=E5=89=8D=E3=81=AB=E7=99=BB=E9=8C=B2=E3=81=97=E3=80=81?= =?UTF-8?q?=E3=82=B5=E3=82=A4=E3=82=BA=E4=B8=8A=E9=99=90=E3=82=82=20positi?= =?UTF-8?q?ve=20int=20=E6=89=B1=E3=81=84=E3=81=AB=E3=81=99=E3=82=8B=20(#10?= =?UTF-8?q?4=20=E3=83=AC=E3=83=93=E3=83=A5=E3=83=BC=E5=AF=BE=E5=BF=9C)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SIGTERM/SIGINT ハンドラをモジュールロード時(async startup 前)に登録。 legacy migration や screenshot scan 中の停止でも graceful に drain する (Codex レビュー 🔴)。server 未 listen・store 未初期化でも安全に畳む - MAX_BODY_BYTES / SCREENSHOT_MAX_BYTES を positiveIntSetting に変更。 0/負は POST を全部壊す誤設定なので警告 + fallback(Codex レビュー 🟡、 README の記述とも一致) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LRdDjwxbAAaTMwwnzipkau --- server/receive.js | 19 +++++++++++++------ test/receiver.test.js | 4 +++- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/server/receive.js b/server/receive.js index 31ad090..8d017b7 100644 --- a/server/receive.js +++ b/server/receive.js @@ -17,9 +17,11 @@ const PORT = numberSetting(process.env.PORT, numberSetting(config.port, 4000, "p const HOST = process.env.HOST || config.host || "127.0.0.1"; const LEGACY_STORE_PATH = process.env.FEEDBACK_STORE_PATH || pathFromConfig(config.feedbackStorePath, path.join(__dirname, "feedback.json")); const DB_PATH = process.env.FEEDBACK_DB_PATH || pathFromConfig(config.feedbackDbPath, path.join(__dirname, "feedback.db")); -const MAX_BODY_BYTES = numberSetting(process.env.MAX_BODY_BYTES, numberSetting(config.maxBodyBytes, 3_000_000, "maxBodyBytes (config)"), "MAX_BODY_BYTES (env)"); +const MAX_BODY_BYTES = positiveIntSetting(process.env.MAX_BODY_BYTES, positiveIntSetting(config.maxBodyBytes, 3_000_000, "maxBodyBytes (config)"), "MAX_BODY_BYTES (env)"); const SCREENSHOT_DIR = process.env.SCREENSHOT_DIR || pathFromConfig(config.screenshotDir, path.join(__dirname, "screenshots")); -const SCREENSHOT_MAX_BYTES = numberSetting(process.env.SCREENSHOT_MAX_BYTES, numberSetting(config.screenshotMaxBytes, 1_500_000, "screenshotMaxBytes (config)"), "SCREENSHOT_MAX_BYTES (env)"); +// Size caps use positiveIntSetting like the shape limits below: a 0 / negative +// cap would reject every POST, so it is a misconfiguration, not a setting. +const SCREENSHOT_MAX_BYTES = positiveIntSetting(process.env.SCREENSHOT_MAX_BYTES, positiveIntSetting(config.screenshotMaxBytes, 1_500_000, "screenshotMaxBytes (config)"), "SCREENSHOT_MAX_BYTES (env)"); // Defense-in-depth shape limits on accepted payloads. MAX_BODY_BYTES already // caps the raw request, but without these a single in-budget request could // still smuggle an oversized string (e.g. a multi-MB comment copied verbatim @@ -351,9 +353,12 @@ function shutdown(signal) { console.warn(`[PatchLoop receiver] drain deadline (${SHUTDOWN_TIMEOUT_MS}ms) exceeded, exiting`); process.exit(1); }, SHUTDOWN_TIMEOUT_MS).unref(); + // A signal can arrive while startup is still running (legacy migration, + // screenshot dir scan): close() on a non-listening server still invokes the + // callback (with an error we can ignore), and the store may not exist yet. server.close(async () => { try { - await store.close(); + if (store) await store.close(); } catch (error) { console.warn(`[PatchLoop receiver] store close failed: ${error.message}`); } @@ -362,6 +367,11 @@ function shutdown(signal) { }); } +// Registered at load — before the async startup — so a stop during startup +// drains via the same path instead of dying mid-migration. +process.on("SIGTERM", () => shutdown("SIGTERM")); +process.on("SIGINT", () => shutdown("SIGINT")); + // Storage is initialized (and the legacy JSON store migrated) before the // server accepts requests, so no handler can run against an unready store. async function start() { @@ -388,9 +398,6 @@ async function start() { console.log(`[PatchLoop receiver] Slack file upload: ${SLACK_BOT_TOKEN && SLACK_UPLOAD_CHANNEL_ID ? "enabled" : "disabled"}`); console.log(`[PatchLoop receiver] GitHub issues: ${GITHUB_CONFIGURED ? `enabled (${GITHUB_REPO})` : "disabled"}`); - process.on("SIGTERM", () => shutdown("SIGTERM")); - process.on("SIGINT", () => shutdown("SIGINT")); - server.listen(PORT, HOST, () => { console.log(`[PatchLoop receiver] listening on http://${HOST}:${PORT}`); }); diff --git a/test/receiver.test.js b/test/receiver.test.js index 980b5c8..a4de9c2 100644 --- a/test/receiver.test.js +++ b/test/receiver.test.js @@ -1235,10 +1235,12 @@ test("SIGTERM drains: an in-flight request completes and the process exits clean }); test("invalid settings warn at startup and the effective values are logged", async (t) => { - const receiver = await startReceiver(t, { MAX_IMPORT_ITEMS: "0", RATE_LIMIT_MAX: "abc" }); + const receiver = await startReceiver(t, { MAX_IMPORT_ITEMS: "0", RATE_LIMIT_MAX: "abc", MAX_BODY_BYTES: "-5" }); assert.match(receiver.logs, /ignored invalid setting MAX_IMPORT_ITEMS \(env\): "0" — using 500/); assert.match(receiver.logs, /ignored invalid setting RATE_LIMIT_MAX \(env\): "abc" — using 120/); + // Size caps are limits too: 0 / negative would reject every POST. + assert.match(receiver.logs, /ignored invalid setting MAX_BODY_BYTES \(env\): "-5" — using 3000000/); // The one-block effective summary shows what the server actually runs with. assert.match(receiver.logs, /limits: body=3000000B .*importItems=500/); assert.match(receiver.logs, /rate limit: 120 req \/ 60000ms per client/);