feat(finalizer): serve finalization jobs with Effect HTTP - #25
Conversation
mrevanzak
changed the title
Review SummaryThis PR introduces an Effect-native recording finalizer spanning the Cloudflare Worker and Node/ffmpeg container. It adds typed domain errors, scoped cleanup, validated manifests/plans, checksum and publication verification, retryable orchestration, and layered tests while preserving the existing HTTP and queue contracts. Key Changes:
Confidence Score: 4/5Recommendation: Review complete. Rationale: The supplied changed-file context covers the architecture and primary execution paths, and the implementation was inspected directly. Automated verification was unavailable because pnpm is not installed in the checkout environment. Special Attention- Lease heartbeat cancellation and behavior when heartbeat renewal fails. - Retry and redelivery semantics for container, D1, R2 publication, and cleanup failures. - Exact checksum, size, content-type, and idempotent publication verification. - Segment byte assembly and ffmpeg remux/concat behavior for multi-part recordings. - Docker/esbuild output freshness and separation of Worker-only versus Node-only dependencies.Important Files Changed
Sequence DiagramsequenceDiagram
participant Q as Cloudflare Queue
participant W as Finalizer Worker
participant D as D1
participant R as R2
participant C as ffmpeg Container
Q->>W: Deliver sessionId
W->>D: Claim finalization lease
D-->>W: Manifest and finalize plan
loop Each manifest part
W->>R: Read and verify part
R-->>W: Bytes, size, checksum
W->>C: Upload part
end
W->>C: Finalize segments
C-->>W: Output stream, checksum, metadata
W->>R: Publish output idempotently
R-->>W: Publication metadata
W->>D: Complete leased job
W->>C: Best-effort delete job
alt Retryable failure
W->>D: Release lease for retry
else Terminal failure
W->>D: Mark finalization failed
end
Audit history- 9c0afef0f3b484ac7ea7b5b677908ec9b28a90e7054ba63d7ba2acade66c6d8e [comment 3794752548] — mrevanzak: Cloudflare treats `pingEndpoint` as a host/path, not a path. `container/health` becomes `GET http://container:8080/health`. The server returns 204 on `/health` - b8b9a4d0a360f7e3a42facabbe92d6a8c53ab6935d22627b55ac63cf16d4a416 [comment 3794752808] — mrevanzak: `heartbeat` is `db.renew.pipe(Effect.repeat(Schedule.fixed("60 seconds")), Effect.asVoid)`. The infinite Schedule never succeeds; `raceFirst` only ends if the |
![access-time-code-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/173532333?s=60&v=4){kind=small}
| ) | ||
| ); | ||
| }).pipe( | ||
| const manifest = yield* decodeManifest(job.manifest); |
There was a problem hiding this comment.
🔴 Critical
Manifest and finalize-plan validation runs outside the terminal-error handling around runAttempt. Invalid persisted data therefore leaves the row in finalizing until lease expiry and retries forever instead of marking it failed. Move both decode calls inside the same catchIf(isTerminalFinalization, ...) boundary.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/finalizer/src/worker/process.ts
Line: 186
Severity: critical
Comment: Manifest and finalize-plan validation runs outside the terminal-error handling around `runAttempt`. Invalid persisted data therefore leaves the row in `finalizing` until lease expiry and retries forever instead of marking it failed. Move both decode calls inside the same `catchIf(isTerminalFinalization, ...)` boundary.
How can I resolve this? If you propose a fix, please make it concise.
Think this is incorrect? Reply false positive: <reason>.
4eba94b to
ac3e1a4
Compare
| return json(404, { error: "not found" }); | ||
| } | ||
| const job = decodeURIComponent(match[1] as string); | ||
| if (request.method === "DELETE" && !match[2] && !match[4]) { |
There was a problem hiding this comment.
🔴 Critical
DELETE does not check sealing, so a client can delete the job while finalization is using it. The in-flight finalizer may then recreate the directory in setOutput, leaving a deleted job resurrected with a missing output path or inconsistent state. Reject deletion while sealing, or coordinate deletion through the same job lifecycle/lock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/finalizer/src/server/http.ts
Line: 83
Severity: critical
Comment: DELETE does not check `sealing`, so a client can delete the job while finalization is using it. The in-flight finalizer may then recreate the directory in `setOutput`, leaving a deleted job resurrected with a missing output path or inconsistent state. Reject deletion while sealing, or coordinate deletion through the same job lifecycle/lock.
How can I resolve this? If you propose a fix, please make it concise.
Think this is incorrect? Reply false positive: <reason>.
| if (size > limit) { | ||
| return Effect.succeed({ ...state, size, tooLarge: true }); | ||
| } | ||
| if (state.tooLarge) { |
There was a problem hiding this comment.
🟡 Warning
After a chunked request exceeds the limit, the handler continues consuming the entire remaining body because tooLarge only stops buffering. An attacker can stream an unbounded body and tie up the container despite the 512 MiB/2 MiB limits. Abort or destroy the request stream as soon as the limit is exceeded and return 413.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/finalizer/src/server/http.ts
Line: 54
Severity: warning
Comment: After a chunked request exceeds the limit, the handler continues consuming the entire remaining body because `tooLarge` only stops buffering. An attacker can stream an unbounded body and tie up the container despite the 512 MiB/2 MiB limits. Abort or destroy the request stream as soon as the limit is exceeded and return 413.
How can I resolve this? If you propose a fix, please make it concise.
Think this is incorrect? Reply false positive: <reason>.
There was a problem hiding this comment.
🤖 review-bot · openai/gpt-5.6-luna · full · 166.1s · 1,066,650 tokens · diff: 18 files · preloaded context: 19 changed + 10 related · 1 noise filtered · policy: github-pr-review, thermo-nuclear-code-quality-review · optional skills invoked: react-useeffect, vercel-react-best-practices
There was a problem hiding this comment.
False positive
critical: The container health probe is configured for container/health, but the HTTP server only returns 204 for /health (http.ts:75). Containers will fail health checks and remain unavailable; align pingEndpoint with /health or add the configured route.
Reviewed by: mrevanzak
Reason: Cloudflare treats pingEndpoint as a host/path, not a path. container/health becomes GET http://container:8080/health. The server returns 204 on /health.
There was a problem hiding this comment.
false positive: Cloudflare treats pingEndpoint as a host/path, not a path. container/health becomes GET http://container:8080/health. The server returns 204 on /health.
There was a problem hiding this comment.
✅ False positive recorded
Reviewed by: mrevanzak
Reason: Cloudflare treats pingEndpoint as a host/path, not a path. container/health becomes GET http://container:8080/health. The server returns 204 on /health.
ac3e1a4 to
4ff6122
Compare
| return json(404, { error: "not found" }); | ||
| } | ||
| const job = decodeURIComponent(match[1] as string); | ||
| if (request.method === "DELETE" && !match[2] && !match[4]) { |
There was a problem hiding this comment.
🟡 Warning
DELETE is allowed while a job is sealing. A concurrent delete can remove the job directory during finalization, after which setOutput may recreate the directory and publish an output for a job the caller already deleted. Reject deletion while sealing.has(job) or coordinate deletion through the same job lock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/finalizer/src/server/http.ts
Line: 83
Severity: warning
Comment: DELETE is allowed while a job is sealing. A concurrent delete can remove the job directory during finalization, after which `setOutput` may recreate the directory and publish an output for a job the caller already deleted. Reject deletion while `sealing.has(job)` or coordinate deletion through the same job lock.
How can I resolve this? If you propose a fix, please make it concise.
Think this is incorrect? Reply false positive: <reason>.
4ff6122 to
f407e36
Compare
f407e36 to
2b2f317
Compare
| container.deleteJob(job).pipe(Effect.orDie) | ||
| ); | ||
| return { outputKey, published }; | ||
| return yield* Effect.raceFirst(work, heartbeat); |
There was a problem hiding this comment.
False positive
critical: heartbeat succeeds after renewing the lease, so Effect.raceFirst(work, heartbeat) can finish the entire finalization before work uploads, finalizes, and publishes the output. Long-running jobs may remain leased without producing output. Run the heartbeat in a scoped background fiber and await work, propagating lease-loss failures separately.
Reviewed by: mrevanzak
Reason: heartbeat is db.renew.pipe(Effect.repeat(Schedule.fixed("60 seconds")), Effect.asVoid). The infinite Schedule never succeeds; raceFirst only ends if the lease renew fails (LeaseLost). The bot treated repeat as a one-shot.
There was a problem hiding this comment.
false positive: heartbeat is db.renew.pipe(Effect.repeat(Schedule.fixed("60 seconds")), Effect.asVoid). The infinite Schedule never succeeds; raceFirst only ends if the lease renew fails (LeaseLost). The bot treated repeat as a one-shot.
There was a problem hiding this comment.
✅ False positive recorded
Reviewed by: mrevanzak
Reason: heartbeat is db.renew.pipe(Effect.repeat(Schedule.fixed("60 seconds")), Effect.asVoid). The infinite Schedule never succeeds; raceFirst only ends if the lease renew fails (LeaseLost). The bot treated repeat as a one-shot.
1 participant
Summary
RecordingsUnavailable,FinalizerDbUnavailable) instead of permanently failing recordings.open → sealing) instead of a process-local Set.Residual
process.tsunpublished-output cleanup usesEffect.exit+flatMap, which is interruptible. A heartbeatraceFirstinterrupt during publish can skip the delete and leave orphan R2. Original ignore-swallow on the LeaseLost/delete path is fixed.Validation
pnpm --filter @interview-web/finalizer test(49 passed)pnpm --filter @interview-web/finalizer check-typespnpm --filter @interview-web/finalizer build:server