[stevehu]

Server info retained raw invalid server options after startup applied runtime defaults. Validate ServerConfig fields on initial load and cache-clear/reload, then register corrections while preserving absent keys, valid raw representations, masking and non-decrypted credentials.

Capture every ServerOption's presence and input value. Compare all options against the later raw read, distinguish value changes from missing/added keys, and retain the later raw value or absence on disagreement. Apply a correction only to matching inputs. Decryption failures have their own diagnostic with the option name and exception type, without exception messages, causes, plaintext or ciphertext; they are not classified as source changes.

Use one construction/validation helper. The canonical branch owns invalid-option warnings, so validation takes no warnInvalid flag and performs no duplicate canonical-name check. Named test loads and blank defaults do not emit those warnings. Validation state is local to loading. A LinkedHashMap preserves ServerOption declaration order for diagnostics, independently of source key order.

Sixteen registry regressions cover reloads, both source-change directions, removed/added keys, missing sources, encrypted credentials/options, typed Integer/Long/Boolean inputs, corrupt ciphertext, mismatched keys, raw key/value preservation, read counts and warning policy. Source-change tests preserve the original cached map and edit the simulated source for subsequent reads.

The independent reads remain an open architectural follow-up. Config exposes separate decrypted-cache and non-decrypted loader APIs, including custom-loader and injected-value contracts. A consistent paired-snapshot API belongs in that layer; these PRs retain the existing contract and do not claim atomic snapshots.

Validation: mvn -pl server-config -am test passed on the current revision, including all 23 server-config tests and prerequisite suites. The warning-order regression fails with HashMap and passes with LinkedHashMap. The full server reactor previously passed at e5b8193; this ordering-only follow-up reran the changed module and prerequisites.

Closes #2838.

[stevehu]

Code review findings

The diff is identical on #2839 and #2840. The new ServerOptionRegistryTest and the existing ServerConfigTest pass on both branches. Findings are ordered by severity.

1. Validated snapshot is lost on config reload/clear (ServerOption.java:55). Registration only happens inside serverOptionInit, which runs only from Server.bind. I probed this: after init, registry ioThreads=48 field=48. After Config.getInstance().clearConfigCache("server") plus ServerConfig.load() (what ConfigReloadHandler.reloadModule does), it was registry ioThreads=null field=0. load() re-registers the raw map at ServerConfig.java:423, and nothing re-validates. The fix only holds until the first reload, and no test covers reload.
2. Possible NPE (ServerConfig.java:435). new HashMap<>(getJsonMapConfigNoCache(...)) throws if the no-cache load returns null, which the Config API documents as possible. load() passes the same expression to registerModule, which tolerates null.
3. Registry failure surfaces as a port-bind failure (Server.java:300). serverOptionInit() runs inside bind()'s try/catch (Exception). An exception from the new registration becomes "No ports available to bind to", or retries the next port when dynamicPort is on. Run option init and registration outside the try, or isolate and log the registry call.
4. Server-info contents change (ServerConfig.java:437). Unset or blank options now always appear with effective defaults, including shutdownTimeout: null. The maskConfigProperties docs say server info is compared with the config-server-generated info, so those keys may show as permanent mismatches. Please confirm the comparison tooling accepts this.
5. Test can't detect decrypted-secret leakage (ServerOptionRegistryTest.java:93). The test server.yml secrets are plain password with no CRYPT: values, so using the decrypted map would still pass. Use an encrypted fixture value.
6. Redundant disk re-read (ServerConfig.java:436). server.yml is re-parsed on every serverOptionInit, including each dynamic-port retry. The snapshot can also mix a fresh disk read with values from the earlier-loaded instance. Capture the raw map once in load() instead.
7. Hand-copied option list (ServerConfig.java:437). It duplicates ServerOption, so a new enum value would be validated but never reported, and the test repeats the same list. Drive it from ServerOption.values().
8. Silent no-op guard (ServerConfig.java:434). When this != instance, the method returns without registering or logging. A debug log would make the skip visible.

🤖 Generated with Claude Code

[stevehu]

Re-review of 67d6885 (#2839) / e95550f (#2840)

The two diffs are identical. The server-config tests pass on both heads. All six earlier findings are resolved. The remaining items are minor.

1. The source-change guard only works one way (ServerConfig.java:451). It protects options that already have a recorded correction. If a raw option becomes invalid after the decrypted read, for example ioThreads: 8 becomes -1, there is no correction, so the registry shows -1 while the runtime uses 8, with no warning. Comparing the raw values against the validated mappedConfig for all nine options would close this.
2. The "differs" warning is also logged when the key is absent (ServerConfig.java:455). The else branch covers both a changed value and a key missing from the raw map. Consider splitting the branches or including both values in the message.
3. Duplicated load(String) branches and a boolean logging flag (ServerConfig.java:433, ServerOption.java:48). Both branches construct and validate. A warnInvalid flag couples a logging policy to the validator. Consider returning the invalid keys and letting the canonical caller log, with one shared construct-and-validate helper.
4. The race test edits the cached map in place (ServerOptionRegistryTest.java:159). A real source change arrives as a new map after a cache clear, so the test doesn't model it, and it doesn't cover item 1.

🤖 Generated with Claude Code

[stevehu]

Re-review of 7f47636 (#2839) / 286229e (#2840)

The source is identical on both heads (only pom.xml differs). The server-config tests pass on both, including 13 in ServerOptionRegistryTest. The earlier items are resolved: the guard now covers all nine options in both directions, the absent, present-only and differs warnings are separate, and the race test no longer mutates the cache. What remains is minor.

1. A decrypt failure is reported as "differs" (ServerOption.java:74). matches() swallows any RuntimeException and returns false. A wrong key or corrupt ciphertext then looks identical to a source change, and the cause is lost. Consider logging the exception class at debug level, or returning a distinct result the caller can word differently.
2. warnInvalid flag remains (ServerConfig.java:437). It is fed by a second CONFIG_NAME.equals(configName) check that duplicates the one in load(). Consider returning the invalid keys and letting the canonical caller log them.
3. Altitude (ServerConfig.java:441). About 45 lines of presence, equality and decrypt-and-compare logic defend against reading the same file twice. A single read that yields both the raw and decrypted views, or deriving the reported map from the raw one and validating that same map, would remove the class of mismatch instead of detecting it.
4. Test gap (ServerOptionRegistryTest.java:235). The encrypted-option test only uses the string option serverString. A CRYPT value on a numeric or boolean option, such as ioThreads, goes through decrypt(raw) versus originalValue.toString() and is untested.

🤖 Generated with Claude Code

[stevehu]

Re-review of e5b8193 (#2839) / bcd0704 (#2840)

The server-config source is identical on both heads. The server-config tests pass on both, including 15 in ServerOptionRegistryTest. Three of the four items from the last round are resolved:

• Decrypt failures now log a distinct "could not be decrypted" warning that names only the exception type and leaks no value. Corrupt-ciphertext and wrong-key tests cover it.
• The warnInvalid flag is gone.
• Numeric and boolean encrypted options are now tested.

The altitude point (comparing two reads instead of making a single read) is left as is, which is a reasonable trade-off.

One remaining nit:

• Warning order is now HashMap order (ServerConfig.java:424). The invalid-option warning moved out of the ServerOption.values() loop into loaded.options().forEach(...) over a HashMap, and validatedConfig iterates the same map. The order of warnings for the same bad configuration can vary between JVMs, which makes log diffs less stable. An EnumMap or LinkedHashMap would keep declaration order.

🤖 Generated with Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[stevehu]

Re-review of 3024be6 (#2839) / 4c2e053 (#2840)

No new findings. The warning-order nit is fixed: validateServerOptions now builds a LinkedHashMap, and invalidOptionWarningsFollowDeclarationOrderRegardlessOfSourceOrder asserts the exact sequence.

The server-config source is identical on both heads. The server-config tests pass on both, including 16 in ServerOptionRegistryTest. I did not re-run the full reactor. The only item left unapplied is the earlier altitude point about comparing two reads instead of making a single read, which I treat as a design trade-off.

🤖 Generated with Claude Code