From b7b65e58574b907c5fa8e8a496ee6fb558673cbb Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 31 Jul 2026 14:53:40 +0800 Subject: [PATCH] fix(selfhost): route the last two bare-Number() env knobs through parsePositiveIntEnv + preflight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS (src/server.ts) and OLLAMA_NUM_CTX (src/selfhost/ai.ts) were the only self-host numeric knobs still read with a bare Number(process.env.X ?? ""), the exact form #9157 replaced everywhere else. Both fail the two ways that comment names: a unit-suffixed/separator value ("30s", "30_000") NaN's and silently disables the opt-in with no signal, and a fractional value ("0.5") is accepted — the shutdown deadline loses every race (bulk lock release fires on every shutdown) and ollamaNumCtx floors 0.5 to 0. Read both via parsePositiveIntEnv (server: { min: 0, fallback: 0 } to keep unset ⇒ wait-for-the-drain; ollama: { min: 1, fallback: 32_768 }), and add the paired positiveInteger preflight entries so a malformed value hard-fails boot with a clear message instead of only warning at use time. Defaults, the shutdown drain-first ordering, and the ollama provider gate are unchanged. Closes #10056 --- .../src/lib/selfhost-env-reference.ts | 4 +-- src/selfhost/ai.ts | 6 ++-- src/selfhost/preflight.ts | 4 +++ src/server.ts | 6 +++- test/unit/selfhost-ai.test.ts | 8 +++++ test/unit/selfhost-preflight.test.ts | 32 +++++++++++++++++++ 6 files changed, 55 insertions(+), 5 deletions(-) diff --git a/apps/loopover-ui/src/lib/selfhost-env-reference.ts b/apps/loopover-ui/src/lib/selfhost-env-reference.ts index a85f36d758..0199348e84 100644 --- a/apps/loopover-ui/src/lib/selfhost-env-reference.ts +++ b/apps/loopover-ui/src/lib/selfhost-env-reference.ts @@ -347,7 +347,7 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [ }, { name: "LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS", - firstReference: "src/server.ts", + firstReference: "src/selfhost/preflight.ts", }, { name: "LOOPOVER_SINGLE_INSTANCE", @@ -807,7 +807,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [ "| `LOOPOVER_REVIEW_CONTINUOUS` | `src/queue/processors.ts` |", "| `LOOPOVER_REVIEW_RAG` | `src/selfhost/ai.ts` |", "| `LOOPOVER_REVIEW_SAFETY` | `src/selfhost/inert-config.ts` |", - "| `LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS` | `src/server.ts` |", + "| `LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS` | `src/selfhost/preflight.ts` |", "| `LOOPOVER_SINGLE_INSTANCE` | `src/selfhost/redis-cache.ts` |", "| `LOOPOVER_VERSION` | `src/selfhost/otel.ts` |", "| `MAINTENANCE_ADMISSION_DEFER_MS` | `src/selfhost/maintenance-admission.ts` |", diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index 69afc6366d..e9d4cd3f2c 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -9,6 +9,7 @@ import { isStructuralProviderConfigError } from "../services/ai-review"; import type { AiContentBlock, CombineStrategy, OnMerge } from "../services/ai-review"; import { isConfiguredSelfHostProvider, resolveConfiguredProviderNames } from "./ai-config"; +import { parsePositiveIntEnv } from "./queue-common"; export { assertNoLegacySharedAiEnv } from "./ai-config"; import { wasLoadedFromFile } from "./file-sourced-secrets"; import { getProviderCredentialResolver } from "./provider-credential-registry"; @@ -740,8 +741,9 @@ export function ollamaContextOptions( /** Context window requested from Ollama for review-sized prompts. Overridable because it trades GPU memory * against how much of a large diff the model can actually see. */ export function ollamaNumCtx(): number { - const raw = Number(process.env["OLLAMA_NUM_CTX"] ?? ""); - return Number.isFinite(raw) && raw > 0 ? Math.floor(raw) : 32_768; + // parsePositiveIntEnv (not a bare Number()): a supplied non-integer/out-of-range OLLAMA_NUM_CTX warns and + // falls back to the default instead of silently disabling the override via NaN, matching #9157's contract. + return parsePositiveIntEnv("OLLAMA_NUM_CTX", { min: 1, fallback: 32_768 }); } export function providerNameFromBaseUrl(baseUrl: string | undefined): "ollama" | "openai" | "openai-compatible" { diff --git a/src/selfhost/preflight.ts b/src/selfhost/preflight.ts index 85400f1807..682f498120 100644 --- a/src/selfhost/preflight.ts +++ b/src/selfhost/preflight.ts @@ -381,6 +381,10 @@ export function preflightEnv(env: SelfHostPreflightEnv): SelfHostPreflightResult positiveInteger(problems, env, "CRON_INTERVAL_MS", CRON_INTERVAL_MIN_MS, 24 * 60 * 60_000); positiveInteger(problems, env, "PORT", 1, 65_535); positiveInteger(problems, env, "GITHUB_CACHE_TTL_SECONDS", 0, 86_400); + // 0 is the documented "wait for the drain" default, so min 0 (mirrors GITHUB_CACHE_TTL_SECONDS above); #10056. + positiveInteger(problems, env, "LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS", 0, 24 * 60 * 60_000); + // 0 is not a meaningful context window, so min 1. + positiveInteger(problems, env, "OLLAMA_NUM_CTX", 1, 1_000_000); checkLedgerAnchorConfig(problems, env); checkLedgerContentWaiverConfig(problems, env); diff --git a/src/server.ts b/src/server.ts index 335606bc92..a905beaa68 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1576,7 +1576,11 @@ async function main(): Promise { // is genuinely imminent and a stranded lock is the worse outcome. Opt in with // LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS; unset means "wait for the drain", which is right wherever the // orchestrator's grace period comfortably exceeds a review (this deployment's stop_grace_period is 300s). - const forceReleaseAfterMs = Number(process.env["LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS"] ?? ""); + // parsePositiveIntEnv (not a bare Number()): a supplied non-integer/out-of-range value (e.g. "30s", + // "30_000", "0.5", "-1") now warns and falls back to 0 — "wait for the drain" — instead of silently + // taking that same branch (NaN) or accepting a fractional millisecond deadline every shutdown loses (#10056). + // { min: 0, fallback: 0 } keeps unset ⇒ 0 ⇒ the `> 0` gate below selecting the drain-first path, unchanged. + const forceReleaseAfterMs = parsePositiveIntEnv("LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS", { min: 0, fallback: 0 }); const drainPromise = backend.shutdown(); const drainedInTime = Number.isFinite(forceReleaseAfterMs) && forceReleaseAfterMs > 0 diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index e7b2862dc8..0db562f4d7 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -2522,6 +2522,14 @@ describe("ollama context window (#9478)", () => { process.env["OLLAMA_NUM_CTX"] = value; expect(ollamaNumCtx()).toBeGreaterThan(8_192); // must exceed the truncation-prone stock defaults }); + + it("REGRESSION (#10056): a fractional OLLAMA_NUM_CTX falls back to the default instead of flooring to 0, and a valid value still wins", () => { + process.env["OLLAMA_NUM_CTX"] = "0.5"; // bare Number() floored this to 0 (a disabled context window) + expect(ollamaNumCtx()).toBe(32_768); + process.env["OLLAMA_NUM_CTX"] = "65536"; + expect(ollamaNumCtx()).toBe(65_536); + delete process.env["OLLAMA_NUM_CTX"]; + }); }); describe("resolveClaudeOauthToken (#9543 — rotate the credential without recreating the container)", () => { diff --git a/test/unit/selfhost-preflight.test.ts b/test/unit/selfhost-preflight.test.ts index d2460a3e5c..915b9c5c11 100644 --- a/test/unit/selfhost-preflight.test.ts +++ b/test/unit/selfhost-preflight.test.ts @@ -355,6 +355,38 @@ describe("self-host environment preflight (#2080)", () => { expect(preflightEnv({ ...baseEnv, GITHUB_CACHE_TTL_SECONDS: "0" })).toEqual({ ok: true, problems: [] }); }); + it("rejects a malformed LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS instead of only warning at use time (#10056)", () => { + for (const LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS of ["30s", "30_000", "0.5", "-1"]) { + expect(preflightEnv({ ...baseEnv, LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS })).toEqual({ + ok: false, + problems: [expect.objectContaining({ var: "LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS" })], + }); + } + }); + + it("accepts unset / '' / '0' (wait-for-the-drain default) / a plain integer for LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS (#10056)", () => { + for (const value of [undefined, "", "0", "30000"]) { + const env = value === undefined ? { ...baseEnv } : { ...baseEnv, LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS: value }; + expect(preflightEnv(env)).toEqual({ ok: true, problems: [] }); + } + }); + + it("rejects a malformed OLLAMA_NUM_CTX, and additionally rejects '0' (not a meaningful context window) (#10056)", () => { + for (const OLLAMA_NUM_CTX of ["30s", "30_000", "0.5", "-1", "0"]) { + expect(preflightEnv({ ...baseEnv, OLLAMA_NUM_CTX })).toEqual({ + ok: false, + problems: [expect.objectContaining({ var: "OLLAMA_NUM_CTX" })], + }); + } + }); + + it("accepts unset / '' / '1' / a plain integer for OLLAMA_NUM_CTX (#10056)", () => { + for (const value of [undefined, "", "1", "65536"]) { + const env = value === undefined ? { ...baseEnv } : { ...baseEnv, OLLAMA_NUM_CTX: value }; + expect(preflightEnv(env)).toEqual({ ok: true, problems: [] }); + } + }); + it("rejects a unit-suffixed or separator-formatted value instead of silently NaN-ing", () => { for (const CRON_INTERVAL_MS of ["2m", "120s", "120_000", "12.5", "-5", "abc"]) { const result = preflightEnv({ ...baseEnv, CRON_INTERVAL_MS });