Skip to content

selfhost(config): route the two remaining bare-Number() env knobs through parsePositiveIntEnv and preflight #10056

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

parsePositiveIntEnv (src/selfhost/queue-common.ts:1103) is this repo's contract for reading a numeric
self-host env knob: a missing value takes the fallback silently, and a supplied value that is non-finite
or out of range takes the fallback with a structured warn line (warnEnvKnobRejected,
src/selfhost/queue-common.ts:1090). positiveInteger in src/selfhost/preflight.ts:259 is the boot-time
half of the same contract, and its own header states the reasoning:

// #9157: a bare `Number(process.env.X ?? default)` at a call site turns a wrong-unit/wrong-type operator
// mistake ("2m", "120s", "120_000", a fraction, a negative number) into NaN — and NaN silently becomes a
// runaway ... or a silent feature disable (a `> 0` gate on NaN is always false), with no boot-time signal
// either way.

CRON_INTERVAL_MS, PORT and GITHUB_CACHE_TTL_SECONDS were converted to that contract on both halves
(src/selfhost/preflight.ts:381src/selfhost/preflight.ts:383; src/server.ts:817, src/server.ts:1165,
src/server.ts:1396). Two self-host numeric knobs were left on the bare-Number() form and are the only
remaining instances in src/:

// src/server.ts:1579
const forceReleaseAfterMs = Number(process.env["LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS"] ?? "");
const drainPromise = backend.shutdown();
const drainedInTime =
  Number.isFinite(forceReleaseAfterMs) && forceReleaseAfterMs > 0
    ? await Promise.race([...])
    : await drainPromise.then(() => true);
// src/selfhost/ai.ts:742
export function ollamaNumCtx(): number {
  const raw = Number(process.env["OLLAMA_NUM_CTX"] ?? "");
  return Number.isFinite(raw) && raw > 0 ? Math.floor(raw) : 32_768;
}

Both are env.SOMETHING reads that already appear in the generated self-host env reference
(apps/loopover-ui/src/lib/selfhost-env-reference.ts:349 for the shutdown knob), so an operator can find and
set them — and both fail in exactly the two ways #9157 named:

  1. Silent feature disable, no warn. LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS=30s (or 2m, or 30_000)
    parses to NaN, Number.isFinite is false, and the shutdown path silently takes the "wait for the drain"
    branch — i.e. the opt-in the operator just enabled does nothing, with no log line, no metric, and no
    preflight error. parsePositiveIntEnv would have emitted warnEnvKnobRejected; positiveInteger would
    have failed boot with a message naming the unit-suffix mistake.

  2. A fractional value is accepted rather than rejected. Number("0.5") is finite and > 0, so
    Promise.race gets a 0.5 ms deadline: every shutdown loses the race, drainedInTime is always false,
    and the proactive bulk releaseAllHeldLocksAtShutdown() fires on every shutdown — the exact
    behaviour the comment immediately above it (src/server.ts:1566src/server.ts:1578) says was removed
    because it "could claim the freed lock at t0+e and duplicate the very actuation the lock exists to
    serialize". parsePositiveIntEnv floors and range-checks; positiveInteger's /^\d+$/ test rejects a
    decimal point outright.

    OLLAMA_NUM_CTX=0.5 is accepted the same way and is then Math.floored to 0, which is sent to Ollama as
    options.num_ctx: 0 by ollamaContextOptions (src/selfhost/ai.ts:731) — a context window of zero,
    rather than the 32768 the fallback exists to provide.

Neither var is listed in preflightEnv (src/selfhost/preflight.ts:381src/selfhost/preflight.ts:383), so
neither gets the boot-time half either.

Requirements

  • src/server.ts:1579 must read the knob via parsePositiveIntEnv (already imported at src/server.ts:76).
    Because 0/unset must continue to mean "wait for the drain, do not force-release", the read must keep that
    meaning explicitly: use { min: 0, fallback: 0 } and keep the existing > 0 gate on the result, so unset
    0 ⇒ wait for the drain (byte-identical to today), while a supplied non-integer/negative value now
    produces a warn line and falls back to 0 instead of silently doing the same thing unannounced.
  • ollamaNumCtx (src/selfhost/ai.ts:742) must read via
    parsePositiveIntEnv("OLLAMA_NUM_CTX", { min: 1, fallback: 32_768 }) and return that value directly. The
    existing default of 32768 and the "supplied valid value wins" behaviour must not change.
  • preflightEnv (src/selfhost/preflight.ts:306) must gain two positiveInteger(...) calls alongside the
    existing three, so a malformed value hard-fails boot with a clear message rather than only warning at use
    time:
    • positiveInteger(problems, env, "LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS", 0, 24 * 60 * 60_000)0
      is a valid, meaningful value here (it is the documented "wait for the drain" default), so the minimum is
      0, matching how GITHUB_CACHE_TTL_SECONDS is treated at src/selfhost/preflight.ts:383.
    • positiveInteger(problems, env, "OLLAMA_NUM_CTX", 1, 1_000_000)0 is not a meaningful context
      window, so the minimum is 1.
  • Absence must remain fine for both: positiveInteger already returns early on a blank value
    (src/selfhost/preflight.ts:265) and must keep doing so — this issue only judges a value the operator
    actually set.
  • Must NOT change: parsePositiveIntEnv or warnEnvKnobRejected themselves, the three existing
    positiveInteger calls, the shutdown sequence's drain-first ordering
    (src/server.ts:1580src/server.ts:1587), or ollamaContextOptions' provider gate
    (src/selfhost/ai.ts:731).
  • No migrations/*.sql file may be added or edited by this issue; it needs no schema change.

⚠️ Required pattern: mirror src/server.ts:1396's parsePositiveIntEnv("CRON_INTERVAL_MS", { min: ..., fallback: ... })
call plus its paired positiveInteger(...) entry at src/selfhost/preflight.ts:381. What does NOT satisfy
this issue: (a) writing a new local parse helper in src/server.ts or src/selfhost/ai.ts instead of using
the existing parsePositiveIntEnv — a second mechanism is what #9157 removed; (b) fixing the runtime read
but skipping the two preflightEnv entries, so a unit-suffix typo still only produces a warn line the
operator has already scrolled past; (c) changing the default of either knob; (d) a test-only PR.

Deliverables

  • src/server.ts's shutdown handler reads LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS through
    parsePositiveIntEnv with { min: 0, fallback: 0 } and keeps the > 0 gate that selects the
    Promise.race branch.
  • ollamaNumCtx (src/selfhost/ai.ts) reads through
    parsePositiveIntEnv("OLLAMA_NUM_CTX", { min: 1, fallback: 32_768 }).
  • preflightEnv (src/selfhost/preflight.ts) adds the two positiveInteger entries described above.
  • Tests in test/unit/selfhost-preflight.test.ts asserting preflightEnv reports a problem whose var
    is LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS for each of "30s", "30_000", "0.5" and "-1", and
    ok: true for each of unset, "", "0" and "30000"; and the same table for OLLAMA_NUM_CTX with
    "0" additionally reported as a problem and "1" accepted.
  • A regression test named for this bug at test/unit/selfhost-ai.test.ts asserting
    ollamaNumCtx() returns 32768 (not 0) when OLLAMA_NUM_CTX is stubbed to "0.5", and returns
    65536 when stubbed to "65536".

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that converts the two runtime reads but leaves preflightEnv unchanged — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts; src/server.ts, src/selfhost/ai.ts and
src/selfhost/preflight.ts are all measured. positiveInteger (src/selfhost/preflight.ts:259) has three
branches per call — blank/early-return, the /^\d+$/ reject, and the range reject — and each new var must
exercise all three plus the accepted path. The > 0 ternary at the src/server.ts shutdown call site needs
both arms: a run with the knob unset (falls to the plain await drainPromise) and one with a valid positive
value (takes the Promise.race branch).

Expected Outcome

An operator who writes LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS=30s or OLLAMA_NUM_CTX=0.5 finds out at
boot, from a preflight error naming the mistake, instead of silently getting a disabled opt-in (or a
zero-width context window) that looks identical to never having set the variable. Every numeric self-host env
knob in src/ then reads through the single parsePositiveIntEnv contract, with no bare Number(env.X)
left behind.

Links & Resources

  • src/server.ts:1579 — the shutdown-lock knob's bare Number()
  • src/selfhost/ai.ts:742ollamaNumCtx
  • src/selfhost/queue-common.ts:1103parsePositiveIntEnv, the contract to use
  • src/selfhost/preflight.ts:259positiveInteger, the boot-time half
  • src/selfhost/preflight.ts:381 — the three existing entries to sit beside
  • src/server.ts:1566 — the comment explaining why an always-firing bulk lock release is harmful
  • apps/loopover-ui/src/lib/selfhost-env-reference.ts:349 — the shutdown knob in the generated reference

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions