Skip to content

Commit 2c18d14

Browse files
author
Jacob Weinhold
committed
fix(capture): an override set to empty is an intent, not an absence
Review round 3 (P3). `if (!raw)` folded a SET-but-empty VEXA_CSRC_INACTIVE_MS into "not set" and fell back in silence — which is exactly the ignored-knob failure the previous commit's warning exists to prevent, and an empty value is what a half-rendered deploy template looks like. Only `undefined` is an absence now; everything else that cannot be honoured says so. The default also stops being written three times; the fallback, the message and the constant cannot drift apart any more. Refs #150 Signed-off-by: Jacob Weinhold <jacob@philflow.io>
1 parent fc58569 commit 2c18d14

2 files changed

Lines changed: 60 additions & 2 deletions

File tree

core/meetings/services/bot/src/capture-bridge.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,25 @@ const TEAMS_ENABLE_CAPTIONS_ATTEMPTS = Math.max(1, Number(process.env.VEXA_TEAMS
397397
* this value was lost once before (#150). Overridable per deployment — and therefore per platform,
398398
* since each platform's bot is its own process — via VEXA_CSRC_INACTIVE_MS.
399399
*/
400-
const CSRC_INACTIVE_MS = Math.max(1, Number(process.env.VEXA_CSRC_INACTIVE_MS || 800));
400+
/** The sensor's poll cadence (`CSRC_POLL_MS`). Not imported: this file must not pull the page-side
401+
* bundle into the Node build. Kept as the floor only, so a drift there is a slacker floor here,
402+
* never a wrong window. */
403+
const CSRC_MIN_INACTIVE_MS = 100;
404+
/** The measured window (#150). One name, so the fallback, the message and the default cannot drift. */
405+
const CSRC_DEFAULT_INACTIVE_MS = 800;
406+
export function resolveCsrcInactiveMs(raw: string | undefined, warn: (m: string) => void = () => { /* silent */ }): number {
407+
// `undefined` is "no override", which is the normal case and silent. An override that was SET —
408+
// including an empty or blank one, which is what a half-finished deploy template looks like — is
409+
// an intent that could not be honoured, and gets said out loud.
410+
if (raw === undefined) return CSRC_DEFAULT_INACTIVE_MS;
411+
const n = Number(raw);
412+
if (!Number.isFinite(n) || n < CSRC_MIN_INACTIVE_MS) {
413+
warn(`[bot] VEXA_CSRC_INACTIVE_MS=${JSON.stringify(raw)} is not a usable window (finite, >= ${CSRC_MIN_INACTIVE_MS}ms) — using the measured ${CSRC_DEFAULT_INACTIVE_MS}ms`);
414+
return CSRC_DEFAULT_INACTIVE_MS;
415+
}
416+
return n;
417+
}
418+
const CSRC_INACTIVE_MS = resolveCsrcInactiveMs(process.env.VEXA_CSRC_INACTIVE_MS, (m) => console.warn(m));
401419

402420
/** Outcome of one enable attempt. `already-on` and `clicked` are successes; `failed` carries WHY,
403421
* because "captions never appeared" has two very different causes — the menu path changed, or the

core/meetings/services/bot/src/csrc-wiring.test.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import { readFileSync, existsSync } from 'node:fs';
2525
import { fileURLToPath } from 'node:url';
2626
import path from 'node:path';
2727
import {
28-
startCaptureBridge, makeCsrcSink, makeObservationSink,
28+
startCaptureBridge, makeCsrcSink, makeObservationSink, resolveCsrcInactiveMs,
2929
type CsrcRecord, type CsrcCapableSink, type ObservationRecord, type ObservationCapableSink,
3030
} from './capture-bridge.js';
3131
import type { Invocation } from './config.js';
@@ -87,6 +87,46 @@ const check = (name: string, cond: boolean, detail?: string) => {
8787
warnings.some((w) => w.includes('observation-clock-skew')) && stored[2].t >= t, warnings.join(' | '));
8888
}
8989

90+
// ── the window is resolved from env, and garbage never reaches the sensor ───────────────────────
91+
// The sensor resolves its option with `??`, which passes NaN through, and a NaN window makes BOTH
92+
// of its comparisons false: every source is re-deactivated on every poll — the fragmentation this
93+
// value exists to end, worse and silent. So an unusable override falls back rather than propagating.
94+
{
95+
check('window: no override ⇒ the measured 800ms', resolveCsrcInactiveMs(undefined) === 800,
96+
String(resolveCsrcInactiveMs(undefined)));
97+
check('window: a usable override wins', resolveCsrcInactiveMs('1200') === 1200,
98+
String(resolveCsrcInactiveMs('1200')));
99+
check('window: garbage falls back instead of becoming NaN',
100+
resolveCsrcInactiveMs('abc') === 800, String(resolveCsrcInactiveMs('abc')));
101+
check('window: zero, negative and empty fall back too — none of them is a window',
102+
resolveCsrcInactiveMs('0') === 800 && resolveCsrcInactiveMs('-5') === 800 && resolveCsrcInactiveMs('') === 800,
103+
`${resolveCsrcInactiveMs('0')}/${resolveCsrcInactiveMs('-5')}/${resolveCsrcInactiveMs('')}`);
104+
// A window shorter than one poll is stale before the next tick — the NaN failure mode wearing an
105+
// ordinary number. It is rejected at the floor, not accepted because it happens to be positive.
106+
check('window: shorter than one 100ms poll is rejected, one poll exactly is kept',
107+
resolveCsrcInactiveMs('1') === 800 && resolveCsrcInactiveMs('99') === 800 && resolveCsrcInactiveMs('100') === 100,
108+
`${resolveCsrcInactiveMs('1')}/${resolveCsrcInactiveMs('99')}/${resolveCsrcInactiveMs('100')}`);
109+
{
110+
const warnings: string[] = [];
111+
const used = resolveCsrcInactiveMs('abc', (m) => warnings.push(m));
112+
check('window: a rejected override is SAID OUT LOUD, never silently ignored',
113+
used === 800 && warnings.length === 1 && warnings[0]!.includes('VEXA_CSRC_INACTIVE_MS="abc"'),
114+
JSON.stringify(warnings));
115+
const quiet: string[] = [];
116+
resolveCsrcInactiveMs(undefined, (m) => quiet.push(m));
117+
resolveCsrcInactiveMs('1200', (m) => quiet.push(m));
118+
check('window: no override and a good override are both silent — only a REJECTION warns',
119+
quiet.length === 0, JSON.stringify(quiet));
120+
// An empty override is what a half-rendered deploy template looks like. Treating it as "not
121+
// set" is how a knob gets ignored in silence — the exact failure the warning exists to prevent.
122+
const blank: string[] = [];
123+
check('window: an override SET to empty or blank warns too — it is an intent, not an absence',
124+
resolveCsrcInactiveMs('', (m) => blank.push(m)) === 800
125+
&& resolveCsrcInactiveMs(' ', (m) => blank.push(m)) === 800
126+
&& blank.length === 2, JSON.stringify(blank));
127+
}
128+
}
129+
90130
// ── The real bundle (built by build-browser-utils.mjs — turbo test depends on build) ─────────────
91131
const BUNDLE = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', 'dist', 'browser-utils.global.js');
92132
if (!existsSync(BUNDLE)) {

0 commit comments

Comments
 (0)