Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Code Review

Overall: A well-motivated fix for a real Kubernetes deployment problem. The approach is correct - resolving ${...} placeholders before handing the config to JUL is the right place to intercept. Backward compatibility is preserved (default stays ./log), the pre-existing stream leak is fixed as a bonus, and the test coverage is solid.

A few issues worth addressing before merge:

Medium: docs/4451-configurable-log-directory.md should not be committed

This file is a development-phase design document for a single issue. It duplicates information already in the PR description and is not user-facing documentation. Committing per-issue design docs to docs/ will clutter the tree over time. The content belongs in the PR body (where it already lives) or in a proper user-facing guide. Please remove this file.

Medium: DEFAULT_LOG_DIR and GlobalConfiguration.SERVER_LOGS_DIRECTORY default can drift

DefaultLogger.java declares:

private static final String DEFAULT_LOG_DIR = "./log";

and GlobalConfiguration.java independently declares the default "./log" for SERVER_LOGS_DIRECTORY. These two constants must always agree, but nothing enforces that. If someone updates one without the other, the resolver fallback and the config default silently disagree.

Consider referencing the constant from one place - e.g. derive it from GlobalConfiguration.SERVER_LOGS_DIRECTORY.getDefaultValue(), or add a comment on both pointing to the other. A test asserting they are equal would also catch the drift.

Medium: Windows batch - unquoted path with spaces

In server.bat:

if not "%ARCADEDB_LOG_DIR%"=="" set JAVA_OPTS_SCRIPT=%JAVA_OPTS_SCRIPT% -Darcadedb.server.logsDirectory=%ARCADEDB_LOG_DIR%

If ARCADEDB_LOG_DIR is C:\Program Files\arcadedb\log, the unquoted expansion splits the -D property value at the space. Consider:

if not "%ARCADEDB_LOG_DIR%"=="" set JAVA_OPTS_SCRIPT=%JAVA_OPTS_SCRIPT% "-Darcadedb.server.logsDirectory=%ARCADEDB_LOG_DIR%"

(The JVM handles quoted -D arguments correctly on Windows.)

Low: Reflection to test a private method

invokeResolveLogDir accesses resolveConfigurableLogDir via getDeclaredMethod + setAccessible(true). This is brittle - a rename or signature change silently becomes a NoSuchMethodException at test time rather than a compile error. The method is a pure transformation with no side effects; making it package-private (removing private) would allow direct calls from the same package's test and be more robust. Alternatively, the existing integration-style tests (fileHandlerPatternResolvesConfigurableLogDir) already cover the behavior through the public API; the unit tests could be rewritten against createLogDirectoryFromConfig/installCustomFormatter instead.

Low: Properties.store() writes a timestamp comment

props.store(buffer, null);

Even with null as the comment argument, Properties.store() prepends a #Mon Jun 02 12:34:56 UTC 2026 date line. While harmless (JUL's readConfiguration calls Properties.load() which skips # lines), it is unnecessary metadata. Passing an empty string "" instead of null avoids the auto-date comment. Entirely cosmetic, but worth noting.

Positive notes

• Stream leak fixed: wrapping stream in try (final InputStream in = stream) is a clean improvement over the previous unclosed-stream pattern.
• Both code paths use the same resolver: createLogDirectoryFromConfig() and installCustomFormatter() both call resolveConfigurableLogDir(), so the directory pre-creation and the pattern handed to JUL are guaranteed to agree.
• Unix script is correct: [ -n "$ARCADEDB_LOG_DIR" ] with a properly quoted expansion handles spaces in the path.
• Test coverage: the four new LoggerTest cases and the GlobalConfigurationTest assertion cover the important scenarios (unset fallback, system property override, literal passthrough, end-to-end init).

Dispositions for the review, pushed in 7656d56:

Windows batch unquoted path (Medium) - fixed. Quoted the -D argument so paths with spaces survive. server.sh stays unquoted to match the rest of $JAVA_OPTS_SCRIPT, which is word-split at the final exec.

Default drift between DEFAULT_LOG_DIR and SERVER_LOGS_DIRECTORY (Medium) - addressed with cross-reference comments. I did not derive DEFAULT_LOG_DIR from GlobalConfiguration.SERVER_LOGS_DIRECTORY.getDefaultValue(): the resolver runs on the first log call, which can occur during GlobalConfiguration's static initialization, and the default (no system property) path deliberately never touches GlobalConfiguration to avoid that init coupling. Drift is already caught by CI - GlobalConfigurationTest pins the config default to ./log and LoggerTest.logDirPlaceholderFallsBackToDefaultWhenUnset pins the resolver fallback to ./log - so a one-sided change fails a test. Added comments on both constants documenting the invariant.

Remove docs/4451-configurable-log-directory.md (Medium) - keeping it. Per-issue notes under docs/NNNN-*.md are an established, committed convention in this repo (e.g. docs/4446-configurable-raft-storage-path.md, docs/4274-*.md, docs/4320-*.md, and many more are tracked). This file follows that precedent, so removing it would be inconsistent with the existing tree.

Reflection to test a private method (Low) - keeping reflection. DefaultLogger is in com.arcadedb.log while LoggerTest is in com.arcadedb, so package-private would not expose the method to the test; the only direct-call option is public, which over-widens a pure internal helper's visibility. The behavior is also covered through the public init() path by fileHandlerPatternResolvesConfigurableLogDir, and a rename surfaces as a test failure in CI.

Properties.store("") to drop the date comment (Low) - no change. Properties.store writes the #<date> line unconditionally; passing "" instead of null does not suppress it (it only adds an empty # line first). The comment is harmless since JUL's Properties.load skips # lines.

Code Review - PR #4462: Configurable Server Log Directory

Overview

This PR fixes a legitimate production issue: ArcadeDB crashes on Kubernetes pods with readOnlyRootFilesystem because JUL's FileHandler eagerly opens ./log/arcadedb.log during GlobalConfiguration static init, before the server root path is available. The fix introduces arcadedb.server.logsDirectory (defaulting to ./log) and resolves ${...} placeholders in the FileHandler.pattern before handing it to JUL.

The approach is sound and backward-compatible. A few issues worth addressing before merge:

Bug: Unquoted $ARCADEDB_LOG_DIR in server.sh

JAVA_OPTS_SCRIPT="$JAVA_OPTS_SCRIPT -Darcadedb.server.logsDirectory=$ARCADEDB_LOG_DIR"

If ARCADEDB_LOG_DIR contains spaces (e.g. /var/log/my app/), the JVM argument splits incorrectly. The Windows .bat correctly quotes it:

"-Darcadedb.server.logsDirectory=%ARCADEDB_LOG_DIR%"

The shell script should match:

JAVA_OPTS_SCRIPT="$JAVA_OPTS_SCRIPT \"-Darcadedb.server.logsDirectory=$ARCADEDB_LOG_DIR\""

Code smell: Two constants that must stay in sync

DefaultLogger.DEFAULT_LOG_DIR = "./log" and GlobalConfiguration.SERVER_LOGS_DIRECTORY default "./log" are independent and tied together only by a comment:

// Default must stay in sync with DefaultLogger.DEFAULT_LOG_DIR

This is fragile. The DEFAULT_LOG_DIR fallback is only needed during GlobalConfiguration's own static initialization (when findByKey can't yet return the value). Given how narrow that window is, it might be worth clarifying this specifically in the comment to reduce the chance of someone "fixing" one without the other.

Properties.store() drops the original file's comments

The round-trip in installCustomFormatter():

props.load(in);
// ... mutate one property ...
props.store(buffer, null);
LogManager.getLogManager().readConfiguration(new ByteArrayInputStream(buffer.toByteArray()));

Properties.store() does not preserve comments from the original arcadedb-log.properties. For a JUL config file this is functionally harmless - JUL only reads key/value pairs. But it also adds an auto-generated timestamp comment (#Mon Jun 02 ...) to the byte stream. Neither breaks anything, just worth being aware of.

props.store() serializes a timestamp on every init

As noted above, props.store(OutputStream, String) always prepends a #<timestamp> comment regardless of the null comment argument. This is invisible to JUL but results in a slightly noisier byte stream. Passing an empty string "" instead of null reduces to a single # line - still cosmetic, but cleaner if anyone ever debugs the byte stream.

Test uses reflection to reach a private method

private static String invokeResolveLogDir(final String pattern) throws ReflectiveOperationException {
    final java.lang.reflect.Method m = DefaultLogger.class.getDeclaredMethod("resolveConfigurableLogDir", String.class);
    m.setAccessible(true);
    return (String) m.invoke(null, pattern);
}

This tightly couples the test to the private implementation. The three unit tests that use it (logDirPlaceholderFallsBackToDefaultWhenUnset, logDirPlaceholderResolvesFromSystemProperty, logDirLiteralPatternPassesThroughUnchanged) verify behavior that is already covered end-to-end by fileHandlerPatternResolvesConfigurableLogDir. Consider either promoting resolveConfigurableLogDir to package-private or removing the three redundant unit tests in favour of the integration test, to avoid the reflection coupling.

Documentation file

docs/4451-configurable-log-directory.md is a useful design document, but the project's CLAUDE.md instructs: "NEVER create documentation files (*.md) or README files unless explicitly requested." If this was auto-generated by Claude Code, it should be removed or moved to the PR description (where it clearly belongs given the detailed write-up).

What works well

• The fix is correct and handles the exact bootstrap ordering problem described in the issue.
• Stream leak in installCustomFormatter is fixed (try-with-resources).
• The null guard in resolveConfigurableLogDir is correct.
• GlobalConfigurationTest and LoggerTest cover the key scenarios: fallback to ./log, system-property override, literal passthrough, and end-to-end directory creation + JUL pattern.
• No new dependencies introduced.
• Backward-compatible default: existing single-host and Docker installs are unaffected.
• Windows .bat quoting is correct.

Summary

One actual bug (shell quoting), one fragile coupling, and a couple of minor style concerns. The core logic is solid. Fixing the shell quoting is the only change I'd consider blocking on.

Second-pass dispositions (commit 32ae1f9):

server.sh quoting (flagged blocking) - fixed, but not with the proposed form. The suggested JAVA_OPTS_SCRIPT="$JAVA_OPTS_SCRIPT \"-D...=$ARCADEDB_LOG_DIR\"" does not work in POSIX sh: quote removal does not apply to quotes produced by parameter expansion, and $JAVA_OPTS_SCRIPT is expanded unquoted at exec, so a spaced path still splits and the literal quote characters leak into the argument. Verified:

ARG2=["-Darcadedb.server.logsDirectory=/var/log/my]
ARG3=[app"]

(The .bat form works only because cmd.exe re-parses quotes from expanded variables; POSIX sh does not.) Instead I pass it as a conditional, quoted argument on the exec line:

${ARCADEDB_LOG_DIR:+"-Darcadedb.server.logsDirectory=$ARCADEDB_LOG_DIR"}

Verified: a path with spaces becomes a single argument, and when unset no argument (not even an empty one) is added.

Two constants in sync - comment clarified. Expanded the DEFAULT_LOG_DIR comment to state that it is the fallback used when ${arcadedb.server.logsDirectory} cannot be resolved (e.g. the first log during GlobalConfiguration's static init, before its values are queryable) and must match SERVER_LOGS_DIRECTORY's default. I'm intentionally not deriving one from the other: that would make the resolver's default path query GlobalConfiguration during its own static init, which is the coupling this design avoids. CI pins both literals independently (GlobalConfigurationTest + LoggerTest.logDirPlaceholderFallsBackToDefaultWhenUnset).

Properties.store() comment/timestamp (Low) - no change. Properties.store writes the #<date> line unconditionally. Passing "" does not reduce output - it adds an empty # line before the date line (2 lines), whereas null skips the user-comment line and yields just the date line (1 line). So null is already the minimal form. Dropping the original file comments is harmless since JUL reads only key/value pairs.

Reflection in tests (Low) - keeping. DefaultLogger is in com.arcadedb.log and LoggerTest in com.arcadedb, so package-private would not expose the method - only public would, which over-widens a pure internal helper. The three unit tests are not redundant with fileHandlerPatternResolvesConfigurableLogDir: that integration test covers the system-property override and directory creation, but not the unset→./log fallback nor the literal-passthrough case, which the unit tests assert directly. A rename surfaces as a CI test failure.

docs/4451-configurable-log-directory.md (CLAUDE.md claim) - keeping. The quoted instruction ("NEVER create documentation files...") is not present in this repo's CLAUDE.md (it is a generic Claude Code system-prompt line, not a project rule). The repo's actual convention is the opposite: 38 per-issue docs/NNNN-*.md files are tracked (e.g. docs/4446-configurable-raft-storage-path.md, the direct sibling of this fix). The doc was also part of the implementation plan that was reviewed and approved before this work. Removing it would break the established convention.

Code Review

Summary: A well-targeted fix for a genuine Kubernetes deployment issue. The core approach - intercepting the JUL config stream before it reaches LogManager and resolving placeholders via SystemVariableResolver - is the correct insertion point given the bootstrap ordering problem. The implementation is clean, backward-compatible, and fixes a pre-existing stream leak as a bonus.

Issue 1 (medium) - Undocumented behavior: any unresolved placeholder falls back to ./log

resolveConfigurableLogDir delegates to SystemVariableResolver.resolveSystemVariables(pattern, DEFAULT_LOG_DIR). The second argument is the per-placeholder fallback applied to EVERY unresolved placeholder in the pattern, not just arcadedb.server.logsDirectory.

Concretely: if an operator has a custom arcadedb-log.properties with a non-standard placeholder, before this PR JUL receives the literal string unchanged. After this PR, unrecognized placeholders are silently replaced with ./log. This is arguably better behavior, but the Javadoc on resolveConfigurableLogDir should clarify this so operators with custom properties files understand the post-upgrade semantics.

Issue 2 (low) - Properties.store() drops original file comments

The round-trip through Properties.store() in installCustomFormatter() strips every comment from arcadedb-log.properties before the byte stream reaches LogManager. JUL ignores comment lines anyway so key/value behavior is unaffected today. Properties.store(out, null) also always prepends a timestamp header. Neither breaks anything, but a brief note in installCustomFormatter() would help future maintainers.

Issue 3 (low) - A direct assertion test would catch constant drift more mechanically

DEFAULT_LOG_DIR in DefaultLogger and the default for SERVER_LOGS_DIRECTORY in GlobalConfiguration both hard-code ./log. CI catches drift via two independent test assertions today. A single test directly comparing them would be more self-documenting and fail at the right abstraction level. Suggestion only, not a blocker.

What works well:

• Correct insertion point: resolving in installCustomFormatter() before readConfiguration() is the only place ArcadeDB controls the stream before JUL eagerly opens log files.
• Both code paths agree: createLogDirectoryFromConfig() and installCustomFormatter() both call resolveConfigurableLogDir(), guaranteeing the pre-created directory and FileHandler pattern always match.
• Stream leak fixed: the pre-existing unclosed InputStream in LogManager.readConfiguration is corrected with try-with-resources.
• server.sh quoting is correct: the POSIX conditional expansion handles paths with spaces and adds no argument when ARCADEDB_LOG_DIR is unset.
• server.bat quoting is correct: the -D argument is quoted, handling paths with spaces on Windows.
• Test coverage: the four LoggerTest cases plus GlobalConfigurationTest cover the essential scenarios.
• Backward-compatible default: existing installs with a writable working directory are completely unaffected.

The main actionable item is a Javadoc clarification on resolveConfigurableLogDir noting that any unresolved placeholder falls back to ./log, so operators with custom properties files understand the behavior after upgrading.

Third-pass dispositions (commit aa90535):

Issue 1 (Medium) - Javadoc clarified. resolveConfigurableLogDir's Javadoc now notes that an unresolved placeholder - including a non-standard one in an operator-supplied arcadedb-log.properties - is replaced by ./log, so operators with custom properties files understand the post-upgrade semantics.

One precision note: the ./log fallback is the per-placeholder default in SystemVariableResolver, but VariableParser processes placeholders via lastIndexOf and recurses without the default, so with multiple unresolved placeholders only one would receive ./log (others resolve to empty). The realistic pattern has a single placeholder (${arcadedb.server.logsDirectory}/arcadedb.log), which is exactly the case the Javadoc now describes; I avoided documenting the multi-placeholder recursion quirk since it lives in VariableParser, not this method, and no real config hits it.

Issue 2 (Low) - comment added. installCustomFormatter now notes the Properties round-trip drops the source file's comments and adds a timestamp header, both harmless because JUL reads only key/value pairs.

Issue 3 (Low) - no change. This is explicitly non-blocking and CI already catches drift via the two independent assertions. A single direct comparison would have to read DefaultLogger.DEFAULT_LOG_DIR (private, and in a different package from the test), reintroducing the reflection coupling flagged in an earlier review pass. The existing assertions fail at meaningful abstraction levels: GlobalConfigurationTest if the config default changes, LoggerTest.logDirPlaceholderFallsBackToDefaultWhenUnset if the resolver fallback changes. The cross-reference comments on both constants document the invariant for maintainers.

Code Review: fix(#4451) - Configurable Server Log Directory

Overall: Well-structured fix for a real operational pain point. The approach is sound, backward compatibility is preserved, and the bonus fix of the unclosed-stream leak in installCustomFormatter() is a nice cleanup.

Issues and Suggestions

1. Fragile dual-source-of-truth for the default log dir

GlobalConfiguration.java and DefaultLogger.DEFAULT_LOG_DIR both hardcode ./log with a comment saying they must stay in sync. If either is changed independently, the resolver and the config will disagree silently. Since the PR's own comment explains the bootstrapping constraint (first log may fire before GlobalConfiguration is queryable), this is a real constraint - but it's worth a test that explicitly fails if they diverge:

// In GlobalConfigurationTest or LoggerTest:
assertThat(GlobalConfiguration.SERVER_LOGS_DIRECTORY.getValueAsString())
    .as("DefaultLogger fallback and GlobalConfiguration default must agree")
    .isEqualTo(DEFAULT_LOG_DIR_value_accessed_via_reflection);

Alternatively, have DefaultLogger.DEFAULT_LOG_DIR be the single source and reference it from GlobalConfiguration's default via a static initializer or a shared constants class - though this may introduce a module dependency issue.

2. Reflection to test a private method is a code smell

invokeResolveLogDir() uses reflection to reach into a private static method:

private static String invokeResolveLogDir(final String pattern) throws ReflectiveOperationException {
    final java.lang.reflect.Method m = DefaultLogger.class.getDeclaredMethod("resolveConfigurableLogDir", String.class);
    m.setAccessible(true);
    return (String) m.invoke(null, pattern);
}

Since the method is pure (no side-effects, no access to internal state), making it package-private would be cleaner and remove the need for reflection in tests. The test class is already in the same package structure (com.arcadedb).

3. Shell quoting in server.sh is subtle

${ARCADEDB_LOG_DIR:+"-Darcadedb.server.logsDirectory=$ARCADEDB_LOG_DIR"} \

Within ${...:+word}, the double quotes have their POSIX quoting effect (preventing word-splitting), so this works correctly for paths without spaces. However, if ARCADEDB_LOG_DIR contains spaces, the expansion's quoting protection is implementation-dependent across different shells. The safer pattern for bash would be an array:

ARCADE_EXTRA_OPTS=()
[ -n "$ARCADEDB_LOG_DIR" ] && ARCADE_EXTRA_OPTS+=("-Darcadedb.server.logsDirectory=$ARCADEDB_LOG_DIR")
exec "$JAVA" ... "${ARCADE_EXTRA_OPTS[@]}" ...

This is low risk in practice (Kubernetes mount paths rarely have spaces), but worth noting.

4. Properties round-trip encoding consideration

Properties.load(InputStream) and Properties.store(OutputStream, ...) use ISO-8859-1 encoding by default. If a custom arcadedb-log.properties file uses UTF-8 encoding with non-ASCII characters in property values (e.g., a log path with accented characters on some European locales), the round-trip would mangle those values. The workaround is to use Properties.load(Reader) with an explicit InputStreamReader(in, StandardCharsets.UTF_8) instead. For standard file paths this is unlikely to be hit, but worth considering for robustness.

5. Design doc in the source tree

docs/4451-configurable-log-directory.md is useful context, but the repo's CLAUDE.md guidance says not to create planning/analysis documents unless explicitly requested. If the project doesn't have a convention for issue-specific docs in docs/, this file should probably not be committed - the same information belongs in the PR description (where it already is) and in targeted code comments.

6. Comments explaining what vs. why

The installCustomFormatter() block has a multi-line comment explaining what the Properties round-trip does (// Load the configuration so we can resolve...). Per the project's guidelines, inline comments should explain the non-obvious WHY, not restate the code. The one-liner // resolve ${...} placeholders before JUL opens the file would be sufficient, with the rest derivable from the code.

Positive Notes

• Fixing the unclosed stream leak with try (final InputStream in = stream) is a good bonus improvement.
• The resolveConfigurableLogDir helper is clean and delegates correctly to SystemVariableResolver.
• Test coverage for the three main cases (unset placeholder, system property set, literal path) is solid.
• Backward compatibility is properly preserved (default ./log matches existing behavior).
• The server.bat and server.sh changes are minimal and correct for the common case.

Fourth-pass dispositions (commit 41d7466):

Issues 1 + 2 (drift test + reflection) - both fixed with one structural change. I made resolveConfigurableLogDir and DEFAULT_LOG_DIR package-private and moved the three pure unit tests into a co-located class com.arcadedb.log.DefaultLoggerLogDirTest, so they call the helper directly - no more reflection. (Note: LoggerTest is in com.arcadedb, not com.arcadedb.log, so package-private alone was not enough; co-locating the test is what removes the reflection.) That class also adds the direct drift test you asked for:

assertThat(GlobalConfiguration.SERVER_LOGS_DIRECTORY.getDefValue()).isEqualTo(DefaultLogger.DEFAULT_LOG_DIR);

Using getDefValue() (not getValueAsString()) so it compares the configured default and is immune to runtime overrides. All 19 tests pass (LoggerTest 5, DefaultLoggerLogDirTest 4, GlobalConfigurationTest 10).

Issue 3 (shell array) - declined. server.sh is #!/usr/bin/env sh, not bash. arr=() / "${arr[@]}" are bash-only and fail under dash/ash (the shells in slim/Alpine images), so that suggestion would break the script. The ${VAR:+"..."} form is POSIX-standard - quotes inside the alternative-value word have their normal quoting effect - and is not implementation-dependent. Verified under dash with a spaced path: it produces a single argument [-Dx=/a b/c].

Issue 4 (Properties ISO-8859-1) - declined, not a regression. JUL's own LogManager.readConfiguration(InputStream) reads via Properties.load, i.e. ISO-8859-1 - so the file was already read that way before this PR; nothing changed there. The added store->reload step is lossless even for non-ASCII, because Properties.store emits \uXXXX escapes for non-Latin-1 chars and load reverses them (verified: a café-über value round-trips equal). Switching to a UTF-8 Reader would actually diverge from how JUL itself parses the file.

Issue 5 (design doc) - keeping. As covered in the prior passes: the cited CLAUDE.md instruction is not in this repo's CLAUDE.md, and 38 docs/NNNN-*.md files are tracked (the convention this file follows).

Issue 6 (comment what-vs-why) - keeping. The comment documents the non-obvious WHY (the bootstrap-ordering constraint that forces interception here rather than a direct readConfiguration) plus the round-trip caveat that an earlier review pass explicitly asked to be added. Trimming it to one line would drop exactly that rationale.

Code Review - fix(#4451): configurable server log directory for read-only root filesystems

Overview

This PR addresses a real Kubernetes deployment problem: JUL's FileHandler opens ./log/arcadedb.log relative to the CWD during GlobalConfiguration static initialization, before the server root path is set, which crashes on pods with readOnlyRootFilesystem. The fix intercepts the JUL config stream, resolves ${...} placeholders before feeding it to LogManager, and adds a new GlobalConfiguration.SERVER_LOGS_DIRECTORY key that maps to an ARCADEDB_LOG_DIR env var forwarded by the launch scripts.

The approach is sound and the implementation is clean. A few observations:

Positives

• Bonus fix: The switch to try-with-resources in installCustomFormatter() closes a pre-existing InputStream leak - good catch.
• Defensive test: defaultLogDirMatchesConfiguredDefault() programmatically asserts that DefaultLogger.DEFAULT_LOG_DIR and GlobalConfiguration.SERVER_LOGS_DIRECTORY.getDefValue() agree. This is much better than the inline comment alone and prevents future drift.
• Package-private test access: Placing DefaultLoggerLogDirTest in com.arcadedb.log so it can reach the package-private resolveConfigurableLogDir() and DEFAULT_LOG_DIR directly, without reflection, is a good test design choice.
• Backward compatibility: The default stays ./log, so single-host and standard Docker deployments are unaffected.
• Properties round-trip rationale is sound: JUL only consumes key/value pairs; the timestamp header added by Properties.store() and the loss of comments are harmless.

Issues

1. docs/4451-configurable-log-directory.md should not be committed

This file is an internal implementation document that duplicates the PR description almost verbatim. Design docs like this belong in PRs (where they're already captured), not in the repository. If there's a need for operator-facing documentation, the right place is the existing docs site or a docs/configuration.md that covers all configuration options, not a per-issue file. Consider dropping it.

2. arcadedb.server.logsDirectory placed in the engine module

The key name starts with arcadedb.server. but lives in GlobalConfiguration (engine module). The PR description explains why (logging initializes before the server root path is set), but a future developer grepping for server config in the server module won't find it. A short comment on the entry explaining why this server-prefixed key lives in the engine module would help:

// Placed in engine (not server) because logging initializes before the server root path is
// set. Must stay in sync with DefaultLogger.DEFAULT_LOG_DIR (enforced by DefaultLoggerLogDirTest).
SERVER_LOGS_DIRECTORY("arcadedb.server.logsDirectory", SCOPE.JVM, ...),

3. resolveConfigurableLogDir is package-private without a visible rationale

The method's Javadoc explains what it does but not why it's package-private. A one-liner noting it's accessible to DefaultLoggerLogDirTest would stop a well-meaning future editor from making it private:

/** ... package-private for {@link DefaultLoggerLogDirTest}. */
static String resolveConfigurableLogDir(final String pattern) {

4. server.bat: log dir paths with spaces

if not "%ARCADEDB_LOG_DIR%"=="" set JAVA_OPTS_SCRIPT=%JAVA_OPTS_SCRIPT% "-Darcadedb.server.logsDirectory=%ARCADEDB_LOG_DIR%"

If ARCADEDB_LOG_DIR contains spaces (e.g. C:\Program Files\logs), %ARCADEDB_LOG_DIR% expands bare and the JVM argument will be split. This is a pre-existing pattern in the script (other variables have the same limitation), so this PR doesn't introduce a regression, but it's worth a comment warning operators to avoid spaces in the path on Windows, or wrapping in setlocal enabledelayedexpansion with !ARCADEDB_LOG_DIR! - out of scope for this PR but worth a follow-up issue.

Minor nits

• The comment in installCustomFormatter() explaining the Properties round-trip is more verbose than the project style encourages (CLAUDE.md: prefer no comments unless the why is non-obvious). The round-trip rationale is non-obvious, so a comment is justified - but three block-comment sentences before the props.load(in) call is a bit long. Consider trimming to one line.
• LoggerTest.fileHandlerPatternResolvesConfigurableLogDir has a multi-paragraph Javadoc referencing the issue number. Per project guidelines, issue references belong in PR descriptions, not in source code (they rot as the codebase evolves).

Summary

The fix is correct, the core logic is clean, and the test coverage is thorough. The main ask is dropping docs/4451-configurable-log-directory.md and adding a short comment in GlobalConfiguration.java explaining why a server-prefixed key lives in the engine module. Everything else is minor polish.

Code Review

Overall: A well-targeted fix for a real Kubernetes deployment problem. The approach - resolving placeholders before handing the config stream to JUL - is exactly the right intercept point. Backward compatibility is preserved, the pre-existing unclosed-stream leak is fixed as a bonus, and the core happy-path test coverage is solid.

The docs/ file follows existing project convention

The previous reviewer flagged docs/4451-configurable-log-directory.md as clutter, but the docs/ directory already holds 44 per-issue design documents using the same naming scheme (e.g. 4274-cluster-topology-consistency.md, 4326-polyglot-js-injection.md). Adding 4451-configurable-log-directory.md is consistent with the project established practice - no change needed here.

Bug: stale class name in comment

DefaultLogger.java, line 51: // (asserted by DefaultLoggerLogDirTest).

No such class exists - the test was added to LoggerTest. The comment should read LoggerTest (or simply be dropped, since it would rot again if the test moves). Small but misleading for future readers.

Missing test: explicit fallback to ./log when placeholder is unset

The PR description says 'unset placeholder falls back to ./log' is tested, but the diff adds only one new test method (fileHandlerPatternResolvesConfigurableLogDir), which covers the case where the system property IS set. The fallback path (no system property, no env var, GlobalConfiguration not yet initialised) is not exercised by any visible test. A simple unit test on resolveConfigurableLogDir covering that case would close this gap:

@Test
void resolveConfigurableLogDir_fallsBackToDefaultWhenUnset() {
    System.clearProperty("arcadedb.server.logsDirectory");
    final String result = DefaultLogger.resolveConfigurableLogDir("${arcadedb.server.logsDirectory}/arcadedb.log");
    // GlobalConfiguration.SERVER_LOGS_DIRECTORY default is "./log", so result should be "./log/arcadedb.log"
    assertThat(result).isEqualTo("./log/arcadedb.log");
}

Observation: GlobalConfiguration default silently provides the answer before DEFAULT_LOG_DIR is ever needed

The comment on DEFAULT_LOG_DIR (lines 48-51) is accurate - the constant acts as a safety net for the very narrow window when GlobalConfiguration is still initialising. In normal operation, SystemVariableResolver.resolveVariable walks system property -> env var -> GlobalConfiguration.findByKey, so SERVER_LOGS_DIRECTORY own default of './log' means the placeholder almost always resolves successfully and DEFAULT_LOG_DIR is never reached. The fallback is the right defence-in-depth choice; the docstring on resolveConfigurableLogDir could mention that GlobalConfiguration default is the primary fallback path, with DEFAULT_LOG_DIR covering only the static-init bootstrap window.

Code quality positives

• The try-with-resources refactor in installCustomFormatter closes the pre-existing stream leak correctly.
• The Properties round-trip (load -> rewrite pattern -> store -> ByteArrayInputStream) is sound: Properties.store escapes backslashes, so Windows absolute paths survive; JUL only consumes key/value pairs, so dropped comments are harmless.
• The VariableParser.resolveVariables fallback contract (when listener.resolve returns null, substitute defaultValue and continue recursion) means a partially-formed pattern like ${arcadedb.server.logsDirectory}/arcadedb.log correctly degrades to ./log/arcadedb.log without corrupting the filename suffix.
• The conditional shell expansion ${ARCADEDB_LOG_DIR:+"-Darcadedb.server.logsDirectory=$ARCADEDB_LOG_DIR"} is idiomatic POSIX sh and does protect paths with spaces (the inner quotes are honoured before word-splitting). The comment accurately describes the intent.
• SERVER_LOGS_DIRECTORY placed between SERVER_ROOT_PATH and SERVER_DATABASE_DIRECTORY in GlobalConfiguration is the right logical location.

Summary

Two actionable items before merge:

1. Fix the stale class name in the comment at DefaultLogger.java:51 (DefaultLoggerLogDirTest -> LoggerTest).
2. Add an explicit unit test for the unset-placeholder fallback to ./log to match the claim in the PR description.

Everything else is solid.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.67%. Comparing base (37682c5) to head (41d7466).
⚠️ Report is 4 commits behind head on main.

@@            Coverage Diff             @@
##             main    #4462      +/-   ##
==========================================
+ Coverage   65.25%   65.67%   +0.41%     
==========================================
  Files        1612     1612              
  Lines      124358   124373      +15     
  Branches    26924    26926       +2     
==========================================
+ Hits        81155    81680     +525     
+ Misses      31807    31187     -620     
- Partials    11396    11506     +110     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.