Skip to content

Fix unprotected JSON.parse in Redis.getObject() crashing viewer stats processing - #7537

Merged
Chocobozzz merged 2 commits into
Chocobozzz:developfrom
lawrence3699:fix/redis-json-parse-crash
Apr 13, 2026
Merged

Fix unprotected JSON.parse in Redis.getObject() crashing viewer stats processing#7537
Chocobozzz merged 2 commits into
Chocobozzz:developfrom
lawrence3699:fix/redis-json-parse-crash

Conversation

@lawrence3699

Copy link
Copy Markdown
Contributor

Description

Noticed while reading through the viewer stats processing code that Redis.getObject() calls JSON.parse() without any error handling. On a self-hosted instance, if a Redis value gets corrupted (partial write during a crash, data from a different PeerTube version, manual Redis intervention), the unhandled parse error propagates up through getLocalVideoViewerByKey() in processViewerStats().

Since getLocalVideoViewerByKey() is called outside the inner try/catch block (line 157, before the transaction at line 164), the exception is caught by the outer catch at line 181 — which exits the entire for loop. This means a single corrupted entry blocks all remaining viewer stats from being persisted to the database.

Minimal reproduction:

// getObject behavior without fix:
JSON.parse('{"lastUpdated": 0, "corrupt')
// => SyntaxError — crashes the whole batch

The same codebase already handles this correctly in server/core/models/server/plugin.ts (lines 209–213):

try {
  return JSON.parse(value)
} catch {
  return value
}

Before: One corrupted Redis viewer stats entry crashes processViewerStats(), preventing all remaining entries from being saved to the database.

After: Corrupted entries are logged, cleaned up from Redis, and skipped. The remaining entries are processed normally.

Changes

  • redis.ts: Wrap JSON.parse in getObject() with try/catch, log warning, return null
  • video-viewer-stats.ts: Add null check in processViewerStats() loop — log, delete the invalid key, and continue

Related issues

Has this been tested?

  • 👍 Verified with a minimal reproduction script that a corrupted JSON value crashes JSON.parse and the fix returns null instead
  • 💭 Full integration test would require a running Redis instance; the fix follows the same pattern already validated in plugin.ts

Screenshots

N/A

… processing

Wrap JSON.parse in getObject() with try/catch and add a null guard in
processViewerStats() so corrupted Redis entries are skipped and cleaned
up instead of crashing the entire batch job.
Copilot AI review requested due to automatic review settings April 2, 2026 14:23

Copilot AI 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.

Pull request overview

Defensive hardening of viewer stats batch processing so a corrupted Redis entry doesn’t crash processViewerStats() and block persistence of subsequent viewer stats.

Changes:

  • Add try/catch around JSON.parse in Redis.getObject() and return null on parse failure.
  • Skip and delete invalid viewer stats Redis keys during processViewerStats() when stats cannot be loaded.
  • Add warning logs for parse failures / invalid viewer stats entries.

Reviewed changes

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

File Description
server/core/lib/stats/shared/video-viewer-stats.ts Skip/delete invalid viewer stats entries during batch processing to prevent aborting the whole loop.
server/core/lib/redis.ts Catch JSON.parse failures in getObject() and log/return null instead of throwing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/core/lib/stats/shared/video-viewer-stats.ts Outdated
@@ -156,6 +156,12 @@ export class VideoViewerStats {
for (const key of allKeys) {
const stats: LocalViewerStats = await this.getLocalVideoViewerByKey(key)

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

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

stats can now legitimately be null (e.g. parse failure in Redis.getObject()), but it’s still typed as LocalViewerStats here. Updating the return types of getLocalVideoViewerByKey/getLocalVideoViewer (and this local variable) to LocalViewerStats | null would make the null-handling explicit and prevent accidental property access elsewhere without a guard.

Suggested change
const stats: LocalViewerStats = await this.getLocalVideoViewerByKey(key)
const stats: LocalViewerStats | null = await this.getLocalVideoViewerByKey(key)

Copilot uses AI. Check for mistakes.
Comment thread server/core/lib/redis.ts Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Chocobozzz
Chocobozzz merged commit 27314c4 into Chocobozzz:develop Apr 13, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants