fix(database): reduce write-lane pressure from one-shot reads

[jeremiah-k]

Overview

This reduces unnecessary database coordination for bounded, one-shot reads without weakening database switching, writer admission, eviction, or shutdown safety.

Routine point queries previously entered the same DatabaseManager.withDb path used for coordinated writes. That serialized them through the temporary write-containment lane and refreshed last-used metadata for every operation, even though these reads do not mutate state.

This introduces a dedicated bounded-read path while preserving the existing coordination rules for writes, Flow and Paging subscriptions, database switching, association, and lifecycle-sensitive operations.

Builds on: #6256
Related: #6259

Changes

• Added DatabaseProvider.withReadDb for bounded, non-replaying reads.
• Kept bounded reads outside writer admission and the serialized write-containment lane.
• Registered each read before capturing the published database.
• Tracked readers against the exact captured database instance.
• Prevented shutdown from closing pools beneath admitted read callbacks.
• Prevented LRU eviction from closing a switched-away pool while a reader or writer still owns it.
• Retried deferred eviction after the final database owner releases the pool.
• Routed device-hardware, firmware-release, node-info, and device-link point queries through the bounded-read path.
• Kept Flow and Paging consumers attached to currentDb so they continue to re-subscribe after a database switch.
• Removed per-operation last-used writes from the common access path while preserving updates at meaningful lifecycle boundaries.
• Preserved read exceptions rather than silently substituting empty, false, null, or zero fallback values.

Validation

Added coverage for:

• bounded reads without writer admission;
• database capture across active-database switches;
• no automatic replay after read failures;
• cancellation-safe reader release;
• shutdown waiting for an admitted bounded read;
• rejection of new reads once shutdown begins;
• eviction deferral for a switched-away database with an active reader;
• deferred eviction after the reader completes;
• unchanged Flow and Paging relatching behavior.

Hardware validation switched between firmware 2.8 over TCP and firmware 2.7 over BLE in one process. Both configuration downloads completed, and the active database followed the selected device.

Summary by CodeRabbit

• Improvements
  • Improved database read handling during switching, shutdown, and cache eviction using bounded read tracking.
  • Reads now complete against a consistent database instance without automatic retry/replay on failures.
  • Local data sources now execute queries in read-only mode and return results directly.
• Documentation
  • Clarified read/write access policy and shutdown behavior in database provider docs.
• Tests
  • Added coverage for bounded reads, cleanup/eviction coordination, cancellation, cache-limit eviction selection, and read failures.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5a13f955-31a8-47cc-87db-07a4b7b71b02

📥 Commits

Reviewing files that changed from the base of the PR and between 413cedd and d018f3c.

📒 Files selected for processing (2)

• core/database/src/commonTest/kotlin/org/meshtastic/core/database/DatabaseManagerReadTest.kt
• core/database/src/commonTest/kotlin/org/meshtastic/core/database/DatabaseManagerTestFixture.kt

---

📝 Walkthrough

Walkthrough

DatabaseManager now provides tracked bounded-read access that coordinates database pool switching, eviction, and shutdown. Database-backed data sources use withReadDb directly, removing nullable fallback conversions. Test and fake providers implement the new read contract, with coverage for lifecycle and failure behavior.

Changes

Database read path

Layer / File(s)	Summary
Bounded read manager semantics core/database/.../DatabaseProvider.kt, core/database/.../DatabaseManager.kt	withReadDb tracks active readers, preserves the captured database during reads, defers eviction and shutdown until access drains, and documents read/write admission rules.
Datasource read migration core/data/.../datasource/*.kt	Device hardware, device link, firmware release, and node information queries use withReadDb and no longer convert nullable results to empty, false, or zero defaults.
Provider implementations and read validation core/testing/.../FakeDatabaseProvider.kt, core/database/.../commonTest/...	Fake and test providers implement withReadDb; fixtures and tests cover pool capture, shutdown waiting, eviction deferral, cancellation, concurrency, and failure propagation.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DataSource
  participant DatabaseManager
  participant DatabasePool
  participant Cleanup
  DataSource->>DatabaseManager: withReadDb DAO query
  DatabaseManager->>DatabasePool: capture and execute read
  Cleanup->>DatabaseManager: request eviction or close
  DatabaseManager-->>Cleanup: defer while reader is active
  DatabaseManager->>DatabaseManager: endRead
  DatabaseManager-->>Cleanup: complete deferred cleanup

Loading

Possibly related PRs

• meshtastic/Meshtastic-Android#6096: Extends DatabaseManager with database association and active-database switching logic related to this read-path change.

Suggested labels: bugfix

🚥 Pre-merge checks | ✅ 4

✅ 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 accurately summarizes the main change: introducing bounded reads to reduce write-lane pressure.
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.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[jamesarich]

Nice find on the per-operation markLastUsed — that was a DataStore disk write plus a coroutine launch on every DB op, and removing it from the hot path is a real win. The removal is also safe for eviction correctness: enforceCacheLimit excludes the active DB by name, and switchActiveDatabase re-stamps both the new and previous DB, so LRU ordering stays accurate without per-op stamps.

One sequencing concern before this merges, though: this PR is only safe on top of #6256, and #6256 is still in draft.

The reads moved from withDb to currentDb.value.dao() lose their registration in activeWriters. On current main, the associateDevice merge path calls drainWriters(source) and then retireDatabase(sourceName), which physically closes and deletes the source DB. A raw currentDb.value read is invisible to that drain, so a read that captures the source instance around the swap can still be running when retirement closes it — the exact "Connection pool is closed" failure class this series is eliminating. Two things widen the window:

1. The moved reads (device-hardware and firmware-release lookups) fire at connect time — precisely when associateDevice merges fire.
2. currentDb is a derived flow (_currentDb.filterNotNull().stateIn(...)), so currentDb.value can briefly lag the publish and hand out the stale source instance even after the swap.

#6256's logical retirement (never physically closing during process lifetime) removes the hazard entirely. Could we either land #6256 first and rebase this on top, or merge them together? If there's a reason to take this one alone, it would at least be worth noting the dependency in the PR body.

[jeremiah-k]

@jamesarich Thanks — agreed on the sequencing concern.

The direct currentDb reads intentionally stop participating in writer admission, so this should not land while main still physically closes a merged source database. I’ll treat #6255 as dependent on #6256 rather than adding a second reader-drain mechanism here, which would duplicate the lifecycle work and broaden this otherwise small PR.

I’ll return this to draft, update the description to state that dependency, and rebase it onto main after #6256 lands. The per-operation markLastUsed removal remains independently valid, but the complete PR will merge after the lifecycle hardening and rerun CI on that base.

[jamesarich]

Reviewed the evolved read path in depth — withReadDb is a solid design and it resolves everything I raised on the original version of this PR:

• Readers register in activeReaders atomically under the same lock the eviction filter holds, so eviction defers a pool with an in-flight read instead of closing it (and endRead retries the eviction when the last accessor leaves) — the stale-instance-vs-eviction window is closed.
• Reads count as manager operations, so close() drains them before closing pools.
• With currentDb now the raw MutableStateFlow, the derived-flow lag I flagged is gone entirely.
• Reads skipping the writer gate is the right trade — documented, and torn reads during association are acceptable for these point-in-time queries.
• The markLastUsed removal stays safe: current-DB exemption plus switch-time stamps preserve LRU ordering.

No defects found. Four test gaps worth covering while the machinery is fresh — DatabaseManagerReadTest covers stale-capture, shutdown, eviction-defer, and failure propagation, but not:

1. a read concurrent with an association/merge (gate armed) — proving reads aren't blocked and survive source retirement;
2. reader cancellation releasing the refcount and any deferred eviction;
3. concurrent readers on one pool (refcount > 1);
4. an assert that a victim can't acquire a new reader after the eviction filter has passed it (the lock ordering argues it's impossible — worth pinning with a test).

None of these block merge for me — happy to see this land after them, or with them as an immediate follow-up.

[jeremiah-k]

@jamesarich Agreed—this is a good time to add the coverage while the concurrency behavior is fresh. I’ve added all four cases you identified; they’re now included in the updated branch.