Skip to content

Fix the crash on the first logged line of a session - #308

Merged
sproctor merged 1 commit into
mainfrom
worktree-logging-before-settings
Aug 29, 2026
Merged

Fix the crash on the first logged line of a session#308
sproctor merged 1 commit into
mainfrom
worktree-logging-before-settings

Conversation

@sproctor

@sproctor sproctor commented Aug 28, 2026

Copy link
Copy Markdown
Owner

Fixes the production crash IllegalStateException: Attempted to print log before settings were loaded (DESKTOP-3W, desktop@3.1.0).

Root cause

LoggingRepository declared its settings flow before the map that flow uses:

private val loggingSettings = ...onEach { loggers.forEach { ... } }
    .stateIn(externalScope, SharingStarted.Eagerly, null)

private val loggers = mutableMapOf<String, FileLogger?>()   // assigned second

stateIn with SharingStarted.Eagerly launches with CoroutineStart.DEFAULT — dispatched, not started in place. The sharing coroutine ran on an IO thread while the constructor was still executing, and its first act (clearing open loggers on the initial emission) dereferenced loggers while it was still null. The NPE killed the sharing coroutine for good, so loggingSettings.value stayed at its null seed forever and every line the game logged afterwards hit error("Attempted to print log before settings were loaded"), taking the client down.

That is why it lands at login rather than at startup: WraythClient buffers log lines until the <app> event names the character, then replays them (WraythClient.kt:403:1080), so the first real call into the repository is the one that kills the connection.

The fix

  • Declare loggers before loggingSettings, so it is assigned before anything is launched — that also gives a real happens-before edge rather than just luckier ordering.
  • Seed stateIn with clientSettingRepository.getLogSettings() instead of null. The config store is a MutableStateFlow with a default, so there is always a value to read; a line arriving ahead of the first emission now logs under the current settings instead of throwing. The error(...) is gone — a logging hiccup should never be fatal.
  • Added ClientSettingRepository.getLogSettings(), sharing the mapping with observeLogSettings().

Verification

LoggingRepositoryTest covers both halves. Reverting just LoggingRepository.kt reproduces the original failures exactly:

sharing coroutine failed: java.lang.NullPointerException: Cannot invoke "java.util.Map.entrySet()" because "$this$forEach$iv" is null
java.lang.IllegalStateException: Attempted to print log before settings were loaded

Both pass with the fix, and ./gradlew check -PiosSkip=true -PlintSkip=true is green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WWiQAWnC8ZUoqQvwhrfvYS

Summary by CodeRabbit

  • Bug Fixes

    • Improved logging initialization so logging works reliably before settings are fully loaded.
    • Prevented startup failures when logging settings begin sharing during application construction.
    • Ensured reactive and synchronous access use consistent log-settings values.
  • Tests

    • Added coverage for early logging and logging initialization scenarios.

LoggingRepository shares its settings into a StateFlow with stateIn, and
CoroutineStart.DEFAULT means that sharing coroutine is dispatched, not started
in place. It ran on an IO thread while the constructor was still assigning
properties, and its first act - clearing the map of open loggers, which was
declared after the settings flow - dereferenced a map that was still null. The
NPE killed the coroutine, so the settings were never published, and every line
the game logged after that hit `error("Attempted to print log before settings
were loaded")` and took the client down. The reports come in on the first real
log call of a session: WraythClient buffers lines until <app> names the
character, then replays them, so the whole connection dies at login.

Declare the map before the flow so it exists before anything is launched, and
seed stateIn with the settings as they stand instead of null - the config store
is a StateFlow with a default, so there is always a value to read - so a line
arriving ahead of the first emission logs under the current settings rather
than throwing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WWiQAWnC8ZUoqQvwhrfvYS
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d10de05-5179-4006-a259-22ee90998dfb

📥 Commits

Reviewing files that changed from the base of the PR and between 952d447 and be66c60.

📒 Files selected for processing (3)
  • core/src/commonMain/kotlin/warlockfe/warlock3/core/prefs/repositories/ClientSettingRepository.kt
  • core/src/commonMain/kotlin/warlockfe/warlock3/core/prefs/repositories/LoggingRepository.kt
  • core/src/jvmTest/kotlin/warlockfe/warlock3/core/prefs/repositories/LoggingRepositoryTest.kt

Included review availability: 5 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour.


📝 Walkthrough

Walkthrough

The change centralizes client log-settings conversion and initializes LoggingRepository with synchronous settings. New JVM tests cover logging before coroutine dispatch and construction with Dispatchers.Unconfined.

Changes

Logging initialization

Layer / File(s) Summary
Shared log-settings conversion
core/src/commonMain/kotlin/warlockfe/warlock3/core/prefs/repositories/ClientSettingRepository.kt
observeLogSettings() and getLogSettings() use shared conversion logic with path fallback, safe LogType parsing, and timestamp settings.
Logging repository initialization and validation
core/src/commonMain/kotlin/warlockfe/warlock3/core/prefs/repositories/LoggingRepository.kt, core/src/jvmTest/kotlin/warlockfe/warlock3/core/prefs/repositories/LoggingRepositoryTest.kt
LoggingRepository initializes logger state before the settings-sharing coroutine and uses current settings as the flow’s initial value. Tests cover undispatched logging and unconfined construction.
Estimated code review effort: 3 (Moderate) ~20 minutes

Merge Risk: ⚪ Minimal · up to be66c

The PR makes a localized logging initialization change and adds regression coverage; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: fixing the crash that occurred on the first logged line of a session.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-logging-before-settings

Comment @coderabbitai help to get the list of available commands.

@sproctor
sproctor merged commit e41c82d into main Aug 29, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant