Skip to content

Commit 8ef9fb1

Browse files
committed
fix(run-engine): gates inherit the run key and never deadlock on their own slot
A gate without a concurrencyKey now resolves to the run's own key inside the scripts, matching the documented semantics for every caller and for payloads written before a fix at the contract layer. Admission also passes a gate or total cap whose set already contains the message: an unmirrored release from an older build leaves the run's own membership behind, and blocking on it would deadlock the run against itself; since acquiring is an idempotent set add, an existing member simply keeps the slot it holds. Gate names and keys in the payload schema are bounded to 128 characters.
1 parent d2fcdb9 commit 8ef9fb1

3 files changed

Lines changed: 114 additions & 28 deletions

File tree

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

Lines changed: 43 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,15 @@ const SemanticAttributes = {
7474
const QUEUE_GATES_LUA_HELPERS = `
7575
local function __gateKeys(gatesKeyPrefix, msg, gate)
7676
local base = gatesKeyPrefix .. '{org:' .. msg.orgId .. '}:proj:' .. msg.projectId .. ':env:' .. msg.environmentId .. ':queue:' .. gate.queue
77+
local gateKey = gate.concurrencyKey
78+
if (not gateKey or gateKey == '') and msg.concurrencyKey and msg.concurrencyKey ~= '' then
79+
gateKey = msg.concurrencyKey
80+
end
7781
local variant = base
78-
if gate.concurrencyKey and gate.concurrencyKey ~= '' then
79-
variant = base .. ':ck:' .. gate.concurrencyKey
82+
if gateKey and gateKey ~= '' then
83+
variant = base .. ':ck:' .. gateKey
8084
end
81-
return base, variant
85+
return base, variant, gateKey
8286
end
8387
8488
local function __gateReconcile(setKey, msgKeyPrefix)
@@ -96,22 +100,23 @@ local function __gateReconcile(setKey, msgKeyPrefix)
96100
end
97101
end
98102
99-
local function __gatesHaveCapacity(gatesKeyPrefix, msg, envLimit, msgKeyPrefix)
103+
local function __gatesHaveCapacity(gatesKeyPrefix, msg, messageId, envLimit, msgKeyPrefix)
100104
if not msg.gates then return true end
101105
for _, gate in ipairs(msg.gates) do
102-
local base, variant = __gateKeys(gatesKeyPrefix, msg, gate)
106+
local base, variant, gateKey = __gateKeys(gatesKeyPrefix, msg, gate)
103107
local occupancy = tonumber(redis.call('SCARD', variant .. ':currentConcurrency') or '0')
104108
local perKeyLimit = math.min(tonumber(redis.call('GET', base .. ':concurrency') or '1000000'), envLimit)
105-
if occupancy >= perKeyLimit then
109+
if occupancy >= perKeyLimit and redis.call('SISMEMBER', variant .. ':currentConcurrency', messageId) == 0 then
106110
__gateReconcile(variant .. ':currentConcurrency', msgKeyPrefix)
107111
return false
108112
end
109-
if gate.concurrencyKey and gate.concurrencyKey ~= '' then
113+
if gateKey and gateKey ~= '' then
110114
local rawTotal = redis.call('GET', base .. ':totalConcurrency')
111115
if rawTotal then
112116
local totalLimit = math.min(tonumber(rawTotal), envLimit)
113-
if tonumber(redis.call('SCARD', base .. ':groupConcurrency') or '0') >= totalLimit then
114-
__gateReconcile(base .. ':groupConcurrency', msgKeyPrefix)
117+
local groupKey = base .. ':groupConcurrency'
118+
if tonumber(redis.call('SCARD', groupKey) or '0') >= totalLimit and redis.call('SISMEMBER', groupKey, messageId) == 0 then
119+
__gateReconcile(groupKey, msgKeyPrefix)
115120
return false
116121
end
117122
end
@@ -123,9 +128,9 @@ end
123128
local function __gatesAcquire(gatesKeyPrefix, msg, messageId)
124129
if not msg.gates then return end
125130
for _, gate in ipairs(msg.gates) do
126-
local base, variant = __gateKeys(gatesKeyPrefix, msg, gate)
131+
local base, variant, gateKey = __gateKeys(gatesKeyPrefix, msg, gate)
127132
redis.call('SADD', variant .. ':currentConcurrency', messageId)
128-
if gate.concurrencyKey and gate.concurrencyKey ~= '' then
133+
if gateKey and gateKey ~= '' then
129134
redis.call('SADD', base .. ':groupConcurrency', messageId)
130135
end
131136
end
@@ -137,9 +142,9 @@ local function __gatesRelease(gatesKeyPrefix, rawPayload, messageId)
137142
local ok, msg = pcall(cjson.decode, rawPayload)
138143
if not ok or type(msg) ~= 'table' or not msg.gates then return end
139144
for _, gate in ipairs(msg.gates) do
140-
local base, variant = __gateKeys(gatesKeyPrefix, msg, gate)
145+
local base, variant, gateKey = __gateKeys(gatesKeyPrefix, msg, gate)
141146
local removed = redis.call('SREM', variant .. ':currentConcurrency', messageId)
142-
if removed == 1 and gate.concurrencyKey and gate.concurrencyKey ~= '' then
147+
if removed == 1 and gateKey and gateKey ~= '' then
143148
redis.call('SREM', base .. ':groupConcurrency', messageId)
144149
end
145150
end
@@ -3605,7 +3610,7 @@ if enableFastPath == '1' then
36053610
local okDecode, decoded = pcall(cjson.decode, messageData)
36063611
if okDecode and type(decoded) == 'table' and decoded.gates then
36073612
gateMsg = decoded
3608-
gatesAllowFastPath = __gatesHaveCapacity(keyPrefix, decoded, envLimit, nil)
3613+
gatesAllowFastPath = __gatesHaveCapacity(keyPrefix, decoded, messageId, envLimit, nil)
36093614
end
36103615
end
36113616
@@ -3719,7 +3724,7 @@ if enableFastPath == '1' then
37193724
local okDecode, decoded = pcall(cjson.decode, messageData)
37203725
if okDecode and type(decoded) == 'table' and decoded.gates then
37213726
gateMsg = decoded
3722-
gatesAllowFastPath = __gatesHaveCapacity(keyPrefix, decoded, envLimit, nil)
3727+
gatesAllowFastPath = __gatesHaveCapacity(keyPrefix, decoded, messageId, envLimit, nil)
37233728
end
37243729
end
37253730
@@ -4079,7 +4084,7 @@ if enableFastPath == '1' then
40794084
local okDecode, decoded = pcall(cjson.decode, messageData)
40804085
if okDecode and type(decoded) == 'table' and decoded.gates then
40814086
gateMsg = decoded
4082-
gatesAllowFastPath = __gatesHaveCapacity(keyPrefix, decoded, envLimit, nil)
4087+
gatesAllowFastPath = __gatesHaveCapacity(keyPrefix, decoded, messageId, envLimit, nil)
40834088
end
40844089
end
40854090
@@ -4250,7 +4255,7 @@ if enableFastPath == '1' then
42504255
local okDecode, decoded = pcall(cjson.decode, messageData)
42514256
if okDecode and type(decoded) == 'table' and decoded.gates then
42524257
gateMsg = decoded
4253-
gatesAllowFastPath = __gatesHaveCapacity(keyPrefix, decoded, envLimit, nil)
4258+
gatesAllowFastPath = __gatesHaveCapacity(keyPrefix, decoded, messageId, envLimit, nil)
42544259
end
42554260
end
42564261
@@ -4652,7 +4657,7 @@ for i = 1, #messages, 2 do
46524657
else
46534658
local gatesAllow = true
46544659
if gatesEnabled then
4655-
gatesAllow = __gatesHaveCapacity(keyPrefix, messageData, envConcurrencyLimit, messageKeyPrefix)
4660+
gatesAllow = __gatesHaveCapacity(keyPrefix, messageData, messageId, envConcurrencyLimit, messageKeyPrefix)
46564661
end
46574662
46584663
if gatesAllow then
@@ -4903,11 +4908,13 @@ local queueConcurrencyLimit = math.min(tonumber(redis.call('GET', queueConcurren
49034908
local envAvailableCapacity = envConcurrencyLimitWithBurstFactor - envCurrentConcurrency
49044909
local actualMaxCount = math.min(maxCount, envAvailableCapacity)
49054910
4906-
-- Total-cap gate: bound this batch by the remaining headroom across ALL ck
4907-
-- variants (groupConcurrency SCARD vs the env-clamped total limit). Each admit
4908-
-- below SADDs into the group set and bumps dequeuedCount, and dequeuedCount is
4909-
-- bounded by actualMaxCount, so tightening here is sufficient to prevent
4910-
-- over-admitting past the cap within a single batch.
4911+
-- Total-cap gate: track the remaining headroom across ALL ck variants
4912+
-- (groupConcurrency SCARD vs the env-clamped total limit). Admission below
4913+
-- decrements the headroom per new member, and a message that is ALREADY a
4914+
-- group member passes even at zero headroom: it holds its own slot (an
4915+
-- unmirrored release from an older build can leave a queued run's membership
4916+
-- behind, and blocking on it would deadlock the run against itself).
4917+
local totalHeadroom = nil
49114918
if totalConcurrencyEnabled then
49124919
local rawTotalLimit = redis.call('GET', totalConcurrencyLimitKey)
49134920
if rawTotalLimit then
@@ -4939,7 +4946,7 @@ if totalConcurrencyEnabled then
49394946
end
49404947
end
49414948
4942-
actualMaxCount = math.min(actualMaxCount, totalConcurrencyLimit - groupCurrentConcurrency)
4949+
totalHeadroom = totalConcurrencyLimit - groupCurrentConcurrency
49434950
end
49444951
end
49454952
@@ -4997,17 +5004,27 @@ for _, ckQueueName in ipairs(ckQueues) do
49975004
else
49985005
local gatesAllow = true
49995006
if gatesEnabled then
5000-
gatesAllow = __gatesHaveCapacity(keyPrefix, messageData, envConcurrencyLimit, messageKeyPrefix)
5007+
gatesAllow = __gatesHaveCapacity(keyPrefix, messageData, messageId, envConcurrencyLimit, messageKeyPrefix)
5008+
end
5009+
5010+
local alreadyInGroup = false
5011+
local totalAllows = true
5012+
if totalHeadroom ~= nil then
5013+
alreadyInGroup = redis.call('SISMEMBER', groupConcurrencyKey, messageId) == 1
5014+
totalAllows = alreadyInGroup or totalHeadroom > 0
50015015
end
50025016
5003-
if gatesAllow then
5017+
if gatesAllow and totalAllows then
50045018
redis.call('ZREM', fullQueueKey, messageId)
50055019
redis.call('ZREM', envQueueKey, messageId)
50065020
decrLengthCounter()
50075021
redis.call('SADD', ckConcurrencyKey, messageId)
50085022
redis.call('SADD', envCurrentConcurrencyKey, messageId)
50095023
if totalConcurrencyEnabled then
50105024
redis.call('SADD', groupConcurrencyKey, messageId)
5025+
if totalHeadroom ~= nil and not alreadyInGroup then
5026+
totalHeadroom = totalHeadroom - 1
5027+
end
50115028
end
50125029
if gatesEnabled then
50135030
__gatesAcquire(keyPrefix, messageData, messageId)

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,75 @@ describe("RunQueue gates", () => {
282282
}
283283
});
284284

285+
redisTest(
286+
"a run already holding its own gate slot is never deadlocked by it",
287+
async ({ redisContainer }) => {
288+
const queue = createQueue(redisContainer, true);
289+
try {
290+
const keys = testOptions.keys;
291+
await queue.updateQueueConcurrencyLimits(authenticatedEnvDev, "task/my-task", 5);
292+
await queue.updateQueueConcurrencyLimits(authenticatedEnvDev, "shared-gate", 1);
293+
294+
/**
295+
* An unmirrored release (an older build's nack) leaves the run's own
296+
* membership behind while the run goes back to waiting in its queue. The
297+
* gate is "full" with the run itself; admission must still let it through.
298+
*/
299+
await queue.redis.sadd(
300+
keys.queueCurrentConcurrencyKey(authenticatedEnvDev, "shared-gate"),
301+
"r0"
302+
);
303+
304+
await queue.enqueueMessage({
305+
env: authenticatedEnvDev,
306+
message: makeMessage({
307+
runId: "r0",
308+
timestamp: Date.now() - 1000,
309+
gates: [{ queue: "shared-gate" }],
310+
}),
311+
workerQueue: "main",
312+
});
313+
314+
const r0Admitted = await waitFor(() => popWorkerQueue(queue, "r0"), 30_000);
315+
expect(r0Admitted).toBe(true);
316+
expect(await queue.currentConcurrencyOfQueue(authenticatedEnvDev, "shared-gate")).toBe(1);
317+
} finally {
318+
await queue.quit();
319+
}
320+
}
321+
);
322+
323+
redisTest(
324+
"a gate without a key inherits the run's concurrency key",
325+
async ({ redisContainer }) => {
326+
const queue = createQueue(redisContainer, true);
327+
try {
328+
await queue.updateQueueConcurrencyLimits(authenticatedEnvDev, "task/my-task", 5);
329+
await queue.updateQueueConcurrencyLimits(authenticatedEnvDev, "tenant", 1);
330+
331+
await queue.enqueueMessage({
332+
env: authenticatedEnvDev,
333+
message: makeMessage({
334+
runId: "r0",
335+
concurrencyKey: "acme",
336+
timestamp: Date.now() - 1000,
337+
gates: [{ queue: "tenant" }],
338+
}),
339+
workerQueue: "main",
340+
});
341+
342+
const admitted = await waitFor(
343+
async () =>
344+
(await queue.currentConcurrencyOfQueue(authenticatedEnvDev, "tenant", "acme")) === 1
345+
);
346+
expect(admitted).toBe(true);
347+
expect(await queue.currentConcurrencyOfQueue(authenticatedEnvDev, "tenant")).toBe(0);
348+
} finally {
349+
await queue.quit();
350+
}
351+
}
352+
);
353+
285354
redisTest("enqueue fast path respects a full gate", async ({ redisContainer }) => {
286355
const queue = createQueue(redisContainer, true);
287356
try {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ import type { MinimalAuthenticatedEnvironment } from "../shared/index.js";
1010
* queue name; the org/project/env scope comes from the run's own payload.
1111
*/
1212
const QueueGate = z.object({
13-
queue: z.string(),
14-
concurrencyKey: z.string().optional(),
13+
queue: z.string().min(1).max(128),
14+
concurrencyKey: z.string().min(1).max(128).optional(),
1515
});
1616
type QueueGate = z.infer<typeof QueueGate>;
1717

0 commit comments

Comments
 (0)