Skip to content

Commit b7b65e5

Browse files
author
RealDiligent
committed
fix(selfhost): route the last two bare-Number() env knobs through parsePositiveIntEnv + preflight
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
1 parent a7673e2 commit b7b65e5

6 files changed

Lines changed: 55 additions & 5 deletions

File tree

apps/loopover-ui/src/lib/selfhost-env-reference.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [
347347
},
348348
{
349349
name: "LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS",
350-
firstReference: "src/server.ts",
350+
firstReference: "src/selfhost/preflight.ts",
351351
},
352352
{
353353
name: "LOOPOVER_SINGLE_INSTANCE",
@@ -807,7 +807,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [
807807
"| `LOOPOVER_REVIEW_CONTINUOUS` | `src/queue/processors.ts` |",
808808
"| `LOOPOVER_REVIEW_RAG` | `src/selfhost/ai.ts` |",
809809
"| `LOOPOVER_REVIEW_SAFETY` | `src/selfhost/inert-config.ts` |",
810-
"| `LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS` | `src/server.ts` |",
810+
"| `LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS` | `src/selfhost/preflight.ts` |",
811811
"| `LOOPOVER_SINGLE_INSTANCE` | `src/selfhost/redis-cache.ts` |",
812812
"| `LOOPOVER_VERSION` | `src/selfhost/otel.ts` |",
813813
"| `MAINTENANCE_ADMISSION_DEFER_MS` | `src/selfhost/maintenance-admission.ts` |",

src/selfhost/ai.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import { isStructuralProviderConfigError } from "../services/ai-review";
1010
import type { AiContentBlock, CombineStrategy, OnMerge } from "../services/ai-review";
1111
import { isConfiguredSelfHostProvider, resolveConfiguredProviderNames } from "./ai-config";
12+
import { parsePositiveIntEnv } from "./queue-common";
1213
export { assertNoLegacySharedAiEnv } from "./ai-config";
1314
import { wasLoadedFromFile } from "./file-sourced-secrets";
1415
import { getProviderCredentialResolver } from "./provider-credential-registry";
@@ -740,8 +741,9 @@ export function ollamaContextOptions(
740741
/** Context window requested from Ollama for review-sized prompts. Overridable because it trades GPU memory
741742
* against how much of a large diff the model can actually see. */
742743
export function ollamaNumCtx(): number {
743-
const raw = Number(process.env["OLLAMA_NUM_CTX"] ?? "");
744-
return Number.isFinite(raw) && raw > 0 ? Math.floor(raw) : 32_768;
744+
// parsePositiveIntEnv (not a bare Number()): a supplied non-integer/out-of-range OLLAMA_NUM_CTX warns and
745+
// falls back to the default instead of silently disabling the override via NaN, matching #9157's contract.
746+
return parsePositiveIntEnv("OLLAMA_NUM_CTX", { min: 1, fallback: 32_768 });
745747
}
746748

747749
export function providerNameFromBaseUrl(baseUrl: string | undefined): "ollama" | "openai" | "openai-compatible" {

src/selfhost/preflight.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,10 @@ export function preflightEnv(env: SelfHostPreflightEnv): SelfHostPreflightResult
381381
positiveInteger(problems, env, "CRON_INTERVAL_MS", CRON_INTERVAL_MIN_MS, 24 * 60 * 60_000);
382382
positiveInteger(problems, env, "PORT", 1, 65_535);
383383
positiveInteger(problems, env, "GITHUB_CACHE_TTL_SECONDS", 0, 86_400);
384+
// 0 is the documented "wait for the drain" default, so min 0 (mirrors GITHUB_CACHE_TTL_SECONDS above); #10056.
385+
positiveInteger(problems, env, "LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS", 0, 24 * 60 * 60_000);
386+
// 0 is not a meaningful context window, so min 1.
387+
positiveInteger(problems, env, "OLLAMA_NUM_CTX", 1, 1_000_000);
384388

385389
checkLedgerAnchorConfig(problems, env);
386390
checkLedgerContentWaiverConfig(problems, env);

src/server.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1576,7 +1576,11 @@ async function main(): Promise<void> {
15761576
// is genuinely imminent and a stranded lock is the worse outcome. Opt in with
15771577
// LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS; unset means "wait for the drain", which is right wherever the
15781578
// orchestrator's grace period comfortably exceeds a review (this deployment's stop_grace_period is 300s).
1579-
const forceReleaseAfterMs = Number(process.env["LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS"] ?? "");
1579+
// parsePositiveIntEnv (not a bare Number()): a supplied non-integer/out-of-range value (e.g. "30s",
1580+
// "30_000", "0.5", "-1") now warns and falls back to 0 — "wait for the drain" — instead of silently
1581+
// taking that same branch (NaN) or accepting a fractional millisecond deadline every shutdown loses (#10056).
1582+
// { min: 0, fallback: 0 } keeps unset ⇒ 0 ⇒ the `> 0` gate below selecting the drain-first path, unchanged.
1583+
const forceReleaseAfterMs = parsePositiveIntEnv("LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS", { min: 0, fallback: 0 });
15801584
const drainPromise = backend.shutdown();
15811585
const drainedInTime =
15821586
Number.isFinite(forceReleaseAfterMs) && forceReleaseAfterMs > 0

test/unit/selfhost-ai.test.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2522,6 +2522,14 @@ describe("ollama context window (#9478)", () => {
25222522
process.env["OLLAMA_NUM_CTX"] = value;
25232523
expect(ollamaNumCtx()).toBeGreaterThan(8_192); // must exceed the truncation-prone stock defaults
25242524
});
2525+
2526+
it("REGRESSION (#10056): a fractional OLLAMA_NUM_CTX falls back to the default instead of flooring to 0, and a valid value still wins", () => {
2527+
process.env["OLLAMA_NUM_CTX"] = "0.5"; // bare Number() floored this to 0 (a disabled context window)
2528+
expect(ollamaNumCtx()).toBe(32_768);
2529+
process.env["OLLAMA_NUM_CTX"] = "65536";
2530+
expect(ollamaNumCtx()).toBe(65_536);
2531+
delete process.env["OLLAMA_NUM_CTX"];
2532+
});
25252533
});
25262534

25272535
describe("resolveClaudeOauthToken (#9543 — rotate the credential without recreating the container)", () => {

test/unit/selfhost-preflight.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,38 @@ describe("self-host environment preflight (#2080)", () => {
355355
expect(preflightEnv({ ...baseEnv, GITHUB_CACHE_TTL_SECONDS: "0" })).toEqual({ ok: true, problems: [] });
356356
});
357357

358+
it("rejects a malformed LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS instead of only warning at use time (#10056)", () => {
359+
for (const LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS of ["30s", "30_000", "0.5", "-1"]) {
360+
expect(preflightEnv({ ...baseEnv, LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS })).toEqual({
361+
ok: false,
362+
problems: [expect.objectContaining({ var: "LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS" })],
363+
});
364+
}
365+
});
366+
367+
it("accepts unset / '' / '0' (wait-for-the-drain default) / a plain integer for LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS (#10056)", () => {
368+
for (const value of [undefined, "", "0", "30000"]) {
369+
const env = value === undefined ? { ...baseEnv } : { ...baseEnv, LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS: value };
370+
expect(preflightEnv(env)).toEqual({ ok: true, problems: [] });
371+
}
372+
});
373+
374+
it("rejects a malformed OLLAMA_NUM_CTX, and additionally rejects '0' (not a meaningful context window) (#10056)", () => {
375+
for (const OLLAMA_NUM_CTX of ["30s", "30_000", "0.5", "-1", "0"]) {
376+
expect(preflightEnv({ ...baseEnv, OLLAMA_NUM_CTX })).toEqual({
377+
ok: false,
378+
problems: [expect.objectContaining({ var: "OLLAMA_NUM_CTX" })],
379+
});
380+
}
381+
});
382+
383+
it("accepts unset / '' / '1' / a plain integer for OLLAMA_NUM_CTX (#10056)", () => {
384+
for (const value of [undefined, "", "1", "65536"]) {
385+
const env = value === undefined ? { ...baseEnv } : { ...baseEnv, OLLAMA_NUM_CTX: value };
386+
expect(preflightEnv(env)).toEqual({ ok: true, problems: [] });
387+
}
388+
});
389+
358390
it("rejects a unit-suffixed or separator-formatted value instead of silently NaN-ing", () => {
359391
for (const CRON_INTERVAL_MS of ["2m", "120s", "120_000", "12.5", "-5", "abc"]) {
360392
const result = preflightEnv({ ...baseEnv, CRON_INTERVAL_MS });

0 commit comments

Comments
 (0)