Skip to content

feat: remix worker engine (t0.5) — RemixJob orchestration + claim/report/upload#19

Open
mtmtian wants to merge 7 commits into
oratis:mainfrom
mtmtian:feat/remix-worker
Open

feat: remix worker engine (t0.5) — RemixJob orchestration + claim/report/upload#19
mtmtian wants to merge 7 commits into
oratis:mainfrom
mtmtian:feat/remix-worker

Conversation

@mtmtian

@mtmtian mtmtian commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

What

Adds a second, worker-driven remix engine alongside the existing in-app single-shot text2video path (which is untouched). This phase ships tier t0.5 only: per-beat text-described regeneration — the storyboard from buildRemixBrief is generated beat-by-beat by an external worker (Cloud Run Job, separate repo), assembled with ffmpeg, brand-QC re-scanned (whisper + OCR), and lands as the same review-gated Creative(source:'remix', reviewStatus:'pending'). Competitor material stays analysis-only — no competitor pixels enter generation, consistent with the "borrow structure, not pixels" rules in remix-brief.ts / competitor-media.ts.

  • RemixJob model + migration: brief snapshot, per-beat progress, qcReport, costTokens; tier / segmentPlan fields reserved for later tiers (API rejects anything but t0_5 for now).
  • POST/GET /api/creatives/remix-jobs (session auth): reuses buildRemixBrief / competitorCreativeToAnalysis; shares the existing remix burst + daily-cap guardrails.
  • Worker endpoints under /api/worker/remix-jobs/* (HMAC via ingest-auth.verifyHmac, new WORKER_WEBHOOK_SECRET):
    • claim — atomic pickup (updateMany count check) with REMIX_JOB_LEASE_MINUTES stale-lease recycling
    • report — atomic status state machine (pending→claimed→running→assembling→qc→succeeded/failed, terminal states locked, 409 on illegal transitions); outputUrl must match the canonical upload path; QC-fail is surfaced to reviewers via Creative.reviewNotes (promotion not blocked — reviewer decides)
    • upload — raw mp4 → GCS via server-side uploadToGCS (content-sha256-bound HMAC, pre-buffer header checks, 100MB cap)

Design notes

  • Zero modifications to the existing remix/competitor files — only imports. storage.ts gains one additive export (gcsPublicPrefix).
  • Both engines feed the same /creatives/review gate; remix history keeps working via Creative(source:'remix').
  • Worker runtime lives in a separate repo (ffmpeg/whisper/tesseract image); dry-run mode reproduces the full flow at zero generation cost.

Test plan

  • npm run lint / npx tsc --noEmit clean; migration verified against schema with prisma migrate diff (no drift).
  • e2e/remix-jobs.spec.ts: 15 passed — happy path (ingest → job → claim → running/assembling/qc → succeeded promotes Creative to ready), claim atomicity, HMAC rejection, terminal-state 409, unclaimed→succeeded 409, non-canonical outputUrl 400, QC-fail reviewNotes.
  • Live cross-repo integration against the real worker: full sequence verified, including the 409 terminal guard.

Context: this is the first slice of the tier-ladder proposal (t0 = your existing path / t0.5 = this PR / t1 & t2 gated pending the policy question in the alignment doc we shared). Happy to adjust RemixJob modeling or StoryboardBeat integration to your preference.

🤖 Generated with Claude Code

mtmtian added 3 commits July 15, 2026 16:47
Add an independent worker-engine RemixJob path alongside the existing
Seedance2-direct remix: control plane creates a pending RemixJob + brief via
POST /api/creatives/remix-jobs, an external HMAC-authenticated worker claims
it, generates beats, assembles/QCs the clip, and reports progress/results
back via /api/worker/remix-jobs/{claim,report,upload}. Purely additive —
reuses buildRemixBrief/competitorCreativeToAnalysis from remix-brief.ts
without touching any existing files.
…n upload

Close the code-review gaps on the worker-facing RemixJob surface: report/
now enforces an atomic ALLOWED_PREV transition table (succeeded/failed are
terminal), validates outputUrl against the canonical GCS upload path, and
surfaces brand-QC failures via Creative.reviewNotes instead of silently
promoting. upload/ pre-checks HMAC headers and content-length before
buffering the body. claim/ recycles stale in-flight jobs after a configurable
lease window and collapses the duplicated jobId/queue branches into one
atomic updateMany. De-dupes asJson + canvas-dims math into
src/lib/growth/remix-job.ts, and shares the e2e HMAC-signing helper between
remix-jobs.spec.ts and competitor-ingest.spec.ts.
@oratis

oratis commented Jul 20, 2026

Copy link
Copy Markdown
Owner

Review — remix worker engine (t0.5)

Thanks for this — the control plane is well-built. Claim atomicity via the single guarded updateMany genuinely holds up, the secret is fail-closed when unset, timingSafeEqual is used correctly, org scoping is derived from session/DB rather than request input everywhere it matters, and the migration is a clean net-new table with zero drift. The sign() extraction into e2e/helpers/hmac.ts is coverage-neutral, verified line by line.

The findings below are concentrated in one endpoint plus a transactionality gap. Severity labels are ours; happy to discuss any of them.


🔴 Blocking — /upload bypasses the state machine entirely

Three things combine in src/app/api/worker/remix-jobs/upload/route.ts:

  1. jobId is outside the signature. It's read from the query string at :31, but verifyWorkerHmac(headers, actualSha256) at :58 signs only ${timestamp}:${contentSha256}. claim and report both sign the full JSON body including jobId, so upload is the one asymmetric case.
  2. No status or ownership gate. findRemixJobById is a bare findUnique with no status filter; :62-66 goes straight from "job exists" to writing bytes.
  3. Deterministic key, unconditional overwrite. uploadToGCS (src/lib/storage.ts:57) issues a plain uploadType=media insert with no ifGenerationMatch, to remix/{orgId}/{jobId}.mp4.

The benign path is reachable through this feature's own design. Worker A claims job J and is merely slow — not dead — past REMIX_JOB_LEASE_MINUTES. Worker B reclaims via the stale-lease branch (claim/route.ts:63-71), completes, QCs, reports succeeded; the Creative is promoted and a human approves it. Worker A then finishes its own un-QC'd render and calls /upload — which succeeds, overwriting the bytes the approved Creative points at. A's subsequent /report correctly 409s, so the DB row stays consistent, but the artifact actually being served no longer matches what was QC'd or reviewed. There's no fencing token from /claim, so nothing tells A it was preempted.

The adversarial variant: because the signature doesn't bind jobId, a captured upload request can be replayed within the 5-minute window (ingest-auth.ts:55) against ?jobId=<another org's job>.

This is what makes "terminal states locked" true of the DB row but not of the deliverable. It's also the one path with no test — e2e/remix-jobs.spec.ts:375-393 skips the upload happy path.

Suggested fix: bind jobId (and ideally the route) into the signed payload; require the job to be in an expected in-flight status before writing; and either use ifGenerationMatch or a claim-scoped/versioned object key so a preempted worker physically cannot clobber a finished one.

🟠 Non-transactional RemixJobCreative writes leave unrecoverable state

report/route.ts:106-145: the status updateMany commits independently, then creative.update runs as a separate statement — no $transaction, no try/catch. If the second write throws (deleted Creative, transient DB error), the job is already committed as succeeded, but terminal states are excluded from both ALLOWED_PREV and the lease-recycling set, so every retry 409s forever and the Creative is stranded at generating. Job creation has the same shape (creatives/remix-jobs/route.ts:158-200) — a failed second create orphans a Creative(status:'generating') no job can ever promote.

🟠 The new e2e suite does not run in CI

e2e/remix-jobs.spec.ts:26 gates the whole file on INGEST_WEBHOOK_SECRET + WORKER_WEBHOOK_SECRET in the runner env. .github/workflows/e2e.yml:37-41 sets only AUTH_TOKEN_SECRET and PORT, and the workflow has no Postgres service container. So all 15 tests report as skipped, not passed — the "15 passed" figure can only come from a local run, and the green checkmark on this PR asserts nothing about the claim/report/tier logic.

Worth wiring properly rather than just amending the description: prompt-eval.yml:34 already sets a precedent for injecting a conditional secret via ${{ secrets.* }}. Note also that playwright.config.ts:30 hardcodes the server-side secret to 'e2e-worker-secret' while the spec reads process.env for the client side — enabling this in CI with any other value would fail every HMAC test for an unrelated reason.

🟡 outputUrl canonical-path check is a prefix + substring test

report/route.ts:90 is startsWith(canonicalPrefix) && includes(canonicalSubstring), not an exact match — and gcsPublicPrefix() (storage.ts:113) returns the bucket root without the uploads/ segment that uploadToGCS always prepends (storage.ts:54). So https://storage.googleapis.com/<bucket>/anything/remix/<orgId>/<jobId>/x.mp4 passes. Bounded to the configured bucket, so not independently critical, but the "must match the canonical upload path" guarantee isn't literally what's implemented. The one negative test varies org/job IDs rather than surrounding path structure.

🟡 Burst limit is not shared between the two remix paths

creatives/remix/route.ts:54 uses key:'remix', creatives/remix-jobs/route.ts:52 uses key:'remix-jobs'; the bucket key is ${key}:${identity} (rate-limit.ts:55), so these are independent buckets and an org gets 2× effective burst across the two endpoints. The daily cap is correctly shared — both count Creative(source:'remix') and both stamp that source, so that half of the claim holds.

🟡 No rate limiting or audit logging on the worker surface

None of the three worker routes import rate-limit.ts, and logAudit() is absent from all four new routes. The sibling session-authed route does rate-limit, so the pattern exists. On audit: AuditAction already carries competitor.video_store precisely because storing competitor video is IP-sensitive and wants provenance — using that material in a remix currently leaves no trail.

🟡 @@index([orgId, status]) doesn't serve the claim hot path

schema.prisma:1098 leads with orgId, but claim/route.ts:58-70 — the endpoint the whole worker fleet polls — filters on status alone with no orgId predicate, so that index can't be used. @@index([orgId, createdAt]) correctly matches the list GET.

🔵 Minor

  • REMIX_JOB_LEASE_MINUTES is undocumented in .env.example (the other two new vars are there).
  • No cleanup path for orphaned GCS blobs when /upload succeeds but /report never lands or is rejected — deleteFromGCS exists but is never called here.
  • Stale-worker progress reports can non-monotonically overwrite costTokens / beats / qcReport while a job is still non-terminal, since those are plain overwrites rather than merges.

The claim/report state machine itself is sound and we'd be glad to see it land — the blocking items are the /upload gating and the report-route transactionality. Happy to look at a revision.

Review assisted by Claude Code; every finding above was manually verified against the diff.

mtmtian added 4 commits July 20, 2026 18:42
…-path index

Review fix for PR oratis#19: claim/upload/report need a way to detect a preempted
worker so it can never overwrite a new holder's output. Adds claimToken
(reissued per claim) + attempt (bumps per claim, versions the output object
key) plus @@index([status, updatedAt]) for the claim hot path. audit.ts gets
the four remix.job_* AuditAction variants used by the routes in the next commits.
Review fixes for PR oratis#19:
- claim: mint+return claimToken/attempt, rate-limit (worker-claim, 600/min/IP),
  audit-log remix.job_claim.
- upload: require x-adex-claim-token, bind the HMAC signature to
  jobId:claimToken:sha256 so a preempted worker's old signature can't be
  replayed; verify claimToken + in-flight status before writing; version the
  object key as v${attempt}.mp4; rate-limit (worker-upload, 120/min/IP);
  audit-log remix.job_upload.
- report: require claimToken in the body, include it in both updateMany
  wheres (atomic fencing), disambiguate count!==1 into 404/409-stale-claim/
  409-illegal-transition; wrap updateMany + re-read + linked-Creative update
  in a single $transaction so the job can never end up terminal with an
  orphaned Creative; exact-match the canonical (versioned) outputUrl instead
  of substring; best-effort delete the current attempt's blob on failed;
  rate-limit (worker-report, 300/min/IP); audit-log remix.job_report.
- creatives/remix-jobs: wrap Creative+RemixJob create in one transaction,
  move the rate-limit key from 'remix-jobs' to 'remix' (shared bucket with
  the sibling /api/creatives/remix so the two endpoints can't double the
  effective burst), audit-log remix.job_create.
Review fix for PR oratis#19: e2e.yml had no DB, so competitor-ingest.spec.ts and
remix-jobs.spec.ts self-skipped in CI, silently. Adds a postgres:16-alpine
service container + DATABASE_URL, INGEST_WEBHOOK_SECRET/WORKER_WEBHOOK_SECRET
(testing-only values, not real secrets) and INVITE_CODES_DISABLED=true so
register works without minting codes, plus a `prisma migrate deploy` step.

playwright.config.ts's webServer.env now reads WORKER_WEBHOOK_SECRET and
INGEST_WEBHOOK_SECRET from process.env (falling back to the same literals the
specs themselves fall back to) instead of hardcoding WORKER_WEBHOOK_SECRET —
previously a runner that set the secrets in its own env to un-skip the specs
but didn't also override this hardcoded value would sign requests the dev
server couldn't verify.
…e cases

Review fix for PR oratis#19: existing claim/report/upload cases updated for the new
contract (claimToken/attempt from claim, claimToken required on report,
versioned canonical outputUrl). New negative cases: report with a wrong
claimToken → 409 stale claim; upload missing x-adex-claim-token → 401; upload
signed against the wrong claimToken → 409. All reject before touching GCS, so
no credentials needed; the upload happy path stays untestable locally (same
as before).

.env.example documents REMIX_JOB_LEASE_MINUTES (already read by claim/route.ts,
was undocumented).
@mtmtian

mtmtian commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Fixes pushed — ea46840..f52482b (4 commits)

Point-by-point against the review:

🔴 /upload state-machine bypass → claim fencing (25160d6 + e6a406f)

  • RemixJob gains claimToken (fresh randomUUID minted on every claim) and attempt (increments on every claim); both returned by /claim.
  • Signature now binds job identity: upload signs ${ts}:${jobId}:${claimToken}:${contentSha256} (new required header x-adex-claim-token). A captured request can no longer be replayed against a different jobId — the signature won't verify.
  • Status + ownership gate: after HMAC, upload requires the job to exist (404), claimToken to match the current holder's (409 stale claim), and status to be in-flight (409 otherwise).
  • Versioned object key: remix/{orgId}/{jobId}/v{attempt}.mp4. Your benign scenario is now dead twice over: preempted worker A holds a stale token (409 before any write), and even in the check-to-write window its would-be object is v1 while B's is v2 — different keys, and /report's canonical check only ever accepts the current attempt's URL.
  • /report is fenced too: claimToken required in the body and included in the atomic updateMany where-clause. This also closes the 🔵 "stale progress reports non-monotonically overwrite" item wholesale — a stale worker can no longer write anything.

🟠 Non-transactional writes (e6a406f)

/report's state transition + Creative promotion now run in one $transaction — if the Creative write fails, the status change rolls back and the job stays retryable instead of stranding a generating Creative. Job creation (Creative + RemixJob) is likewise transactional.

🟠 e2e suite not running in CI (792ae32)

e2e.yml now runs a postgres:16 service container, DATABASE_URL, prisma migrate deploy, both webhook secrets (testing-only literals), and INVITE_CODES_DISABLED=true so registration works on the runner. playwright.config.ts's webServer env now reads process.env first — the hardcoded-server-secret drift you flagged is gone. The remix suite runs for real in CI now (20 tests incl. the new negatives).

🟡 Items (e6a406f, f52482b, 25160d6)

  • outputUrl: exact match against ${gcsPublicPrefix()}uploads/remix/{orgId}/{jobId}/v{attempt}.mp4 — prefix+substring test gone, and the missing uploads/ segment you spotted is in the canonical string.
  • Burst limit: remix-jobs now uses the shared 'remix' rate-limit key — one bucket across both endpoints.
  • Worker surface: all three routes rate-limited (generous abuse-guardrails, commented as such) and audited — new remix.job_create/claim/upload/report AuditActions.
  • Index: added @@index([status, updatedAt]) for the claim hot path.

🔵 Minor

  • REMIX_JOB_LEASE_MINUTES documented in .env.example.
  • Failed reports best-effort delete the current attempt's blob; upload-then-vanish orphans remain uncovered (commented in-code as a known partial-coverage gap).

Verification

tsc clean · full Playwright suite 45/45 green against a live Postgres locally · the worker repo is updated to the new contract and re-verified against its mock, including the 409 stale-claim abandon path. The upload GCS happy path still isn't e2e-covered (local/CI creds are read-only) — all fencing rejections (401/409) fire pre-GCS and are covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants