Skip to content

perf(api): cjson.decode in startTransitionScript Lua script blocks Redis event loop under concurrent evictions #3602

Description

@AdaAibaby

Problem

`startTransitionScript` in `packages/api/internal/sandbox/storage/redis/scripts.go` conditionally calls `cjson.decode` on the full sandbox JSON blob when `ARGV[5]` (the expected `executionID`) is non-empty:

```lua
if ARGV[5] ~= '' then
local current = redis.call('GET', KEYS[1])
local ok, decoded = pcall(cjson.decode, current) -- ← full JSON parse on main thread
if not ok or decoded['executionID'] ~= ARGV[5] then
return 0
end
end
redis.call('SET', KEYS[1], ARGV[1])
redis.call('SET', KEYS[2], ARGV[2], 'EX', ARGV[3])
redis.call('SET', KEYS[3], '', 'EX', ARGV[4])
return 1
```

Redis executes all Lua on its single-threaded event loop. While a script runs, every other client command — including unrelated `GET`, `SET`, `ZADD` — is blocked.

`cjson.decode` is O(N) in the size of the JSON string. A full sandbox record is typically several hundred bytes. Under concurrent evictions (many sandboxes expiring simultaneously, all API Nomad allocations calling `StartRemoving` with a non-empty `ExpectExecutionID`), each Lua invocation serializes JSON parsing on the main thread. Command latency compounds: with 100 concurrent calls each taking ~100 µs of CPU for the decode, that is 10 ms of pure event-loop stall during which no other command makes progress.

This is introduced by `c29ee26` (feat(api): let a removal pin the sandbox incarnation it meant to remove).

Reproduction scenario

  • High sandbox density (400+ sandboxes/node) with concurrent TTL expiration
  • Multiple API Nomad allocations, each running the 50 ms evictor loop
  • All allocations call `StartRemoving` with `RemoveOpts.ExpectExecutionID` set (evictor path)
  • Observed: Redis `latency` spikes, evictor falling behind its own SLA, commands timing out

Root cause

The `executionID` is embedded inside the JSON blob. Validating it atomically requires either JSON-decoding the blob inside Lua or storing the field separately so it can be fetched with a plain `GET`.

The `cjson.decode` path was chosen because `Add` is lockless, making it impossible to guard the check in Go: a resume can install a new incarnation between the Go-side comparison and the Lua write. This reasoning is correct — the fix must preserve that atomicity.

Proposed fix

Store `executionID` in a dedicated auxiliary Redis key alongside the main sandbox key:

```
sandbox key: {team}:sbx:{sandboxID} → full JSON blob (existing)
execution key: {team}:sbx:{sandboxID}:eid → executionID string only (new)
```

Both keys share the same hash tag `{team}`, so they land on the same cluster slot and can be read/written atomically inside a single Lua script.

Updated `startTransitionScript`:

```lua
-- KEYS[4] = execution ID key (new)
-- ARGV[5] = expected executionID, or "" to write unconditionally
-- ARGV[6] = new executionID to write (new)

if ARGV[5] ~= '' then
local eid = redis.call('GET', KEYS[4]) -- O(1) string fetch, no JSON parse
if not eid or eid ~= ARGV[5] then
return 0
end
end
redis.call('SET', KEYS[1], ARGV[1])
redis.call('SET', KEYS[4], ARGV[6]) -- keep eid key in sync
redis.call('SET', KEYS[2], ARGV[2], 'EX', ARGV[3])
redis.call('SET', KEYS[3], '', 'EX', ARGV[4])
return 1
```

`addSandboxScript` also writes the eid key atomically:

```lua
-- KEYS[3] = execution ID key (new)
-- ARGV[3] = executionID (new)
redis.call('SET', KEYS[1], ARGV[1])
redis.call('SET', KEYS[3], ARGV[3]) -- write eid alongside main key
redis.call('SADD', KEYS[2], ARGV[2])
return 1
```

`removeSandboxScript` deletes the eid key together with the sandbox key.

This replaces `cjson.decode` (O(N) CPU on main thread) with `GET` (O(1) hash-table lookup), while preserving the same atomicity guarantee.

Impact

  • No behaviour change: same correctness properties, same Cluster slot constraints
  • Reduces Lua CPU time on the hot path by ~10–100× depending on JSON payload size
  • Introduces one additional Redis key per sandbox (negligible memory overhead)
  • Requires a short migration window for existing deployments: sandboxes created before the deploy lack the eid key; the script should treat a missing eid key as a mismatch when `ARGV[5]` is set (safe: the removal is rejected until the sandbox is next written with the new code path)

Alternatives considered

  • Move the check to Go with a distributed lock — rejected: `Add` is lockless, so the check cannot be made atomic with the write from outside Lua
  • Use Redis Hash instead of String for sandbox data — would allow `HGET key executionID` inside Lua without full decode; viable but requires changing the storage layout everywhere and a data migration
  • Accept the status quo — acceptable at low sandbox counts; becomes a bottleneck at 400+ sandboxes/node with concurrent evictions

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions