Skip to content

Commit de774ca

Browse files
committed
fix(run-engine,webapp): address whole-branch adversarial review
Three-model blind review (no Critical). Fixes: - Refresh ckVtimeFloor TTL on enqueue/nack, not just dequeue: a dequeue-quiescent but enqueue-active base queue could expire the floor key while ckVtime survived, making a new variant register at 0 and jump the backlog (fairness inversion). - tostring() the vtime tag advance so a fractional quantum/weight is not truncated to an integer by Redis's Lua-number ZADD conversion (silent tag freeze). - Validate the env config: enable flag now uses BoolEnv (so =true/1/yes work, not only '1'); quantum/windowMultiplier/stateTtlSeconds are int().positive() (a 0 TTL errored SET ... EX 0 and stopped all CK dequeues; a 0 multiplier caused a full ZRANGE scan). Constructor clamps as defense-in-depth. - Tests: floor-not-lost regression (H1), large-N (>window) sharding no-starvation, tightened ckBalanced no-harm bound, corrected op-count budget (EXISTS = 7 fixed). - Softened a dangling plan-doc path in a comment. Old Lua command bodies remain byte-identical (edits are in the ...Vtime... commands + env/wiring/tests only).
1 parent b748a4f commit de774ca

6 files changed

Lines changed: 183 additions & 16 deletions

File tree

apps/webapp/app/env.server.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -943,10 +943,10 @@ const EnvironmentSchema = z
943943

944944
// Fair (virtual-time) ordering across concurrency-key variants of a base queue.
945945
// Off by default; when off the run queue behaves exactly as before.
946-
RUN_ENGINE_CK_VTIME_SCHEDULING_ENABLED: z.string().default("0"),
947-
RUN_ENGINE_CK_VTIME_QUANTUM: z.coerce.number().default(1),
948-
RUN_ENGINE_CK_VTIME_WINDOW_MULTIPLIER: z.coerce.number().default(3),
949-
RUN_ENGINE_CK_VTIME_STATE_TTL_SECONDS: z.coerce.number().default(86400),
946+
RUN_ENGINE_CK_VTIME_SCHEDULING_ENABLED: BoolEnv.default(false),
947+
RUN_ENGINE_CK_VTIME_QUANTUM: z.coerce.number().int().positive().default(1),
948+
RUN_ENGINE_CK_VTIME_WINDOW_MULTIPLIER: z.coerce.number().int().positive().default(3),
949+
RUN_ENGINE_CK_VTIME_STATE_TTL_SECONDS: z.coerce.number().int().positive().default(86400),
950950

951951
/** Optional maximum TTL for all runs (e.g. "14d"). If set, runs without an explicit TTL
952952
* will use this as their TTL, and runs with a TTL larger than this will be clamped. */

apps/webapp/app/v3/runEngine.server.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ function createRunEngine() {
107107
batchMaxWaitMs: env.RUN_ENGINE_TTL_WORKER_BATCH_MAX_WAIT_MS,
108108
},
109109
ckVirtualTimeScheduling:
110-
env.RUN_ENGINE_CK_VTIME_SCHEDULING_ENABLED === "1"
110+
env.RUN_ENGINE_CK_VTIME_SCHEDULING_ENABLED
111111
? {
112112
enabled: true,
113113
quantum: env.RUN_ENGINE_CK_VTIME_QUANTUM,

internal-packages/run-engine/src/run-queue/index.ts

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,8 @@ export type RunQueueOptions = {
121121
/**
122122
* Fair (virtual-time / SFQ) ordering across concurrency-key variants of a
123123
* base queue. Off by default; when off, the exact pre-existing Lua commands
124-
* run and no vtime keys are created. See docs/superpowers/plans/
125-
* 2026-07-23-ck-virtual-time-scheduling-plan.md.
124+
* run and no vtime keys are created. See the CK virtual-time scheduling
125+
* design for the ordering model.
126126
*/
127127
ckVirtualTimeScheduling?: {
128128
enabled: boolean;
@@ -226,9 +226,19 @@ export class RunQueue {
226226
this.shardCount = options.shardCount ?? 2;
227227
this.counterTtlSeconds = options.counterTtlSeconds ?? 86400;
228228
this.#ckVtimeEnabled = options.ckVirtualTimeScheduling?.enabled ?? false;
229-
this.#ckVtimeQuantum = options.ckVirtualTimeScheduling?.quantum ?? 1;
230-
this.#ckVtimeWindowMultiplier = options.ckVirtualTimeScheduling?.scanWindowMultiplier ?? 3;
231-
this.#ckVtimeStateTtl = options.ckVirtualTimeScheduling?.stateTtlSeconds ?? 86400;
229+
// Defense-in-depth: clamp so a directly-constructed RunQueue can't get bad
230+
// values that would freeze tags (quantum <= 0) or force an O(N) scan /
231+
// EX 0 error (multiplier / ttl <= 0).
232+
const resolvedQuantum = options.ckVirtualTimeScheduling?.quantum ?? 1;
233+
this.#ckVtimeQuantum = resolvedQuantum > 0 ? resolvedQuantum : 1;
234+
this.#ckVtimeWindowMultiplier = Math.max(
235+
1,
236+
Math.floor(options.ckVirtualTimeScheduling?.scanWindowMultiplier ?? 3)
237+
);
238+
this.#ckVtimeStateTtl = Math.max(
239+
1,
240+
Math.floor(options.ckVirtualTimeScheduling?.stateTtlSeconds ?? 86400)
241+
);
232242
this.retryOptions = options.retryOptions ?? defaultRetrySettings;
233243
this.redis = createRedisClient(options.redis, {
234244
onError: (error) => {
@@ -3918,6 +3928,7 @@ end
39183928
local vfloor = redis.call('GET', ckVtimeFloorKey) or '0'
39193929
redis.call('ZADD', ckVtimeKey, 'NX', vfloor, queueName)
39203930
redis.call('EXPIRE', ckVtimeKey, stateTtl)
3931+
redis.call('EXPIRE', ckVtimeFloorKey, stateTtl)
39213932
39223933
-- Rebalance master queue with ck:* member
39233934
local earliestIdx = redis.call('ZRANGE', ckIndexKey, 0, 0, 'WITHSCORES')
@@ -4045,6 +4056,7 @@ end
40454056
local vfloor = redis.call('GET', ckVtimeFloorKey) or '0'
40464057
redis.call('ZADD', ckVtimeKey, 'NX', vfloor, queueName)
40474058
redis.call('EXPIRE', ckVtimeKey, stateTtl)
4059+
redis.call('EXPIRE', ckVtimeFloorKey, stateTtl)
40484060
40494061
-- Rebalance master queue with ck:* member
40504062
local earliestIdx = redis.call('ZRANGE', ckIndexKey, 0, 0, 'WITHSCORES')
@@ -4826,7 +4838,7 @@ local function tryServe(ckQueueName)
48264838
local weight = 1
48274839
local tag = tonumber(redis.call('ZSCORE', ckVtimeKey, ckQueueName) or floor)
48284840
if tag < floor then tag = floor end
4829-
redis.call('ZADD', ckVtimeKey, tag + (quantum / weight), ckQueueName)
4841+
redis.call('ZADD', ckVtimeKey, tostring(tag + (quantum / weight)), ckQueueName)
48304842
end
48314843
else
48324844
redis.call('ZREM', fullQueueKey, messageId)
@@ -5577,6 +5589,7 @@ end
55775589
local vfloor = redis.call('GET', ckVtimeFloorKey) or '0'
55785590
redis.call('ZADD', ckVtimeKey, 'NX', vfloor, messageQueueName)
55795591
redis.call('EXPIRE', ckVtimeKey, stateTtl)
5592+
redis.call('EXPIRE', ckVtimeFloorKey, stateTtl)
55805593
55815594
-- Rebalance master queue with ck:* member
55825595
local earliestIdx = redis.call('ZRANGE', ckIndexKey, 0, 0, 'WITHSCORES')

internal-packages/run-engine/src/run-queue/tests/ckVtime.test.ts

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,99 @@ describe("CK virtual-time (SFQ) dequeue", () => {
397397
}
398398
);
399399

400+
// H1 regression: the floor key must not be allowed to expire while ckVtime
401+
// survives. Before the fix, only the dequeue command refreshed the floor
402+
// key's TTL, so a dequeue-quiescent + enqueue-active base queue let the floor
403+
// key expire underneath a live ckVtime; a brand-new variant then read a
404+
// missing floor as 0 and jumped ahead of the whole established backlog. The
405+
// enqueue/nack registration paths now refresh the floor key TTL too.
406+
redisTest(
407+
"enqueue refreshes the floor key TTL and a new variant registers at the current floor",
408+
async ({ redisContainer }) => {
409+
const stateTtlSeconds = 3600;
410+
const queue = createQueue(redisContainer, { stateTtlSeconds });
411+
try {
412+
const t0 = Date.now() - 100_000;
413+
const cks = ["a", "b"];
414+
415+
for (const ck of cks) {
416+
for (let i = 0; i < 25; i++) {
417+
await queue.enqueueMessage({
418+
env: authenticatedEnvDev,
419+
message: makeMessage({
420+
runId: `r-${ck}-${i}`,
421+
concurrencyKey: ck,
422+
timestamp: t0 + i,
423+
}),
424+
workerQueue: authenticatedEnvDev.id,
425+
skipDequeueProcessing: true,
426+
});
427+
}
428+
}
429+
430+
const ckVtimeKey = testOptions.keys.ckVtimeKeyFromQueue(variantName("a"));
431+
const ckVtimeFloorKey = testOptions.keys.ckVtimeFloorKeyFromQueue(variantName("a"));
432+
const shard = testOptions.keys.masterQueueShardForEnvironment(authenticatedEnvDev.id, 2);
433+
434+
// Drive tags and the floor above 0 with a run of serves.
435+
for (let call = 0; call < 20; call++) {
436+
const messages = await queue.testDequeueFromMasterQueue(shard, authenticatedEnvDev.id, 2);
437+
for (const m of messages) {
438+
await queue.acknowledgeMessage(authenticatedEnvDev.organization.id, m.messageId, {
439+
skipDequeueProcessing: true,
440+
});
441+
}
442+
}
443+
444+
const floor = Number((await queue.redis.get(ckVtimeFloorKey)) ?? "0");
445+
expect(floor).toBeGreaterThan(10);
446+
expect(await queue.redis.exists(ckVtimeKey)).toBe(1);
447+
448+
// Simulate the floor key's TTL decaying toward expiry while dequeues are
449+
// quiescent. Without the fix, only a dequeue would ever bump it back.
450+
await queue.redis.pexpire(ckVtimeFloorKey, 2_000);
451+
452+
// WITHOUT dequeuing, enqueue several more messages on an existing
453+
// variant. The enqueue registration path must refresh the floor key TTL.
454+
for (let i = 0; i < 5; i++) {
455+
await queue.enqueueMessage({
456+
env: authenticatedEnvDev,
457+
message: makeMessage({
458+
runId: `r-a-more-${i}`,
459+
concurrencyKey: "a",
460+
timestamp: t0 + 500 + i,
461+
}),
462+
workerQueue: authenticatedEnvDev.id,
463+
skipDequeueProcessing: true,
464+
});
465+
}
466+
467+
// The floor key TTL was pushed back up to (about) stateTtl, well above
468+
// the 2s decay we forced.
469+
const floorPttl = await queue.redis.pttl(ckVtimeFloorKey);
470+
expect(floorPttl).toBeGreaterThan(2_000);
471+
expect(floorPttl).toBeLessThanOrEqual(stateTtlSeconds * 1000);
472+
473+
// Enqueue-only activity does not move the floor value itself.
474+
const floorAfter = Number((await queue.redis.get(ckVtimeFloorKey)) ?? "0");
475+
expect(floorAfter).toBe(floor);
476+
477+
// A brand-new variant enqueued now registers at the CURRENT floor, so it
478+
// cannot leapfrog the established backlog back to 0.
479+
await queue.enqueueMessage({
480+
env: authenticatedEnvDev,
481+
message: makeMessage({ runId: "r-fresh", concurrencyKey: "fresh", timestamp: t0 }),
482+
workerQueue: authenticatedEnvDev.id,
483+
skipDequeueProcessing: true,
484+
});
485+
const freshTag = Number(await queue.redis.zscore(ckVtimeKey, variantName("fresh")));
486+
expect(freshTag).toBe(floor);
487+
} finally {
488+
await queue.quit();
489+
}
490+
}
491+
);
492+
400493
redisTest("no service, no advance", async ({ redisContainer }) => {
401494
const queue = createQueue(redisContainer);
402495
try {

internal-packages/run-engine/src/run-queue/tests/ckVtimeConcurrency.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -346,10 +346,10 @@ describe("CK virtual-time concurrency and op-count budget", () => {
346346
expect(off.served).toBe(cks.length * perKey);
347347
expect(on.served).toBe(cks.length * perKey);
348348

349-
// Per dequeue call the vtime path adds at worst: GET floor, ZRANGE min,
350-
// ZRANGE window, SET floor, EXPIRE, the pass-2 ZRANGEBYSCORE, plus per
351-
// serve one ZSCORE and one ZADD.
352-
const budget = dequeueCalls * (6 + 2 * maxCount);
349+
// Per dequeue call the vtime path adds at worst 7 fixed ops: GET floor,
350+
// ZRANGE min, ZRANGE window, the pass-2 ZRANGEBYSCORE, SET floor,
351+
// EXISTS ckVtime, EXPIRE ckVtime — plus per serve one ZSCORE and one ZADD.
352+
const budget = dequeueCalls * (7 + 2 * maxCount);
353353
expect(
354354
on.totalCalls,
355355
`on_total ${on.totalCalls} exceeds off_total ${off.totalCalls} + budget ${budget}`

internal-packages/run-engine/src/run-queue/tests/ckVtimeFairness.test.ts

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,68 @@ describe("CK virtual-time fairness on the real batched dequeue path", () => {
432432
const offMax = maxPerKeyMeanWait(off);
433433
debugLog("ckBalanced", { onMax, offMax, ratio: onMax / offMax });
434434

435-
expect(onMax).toBeLessThanOrEqual(1.25 * offMax);
435+
// Observed ratio is 1.0 (the fair order is neutral on the symmetric case),
436+
// so allow only modest headroom rather than the original 1.25.
437+
expect(onMax).toBeLessThanOrEqual(1.1 * offMax);
438+
}
439+
);
440+
441+
// ckManyKeys (sharding coverage): cardinality ABOVE the pass-1 fair window.
442+
// The batched dequeue uses maxCount 10, so window = actualMaxCount * 3 = 30.
443+
// With ~60 attacker keys (all on the same old head) plus 1 light key on a
444+
// newer head, 61 variants sit above the 30-wide pass-1 ZRANGE window, so no
445+
// single fair pass can even see every key. The property to hold is that this
446+
// does NOT permanently starve the light key: as attackers advance their tags
447+
// out of the bottom of the window, the light key (still at the floor) rises
448+
// into it and gets served, and every message drains exactly once. A bounded
449+
// first-serve delay is fine; permanent starvation or a stuck drain is not.
450+
redisTest(
451+
"ckManyKeys: light key is not starved when cardinality exceeds the fair window",
452+
async ({ redisContainer }) => {
453+
const t0 = Date.now() - 500_000;
454+
const messages: ScenarioMessage[] = [];
455+
const attackerCount = 60;
456+
for (let i = 0; i < 8; i++) {
457+
for (let k = 0; k < attackerCount; k++) {
458+
const ck = `att${String(k).padStart(2, "0")}`;
459+
// All attackers share the same old head timestamp (tied heads).
460+
messages.push({ runId: `${ck}-${i}`, ck, timestamp: t0 });
461+
}
462+
}
463+
for (let i = 0; i < 10; i++) {
464+
messages.push({ runId: `light-${i}`, ck: "light", timestamp: t0 + 50_000 + i });
465+
}
466+
const scenario: Scenario = {
467+
name: "ckManyKeys",
468+
messages,
469+
envConcurrencyLimit: 25,
470+
holdSteps: 3,
471+
maxSteps: 1_000,
472+
};
473+
474+
const on = await runScenario(redisContainer, scenario, true);
475+
const off = await runScenario(redisContainer, scenario, false);
476+
477+
// No loss and no double-serve in either run: the run terminates and every
478+
// message (attackers + light) is served exactly once within maxSteps.
479+
assertConservation(scenario, on, off);
480+
481+
const isLight = (ck: string) => ck === "light";
482+
483+
// The light key IS eventually served (no permanent starvation) in both
484+
// runs, and drains fully.
485+
const onFirstServe = firstServeStep(on, isLight);
486+
const offFirstServe = firstServeStep(off, isLight);
487+
expect(on.drainStep).toBeGreaterThanOrEqual(0);
488+
expect(off.drainStep).toBeGreaterThanOrEqual(0);
489+
490+
debugLog("ckManyKeys", {
491+
variants: attackerCount + 1,
492+
onFirstServe,
493+
offFirstServe,
494+
onDrainStep: on.drainStep,
495+
offDrainStep: off.drainStep,
496+
});
436497
}
437498
);
438499

0 commit comments

Comments
 (0)