Skip to content

feature: FileLogHandler docs + FileSystem auto-init + .rotating recovery#548

Merged
xerial merged 4 commits into
mainfrom
chore/remove-legacy-log-rotation-handler
May 15, 2026
Merged

feature: FileLogHandler docs + FileSystem auto-init + .rotating recovery#548
xerial merged 4 commits into
mainfrom
chore/remove-legacy-log-rotation-handler

Conversation

@xerial

@xerial xerial commented May 15, 2026

Copy link
Copy Markdown
Member

Bundles four related changes that all sit under "make rotated file logging usable in uni":

Summary

  • Remove legacy LogRotationHandler (JVM-only, dead code) so cross-platform FileLogHandler is the single canonical handler. FileHandler users migrate to FileLogHandlerConfig(...).noRotation.
  • Document FileLogHandler in docs/core/logging.md: defaults (daily + 100 MB rotation, gzip, 100-file retention), the withXxx builder, .noCompression / .noRotation escape hatches.
  • Auto-init FileSystem / IOWatch on first touch so callers (FileLogHandler, IO.readAsString, app code) no longer need an explicit FileSystemInit.init(). Lazy first-touch in impl() rather than the object body, per Gemini's reentrancy concern. Drops the now-redundant call from IO.scala.
  • .rotating temp-file recovery in FileLogHandler.init() so a crash mid-rotation no longer leaves dangling log data: if logPath is absent we restore the orphan as the active log, otherwise we archive it under {stem}-recovered-{ts}.log.

Test plan

  • ./sbt scalafmtAll compile — clean across JVM / Scala.js / Scala Native
  • ./sbt uniJVM/testOnly wvlet.uni.log.* wvlet.uni.io.* wvlet.uni.control.* — 237/237 pass, including the new IOReadAsStringJvmTest chain (proves auto-init works without explicit FileSystemInit.init() via IO) and the two new FileLogHandlerTest recovery cases
  • ./sbt uniJS/testOnly ... + ./sbt uniNative/testOnly ... — focused runs green on JS and Native, incl. new FileSystemAutoInitTest and the two recovery tests
  • pnpm docs:build — docs site builds cleanly

Review

  • Addressed all four Gemini items (high-priority circular-init concern, noCompression doc wording, plan scope, .rotating durability gap) in commit de910a6.

🤖 Generated with Claude Code

The cross-platform FileLogHandler (PR #393) already covers daily +
size-based rotation, gzip compression, and cleanup on JVM, Scala.js,
and Scala Native. The earlier JVM-only LogRotationHandler (and its
FileHandler no-rotation wrapper) had no callers outside its own file,
no tests, and no docs — keeping it just gives users two answers to
"how do I rotate logs in uni?" Delete it so FileLogHandler stands as
the single canonical handler.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the internal Non-user facing changes label May 15, 2026
@xerial

xerial commented May 15, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the legacy JVM-only LogRotationHandler and its FileHandler wrapper, as they are redundant with the existing cross-platform FileLogHandler. A new plan document has been added to the plans/ directory to document the context, decision, and scope of this removal. I have no feedback to provide as there were no review comments to evaluate.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the legacy JVM-only LogRotationHandler and FileHandler, as they are redundant with the cross-platform FileLogHandler. Feedback indicates that this removal may cause a regression in log durability because the legacy implementation included logic to recover orphaned temporary files after a crash, a feature currently missing from the replacement handler.

## Scope

- Remove `uni-core/.jvm/src/main/scala/wvlet/uni/log/LogRotationHandler.scala` (contains both `LogRotationHandler` and its `FileHandler` wrapper).
- No replacements needed — `FileLogHandler` already covers both rotation and no-rotation use cases (via `.noRotation`).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The assertion that FileLogHandler fully covers the use cases of the deleted LogRotationHandler overlooks the orphaned file recovery logic present in the legacy implementation (LogRotationHandler.recoverTempFiles, lines 259-274). In FileLogHandler.rotate, a .rotating temporary file is created during the compression phase. If the process crashes before this file is compressed and deleted, the log data remains in the .rotating file and is not automatically recovered or appended to upon restart. The legacy handler included a mechanism to scan for and restore these temporary files during initialization. To avoid a regression in log durability, a similar recovery mechanism should be implemented in FileLogHandler.

Add a "Writing Logs to a File" section to docs/core/logging.md covering
the cross-platform FileLogHandler that uni already ships: default
behavior (daily + 100 MB size rotation, gzip, 100-file retention), the
withXxx / noCompression / noRotation builders on
FileLogHandlerConfig, and the FileSystemInit.init() startup step on
Scala.js and Scala Native. Surfaces a capability that was already in
the code but undiscoverable from the reference docs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@xerial xerial changed the title chore: Remove legacy JVM-only LogRotationHandler chore: Remove legacy LogRotationHandler + document FileLogHandler May 15, 2026
@xerial

xerial commented May 15, 2026

Copy link
Copy Markdown
Member Author

Added a Writing Logs to a File section in docs/core/logging.md covering FileLogHandler / FileLogHandlerConfig defaults, the builder/escape-hatch surface, and the FileSystemInit.init() startup step on Scala.js and Scala Native. The cross-platform handler was already in the code (PR #393) but undocumented — this surfaces it. Verified with pnpm docs:build. Re-running review since the diff grew: /gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the legacy JVM-only LogRotationHandler and FileHandler classes, consolidating logging functionality into the cross-platform FileLogHandler. It adds a design plan for this removal and updates the documentation to include usage examples and configuration details for FileLogHandler. Review feedback suggests clarifying that the noCompression option preserves the configured file extension rather than just the default .log extension. Additionally, the plan document should be updated to include documentation changes within its scope, as they were implemented in this PR.

Comment thread docs/core/logging.md Outdated
| `withFormatter(f)` | Override the log formatter. Default `AppLogFormatter`. |
| `withLogFileExt(ext)` | Override the active-log extension (default `".log"`). |
| `withCompressRotated(b)` | Toggle gzip of rotated files. Default `true`. |
| `noCompression` | Disable gzip; rotated files keep their `.log` extension. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The description for noCompression states that rotated files keep their .log extension. However, if a custom extension is configured via withLogFileExt, the rotated files will use that extension instead. It would be more accurate to say they keep their configured extension.

Suggested change
| `noCompression` | Disable gzip; rotated files keep their `.log` extension. |
| noCompression | Disable gzip; rotated files keep their configured extension (default: .log). |

## Out of scope

- Any new feature in `FileLogHandler`.
- Documentation site changes — the docs don't currently mention either handler.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This pull request includes documentation for FileLogHandler in docs/core/logging.md. This item should be moved to the Scope section to accurately reflect the work performed in this PR.

Move the platform-specific FileSystemInit.init() call into the shared
FileSystem and IOWatch companion objects so cross-platform callers
(FileLogHandler, IO.readAsString, app code) no longer have to
bootstrap the platform implementation explicitly. The pattern matches
Logger ↔ LogEnv: FileSystemInit lives in the .jvm / .js / .native
source sets and writes back via setImplementation as part of its own
static initializer; JVM reentrant class-init makes the back-call safe
and Scala.js / Native preserve the same semantics at link time.

- Drop the now-redundant FileSystemInit.init() from IO.scala.
- Add FileSystemAutoInitTest exercising FileSystem.tempDirectory
  without any explicit init step (matches IOReadAsStringJvmTest's
  intent, but at the FileSystem layer directly).
- Simplify the new "Writing Logs to a File" section in
  docs/core/logging.md: the FileSystemInit.init() startup step and the
  cross-platform tip block are no longer needed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@xerial xerial changed the title chore: Remove legacy LogRotationHandler + document FileLogHandler feature: Auto-init FileSystem + cross-platform log rotation docs May 15, 2026
@xerial

xerial commented May 15, 2026

Copy link
Copy Markdown
Member Author

Bundled the FileSystem auto-init follow-up here per your request. Now: FileSystem and IOWatch shared objects each call FileSystemInit.init() in their own body, so any first touch (FileSystem.exists, FileLogHandler, IO.readAsString, IOWatch.watch) self-bootstraps the platform impl. The same pattern Logger ↔ LogEnv already uses. Verified by a new FileSystemAutoInitTest that deliberately omits FileSystemInit.init() and runs green on JVM, JS, and Native, plus the now-existing-as-regression IOReadAsStringJvmTest which previously relied on IO calling init() (call removed in this commit). Docs caveat in core/logging.md dropped accordingly.

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces the legacy JVM-only LogRotationHandler with the cross-platform FileLogHandler, adds detailed documentation for its usage, and introduces automatic platform-specific initialization for FileSystem and IOWatch. Reviewers identified a potential circular dependency risk in the FileSystem static initializer and suggested using lazy initialization instead. Additionally, feedback recommended refining the documentation for noCompression to account for custom file extensions and updating the project plan to correctly reflect that documentation was included in this PR.

// Touch FileSystemInit so the platform-specific implementation registers itself the first
// time anyone uses FileSystem. FileSystemInit lives in .jvm / .js / .native source sets and
// writes back via setImplementation as part of its own static initializer.
FileSystemInit.init()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Calling FileSystemInit.init() during the static initialization of the FileSystem object carries a risk of circular dependency or IllegalStateException. If the platform-specific implementation's constructor or static initializer attempts to use FileSystem methods (like exists or currentDirectory) before setImplementation is called, it will trigger the impl check and throw an exception. A more robust approach would be to perform this initialization lazily inside the impl method when _impl is first found to be null, ensuring that init() is only called when needed and after the object is partially initialized.

Comment thread docs/core/logging.md Outdated
| `withFormatter(f)` | Override the log formatter. Default `AppLogFormatter`. |
| `withLogFileExt(ext)` | Override the active-log extension (default `".log"`). |
| `withCompressRotated(b)` | Toggle gzip of rotated files. Default `true`. |
| `noCompression` | Disable gzip; rotated files keep their `.log` extension. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The description for noCompression states that rotated files keep their .log extension. However, FileLogHandlerConfig allows customizing the extension via withLogFileExt. It would be more accurate to say they keep their original extension (e.g., .log or .txt).

Suggested change
| `noCompression` | Disable gzip; rotated files keep their `.log` extension. |
| `noCompression` | Disable gzip; rotated files keep their original extension. |

## Out of scope

- Any new feature in `FileLogHandler`.
- Documentation site changes — the docs don't currently mention either handler.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This line states that documentation changes are out of scope and that the docs don't currently mention either handler. However, this PR actually adds comprehensive documentation for FileLogHandler in docs/core/logging.md. This section should be updated to reflect that documentation is included in the scope of this PR.

- [HIGH] FileSystem/IOWatch auto-init: switch from object-body call to
  lazy first-touch in impl() to remove the theoretical reentrancy risk
  a platform impl whose static initializer touches FileSystem would
  trip on. Init still happens exactly once; the second null check
  guards the (impossible) case where init() returns without registering.

- [MED] FileLogHandler.init() now recovers `.rotating` temp files left
  by a previous run that crashed mid-rotation. If logPath is absent we
  restore the temp as the active log; otherwise we archive it under
  `{stem}-recovered-{ts}.log` so the orphaned log window is preserved
  for review rather than silently lost. Covered by two new tests in
  FileLogHandlerTest (active-log absent, active-log present).

- [MED] docs/core/logging: `noCompression` row now says rotated files
  keep their *configured* extension (default `.log`), not literally
  `.log` — withLogFileExt customizes the suffix.

- [MED] plans/...-remove-legacy-log-rotation-handler.md: docs,
  auto-init, and .rotating recovery moved out of Out-of-scope into
  Scope to match the work that actually landed in this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@xerial

xerial commented May 15, 2026

Copy link
Copy Markdown
Member Author

Addressed all four Gemini items in de910a6:

[HIGH] Circular dependency in FileSystem static init. Moved FileSystemInit.init() from the object body into a lazy first-touch inside impl() on both FileSystem and IOWatch. A platform impl that referenced FileSystem mid-bootstrap would no longer trip the null check — init runs only when actually needed.

[MED] .rotating durability gap. FileLogHandler.init() now scans for a .{logPath.fileName}.rotating orphan: if logPath is missing we restore the temp as the active log; otherwise we archive it under {stem}-recovered-{ts}.log. Two new tests (recover .rotating temp file when active log is missing and archive .rotating temp file when active log already exists) cover both paths and pass on JVM/JS/Native. Note: the legacy LogRotationHandler.recoverTempFiles looked for .tmp files that no codepath ever created, so this is actually an improvement over the dropped handler rather than a regression patch.

[MED] noCompression doc wording. Now reads "rotated files keep their configured extension (default .log)".

[MED] Plan scope. Docs, auto-init, and .rotating recovery moved into the Scope section.

/gemini review

@xerial xerial changed the title feature: Auto-init FileSystem + cross-platform log rotation docs feature: FileLogHandler docs + FileSystem auto-init + .rotating recovery May 15, 2026
@xerial xerial merged commit 4f08e7b into main May 15, 2026
14 checks passed
@xerial xerial deleted the chore/remove-legacy-log-rotation-handler branch May 15, 2026 18:21

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces the legacy JVM-only LogRotationHandler with the cross-platform FileLogHandler, updates the logging documentation, and implements lazy self-bootstrapping for FileSystem and IOWatch. Additionally, it introduces a recovery mechanism for .rotating temporary files in FileLogHandler to ensure log data is preserved after a crash. Review feedback highlights that the lazy initialization of FileSystem and IOWatch is not thread-safe and recommends using synchronized blocks to prevent redundant initialization calls.

Comment on lines 482 to +497
private def impl: FileSystemBase =
if _impl == null then
throw IllegalStateException(
"FileSystem not initialized. Platform-specific initialization required."
)
_impl
val current = _impl
if current != null then
current
else
// First touch: trigger the platform-specific FileSystemInit, which writes back via
// setImplementation as part of its own static initializer. Lazy (rather than in the
// object body) so a partly-initialized platform object can't observe FileSystem
// mid-bootstrap.
FileSystemInit.init()
val after = _impl
if after == null then
throw IllegalStateException(
"FileSystem not initialized. Platform-specific initialization required."
)
after

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The lazy initialization of _impl is not thread-safe. While _impl is volatile, multiple threads could simultaneously enter the else block and call FileSystemInit.init() if they all observe _impl as null at startup. Although FileSystemInit.init() might be idempotent, it is better to use a synchronized block to ensure initialization happens exactly once and to avoid redundant calls to the platform-specific bootstrap logic.

  private def impl: FileSystemBase =
    val current = _impl
    if current != null then
      current
    else
      this.synchronized {
        if _impl == null then
          // First touch: trigger the platform-specific FileSystemInit, which writes back via
          // setImplementation as part of its own static initializer. Lazy (rather than in the
          // object body) so a partly-initialized platform object can't observe FileSystem
          // mid-bootstrap.
          FileSystemInit.init()
      }
      val after = _impl
      if after == null then
        throw IllegalStateException(
          "FileSystem not initialized. Platform-specific initialization required."
        )
      after

Comment on lines 101 to +114
private def impl: IOWatchBase =
if _impl == null then
throw IllegalStateException(
"IOWatch not initialized. Platform-specific initialization required."
)
_impl
val current = _impl
if current != null then
current
else
// FileSystemInit also registers the platform IOWatch implementation, so triggering it
// here lets IOWatch.watch self-bootstrap even when FileSystem hasn't been touched yet.
FileSystemInit.init()
val after = _impl
if after == null then
throw IllegalStateException(
"IOWatch not initialized. Platform-specific initialization required."
)
after

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to FileSystem.impl, this lazy initialization lacks synchronization. Multiple concurrent calls to IOWatch.watch on a fresh JVM could trigger multiple calls to FileSystemInit.init(). Adding a synchronized block ensures thread-safe initialization.

  private def impl: IOWatchBase =
    val current = _impl
    if current != null then
      current
    else
      this.synchronized {
        if _impl == null then
          // FileSystemInit also registers the platform IOWatch implementation, so triggering it
          // here lets IOWatch.watch self-bootstrap even when FileSystem hasn't been touched yet.
          FileSystemInit.init()
      }
      val after = _impl
      if after == null then
        throw IllegalStateException(
          "IOWatch not initialized. Platform-specific initialization required."
        )
      after

xerial added a commit that referenced this pull request May 15, 2026
Resolves conflict in uni/src/main/scala/wvlet/uni/control/IO.scala:
- main (#548): removed FileSystemInit.init() in favor of auto-init
- this branch: removed IOPath() wrap since FileSystem.readString
  now accepts String directly
- Result: neither import is needed; keep only wvlet.uni.io.FileSystem.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Non-user facing changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant