Fix duplicate extension config entries

[xbmc4lyfe]

Closes #588.

Problem

ScriptConfig::SaveConfig tracked saved options by OptEntry pointer. FindOption resolves duplicate names case-insensitively to the first entry, so later same-name entries appeared unwritten and were appended repeatedly whenever settings were saved.

That is the amplifier behind the exponential config growth in #588. With N duplicate lines of an option in the file, a client that echoes loadconfig back sends all N entries (loadconfig returns raw file lines, duplicates included), the replace loop rewrites the N existing lines, and the append loop adds N−1 more — N → 2N−1 per save, even though the client sent back exactly what it read. The reporter's attached config contains exactly the 2^k+1 copy counts this recurrence predicts (Server3.* ×257, Server2.* ×513, Category2-4 options ×2049).

There is a second, quieter half to the same defect. SaveConfig wrote the first matching entry's value, while Options::SetOption overwrites the entry in place while parsing — so the last line for a name is the value nzbget actually runs on. The two disagreed. Before deduplication that went unnoticed, because the appended tail happened to preserve the last value; once duplicates are collapsed, writing the first value silently discards the live setting.

Full analysis, measurements and reproduction steps are in the PR comments.

Fix

• Track written options by case-insensitive option name and record names in both the replacement and append paths, so exact or case-variant duplicates in a save request can no longer append copies.
• Write each option only once in the replacement path too, so a single save converges a config already damaged by earlier versions back to one line per option (comments and unknown-line handling unchanged).
• Resolve duplicates the way the loader does: keep the first occurrence's name and position, but the last occurrence's value. Without this, healing a damaged config dropped the live setting and the option fell back to its default on the next start — a disabled news server came back enabled.

Scope

This fixes the growth and makes healing safe, but it does not cover one related path. The Settings page resolves duplicates to the first entry as well (findOption, webui/config.js), so on an already-damaged config it displays a value the daemon is not using and persists that value on save. The correct value is discarded in the browser before the request reaches SaveConfig, so nothing here can recover it.

The clean fix for that is to have loadconfig return one entry per option name carrying the effective (last) value. That would correct the Settings page display, stop it from overwriting good lines, and prevent duplicates from ever reaching any client. I'd rather do it as a separate PR than widen this one — happy to take it.

Verification

• Regression test: an absent option supplied twice using case variants is written once, carrying the last entry's value, and stays stable on re-save
• Regression test: a config pre-seeded with exact and case-variant duplicate lines is deduplicated by a single SaveConfig
• Regression test: when the file carries duplicate lines for one option, the collapsed line keeps the value the loader would have used (fails on the previous revision of this branch, which kept the first)
• Live daemon check via JSON-RPC: a damaged file (63 lines, Server2.Active ×17) converges after one save to a single line carrying no, the effective setting stays disabled, and no config error is logged; the same experiment on develop grows 2 → 3 → 5 → 9 → 17
• Full CTest suite passes (10/10)
• git diff --check passes
• Commits are SSH-signed by xbmc4lyfe

[dnzbk]

Thanks for working on this!
It looks like this PR only masks the issue and likely only handles extension duplicates. It doesn't fix the root cause of why duplicate settings (like server settings) are generated in the first place
Could you investigate further to find where these duplicate entries originate and address the core issue?

[xbmc4lyfe]

I did a full root-cause investigation of #588 using the reporter's attached configs and extension scripts, and reproduced the whole chain end to end against live daemons on v25.0, develop, and this branch.

Revised after further testing. Re-running this against real daemons corrected two things in my original write-up: the stock Settings page does not drive the growth loop, and the healing commit I pushed had a real defect of its own. Both are documented below with the measurements, and the defect is fixed.

TL;DR

The amplifier	SaveConfig maps N same-name entries to 2N−1 file lines. Matches the reporter's config exactly (2^k+1 counts) and reproduces live.
The actual defect	Lookups take the first match; Options::SetOption makes the last line authoritative. Duplicate lines just make that disagreement observable.
Correction	The growth needs a client that echoes loadconfig back. The stock Settings page never sends duplicates.
Wider than the file size	One Settings-page save with no edits silently turns a disabled news server back on. No API client involved.
Fixed in this PR	Growth stopped, damaged configs healed, and the healed line now carries the value nzbget was actually running on.

1. The real defect is a first/last disagreement

Three lookups return the first match:

• Options::OptEntries::FindOption — daemon/main/Options.cpp:93
• SaveConfig's write path — daemon/extension/ScriptConfig.cpp
• the Web UI's findOption — webui/config.js:559

But Options::SetOption finds the entry and overwrites its value in place while parsing, so when a name occurs on several lines the last one is what nzbget runs on.

Everything below falls out of those two halves disagreeing. Duplicate lines are the symptom that makes it visible, not the bug itself.

2. The growth loop

saveconfig passes client entries verbatim to SaveConfig. With N duplicate lines of one option and the same N entries echoed back:

• the replace loop rewrites all N existing lines (each matches FindOption, which returns the first entry), and
• the append loop appends N−1 more, because writtenOptions tracked OptEntry* pointers and only the first entry was ever marked written.

N → 2N−1 per save. From a single duplicated entry: 2 → 3 → 5 → 9 → 17 → … → 2^k+1.

loadconfig returns raw file lines including duplicates, so a client that round-trips them closes the loop.

The reporter's config matches the recurrence exactly. The sanitized config attached to #588 (37,999 option lines):

Option group	Copies in file	2^k+1
Server3.* (each option)	257	2^8+1
Server2.* (each option)	513	2^9+1
Category2-4.{DestDir,Unpack,Extensions,Aliases}	2049	2^11+1
Server1.*, Category1.*, Category5.*	1	—

Powers of two plus one are the signature of the 2N−1 loop, not of a client spamming entries linearly. The differing exponents just mean each group got its initial duplicate on a different save.

Reproduced live. Seeded a 33-line config with one glitched duplicate, then ran four faithful loadconfig → saveconfig round-trips with no edits. Every figure below is read from the real file on disk after each save:

                    file lines   Server2.Active lines
seed                        33                      2
after save #1               35                      3
after save #2               39                      5
after save #3               47                      9
after save #4               63                     17

On this branch the same experiment produces no growth at all.

3. Correction: the Settings page does not amplify

My original write-up said the round-trip above is "what the Settings page does". That is wrong, and I want it on the record.

Clicking Save all changes in the Web UI with zero edits leaves a 2-duplicate config at exactly 2. prepareSaveRequest (webui/config.js:2297) builds its payload from the template option model — one entry per option name — so a duplicate can never reach the wire.

The amplifier needs a client that echoes loadconfig back: a script, an API consumer, a third-party manager. That matters for who is affected and how this fix should be validated, so it shouldn't stand uncorrected.

4. A single Settings-page save turns a disabled server back on

This one needs no API client and is, I think, the more serious half.

The Web UI binds the first duplicate for display; the daemon runs on the last. So the Settings page shows a value the daemon isn't using, and saving persists the wrong one over the good line.

Seed config:

Server2.Active=          <- glitched duplicate, empty
Server2.Encryption=
Server2.JoinGroup=
Server2.Name=xsusenet
...
Server2.Active=no        <- the real value, what the daemon runs on

After one Save all changes, no edits:

Server2.Active=
...
Server2.Active=          <- the "no" is gone

Measured on a live daemon:

State	Server2.Active on disk	Effective setting	Config error
seed	"" + "no"	disabled	none
after one Web UI save, no edits	"" + ""	ENABLED	logged

On restart the empty value fails validation, ParseEnumValue logs Invalid value for option "Server2.Active": "" and falls back to the option default, which is yes. A server the user deliberately disabled starts carrying traffic — and this is reachable from a plain Settings-page save on any config that already has one duplicate.

5. What this PR does now

The original commit stopped the append side but kept every existing duplicate line, so a config damaged by v25 (the reporter's is 1 MB) would stay duplicated forever. I pushed an update to heal those — and re-testing showed that update had a defect of its own:

It collapsed duplicates to the first occurrence's value while the loader uses the last. Before the dedup, the appended tail accidentally preserved the last value — which is why a damaged config still booted with the right setting. Removing the duplicates removed that accidental safety net and turned a latent inconsistency into real data loss, on exactly the configs the change exists to repair.

Fixed by mirroring Options::SetOption: keep the first occurrence's name and position, but the last occurrence's value.

// Options::SetOption overwrites the existing entry while parsing, so when a
// name occurs on several lines the last one is the value nzbget runs on.
// Collapsing duplicates must preserve that value, otherwise saving a damaged
// config silently changes settings.
auto findLastValue = [optEntries](const char* name) -> const char*
{
    for (auto it = optEntries->rbegin(); it != optEntries->rend(); ++it)
    {
        if (!strcasecmp(it->GetName(), name))
        {
            return it->GetValue();
        }
    }
    return nullptr;
};

Applied in both the replace loop and the append loop.

Tests: added SaveConfigKeepsLastValueWhenConfigHasDuplicateLines, which fails on the previous version of the fix (Server2.Active=yes where the loader would use no) and passes now. The existing duplicate test also had to change — it asserted the survivor was the first entry's value, which is precisely the behaviour that caused the flip. Full ctest suite: 10/10.

6. Second origin bug: legacy script parser eats the first character of option names

Unchanged from my original analysis, and still worth a separate PR.

The reporter's config contains SizePriority:eryBigSize, SizePriority:igSize, SizePriority:riorityVerySmall — every option of that legacy script missing its first letter.

ExtensionLoader::V4::ParseSectionAndSet (ExtensionLoader.h) does

opt.name = line.substr(1, sepPos - 1);

unconditionally — it assumes the option line starts with #. Old-style scripts whose OPTIONS section ships with enabled (uncommented) lines get every option name truncated by one character.

I reconstructed SizePriority.py with uncommented option lines, dropped it into ScriptDir on v25.0, and loadextensions returns:

erySmallSize, riorityVerySmall, mallSize, rioritySmall, ormalSize,
riorityNormal, igSize, riorityBig, eryBigSize, riorityVeryBig

Character-for-character identical to the reporter's config entries, including the quoted default values ('100'). This makes the Web UI save settings under the mangled names while the correctly-named entries are dropped as unknown — the settings loss and "extensions get re-added on every save" part of #588. Happy to take that as a follow-up.

Worth considering

A complementary change: have loadconfig return one entry per option name (the effective, last value). That would fix the Web UI's display, remove the save damage in §4, and starve the amplifier at its source — all three symptoms with one change, and it would make saveconfig payloads well-formed by construction rather than by the receiver's tolerance.

Reproduction

Everything above comes from snapshots written by running daemons, not simulations. Method: seed a scratch config, drive loadconfig/saveconfig over JSON-RPC, and count name= occurrences in the file after each save; then boot a fresh daemon on each snapshot to read back the effective value and whether a config error was logged.

One trap worth passing on: leftover nzbget daemons squat on control ports. A new daemon fails to bind, exits, and the RPC calls silently reach the old daemon — the run looks clean and means nothing. Worth asserting that the ConfigFile a daemon reports matches the one you launched it with.

[dnzbk]

Since #588 is about where duplicate entries come from, the two causes you identified — the webui showing/saving the wrong value when duplicates exist, and the option-name mangling in ExtensionLoader - look in scope for this PR rather than follow-ups. Both are small fixes and resolving them here would actually close the issue. Could you include them?

[dnzbk]

Please drop the (issue #588) references from the codebase.