Skip to content

fix(finalizer): boot the container and publish remuxed output - #28

Merged
mrevanzak merged 4 commits into
mainfrom
fix/finalizer-container-boot-and-r2-put
Aug 17, 2026
Merged

fix(finalizer): boot the container and publish remuxed output#28
mrevanzak merged 4 commits into
mainfrom
fix/finalizer-container-boot-and-r2-put

Conversation

@mrevanzak

Copy link
Copy Markdown
Collaborator

Summary

  • Stop committing the finalizer server.js bundle. Emit CJS to output/ and build that in a multi-stage image so the container no longer crashes on boot with Dynamic require of "events" is not supported (worker mapped that as part upload: 500).
  • Buffer remux fetch streams before R2.put. Workers R2 rejects unknown-length streams (FixedLengthStream TypeError → RecordingsUnavailable).

Test plan

  • pnpm --filter @interview-web/finalizer test — 51/51
  • Bundle boot test: node output/server.cjs answers GET /health 204
  • makeRecordings.put regression: fetch stream is buffered to Uint8Array before R2
  • Restart pnpm dev and finalize session 56e71174-72a0-4bea-8b2b-4913e6c79a36 (or a new recording) — should pass container boot and R2 publish

The committed ESM server.js bundle crashed on boot with
`Dynamic require of "events" is not supported` from ws, so the
container exited 1 and the worker mapped the proxy 500 as
`part upload: 500`. Stop committing the bundle, emit CJS to
output/, and build that in the image.

R2 then rejected the remux stream (`known length` TypeError →
RecordingsUnavailable). Buffer fetch streams before put.
@access-time-code-pr-reviewer

access-time-code-pr-reviewer Bot commented Aug 17, 2026

Copy link
Copy Markdown

Review Summary

This PR fixes finalizer container boot failures by bundling the server as CommonJS in a multi-stage Docker image and addresses Cloudflare R2 uploads that reject unknown-length streams. It also re-dispatches finalization for sessions stuck in the finalizing state, but the changed recording APIs and submission routes remain unauthenticated, creating critical data exposure and deletion risks.

Key Changes:

  • Build the finalizer server into output/server.cjs and run it from a multi-stage ffmpeg container image.
  • Buffer streamed remux output before publishing it to R2.
  • Re-dispatch finalization when the database reports a finalizing status.
  • Consolidate and rename database migrations.
  • Add regression coverage for finalizer boot, R2 stream handling, and recording flows.

Confidence Score: 5/5

Recommendation: Request changes before merge.

Rationale: The critical authorization issues and upload-orphan risk are directly evidenced by the changed API and route implementations, while the finalizer behavior is covered by focused tests and the supplied test results.

Special Attention

- Add authentication and authorization to recording procedures, including operator scoping and candidate download capabilities. - Protect GET and DELETE recording submission operations from unauthenticated callers. - Validate recording session and segment ownership before writing upload blobs, or reliably delete blobs when acknowledgement fails. - Decompose the 1,533-line live recording module into focused persistence, upload, recovery, polling, and finalization components. - Verify migration renames and consolidation are safe for existing deployed databases.

Important Files Changed

File Overview
packages/api/src/index.ts Defines the oRPC procedure hierarchy; recording procedures currently inherit from publicProcedure without authentication or authorization.
apps/web/src/server/recording-submission.ts Handles recording download and deletion requests; these operations are reachable without an authorization check.
apps/web/src/server/recording-upload.ts Writes upload blobs to R2 and then acknowledges them in the database, allowing orphaned storage when acknowledgement rejects the request.
packages/finalizer/Dockerfile Builds the finalizer bundle and packages ffmpeg with the generated CommonJS server used by the container.
packages/finalizer/src/worker/recordings.ts Provides the R2 recording abstraction and handles known-length blob publication for streamed output.

Sequence Diagram

sequenceDiagram
  participant Caller
  participant API as Recording API
  participant DB
  participant R2
  participant Finalizer
  Caller->>API: List or request recording by session ID
  API->>DB: Query recording metadata
  API-->>Caller: Data or recording output without auth check
  Caller->>API: Upload segment
  API->>R2: Write blob
  API->>DB: Acknowledge session/segment
  alt Acknowledgement rejected
    API-->>Caller: Error
    Note over R2: Orphaned object remains
  else Finalizing session
    API->>Finalizer: Dispatch finalization
    Finalizer->>R2: Publish remuxed output
  end
Loading
Audit history

@access-time-code-pr-reviewer access-time-code-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


🤖 review-bot · openai/gpt-5.6-luna · full · 108.0s · 476,788 tokens · diff: 8 files · preloaded context: 8 changed + 10 related · policy: github-pr-review, thermo-nuclear-code-quality-review · optional skills invoked: performance

Comment thread packages/finalizer/src/worker/recordings.ts Outdated
R2 rejects unknown-length streams, so put was buffering the remuxed
body. Pass the known output size through a FixedLengthStream instead
so large recordings publish without exhausting Worker memory.
Queue drops after maxRetries leave rows in finalizing with no consumer.
Claim can reclaim an expired lease, so retry the dispatch from finalize.
Alchemy applies migrations through D1's HTTP query API, which splits on
semicolons and rejects CREATE TRIGGER. Replace the incremental history
with one CREATE TABLE migration that uses CHECK constraints instead.
@mrevanzak
mrevanzak merged commit fdbec0a into main Aug 17, 2026
2 of 3 checks passed
@mrevanzak
mrevanzak deleted the fix/finalizer-container-boot-and-r2-put branch August 17, 2026 10:54
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.

1 participant