Skip to content

models: parse both shapes of user_failures - #8929

Merged
myleshorton merged 1 commit into
mainfrom
fisk/fix-user-failures-parse
Jul 25, 2026
Merged

models: parse both shapes of user_failures#8929
myleshorton merged 1 commit into
mainfrom
fisk/fix-user-failures-parse

Conversation

@myleshorton

@myleshorton myleshorton commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

What's wrong

lantern-box #286 (b2c28bb, 2026-07-09, first tagged v0.0.101) changed TagHistory.UserFailures from []time.Time to []UserFailure:

// v0.0.100 — adapter/autoselect_history.go:31
UserFailures []time.Time    `json:"user_failures,omitempty"`
// v0.0.101 — adapter/autoselect_history.go:81
UserFailures []UserFailure  `json:"user_failures,omitempty"`   // {"at": ..., "kind": ...}

So user_failures went from ["2026-07-06T12:00:00Z"] to [{"at":"2026-07-06T12:00:00Z","kind":"stall"}]. Go got back-compat for its own reads (UserFailure.UnmarshalJSON accepts both forms) — the Dart consumer didn't, and nothing tests the cross-language contract. available_servers.dart:261 still did DateTime.parse(e as String).

AvailableServers.fromJson maps over the whole list with no per-entry isolation, so one server carrying a recorded user failure throws out of the entire parse and the client is left with no server list at all.

How it reached users

The Dart cast came first and was correct when written — fc84cbe45 (#8833, 2026-06-18) added user_failures parsing three weeks before the Go side changed shape.

What broke it was the dep bump: 6d2c05c8b (#8912, 2026-07-17) moved lantern-box v0.0.100 → v0.0.103, crossing the v0.0.101 boundary. It touched go.mod and go.sum and nothing else — zero Dart files — which is exactly why nobody noticed: the breaking change is invisible in the diff, and no test covers the Go↔Dart contract.

release lantern-box parses?
v9.1.15-beta2 (07-13) v0.0.100
v9.1.16-beta (07-17) v0.0.104
v9.1.17-beta (07-22) v0.0.104

So 9.1.16 is affected too, not just 9.1.17. The ticket only shows 9.1.17 because that user's upgrade history goes 9.1.15 → 9.1.17 — they skipped 9.1.16 entirely.

sequenceDiagram
    autonumber
    participant UI as VPN screen<br/>available_servers_notifier.dart
    participant Svc as platform service<br/>lantern_platform_service.dart
    participant LB as lantern-box v0.0.104<br/>adapter/autoselect_history.go
    participant Model as model<br/>available_servers.dart

    Note over LB: autoselect_history.go:81 ⚠️<br/>UserFailures is []UserFailure<br/>(was []time.Time in v0.0.100)
    UI->>Svc: available_servers_notifier.dart:37<br/>getLanternAvailableServers()
    Svc->>LB: lantern_platform_service.dart:1436<br/>MethodChannel getLanternAvailableServers
    LB-->>Svc: JSON string — user_failures entries<br/>are now objects (at + kind)
    Svc->>Model: lantern_platform_service.dart:1439<br/>AvailableServers.fromJson(jsonDecode(result))
    Model->>Model: available_servers.dart:6<br/>map over all 55 servers, no per-entry isolation
    Model->>Model: available_servers.dart:138<br/>SelectionHistory.fromJson(selection_history)
    rect rgba(255, 200, 200, 0.3)
        Note over Model: available_servers.dart:261 🐛<br/>DateTime.parse(e as String)<br/>the entry is a Map, not a String
    end
    Model-->>Svc: throws — _Map is not a subtype of String in type cast
    Svc-->>UI: lantern_platform_service.dart:1446<br/>Left(Failure) — all 55 servers lost
    Note over UI: available_servers_notifier.dart:46<br/>logs and returns, so state stays stale<br/>(build() at :25 throws outright)
Loading

Evidence

Freshdesk #180591 — China, Android, 9.1.17. From that user's flutter.log:

  • 9.1.17 first appears at 11:28:11.225; the first parse failure is at 11:29:43.93792 seconds later, as soon as radiance recorded its first user-traffic failure.
  • 339 occurrences of type '_Map<String, dynamic>' is not a subtype of type 'String' in type cast, from 11:29:43 through 16:35:40, with the stack trace naming available_servers.dart:261.
  • 2329 Servers JSON: [ lines, all well-formed — the payload was fine, only the parse wasn't.

Downstream: 130 updateServerLocation: type=auto, 58 "falling back to auto", ~23 churned tags. The UI can't learn which servers exist, so it keeps re-resolving against a stale list.

Why this escaped testing: user_failures is omitempty, so a fresh upgrade parses fine; and defaultUserFailureWindow is 5 minutes, so the window empties again after five failure-free minutes. On a clean network the bug is invisible. In China it never clears.

Reproduction

The model has no imports beyond dart:core, so it runs standalone against the payload the ticket's log captured verbatim (55 servers, 2 carrying user_failures):

A  v0.0.104 shape (as shipped in 9.1.17)
    THREW — type '_Map<String, dynamic>' is not a subtype of type 'String' in type cast
B  v0.0.100 shape (list of timestamp strings)
    OK — parsed 55 servers, 33 locations
C  no user_failures recorded yet
    OK — parsed 55 servers, 33 locations

Mutating only user_failures flips the outcome, which is what makes the dependency bump causal rather than correlated. One poisoned server out of 55 is enough.

The fix

Accept both shapes, and drop entries we can't read instead of throwing:

static DateTime? _userFailureAt(dynamic entry) {
  final at = entry is Map ? entry["at"] : entry;
  return at is String ? DateTime.tryParse(at) : null;
}

entry is Map rather than Map<String, dynamic> on purpose — this has to hold if the payload ever arrives via StandardMessageCodec rather than jsonDecode, which yields Map<Object?, Object?>.

Dropping rather than throwing is the load-bearing half. userFailures has no readers anywhere in lib/ — it is parsed, defaulted, and re-serialized, nothing else. A field nobody consumes should not be able to cost the client its server list, whatever shape a future lantern-box gives it.

Tests

This model had zero fromJson coverage before now. Seven new tests; five fail without the fix:

test fails pre-fix
parses the object form
parses an object form with no kind
parses a window holding both forms at once
drops entries with no readable timestamp
parses a list where only one server carries user_failures
parses the legacy bare-timestamp form — (guards against over-correcting)
an absent window parses as empty — (guards the omitempty path)

The list-level test round-trips its fixture through jsonEncode/jsonDecode so element types match the platform channel's, and pre-fix it reproduces the ticket's error string exactly: type '_Map<String, dynamic>' is not a subtype of type 'String' in type cast.

Deliberately out of scope

  • Per-entry isolation in AvailableServers.fromJson:6. One bad server still takes down the list for any other parse failure. Worth fixing, but it's a behavior change (partial lists) that deserves its own review.
  • last_outcome_at / updated_at are never null. Go's omitempty doesn't apply to structs, so a zero time.Time marshals as 0001-01-01T00:00:00Z. The json[...] == null ? null : ... guards are dead code and toJson's if (lastOutcomeAt != null) never trips.
  • A test that actually pins the Go↔Dart contract. Nothing here catches the next shape change on the Go side; that needs a fixture generated from lantern-box.

Note

test/widget_test.dart ("Counter increments smoke test") fails on origin/main as well — verified by stashing these two files and re-running. Pre-existing, untouched here.

🤖 Generated with Claude Code

lantern-box v0.0.104 changed each `user_failures` entry from a bare
RFC3339 timestamp to a `{"at": ..., "kind": ...}` object. Go got
back-compat for its own reads (`UserFailure.UnmarshalJSON`); Dart still
cast every entry to String, so the first entry radiance recorded threw
out of `SelectionHistory.fromJson`. `AvailableServers.fromJson` maps
over the whole list with no per-entry isolation, so one poisoned server
cost the client every server it had — Freshdesk #180591 shows a China
Android user on 9.1.17 hitting this 339 times over five hours, 92
seconds after upgrading.

Accept both shapes and drop entries we can't read instead of throwing:
`user_failures` has no readers on this side, so an unparseable timestamp
should never be able to take down the server list.

Also adds the first `fromJson` coverage this model has had. Five of the
seven new tests fail without the fix, one of them reproducing the
verbatim runtime error from the ticket.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 25, 2026 17:29
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

User failure deserialization

Layer / File(s) Summary
Tolerant user failure parser
lib/core/models/available_servers.dart
SelectionHistory accepts legacy timestamp strings and object entries with an at field, drops unreadable entries, and documents its serialized format.
Deserialization coverage
test/core/models/available_servers_test.dart
Tests cover supported formats, mixed and missing windows, invalid timestamps, and server-list parsing when only one server includes failures.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: copilot, atavism

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: supporting both legacy and new user_failures formats in the models.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fisk/fix-user-failures-parse

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

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

Copilot AI 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.

Pull request overview

Updates the Flutter/Dart AvailableServers model layer to be backward-compatible with a lantern-box JSON schema change where selection_history.user_failures entries changed from timestamp strings to {at, kind} objects, preventing a single malformed/unknown entry from breaking parsing of the entire server list.

Changes:

  • Make SelectionHistory.fromJson accept both legacy string entries and the new object form for user_failures, dropping unreadable entries instead of throwing.
  • Keep toJson() emitting the legacy “bare timestamp” shape for user_failures.
  • Add focused unit tests covering both shapes, mixed windows, invalid entries, and a multi-server payload where only one server has user_failures.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/core/models/available_servers.dart Adds tolerant parsing for both user_failures shapes and filters invalid entries to avoid poisoning full server-list parsing.
test/core/models/available_servers_test.dart Adds regression/unit tests for user_failures parsing across legacy/new/mixed/invalid cases and multi-server payload behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/core/models/available_servers_test.dart
@myleshorton
myleshorton merged commit 6fc4c97 into main Jul 25, 2026
10 checks passed
@myleshorton
myleshorton deleted the fisk/fix-user-failures-parse branch July 25, 2026 17:45
myleshorton added a commit that referenced this pull request Jul 25, 2026
)

The comments introduced in #8929 cite v0.0.104 as the release that
changed each `user_failures` entry from a bare RFC3339 string to a
{"at", "kind"} object. That was the edge of the module cache I sampled,
not the actual boundary. Verified against the tags:

  v0.0.99, v0.0.100    UserFailures []time.Time
  v0.0.101 .. v0.0.106 UserFailures []UserFailure

The change landed in lantern-box #286 (b2c28bb), first tagged v0.0.101.
These comments exist to point a future reader at the version where the
shape changed, so the wrong number defeats their only purpose.

Comments only — no behavior change.

Co-authored-by: Adam Fisk <a@lantern.io>
Co-authored-by: Claude <noreply@anthropic.com>
myleshorton added a commit that referenced this pull request Jul 26, 2026
Nothing ran `flutter test test/` on a PR. The only two references to
`flutter test` live in build-linux.yml and build-windows.yml, both
workflow_call-only and both scoped to integration_test/ — so the unit
suite never executed in CI at all.

That is part of why the user_failures deserialization break (#8929)
reached three releases: a unit test would have caught it, and a unit
test now pins it, but on `pull_request` nothing would have run it.

flutter test needs no Go toolchain, Android SDK, or Java, so this gate
is just checkout + flutter + pubget/gen + test.

Deletes test/widget_test.dart, which had to go first: it is the stock
`flutter create` counter template with `pumpWidget` commented out, so it
asserts against an empty widget tree and fails unconditionally. It was
never adapted to this app and tests nothing.

Co-authored-by: Adam Fisk <a@lantern.io>
Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants