Skip to content

Review fixes for proxy robustness, connection UX and performance - #33

Merged
andre487 merged 15 commits into
mainfrom
review/global-code-review
Sep 8, 2026
Merged

Review fixes for proxy robustness, connection UX and performance#33
andre487 merged 15 commits into
mainfrom
review/global-code-review

Conversation

@andre487

@andre487 andre487 commented Sep 8, 2026

Copy link
Copy Markdown
Owner

This review fixes failures in proxy connection isolation, Android trust prompts, cancellation, configuration validation, and build portability.

Findings and fixes

  • External SSH trust prompts: the exported launcher accepted arbitrary host-key challenge extras. Notification restoration now goes through a non-exported activity; MainActivity no longer consumes those extras. The activity immediately returns to the existing navigation host.
  • Shared sessions interrupted by one failed destination: SSH channel rejection/cancellation no longer closes the shared session. Failures from an old SSH client cannot invalidate its replacement. HTTP/2 CONNECT failures preserve a usable shared session; unsupported CONNECT still uses the existing HTTP/1.1 fallback.
  • Hanging diagnostics: cancellation closes the underlying connection during manual HTTP request/response I/O, and diagnostic response headers are limited to 64 KiB.
  • HTTP/2 cancellation cleanup: an unbuffered result handoff ensures a late CONNECT response is closed after its caller has left.
  • IP literal bootstrap: numeric proxy addresses bypass DNS rather than being queried as hostnames; hostname resolution rejects a missing socket protector.
  • Native startup cleanup: stack creation failure closes the proxy session, and a superseded start also closes its TUN device.
  • Stop/reconnect race: deferred reconnects check that the service is alive and connection is still desired; Stop clears a pending reconnect request.
  • Diagnostic storage failures: background log I/O errors no longer escape the executor and terminate the VPN process. Programming errors still propagate.
  • Configuration mismatch: Android JA3 and custom DoH validation now enforce the native constraints; imported SSH profiles without a port use 22.
  • Toolchain portability: shared JDK 21 discovery replaces hard-coded Homebrew paths in release scripts and also validates native builds. English/Russian Fastlane references are updated.

Connection UX review

  • Allow Stop/Cancel while configuration writes are pending or failed; only starting a VPN depends on saved valid settings.
  • A quick rejection of VPN consent is reported as denial, not falsely diagnosed as another Always-on VPN.
  • Reopening an in-flight diagnostic preserves its state/log. Terminal diagnostic state updates cannot be dropped under log pressure.
  • Consolidate three SSH confirmation implementations into the navigation destination. Save keys off the main thread, retain pending/error state across rotation, allow retry after failure, use test-specific button labels, and handle Back and externally cleared challenges without an empty screen.
  • Preserve the actual failover profile on resume. Test a running VPN with its runtime configuration rather than validating an unrelated selected profile draft.
  • Compute traffic rates using monotonic elapsed sample time instead of assuming every poll took exactly one second.

Performance review

  • Remove full profile decryption from editor recompositions, connection-ID lookups, and repeated settings-list rendering. Refresh editor global settings off the main thread after pending writes finish.
  • Reuse the Android Keystore key handle within each ConfigStore instead of looking it up for each encrypted field. Stored ciphertext and encryption algorithms are unchanged.
  • Read the diagnostic tail only when the logger revision changes, and split up to 512 KiB of text on the I/O dispatcher. Writes, rotation/limit enforcement, clear and crash logging advance the revision.
  • Replace the nested imported-profile scan with an ID map, making the merge linear while preserving local order and untouched entries. A 1000-profile regression test covers partial replacement and additions.
  • Precompile the log sanitizer's newline pattern.

These changes remove repeated work visible in the source; no FPS, battery or launch-time improvement is claimed without device profiling. First-screen configuration loading and the ordered per-edit persistence queue remain profiling targets; write coalescing needs separate ordering/retry validation.

Go crash, memory and error review

  • Bound native JSON configuration at 1 MiB, fallback provider count at 16, DNS query size at 65535 bytes and bootstrap hostnames at 253 bytes. Validate SSH keepalive/rotation values before duration conversion to avoid ticker panics. Reject a missing protector at the connection-test bridge.
  • Acquire DoH admission before copying a packet. Drop and report replies when the bounded UDP reply queue is full instead of blocking every shared provider slot. Implement read/write deadlines for already-waiting operations and stop their timers on close.
  • Limit HTTP/2 response headers to 64 KiB. Generation checks prevent obsolete deadline callbacks from closing a stream after its deadline has been cleared/replaced. Closed dialers reject late HTTP/2 sessions and new SSH sessions.
  • Keep cancelled SSH opens charged against the channel limit until the actual worker finishes; x/crypto DialContext cancellation alone does not stop the underlying channel-open wait. Transport close releases the blocked worker.
  • Recognize wrapped timeout errors. Log keepalive failures and invalidate the matching SSH session.
  • Add malformed-input fuzz seeds and regressions for resource limits, full reply queues, changing deadlines, cancelled SSH workers and closed dialers. Add the documented native_fuzz Fastlane lane for a bounded 20-second, two-worker campaign; its seeds run in the normal native tests.

Native race tests pass, and the fuzz campaign executed 244,214 inputs without a crash. Android/JNI builds, JVM checks and lint passed during this review; the final SSH close guard was then rechecked with native race tests. This is bounded validation, not proof that OOM is impossible. Cleanup Close errors remain best-effort rather than masking the primary failure or forcing a panic.

JVM crash and memory review

  • Guard external JSON before recursive parsing: at most 32 nesting levels, 250,000 structural tokens and the existing file-size ceiling. Oversized/deep/wide input now produces a localized error before allocating a full tree. Nonstandard comments and single-quoted JSON are rejected so Android parser extensions cannot bypass the guard.
  • Do not turn VM errors into empty/default profiles or credentials. Configuration parsing, decryption and writes now catch Exceptions while propagating cancellation and Errors; write queue bookkeeping runs in finally.
  • Preserve the last diagnostic snapshot and show a localized retry notice after storage/access failures, instead of letting the viewer coroutine crash the process.
  • Replace the nullable retry-status assertion with a local snapshot. Handle background VPN command failures with a visible connection warning and a diagnostic event.
  • Use generated, typed JNI callback implementations with normal Object contracts. Keep TUN ownership explicit across the call and stop a started core if subsequent status delivery fails (see boundary review below).
  • Close an unpublished TUN and started native core in finally on failed/superseded starts. An IOException during TUN close no longer escapes normal service teardown.

bundle exec fastlane android android_checks passes: 96 JVM tests (10 new), lint, debug APK and verified unsigned release APK. Tests cover deep/wide JSON, size limits, escaped string delimiters, lenient-syntax rejection, storage failure/recovery, fatal-error propagation and JNI callback Object contracts. No real heap-exhaustion or device lifecycle stress run was performed.

Remaining memory risks: the ordered per-edit configuration queue retains snapshots without a bound, and repeated imports can grow the total stored profile set beyond a single import limit. Addressing these requires an explicit persistence/backpressure policy and compatibility handling for existing configurations; this change does not claim to eliminate every OOM scenario.

JVM/native boundary review

  • Replace reflective Mobile calls and dynamic interface proxies with direct gomobile types. The generated AAR is now a compile-time dependency, so changes to Go signatures fail Android compilation. Inspect the actual AAR signatures and its keep rules; rebuild all four Android ABIs.
  • Native Start borrows the Java-owned descriptor for the duration of the call and duplicates it with CLOEXEC on entry. Java closes its own temporary duplicate with use; Go closes only its own descriptor on all ordinary failure paths. This removes the ambiguous transfer before JNI entry and its potential leak on invocation failure.
  • Pass the actual Android TUN MTU explicitly to Go. Both sides now use 1400 instead of Android using 1400 and the native endpoint hard-coding 1500; validate the native MTU argument.
  • Bound Long-to-Int descriptor conversion in Protector. Ordinary callback exceptions are handled on the JVM side and logged by error class; socket protection fails closed. Reporter failures do not escape into a JNI callback without a Go error result. Fatal VM errors are not disguised as successful callbacks.
  • Gate callbacks when the core is stopped or its service/start generation is obsolete.
  • Reject native Start while Stop is still cleaning up. Release upstream sessions before waiting for the old stack; a concurrent second Stop cannot clear the first cleanup guard.
  • Replace the global tunnel proxy with a rejecting implementation on Stop and failed startup. The global singleton no longer retains the old dialer, credentials and JVM service callbacks indefinitely.

bundle exec fastlane android test passes: native race tests, 104 JVM tests, lint, debug APK and verified unsigned release APK. Regressions cover borrowed-FD survival, duplicate CLOEXEC, overlapping Start/Stop, global dialer release, protection failures/range checks, stale callbacks and reporter recovery. Actual JNI execution, GC timing and rapid lifecycle transitions on an Android device remain untested locally.

Full-diff regression review

Re-reviewed the complete PR against main, including interactions between the earlier review fixes. Corrected these regressions and gaps:

  • Wrong SSH trust destination after failover: diagnostics used the runtime configuration but the currently selected profile ID for key prompts. The service now publishes a single profile/config snapshot and uses its profile ID through temporary startup and diagnostics.
  • Repeated approval of the same runtime SSH key: a fresh diagnostic reused the old pin from the live VPN snapshot. It now picks up persisted trust for the same profile and endpoint while keeping the actual runtime credentials/settings; trust from an edited endpoint or another profile is not substituted.
  • Permanent SSH admission exhaustion: keeping cancelled channel opens charged indefinitely could prevent all future requests if the peer never replied. A worker gets 30 seconds after caller cancellation to finish; otherwise the matching stalled session is invalidated. Promptly completed cancellations retain the shared session. Recycling a stalled session can terminate its other channels, but avoids leaving the connection permanently unusable.
  • Incorrect DoH error after Close: closing both the socket and deadline signal let select return a random timeout. Deadline wakeups now prioritize net.ErrClosed when the socket has closed.
  • Incomplete JSON complexity guard: reject Android's semicolon/equals extensions outside strings, which otherwise bypassed structural-token counting.

Final local validation: bundle exec fastlane android test passes native race tests, 104 JVM tests, lint, debug APK and verified unsigned release APK. bundle exec fastlane android python_checks passes 31 tests and Black/isort. Added regression coverage for failover trust identity, newly approved pins versus edited endpoints, stalled SSH worker recovery and deterministic closed-socket errors. git diff --check passes. Android device lifecycle/visual smoke testing remains outstanding; no device execution is claimed.

Compose interaction tests on the JVM

  • Add Robolectric 4.16, Compose UI Test dependencies and Android resources for local unit tests, with JDK 21 module access. All 36 interaction tests run in the existing Android Fastlane/PR check without an emulator, ADB, KVM or device farm.
  • Exercise main-screen connect/reconnect/stop, VPN consent approval/denial, Always-on conflicts, invalid configurations, disabled actions while connecting, and profile selection.
  • Cover draft creation, editor persistence and port validation, SSH/HTTPS Jump, certificate-bypass confirmation, cloning, deletion with active-profile fallback and last-profile protection, file import/error handling, and password-free JSON export.
  • Verify traffic units, TLS fingerprint selection, failover confirmation, selected-app routing, all settings destinations and Back, profile/diagnostic navigation, and externally cleared SSH prompts.
  • Exercise diagnostic running/success/failure states, exit IP, log copying and clear confirmation. SSH trust tests cover successful save, pending-save action/Back blocking, save failure/retry, and diagnostic cancellation.
  • Use real screens and ConfigStore with a test-only in-memory Keystore provider. Robolectric intercepts service commands and supplies permission/document-picker results; the connection statistics reader and SSH save operations are injectable. No real VPN forwarding, Go JNI, device Keystore or network requests run in these tests.
  • Pin Android API 35 and English resources with a plain test Application. Document coverage and extension conventions in both Fastlane guides.

Latest local validation: bundle exec fastlane android android_checks passes 140 JVM tests, including all 36 Robolectric Compose tests, lint, debug APK and verified unsigned release APK. This covers UI interactions and persistence; screenshot comparisons and physical-device lifecycle checks remain outside this suite.

Test quality review

  • Make Compose persistence checks fail on completed-but-failed writes, and always clean up the test Keystore. Drain the ordered I/O executor before positive/negative service-command assertions; reject unexpected or duplicate commands and verify desired state after Stop/denied consent.
  • Add six ConfigStore integration tests with real serialization/crypto and synthetic keys: credential reopening/encryption, fresh IVs, preserving omitted imported secrets, clearing explicitly empty secrets, deletion/failover reference cleanup, reconnect token ordering, and jump trust isolation.
  • Add three queue regressions: pending Retry deduplication, deletion before the initial write, and independent-profile failure retention.
  • Record the scenario audit in docs/reviews/test-quality.md. Main remaining gaps are service lifecycle/recovery, activity recreation during transfers, real storage/key failures, UI configuration variants and the native HTTP/2 failure matrix. Intent-recording UI tests do not cover actual VPN lifecycle execution.

Latest validation: 149 JVM tests, lint, debug and unsigned release builds pass via android_checks; native race tests and all 31 Python tests/Black/isort pass through Fastlane. No line-coverage percentage or device execution is claimed.

Full CI reruns including skipped checks

The launcher now labels its full rerun option explicitly as including skipped checks. When Change scope executes on GitHub run attempt 2 or later, it enables Android/Compose, native and Python suites without diff/history filtering. Initial PR runs retain normal change filtering; pushes to main always run all suites. Failed-only reruns retain prior scope unless Change scope itself must rerun. This also works with GitHub's Re-run all jobs button; old workflow runs retain their original definition.

Add regression tests for forced scope/output/summary without Git or history lookup, initial docs-only filtering, and full reruns of successful runs with skipped jobs. All 34 Python tests and Black/isort pass via Fastlane; workflow YAML and git diff --check pass. Both Fastlane references and project memory document the behavior.

Complete CI coverage on main

Every push to main enables Android/Compose UI, Go and Python checks without consulting change history or the diff, including documentation-only updates. Initial PR runs retain selective checks. Pin the README badge and its link to main push runs, and update both Fastlane guides and project memory.

A regression verifies that the initial push attempt enables every suite even with identical base/head commits and without Git/history lookup. All 35 Python tests and Black/isort pass through Fastlane; git diff --check passes. This behavior takes effect in main when the PR is merged.

Documentation consistency review

  • Align README/native build instructions with Gradle 8.11.1, Go 1.26.3, portable JDK 21 discovery, pinned gomobile tooling and Fastlane. Correct borrowed TUN descriptor ownership, remove an obsolete release-tag example, and distinguish optional local emulators from JVM UI tests.
  • Complete the English/Russian Fastlane command tables and update UI navigation instructions, language conventions, traffic/diagnostic features and HTTPS Jump store descriptions.
  • Correct privacy disclosures to describe local encrypted credentials, proxy authentication, direct bootstrap DNS and all diagnostic IP/country providers. Clarify voluntary report sharing and deletion of exported copies. Update foreground-service documentation for reconnect/update recovery and Android-controlled Always-on sessions.
  • Keep historical changelogs intact and identify the test review as a dated snapshot. Describe the universal APK as intended for reproducible F-Droid verification rather than claiming external verification of every release.

Validation: local Markdown link targets exist; all nine public Fastlane lanes appear in each of the three command references; toolchain versions and diagnostic-provider lists match source; published v0.0.12 asset names match download links; git diff --check passes. Documentation-only changes do not require rerunning application tests. Store text/policy changes are repository updates, not publication to Play Console.

Focused privacy policy review

Revise the policy around data categories, purpose, recipients and deletion. Remove unnecessary cipher/TLS details and link to README for the diagnostic endpoint inventory. Add local installed-app/routing processing, source-IP exposure during bootstrap DNS, voluntary report contents, clipboard/share handoff and support email handling. State the developer-confirmed retention practice: support correspondence and attachments are kept until the reported issue is fixed, then deleted.

Record source evidence and Google Play references in docs/reviews/privacy-policy.md. The policy is now linked at the bottom of Settings. Play Console/Data safety and in-app VPN disclosure still need separate verification. These documentation changes do not claim store compliance or modify consent UI. Source correspondence, local links and git diff --check were checked; no application code changed.

Privacy policy access in Settings

Add a full-width text link at the bottom of Settings, after support actions, with English/Russian labels. It opens the public policy on main in a browser and shows the existing localized error if opening fails. Update the privacy review to mark the missing in-app link as addressed.

Review scope and validation

Final source review of head 3b3b406 against main 4acca42 found no additional confirmed regressions in the PR diff. Local validation passed: native race tests, 149 JVM tests (36 Compose interactions), lint, debug/unsigned release builds, 35 Python tests and Black/isort. No source changes were needed. Device lifecycle/network-handover coverage and unbounded configuration queue/profile-set growth remain the previously documented limitations; this review does not establish device-level certification.

Reviewed native HTTPS/SSH/Jump connection paths, bootstrap and DoH handling, native startup, Android VPN lifecycle, intent exposure, configuration import/persistence and editor paths, logging, and CI/launcher/release tooling. This is a source review with deterministic checks, not certification of all device/network scenarios.

  • Native tests with the race detector passed through bundle exec fastlane android test. The latest bundle exec fastlane android android_checks passes 149 JVM tests, lint, debug APK and verified unsigned release APK.
  • bundle exec fastlane android python_checks: 31 tests and pinned Black/isort checks pass.
  • Shell syntax and git diff --check pass. Regression tests exercise session isolation, HTTP cancellation, IP literals, validation, log I/O failure and JDK selection. A manifest contract keeps the trust-review activity private.
  • No emulator/device was connected locally. Notification delivery, rapid connect/reconnect/stop, and actual network handovers still need device smoke testing. Signed release workflows were not run.

The SSH error distinction follows the OpenChannelError contract.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

APK artifacts

Built from commit 3b3b4064 by CI run #134.
Artifacts expire after 14 days. Neither APK uses the MegaProxy release key.

@andre487 andre487 changed the title Fix proxy lifecycle, SSH trust prompts and configuration validation Fix proxy lifecycle, trust prompts and connection UX Sep 8, 2026
@andre487 andre487 changed the title Fix proxy lifecycle, trust prompts and connection UX Review fixes for proxy robustness, connection UX and performance Sep 8, 2026
@andre487
andre487 enabled auto-merge (squash) September 8, 2026 15:14
@andre487
andre487 merged commit 105fb7d into main Sep 8, 2026
4 checks passed
@andre487
andre487 deleted the review/global-code-review branch September 8, 2026 15:16
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