fix(web): make Video Pack Redis claims atomic - #1639
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
javascript
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
…`cjson.encode` after `cjson.decode`, corrupting empty JSON arrays (`[]`) into empty objects (`{}`) before returning them to the caller.
This commit fixes the issue reported at apps/web/src/lib/video-pack-store.ts:72
## Bug
In `apps/web/src/lib/video-pack-store.ts`, the Lua `CLAIM_PROCESSING_SCRIPT` decodes the stored value with `cjson.decode(raw)` and, for the two `'existing'` branches, re-serializes it with `cjson.encode(current)` before returning.
Redis's bundled `cjson` cannot distinguish an empty array from an empty object once decoded: `cjson.decode('[]')` yields an empty Lua table, and `cjson.encode({})` emits `{}` (object), **not** `[]`. So every empty-array field in a ready pack is silently converted to an empty object on the way back to the JS caller.
> Note: commit `1511796` added a `source_hash` guard to both branches but left the lossy `cjson.encode(current)` return intact, so the corruption persists.
### Trigger (concrete cross-isolate race)
1. Isolate A handles a POST; `getPackRecord` returns null/stale/identity-only, so A calls `claimPackProcessing`.
2. Between A's read and its `EVAL`, isolate B persists a ready pack (or a ready-but-identity-only pack already exists) with the same `source_hash`.
3. The script hits the `current['state'] == 'ready' ... source_hash == source_hash` block and returns `cjson.encode(current)`.
Ready packs routinely contain empty arrays — `buildIdentityPack` sets `keyframes: []`, `concepts: []`, `requirements: []`, `code_snippets: []`, `artifacts: []`, `stack.tools: []`, `transcript.segments: []`; `emptyPackFormation` returns `artifacts: []`, `stack.tools: []`. All of these become `{}`.
`claimPackProcessing` returns the corrupted record, `handleIdentityPackPost` serves it verbatim, and it is also written into `memoryStore`, so subsequent reads in that isolate serve the broken shape too. API consumers doing `.map()` / `.length` on `requirements` / `code_snippets` / `artifacts` / `stack.tools` break.
The existing test "atomically preserves an existing ready pack" does not catch this because the mock `eval` uses `JSON.stringify(current)` on the JS-decoded object, which preserves real arrays and does not replicate Redis cjson's `[] -> {}` behavior.
## Fix
Return the original `raw` value for both `'existing'` branches instead of re-encoding via `cjson.encode(current)`:
```lua
if current['state'] == 'ready'
and type(current['pack']) == 'table'
and type(current['pack']['provenance']) == 'table'
and current['pack']['provenance']['source_hash'] == source_hash then
return { 'existing', raw }
end
...
if valid_iso and started_at > stale_before then
return { 'existing', raw }
end
```
`raw` is exactly what was stored, so arrays are preserved bit-for-bit. The JS side's `decodeRecord` already handles single- and double-encoded strings (up to two `JSON.parse` passes), so returning the raw stored value requires no further changes and avoids the lossy decode→encode round-trip entirely.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: groupthinking <garveyht@gmail.com>
… memoized in `redisPromise`) permanently poisons the Redis client and makes the read path `getPackRecord` throw instead of falling back to the in-memory store.
This commit fixes the issue reported at apps/web/src/lib/video-pack-store.ts:120
## Bug
`getRedis()` in `apps/web/src/lib/video-pack-store.ts` was changed to `throw error` on client-init failure instead of returning `null`. Two problems compound:
1. **Permanent poisoning.** The result of the IIFE is stored in the module-level `redisPromise`, and the function short-circuits with `if (redisPromise) return redisPromise`. When the async IIFE rejects, that *rejected* promise is cached. Every subsequent call returns the same rejected promise, so a single transient failure (e.g. dynamic `import('@upstash/redis')` hiccup or a `new Redis(...)` throw) is never retried for the lifetime of the process.
2. **Read path throws instead of degrading.** In `getPackRecord`, the call is outside the `try/catch`:
```ts
const redis = await getRedis(); // NOT wrapped
if (redis) {
try { /* redis.get */ } catch { /* logged */ }
}
return memoryStore.get(key) ?? null;
```
Only `redis.get` is wrapped. A rejected `getRedis()` therefore propagates straight out of `getPackRecord`, a read path that previously always fell back to `memoryStore`. `putPackRecord` and `claimPackProcessing` are similarly affected (their `await getRedis()` is also outside the try/catch).
### Trigger
`resolveUpstashRedisCredentials()` returns non-null creds, but `await import('@upstash/redis')` or `new Redis(...)` throws. From that point on, all three exported functions throw on every invocation rather than serving from the in-memory store.
## Fix
Restore the established pattern used by every sibling store (`entitlement-store.ts`, `chat-quota.ts`, `kaizen-trace.ts`, etc.): return `null` on init failure so callers gracefully degrade to `memoryStore`. Additionally, clear the memoized `redisPromise` on failure (guarded by `redisPromise === promise` to avoid clobbering a newer attempt) so a transient init failure can be retried on the next call instead of poisoning the client permanently.
```ts
async function getRedis(): Promise<VideoPackRedisClient | null> {
if (redisPromise) return redisPromise;
const promise = (async () => {
const creds = resolveUpstashRedisCredentials();
if (!creds) return null;
try {
const { Redis } = await import('@upstash/redis');
return new Redis({ url: creds.url, token: creds.token }) as unknown as VideoPackRedisClient;
} catch (error) {
console.error('[video-pack-store] Redis client init failed:', error);
if (redisPromise === promise) redisPromise = null; // allow retry, fall back to memory
return null;
}
})();
redisPromise = promise;
return promise;
}
```
The ordering is safe: the IIFE suspends at the first `await`, so `redisPromise = promise` executes before the `catch` can run, guaranteeing `redisPromise === promise` when a real init failure resolves.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: groupthinking <garveyht@gmail.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Record validation gaps and lack of Redis-compatible Lua integration coverage remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces atomic Redis Lua claims for Video Pack processing and improves persistence failure handling.
Changes:
- Replaces multi-command claims with Lua
EVAL. - Handles stale, malformed, and double-encoded records.
- Adds Redis claim and persistence regression tests.
File summaries
| File | Review |
|---|---|
apps/web/src/lib/video-pack-store.ts |
Atomic claims added, but record validation does not enforce matching hashes or complete pack shape. |
apps/web/src/lib/__tests__/video-pack-store.test.ts |
Adds regression coverage, but the fake reimplements rather than executes the production Lua script. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+68
to
+72
| async eval<TResult>(_script, _keys, args) { | ||
| evalCalls += 1; | ||
| const processing = JSON.parse(String(args[0])) as VideoPackRecord; | ||
| const staleBefore = String(args[1]); | ||
| const sourceHash = String(args[2]); |
Owner
Author
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
Contributor
There was a problem hiding this comment.
Addressed in the latest commit: the Redis integration test now imports and executes the production CLAIM_PROCESSING_SCRIPT directly against a real Redis server, asserting both the Lua result and persisted processing claim.
Comment on lines
+71
to
+75
| if current['state'] == 'ready' | ||
| and type(current['pack']) == 'table' | ||
| and type(current['pack']['provenance']) == 'table' | ||
| and current['pack']['provenance']['source_hash'] == source_hash then | ||
| return { 'existing', cjson.encode(current) } |
Owner
Author
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
Contributor
There was a problem hiding this comment.
Implemented in 46cdc57: ready records now require the transcript shape used by response handling, malformed ready values are reclaimed atomically, and preflight reads reject records whose embedded source hash does not match the requested key. Added regression coverage plus a real Redis Lua integration test wired into CI.
…sed before being assigned" because a `const promise` is referenced inside the async IIFE that initializes it, breaking `next build`.
This commit fixes the issue reported at apps/web/src/lib/video-pack-store.ts:129
## Bug
In `getRedis()` (apps/web/src/lib/video-pack-store.ts), the memoized init promise was declared as:
```ts
const promise = (async () => {
// ...
if (redisPromise === promise) { // references `promise` inside its own initializer
redisPromise = null;
}
// ...
})();
```
The async IIFE body references the `promise` binding, but `promise` is a `const` being assigned by that very expression. TypeScript's definite-assignment (flow) analysis is conservative here: because the closure could conceptually observe `promise` before the initializer completes, it reports:
```
./src/lib/video-pack-store.ts:129:28
Type error: Variable 'promise' is used before being assigned. (TS2454)
```
This is a compile-time failure, so `pnpm/next build` runs `tsc --noEmit` (the `type-check` script) and exits with code 1 — the Vercel build fails.
**Trigger:** any `next build` / `tsc --noEmit` run. Deterministic, not runtime-conditional.
## Fix
Declare the binding with `let` and an explicit type annotation, then assign:
```ts
let promise: Promise<VideoPackRedisClient | null>;
promise = (async () => { ... })();
```
TypeScript treats an explicitly type-annotated `let` variable as "assumed assigned" within closures, so the self-reference no longer triggers TS2454. Runtime behavior is unchanged: the `redisPromise === promise` guard (which clears the memoized promise only if it hasn't been superseded, avoiding poisoning the client on transient init failures) still works, and the types line up (`redisPromise` is `Promise<VideoPackRedisClient | null> | null`, `promise` is `Promise<VideoPackRedisClient | null>`), so both the `===` comparison and `redisPromise = promise` assignment type-check.
Note: I could not execute `tsc` locally because `node_modules` is not installed in this sandbox, but the fix follows the standard, well-established resolution for TS2454 in self-referential closure initializers.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: groupthinking <garveyht@gmail.com>
Owner
Author
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
| }) as unknown as VideoPackRedisClient; | ||
| } catch (error) { | ||
| console.error('[video-pack-store] Redis client init failed:', error); | ||
| // Clear the memoized promise so a transient init failure does not |
Contributor
There was a problem hiding this comment.
getRedis() reads the promise variable inside the async IIFE that initializes it (if (redisPromise === promise)), so TypeScript's definite-assignment analysis flags TS2454 "Variable 'promise' is used before being assigned", failing next build.
Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1631.
What changed
EVALclaim/recovery decision202with no scheduled durable workWhy
PR #1627 attempted fail-closed hardening but produced the production
POST 202 → GET 404hole recorded in #1629. The previous implementation restored by #1629 remained vulnerable to a read/overwrite race. Redis-side scripting keeps the dependent read and conditional write in one atomic operation.Atomicity/API basis: Upstash — Lua Scripting on Upstash Redis: Atomic Operations Over HTTP (June 23, 2026).
Verification
Repository CI is required. This environment did not have the repository dependency tree, so no local test result is claimed.
Safety boundary
Draft only. No merge, deployment, configuration change, billing action, or production mutation.