Skip to content

Batch access/usage DB writes, debounce network reloads, back off WG restarts - #596

Merged
kasnder merged 4 commits into
gotatun-migrationfrom
battery-fixes-1-4
Jul 7, 2026
Merged

Batch access/usage DB writes, debounce network reloads, back off WG restarts#596
kasnder merged 4 commits into
gotatun-migrationfrom
battery-fixes-1-4

Conversation

@kasnder

@kasnder kasnder commented Jul 7, 2026

Copy link
Copy Markdown
Member

Summary

Implements the four highest-ROI fixes from a battery review of the VPN service, WireGuard egress, and database layer:

  • Batch updateAccess/updateUsage (DatabaseHelper.java): mirrors the existing insertLog batching — repeated writes to the same access row within a 2s/50-entry window coalesce into one transaction (last-write-wins for state, summed deltas for usage), and updateUsage's old SELECT-then-UPDATE collapses into a single UPDATE ... SET sent = COALESCE(sent,0) + ?. Access-table readers/writers flush pending batches first for read-your-writes consistency; onDestroy flushes on shutdown so nothing is lost.
    • Scoped down from the original review: insertDns is intentionally left unbatched, since its return value synchronously gates prepareUidIPFilters (firewall rule enforcement for newly resolved trackers) — batching it would introduce a correctness-relevant delay.
  • Serialize + back off WireGuard recovery (WgEgress.kt): network-change rebinds now run through a single-thread executor with dedup instead of spawning an unbounded raw Thread per event. The broken→full-restart loop now backs off exponentially (20s → 40s → 80s → ... capped at 5 min) instead of retrying every ~20s forever on a captive portal or blocked UDP path. Backoff resets on any confirmed fresh handshake.
  • Debounce network reloads (ServiceSinkhole.java): bursts of ConnectivityManager callbacks (common during Wi-Fi↔cellular handoffs) now coalesce into a single reload 1.5s after the burst settles, instead of restarting the VPN/WireGuard per callback. Cancelled cleanly on service destroy.
  • Bounded wakelock: onStartCommand's wakelock now has a 30s timeout as a safety net against a hung or throwing command path holding it indefinitely.

Test plan

  • ./gradlew compileFdroidDebugJavaWithJavac and compileFdroidDebugKotlin — clean
  • ./gradlew testFdroidDebugUnitTest — full suite passes
  • Added DatabaseHelperAccessBatchTest covering coalescing (last-write-wins), usage accumulation, and distinct-key isolation within a batch
  • Not yet exercised on a real device — recommend a manual pass (network flapping, WireGuard recovery on a blocked/captive network, and traffic-heavy app usage) before merging, especially given the timing-sensitive WireGuard recovery changes

🤖 Generated with Claude Code

…estarts

Addresses the top four battery findings from the review:

- DatabaseHelper.updateAccess/updateUsage now batch like insertLog already
  did, coalescing repeated writes to the same (uid, version, protocol,
  daddr, dport) row into one transaction, and updateUsage collapses the old
  SELECT-then-UPDATE round trip into a single UPDATE with COALESCE.
- ServiceSinkhole.reloadAfterNetworkChange debounces bursts of
  ConnectivityManager callbacks into a single reload instead of restarting
  the VPN/WireGuard per callback.
- WgEgress serializes network-change rebinds through a single-thread
  executor with dedup instead of spawning an unbounded raw Thread per
  event, and adds exponential backoff to the broken-tunnel full-restart
  loop so a captive portal/blocked UDP path doesn't retry every ~20s
  forever.
- The onStartCommand wakelock now has a bounded timeout as a safety net
  against a hung or throwing command path holding it indefinitely.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kasnder

kasnder commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

For context: this PR only covers fixes 1-4 from the battery review. Not addressed here, in case they're worth follow-up PRs:

  • blockKnownTracker read-lock scopedh.getQAName(uid, daddr, true) still runs a DB cursor loop while holding the service's read lock on every tracker-IP cache miss, blocking writers during that query.
  • System.gc() after every command (ServiceSinkhole.java) — forces a stop-the-world GC on every start/reload/watchdog/householding; safe to remove and let ART schedule collection itself.
  • Missing index on access.timegetInsightsData7Days/getRecentTrackerActivity filter on time with no supporting index, so they run full/partial table scans; the table also has no periodic pruning (only log and dns get cleaned up). Landing an index here trades a small write-side cost (each access upsert) for query wins on the Insights/Timeline screens.
  • WgConnectivityMonitor's fixed 1s poll interval — not urgent (no wakelock is held, so it doesn't block doze), but could go adaptive (e.g. 5-15s when healthy/idle) for a modest win.

Also worth noting, since it didn't fit clean batching: insertDns has the same per-call transaction overhead as updateAccess/updateUsage did, but its return value synchronously drives prepareUidIPFilters (rebuilding firewall IP filters), so batching it risks delaying enforcement for freshly resolved trackers. Left as-is; would need a design that preserves the synchronous "is this new" signal while deferring only the persisted write.

kasnder and others added 2 commits July 7, 2026 15:10
flushUsageBatch() could match zero rows (silently dropping sent/received
deltas) when its access row was still sitting unflushed in accessBatch;
it now flushes access first and logs on a zero-row update again.

requestFullRestart()'s backed-off restart runnable (up to 5 min out) was
never cancelled on disable/stop/config-change, risking a stale reload
firing later. It's now a stored Runnable removed in clearRecoveryState(),
guarded by a restartScheduled flag so callers that never scheduled one
don't force verifyHandler's lazy init.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- WgEgress: coalesce (not drop) network-change rebinds via in-flight +
  dirty flags, so a change mid-rebind re-runs on the current network
  instead of leaving sockets bound to a departed one.
- WgEgress: reset restart backoff on a confirmed-fresh handshake even
  when concurrent network events bump verificationGeneration, so a later
  breakage isn't stuck behind a stale (up to 5 min) backoff.
- WgEgress: guard the delayed restart runnable on forceRestartPending so
  a restart already handled by another recovery path doesn't fire a
  redundant restart of a healthy tunnel.
- DatabaseHelper: schedule a bounded time-based flush for the tail of an
  access/usage batch so a lone late update isn't invisible until the next
  reader or shutdown (and bounds unclean-kill loss to the flush window).
- DatabaseHelper: preserve a specified block across last-write-wins
  coalescing when a later same-key update omits it; add regression test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kasnder

kasnder commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Code-review fixes + emulator smoke test (through a483b59)

Applied the findings from a review of this branch, plus one deferred battery item, and ran an emulator pass.

Fixes pushed

Five review findings, all in the new batching / WG-recovery mechanics:

  • Rebind events are now coalesced, not dropped. The old rebindQueued skip meant a second network change during an in-flight rebind was discarded, potentially leaving sockets bound to a network that had already gone away. Replaced with in-flight + dirty flags so the executor re-runs once on the current network.
  • Restart backoff resets on a confirmed-fresh handshake even under generation churn. A concurrent onUnderlyingNetworkChanged (which bumps verificationGeneration) could suppress the restartAttempts = 0 reset, leaving the next transient breakage stuck behind a stale backoff of up to 5 min. The reset now happens ahead of the generation guard, gated only on a genuinely fresh handshake.
  • The delayed (backed-off) restart no longer fires redundantly. pendingRestartRunnable now checks forceRestartPending first, so a restart already handled by another recovery path (e.g. a network-change reload) does not kick a needless restart of a healthy tunnel.
  • Access/usage batches get a bounded time-based flush. Previously the tail of a burst stayed invisible (no notifyAccessChanged) until the next reader or shutdown, and was lost outright on an unclean kill. The first entry of an idle batch now schedules a flush on the DB handler thread, bounding both staleness and the loss window to ~2s.
  • A specified block value is preserved across last-write-wins coalescing when a later same-key update omits it (latent today, since production always passes block = -1). Added a regression test.

Plus the deferred item from the battery review:

  • Removed the forced System.gc() after every command (start/reload/stop/watchdog/householding). Let ART schedule collection itself; the explicit call only added latency/jank on the command path.

Build/unit verification: Java + Kotlin compile clean and the full testFdroidDebugUnitTest suite passes (including the new block-preservation test).

Emulator smoke test (Small_Phone AVD, Android 16)

Built and installed the full fdroidDebug APK (Rust wgbridge native lib for all 4 ABIs included) and exercised it live:

  • ✅ App installs & launches, libnetguard.so loads, VPN service starts and establishes tun0 (both WG mode MTU 1420 and plain-filter MTU 10000 observed).
  • ✅ Fail-closed WG init did not block startup — service ran, "WireGuard — Connected" in settings.
  • Live capture → DB write pipeline works under the changed code: 70 dns rows + 263 app rows written by the running service; resolved tracker domains (doubleclick.net, graph.facebook.com, app-measurement.com, region1.google-analytics.com) landed in dns.
  • WgConnectivityMonitor is live and handled a real cellular→WiFi transition without crashing.
  • ✅ Service ran through many command/reload cycles (exercising the System.gc() removal + reload debounce).
  • Full-session crash/ANR/native-fault/ACRA scan came back empty — zero crashes.

Honest limitations of the emulator pass:

  • The batched updateAccess path did not write live access rows: that call is gated on isTracker and a data packet whose IP reverse-resolves to a tracker hostname, and updateUsage needs track_usage (off by default). am start-driven Chrome doesn't reliably complete that. The gating lives in unchanged service code; the batching/coalescing/flush/block-preservation logic itself is covered deterministically by the unit test.
  • The WG recovery changes (the first three fixes) were the least exercised — WG egress was only briefly the active path; most of the session ran plain NetGuard filtering, where the rebind/backoff code is dormant. The emulator does not substitute for the on-device WG-recovery pass.

Still outstanding before merge

  • Manual on-device pass is still required and remains the real validation for the WG changes — network-flapping / captive-portal / blocked-UDP scenarios against this updated branch.
  • The other follow-up items remain out of scope (documented in the PR discussion): blockKnownTracker read-lock scope, the missing access.time index plus access-table pruning, WgConnectivityMonitor's fixed 1s poll, and the un-batched insertDns transaction overhead.

handleMessage forced a stop-the-world GC on every start/reload/stop/
watchdog/householding command. Drop it and let ART schedule collection
itself — the explicit call only added latency and jank on the command
path with no benefit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kasnder
kasnder merged commit 1df18b4 into gotatun-migration Jul 7, 2026
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