Delete the Playwright source video once it's been copied

[a-abdellatif98]

Fixes #110.

Chromium writes its own recording into record_video_dir, which video_recording_options points at video_storage_dir. save_video then streams a second copy into that same directory via Video#save_as, and attach_video uploads that copy and removes it. Nothing ever removed Playwright's original, so every check leaked one page@<hash>.webm, forever. The reporter saw storage/playwright_videos reach 36 GB at roughly 0.4 GB/day.

Video#save_as only copies the artifact, it goes through saveAsStream and leaves the source in place. Video#delete is what removes it, and Upright never called it.

The change

Hoist the video to a local and call video&.delete in the existing ensure:

def save_video
  return unless pending_video_recording

  video = pending_video_recording.fetch(:video)
  video_path = video_dir.join("#{SecureRandom.hex}.webm").to_s
  video.save_as(video_path)

  self.video_artifacts ||= []
  video_artifacts << pending_video_recording.merge(path: video_path)
ensure
  video&.delete rescue Rails.error.report($!)
  self.pending_video_recording = nil
end

Deleting in ensure is the point: if save_as raises, the source would otherwise be leaked permanently.

The rescue is load-bearing rather than merely defensive. Video#delete can itself raise, since @artifact.value! re-raises when the page closed without producing frames and the channel may already be gone. Without the rescue, a failure there in ensure would replace whatever save_as raised and hide the original error. The form matches the existing page&.close rescue Rails.error.report($!) in Upright::Playwright::Lifecycle.

Scope

This fixes the primary leak, the Playwright source, which leaks once per check unconditionally.

It does not address the secondary case the issue mentions, the <hex>.webm copy stranded when attaching never completes. attach_video only removes files it successfully attaches, so a failure before or during attach still leaves that copy behind. That is a different code path and a different failure mode, so I left it out to keep this diff focused. Happy to follow up if you'd like it in the same PR.

The issue also suggests writing the save_as copy to a tmp dir rather than into record_video_dir. That is a reasonable cleanup but a separate change, so it's not included here.

Tests

Added test/models/upright/playwright/video_recording_test.rb. A FakeVideo mirrors the real semantics, the constructor writes the source file, save_as copies it, and delete removes the source, with the video dir pointed at a Dir.mktmpdir. Three cases:

1. happy path, the source is gone and the copy is recorded in video_artifacts
2. save_as raises, the source is still cleaned up
3. delete raises, the error is reported and save_video does not raise

All three were written first and fail against unmodified code.

Verification

• bin/rubocop: 188 files, no offenses.
• The three new tests pass, and all three fail with the source change reverted.
• Full bin/rails test: 5 pre-existing environmental failures on my machine, 3 in playwright_probe_test.rb because Chromium isn't installed locally and 2 in traceroute_probe_test.rb because mtr-packet can't open raw sockets unprivileged on macOS. Both files fail identically on unmodified main.

I could not verify the real Video#delete round-trip against a live Playwright server locally, for the Chromium reason above. The unit tests cover the logic. The issue reporter states they ran this patch in production and confirmed record_video_dir stays empty between checks.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR ensures Playwright-recorded source videos are deleted after being copied into Upright’s storage directory, preventing unbounded growth of the configured recording directory.

Changes:

• Delete the Playwright source video in an ensure block after attempting to save the copied video.
• Add test coverage for successful save, save failure, and delete failure reporting.
• Document the behavior change in the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
test/models/upright/playwright/video_recording_test.rb	Adds tests verifying source video deletion and error reporting behavior.
app/models/concerns/upright/playwright/video_recording.rb	Deletes Playwright source video after copy (even on failure) and reports delete errors.
CHANGELOG.md	Notes the new behavior preventing record_video_dir growth.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

app/models/concerns/upright/playwright/video_recording.rb:52

• Using a rescue modifier here makes the control flow harder to read and relies on $!, which is implicit/global state. Consider expanding this into an explicit begin ... rescue => e ... end (capturing the exception as a local) and then reporting e. This is clearer and avoids any ambiguity if this line is ever refactored or wrapped by additional exception handling.

      video&.delete rescue Rails.error.report($!)