Four more falsy-zero env parses in backend/ — two real, one deliberately protective

[lilyshen0722]

#985 was about AGENT_ASK_RATE_LIMIT_PER_HOUR=0 silently becoming 30, and #990 fixes it. While verifying that fix I grepped backend/ for the same shape — parseInt(process.env.X || '…', 10) || FALLBACK — to see whether it was one site or a habit.

It is five sites. They do not all mean the same thing, and that is the point of this issue: a shared mechanism licenses "check the other sites", never "the other sites are also broken". I read each consumer. Two are real, one is load-bearing protection that must be left alone, one is marginal.

Real — backend/services/agentEventService.ts:531

const thresholdKb = Math.max(
  64,
  Number.parseInt(process.env.AGENT_SESSION_MAX_SIZE_KB || '', 10) || 400,
);

env	resolves to
unset / '' / abc	400
10	64 (floor)
0	400

The floor exists to stop an operator setting the session-clear threshold absurdly low. 0 is the most aggressive value expressible, and it produces the laxest threshold of any input — 6× the floor it was trying to undercut. Someone reaching for 0 here is trying to clear sessions eagerly (the CLAUDE.md rule about session bloat being broken behaviour is exactly the pressure that produces that reach); they get the opposite, with no error. Math.max makes the fallback unnecessary anyway: parse, Number.isNaN → 400, then clamp.

Real, and sharper — backend/services/schedulerService.ts:767

const BATCH_PAUSE_MS = parseInt(process.env.SUMMARIZER_FANOUT_BATCH_PAUSE_MS || '500', 10) || 500;
…
if (i + BATCH_SIZE < installations.length && BATCH_PAUSE_MS > 0) {
  await new Promise((resolve) => setTimeout(resolve, BATCH_PAUSE_MS));
}

The consumer already branches on BATCH_PAUSE_MS > 0 — the code anticipates "no pause between batches" as a supported configuration. The parser on the line above guarantees that branch is never taken: every input yields 500 or a positive integer, and 0 yields 500. So this is two defects wearing one line:

1. SUMMARIZER_FANOUT_BATCH_PAUSE_MS=0 silently does the opposite of what it says — a half-second pause per batch.
2. && BATCH_PAUSE_MS > 0 is dead code. It can never be false.

Whichever way it is resolved, both halves should move together — fixing the parse without noticing the guard leaves a reader thinking the guard was always live, and deleting the guard without fixing the parse removes the only evidence that 0 was meant to work.

Not a defect — backend/services/schedulerService.ts:766

const BATCH_SIZE = parseInt(process.env.SUMMARIZER_FANOUT_BATCH_SIZE || '10', 10) || 10;

Three lines above the previous one and syntactically identical, so a mechanical sweep would "fix" it. Don't. The loop is for (let i = 0; i < installations.length; i += BATCH_SIZE). A delivered 0 is an infinite loop that enqueues the same batch forever against the event queue. Here || 10 is the only thing standing between a typo and a runaway hourly job. If anything it deserves an explicit Math.max(1, …) and a comment saying why, not a Number.isNaN conversion.

Marginal — backend/gateway/index.ts:80

const port = options.port || parseInt(process.env.GATEWAY_PORT, 10) || 5001;

0 is meaningful to Node (bind an ephemeral port) and is a real thing to want in tests, so it is the same class of bug. But nothing in-tree passes 0, and the failure is "you got 5001 instead of a random port", which is loud immediately. Worth folding in if the file is being touched; not worth a PR.

Suggested scope

One PR covering agentEventService.ts:531 and schedulerService.ts:767 (+ its dead guard), each with a test in the shape #990 established — pin 0, '', unset, and a valid value, so that once 0 stops routing to the default an empty string reading as 0 cannot become the next regression. A k8s env block spells "unset" as '', which is why that control pair matters more here than it looks.

Explicitly out of scope: schedulerService.ts:766. Leaving a note in this issue is cheaper than re-deriving why it is different when someone greps the pattern again.

Found while reviewing #990 (verified red/green there); not filed on that PR so it does not race the merge.

[lilyshen0722]

Sharpening the exclusion, because the issue text alone protects the wrong reader.

@pod-architect's point is that a blanket sweep of this shape is itself the regression — right, and it's the one case in the five where || intercepting zero is load-bearing rather than accidental. That distinction is the general rule worth carrying out of this issue: before converting a || FALLBACK to a NaN check, ask whether the delivered zero is merely wrong or actually unsafe. At :767 a zero is a supported configuration the consumer already branches on. Three lines up at :766 a zero is an infinite loop.

But the person this needs to reach is grepping parseInt(process.env — and a grep lands in the source, not here. So the exclusion should ship in the code, as part of the same PR that fixes the other two:

// A delivered 0 here is an infinite loop: the caller advances with
// `i += BATCH_SIZE`. Unlike the PAUSE_MS line below, `|| 10` is a guard,
// not the #985 falsy-zero bug — do not convert it to a NaN check. See #991.
const BATCH_SIZE = Math.max(1, parseInt(process.env.SUMMARIZER_FANOUT_BATCH_SIZE || '10', 10) || 10);

Math.max(1, …) is worth adding alongside the comment: it makes the intent enforced rather than described, so the protection survives someone who edits the line without reading three lines of comment.

Treat that as a deliverable of the same PR, not a nice-to-have. The two fixed sites and the one deliberately-unfixed site sitting adjacent in the same diff is what makes the distinction legible to the next reader — and :766 and :767 being byte-adjacent and syntactically identical is precisely why one of them will otherwise get swept.