fix(settings): serialize settings writes and prevent partial overwrites

[fallenbagel]

Description

Addresses a rare edge case where the in-memory settings state could become corrupted if concurrent operations read/write the settings singleton simultaneously. This has intermittently affected a handful of users across both Jellyseerr and Seerr, manifesting as undefined values for settings fields like the Jellyfin connection URL.

This PR hardens how settings are loaded and persisted in memory..

• Fixes Jellifyn url returns http://undefined:undefinedundefined #2149 (This is unrelated to seerr migration. The issue predates Seerr entirely and was reported against Jellyseerr).

Additional context:

• A restart has always resolved this when it occurs
• We have never been able to reproduce it in testing
• Root cause is likely external factors (e.g. the settings file being momentarily unreadable at startup) combined with the lack of concurrency protection on the singleton
• These changes make the settings class defensively resilient against these conditions

How Has This Been Tested?

• Verified settings load/save cycle preserves all fields across restarts
• Confirmed setter merge behavior retains existing values when partial data is written

the original issue has never been reproducible in testing and these changes are preventative hardening

Screenshots / Logs (if applicable)

Checklist:

• I have read and followed the contribution guidelines.
• Disclosed any use of AI (see our policy)
• I have updated the documentation accordingly.
• All new and existing tests passed.
• Successful build pnpm build
• Translation keys pnpm i18n:extract
• Database migration (if required)

Summary by CodeRabbit

• Bug Fixes

  • Improved settings save reliability by serializing writes to prevent concurrent write conflicts and potential data loss.
  • Migrations now merge safely into current settings to avoid lost updates.

• Refactor

  • Settings updates now merge with existing configuration rather than overwriting whole sections, and incoming arrays replace prior arrays to prevent stale entries.

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaced lodash merge with a custom mergeSettings (using mergeWith) that shallow-merges objects and replaces arrays; introduced a saveLock Promise to serialize file writes. Settings constructor and migrations use mergeSettings; multiple public setters now merge incoming data into existing state instead of replacing it. save() enqueues writes via saveLock and returns the chained Promise.

Changes

Cohort / File(s)

Summary

Settings implementation server/lib/settings/index.ts

Replaced merge import with mergeWith and added mergeSettings helper (shallow merge, incoming arrays replace). Constructor and migration load() use mergeSettings. Nine public setters (main, plex, jellyfin, tautulli, metadataSettings, public, notifications, jobs, network) now merge with existing state. Added private saveLock: Promise<void> and changed save() to serialize write/rename operations through saveLock, returning the resulting Promise.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibble through settings, arrays replaced with care,

Merges stitch the fields, no stale bits linger there.
A promise locks the door, saves hop one by one,
Quiet writes by moonlight, race conditions undone. 🥕✨

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: serializing settings writes and preventing partial overwrites that cause undefined values.
Linked Issues check	✅ Passed	The PR directly addresses issue #2149 by fixing the root cause of partially undefined settings fields through serialized writes and merge-based updates.
Out of Scope Changes check	✅ Passed	All changes in server/lib/settings/index.ts are directly related to fixing the serialization and concurrent access issues described in the PR objectives.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Updates the server-side settings subsystem to support safer concurrent persistence and to apply partial updates to several settings groups by merging into existing state.

Changes:

• Serialize concurrent Settings.save() calls using a promise-based lock to avoid overlapping writes/renames.
• Change multiple settings setters (main, plex, jellyfin, tautulli, metadataSettings, public, notifications, jobs, network) to merge new values into existing settings rather than replacing the entire object.

---

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@server/lib/settings/index.ts`:
- Around line 616-617: The current use of lodash.merge (called as
merge(this.data.main, data) in the Settings setters) performs index-wise merging
of arrays which preserves stale entries when the incoming array is shorter;
change these merges to use lodash.mergeWith (or equivalent) with a customizer
that replaces arrays instead of merging them (i.e., if _.isArray(source) return
source) so arrays in incoming payloads fully replace target arrays; update all
affected setters that call merge(this.data.main, data) (and similar merge usages
at the other listed locations) to use the mergeWith + array-replacing customizer
to ensure arrays are overwritten rather than merged element-by-element.
- Around line 829-835: The saveLock chain can become permanently rejected after
a failed fs.writeFile/fs.rename, so change the chaining in save() to first
neutralize prior rejections and ensure errors inside the write/rename do not
leave saveLock rejected; specifically, replace the current this.saveLock =
this.saveLock.then(...) with this.saveLock = this.saveLock.catch(() =>
Promise.resolve()).then(async () => { try { const tmp = SETTINGS_PATH + '.tmp';
await fs.writeFile(tmp, JSON.stringify(this.data, undefined, ' ')); await
fs.rename(tmp, SETTINGS_PATH); } catch (err) { /* log error via your logger
(e.g. processLogger.error) */ } }); this ensures saveLock never stays rejected
and subsequent save() calls proceed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 12bac796-af26-42bd-89f1-a79f62b26e98

📥 Commits

Reviewing files that changed from the base of the PR and between f40323c and a295a53.

📒 Files selected for processing (1)

• server/lib/settings/index.ts

[coderabbitai]

🧹 Nitpick comments (1)

server/lib/settings/index.ts (1)

621-740: Address inconsistent merge patterns across settings routes.

The /main and /network routes use merge(settings.X, req.body) before setter assignment, while other routes (/plex, /jellyfin) use Object.assign() or spread operators. This creates both split merge behavior and double-merging: the route layer merges via lodash, then the setter re-merges via mergeSettings().

Consider standardizing on a single pattern—either consolidate all merges into the setters and pass raw payloads, or consistently pre-merge at the route layer. Changing setter types to Partial<> would require broader refactoring and may conflict with existing callers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/lib/settings/index.ts` around lines 621 - 740, The code is
double-merging settings because some routes pre-merge payloads before calling
the class setters which also call mergeSettings (e.g., set main, set network,
set plex, set jellyfin, set notifications); standardize by removing pre-merge
logic at the route layer and always pass the raw request body into the setters
so mergeSettings is only invoked inside each setter (or conversely, if you
prefer route-layer merging, remove mergeSettings calls from the setters), update
callers accordingly, and ensure you consistently use the mergeSettings function
in one place (refer to the setters named set main, set network, set plex, set
jellyfin, set notifications and the mergeSettings helper when making this
change).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@server/lib/settings/index.ts`:
- Around line 621-740: The code is double-merging settings because some routes
pre-merge payloads before calling the class setters which also call
mergeSettings (e.g., set main, set network, set plex, set jellyfin, set
notifications); standardize by removing pre-merge logic at the route layer and
always pass the raw request body into the setters so mergeSettings is only
invoked inside each setter (or conversely, if you prefer route-layer merging,
remove mergeSettings calls from the setters), update callers accordingly, and
ensure you consistently use the mergeSettings function in one place (refer to
the setters named set main, set network, set plex, set jellyfin, set
notifications and the mergeSettings helper when making this change).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3e735b97-617c-4b24-995a-552b2e390c4f

📥 Commits

Reviewing files that changed from the base of the PR and between a295a53 and 6e12c64.

📒 Files selected for processing (1)

• server/lib/settings/index.ts

[0xSysR3ll]

LGTM