Walkthrough

The PR modifies DynamicPropServerConfiguration.reload() to clear the changedKeys set at the start of the synchronized block before loading new properties, ensuring only actual configuration changes from the reload are tracked. A test is added to verify that secret file rotation does not trigger listeners for unrelated watched properties.

Changes

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• questdb/questdb#6719: Directly related—implements the secret-file reload handling in DynamicPropServerConfiguration that this PR extends by clearing changedKeys and validates with the new test.

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.

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

[PR Coverage check]

😍 pass : 1 / 1 (100.00%)

file detail

@CodeRabbit review critically

#!/bin/bash
# Get the PR diff to see the exact changes
gh pr diff 6756 --repo questdb/questdb

#!/bin/bash
# Get the full file content of the modified class
fd -t f "DynamicPropServerConfiguration.java" --exec cat {}

#!/bin/bash
# Get the test file content
fd -t f "DynamicPropServerConfigurationTest.java" --exec cat {}

#!/bin/bash
# Look for other usages of changedKeys to understand the full context
rg -n "changedKeys" --type java -C 5

@bluestreak01 Here's my critical analysis of this fix:

Critical Issues

1. Breaking Change to Public Static Method Contract (Line 229)

The updateSupportedProperties() method is marked "// used in ent" and previously cleared changedKeys internally. Now callers must clear it themselves. This is an undocumented breaking change. If enterprise code calls this method directly, it will accumulate stale keys unless updated. Why wasn't this mentioned in the PR description or test plan?

Recommendation: Either document this as a breaking change for enterprise code, or add a parameter to control clearing behavior.

2. Inconsistent State on Exception (Lines 451-454)

If checkSecretFileChanges() throws CairoException after partially populating changedKeys (line 524), we abort the reload but leave changedKeys in a partially-mutated state. While it gets cleared on the next reload cycle, this makes the class's invariants fragile and error-prone.

Question: Should we wrap the entire mutation block in try-finally and clear changedKeys on any exception path?

3. Incomplete Test Coverage

The test testSecretFileRotationDoesNotNotifyStaleWatchers has gaps:

a) Missing verification: The test never confirms the password actually changed to "rotated_password". It only checks the listener counter. How do we know the reload worked at all?

// Missing after line 1486:
Assert.assertEquals("rotated_password", 
    serverMain.getConfiguration().getPGWireConfiguration().getDefaultPassword());

b) Scenario not tested: Secret-file-only reload as the first reload (no prior property changes). The bug could still exist in that case if changedKeys starts non-empty from previous test pollution.

c) Scenario not tested: Rotating a secret file for a property that IS being watched (e.g., watching pg.password, then rotating pg.password.file). Should that notify the watcher?

4. Lock Held During User Callbacks (Lines 460, 117-121)

The reload() method calls watchRegistry.notifyWatchers(changedKeys) while holding reloadLock, and notifyWatchers() is synchronized and directly invokes listener.configChanged(). If a listener callback is slow or attempts to call back into the configuration system, you risk deadlock or blocking all reloads.

Design question: Should notifications happen asynchronously or outside the synchronized block?

5. Edge Case: IOException Before Any Mutation (Lines 436-440)

If loading newProperties throws IOException at line 437, we return false at line 440 after having cleared changedKeys at line 433. This means:

• Previous reload's changed keys are lost
• We've mutated state before validating the operation can succeed

Is this acceptable behavior? It violates atomicity—either reload succeeds fully or leaves state unchanged.

Minor Issues

6. Memory Accumulation

The changedKeys ObjHashSet is cleared but never shrunk. After reloads with many changed keys followed by reloads with few, the set retains unnecessary capacity. Negligible in practice, but sloppy.

Proposed Test Enhancement

`@Test`
public void testSecretFileOnlyReloadAsFirstReload() throws Exception {
    // Test secret rotation when NO prior property changes occurred
    Path secretFilePath = Paths.get(temp.getRoot().getAbsolutePath(), "secrets", "pg_password.txt");
    Files.createDirectories(secretFilePath.getParent());
    Files.writeString(secretFilePath, "initial_password");
    
    try (FileWriter w = new FileWriter(serverConf)) {
        w.write("pg.password.file=" + secretFilePath.toString().replace("\\", "\\\\") + "\n");
    }
    
    try (ServerMain serverMain = new ServerMain(getBootstrap())) {
        serverMain.start();
        
        final AtomicLong listenerCounter = new AtomicLong(0);
        // Register listener for a property that's NOT changing
        serverMain.getEngine().getConfigReloader().watch(new ConfigReloader.Listener() {
            `@Override`
            public void configChanged() { listenerCounter.incrementAndGet(); }
            
            `@Override`
            public `@NotNull` ConfigPropertyKey[] getWatchedConfigKeys() {
                return new ConfigPropertyKey[]{ PropertyKey.CAIRO_SQL_ASOF_JOIN_EVACUATION_THRESHOLD };
            }
        });
        
        // Rotate secret file - listener should NOT fire since evacuation.threshold unchanged
        Files.writeString(secretFilePath, "rotated_password");
        serverMain.getEngine().getConfigReloader().reload();
        
        Assert.assertEquals(0, listenerCounter.get());
        // Verify password actually changed!
        Assert.assertEquals("rotated_password", 
            serverMain.getConfiguration().getPGWireConfiguration().getDefaultPassword());
    }
}

Bottom Line

The fix addresses the immediate bug but introduces a breaking change and has fragile exception handling. The test is insufficient—it doesn't verify the secret rotation actually worked and misses edge cases. Bump this back for:

1. Documentation of breaking change
2. Exception safety review
3. Enhanced test coverage
4. Consideration of lock-hold-time during callbacks

Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: In questdb/questdb, for PRs requested by javier, provide line-cited, push-back style reviews with concrete follow-ups (tests, diffs, and performance considerations).

Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: For questdb/questdb, javier expects a rigorous, push-back style review; always include concrete line-cited findings and propose targeted tests/fixes when reviewing javier’s PRs.

Learnt from: jerrinot
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-04T11:18:36.492Z
Learning: For PRs by jerrinot, always use a thorough and critical review mode: question design decisions, challenge assumptions, scrutinize implementations for edge cases and potential issues, and avoid superficial validation. They prefer hard-core reviews that push back on the code.