refactor: clean-architecture + perf pass on evaluation hot path by Devild007 · Pull Request #10 · Fluent-Health/featbit-android-sdk

Devild007 · 2026-06-25T17:47:02Z

Summary

Three-phase refactor + hardening on refactor/clean-architecture (23 commits since main):

Phase 1 — Hardening (8 commits, prior work): race-safe identify, bounded insight
pipeline, centralized endpoints, plus aggressive test pinning for LifecycleController,
PollingDataSynchronizer, StreamingDataSynchronizer, FlagTrackerImpl, DefaultMemoryStore,
TrackInsight, and FBClient identify+close contracts.

Phase 2 — Clean architecture (7 commits): layered package split with public API
FQNs unchanged.

domain/ — pure: Evaluator, EvalResult, ValueConverters, MemoryStore (port)
app/ — services: DefaultMemoryStore (adapter), FlagTrackerImpl, LifecycleController
data/{http,sync,insights}/ — outbound adapters
wire/ — kotlinx-serialization DTOs (Insight, EndUser)
Public API at co.featbit.client.* (FBClient, FBClientImpl, FBUser, etc.) unchanged
Two documented carve-outs (architecture.md): model.FeatureFlag carries @Serializable
(domain knows wire format); MemoryStore stays public for domain-port boundary even
though no consumer-facing injection point exists today

Phase 3 — Perf pass (5 commits): allocation + lock-contention reductions on the
evaluation hot path. Zero product behavior change.

perf: skip alloc on evaluation hot path — evaluateValue fast-path (no EvalDetail
alloc on *Variation()), insightsEnabled short-circuit when tracker is no-op,
FBUser.endUser cache (immutable wire form, one alloc per identify not per eval),
allFlags() LinkedHashMap rewrite (one pass not two).
perf: bulk upsert on store to halve write-lock contention — MemoryStore.upsertAll
default + DefaultMemoryStore override; polling/streaming now take the store's
writeLock once per batch instead of N times.
perf: single-pass JSON, cached serializer, alloc-free bool — typed LatestAllEnvelope
in GetUserFlags (eliminates AST allocation), cached ListSerializer in
HttpTrackInsight companion, equals(ignoreCase=true) for bool conversion.
fix: address audit findings on perf pass — 6 reviewer findings addressed
({"data": null} no-longer-silent, fast-path insight emission tests with mutation
verification, -Xjvm-default=all-compatibility for MemoryStore binary-compat,
defensive unmodifiableList on cached EndUser, listener-throw doc, MockWebServer race
fix on insight test).
chore: opt in to ExperimentalSerializationApi for explicitNulls — acknowledges
load-bearing dependency on the experimental decoding contract.

Numbers

111 unit tests / 0 fail / 0 err (+13 from main: 3 perf + 1 null-data MEDIUM + 2 fast-path
- others backfilled through hardening rounds)
assembleDebug × 3 modules green
3 adversarial audit rounds on the perf pass — final = ZERO FINDINGS

Test plan

./gradlew :featbit-client:testDebugUnitTest — 111/0/0
./gradlew assembleDebug — all 3 modules
E2E via Colima Docker (FEATBIT_E2E=1) — FeatBitE2ETest (polling + identify)
+ FeatBitStreamingE2ETest (WS push) green against live FeatBit stack
NOT YET TESTED: Android emulator
NOT YET TESTED: physical device
Emulator / device smoke required before moving this PR out of draft

The SDK is a library consumed by Android apps; behavior on a real ART runtime
(coroutines + OkHttp + WebSocket + Testcontainers-free environment) is not exercised by
the Docker E2E suite. Public-API surface unchanged so consumer apps should compile
without modification, but runtime smoke is still owed.

🤖 Generated with Claude Code

…ndpoints Core changes (functionality preserved, public API unchanged): * InsightDispatcher: bounded Channel(256, DROP_OLDEST) + batching consumer (50 events / 1s) replaces unbounded per-evaluation scope.launch. Hot eval path is non-suspending. Suspending closeAndDrain() flushes pending events on shutdown within a 2s budget. * DataSynchronizer.closeAndJoin(): new suspend method that awaits in-flight upserts before returning. FBClientImpl.identify() uses it so a late poll response from the previous user cannot land in the store under the new user (prior close() was non-suspending and didn't wait for the forEach { store.upsert(it) } loop to finish). * FBEndpoints: single eagerly-parsed source of truth for FeatBit URLs; malformed URIs fail at SDK init, not on first network call. * EvalResult: sealed interface Found(flag) / NotFound replaces stringly- typed (reason, flag?, isValid) triple. Wire-compat reason strings preserved. * FBOptions.Builder.build(): eager require(...) validation on secret + URIs + positive polling interval + non-negative grace. * FBClientImpl.close(): per-phase 2s+2s timeouts (sync, insights) replace a shared 3s budget so a slow sync teardown can't starve the insight flush (which would otherwise leak OkHttp dispatcher threads). * StreamingDataSynchronizer: ownsClient flag mirrors FbApiClient, shutting down the SDK-owned OkHttpClient dispatcher on close. closeAndJoin uses scope.coroutineContext.job.cancelAndJoin() — the idiomatic Kotlin primitive for "stop everything on this scope and wait." Late onMessage events are dropped via the closed guard. * GetUserFlags JSON: strict shape validation. Missing fields -> empty list. Shape mismatches -> throw (caught + logged by safePoll) rather than the prior silent emptyList downgrade that masked wire-format regressions as "user has no flags". Tests: +31 new tests across InsightDispatcher (7), FBEndpoints (9), EvalResult (4), FBOptionsBuilder (8), GetUserFlags shape-mismatch (3). Total: 36 -> 67 unit tests pass / 0 fail / 2 e2e-skipped (run via FEATBIT_E2E=1, both pass against a real FeatBit stack in Colima). Docs: docs/superpowers/architecture.md — reverse-engineered architecture walkthrough with ASCII + Mermaid diagrams. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Closes M5 from the prior wider-scope audit: streaming was previously covered only by FeatBitStreamingE2ETest (env-gated, Docker-required). Three deterministic tests via MockWebServer.withWebSocketUpgrade — no real-time sleeps, no virtual-time abuse against OkHttp's real dispatcher. Each test ships with a documented mutation that would fail it; the structural JSON assertions (decoded via kotlinx.serialization) catch regressions that contains() substring-matching would not. * `start opens websocket, sends data-sync, and initializes store from server snapshot` — pins messageType="data-sync", data.user.keyId="u1", initial timestamp=0L; verifies the server snapshot lands in the store and initialized flips true. * `closeAndJoin preserves store state and initialized flag` — defensive pin: close() is for streaming lifecycle, not data. Pre-close evaluations survive; initialized is sticky. * `pause closes websocket with reason and resume reconnects with advanced timestamp` — pause uses NORMAL_CLOSURE (1000) with reason="paused"; resume opens a fresh WS and re-sends data-sync with timestamp > 0 (proving handleMessage advanced it after the first snapshot). The `if (closed) return` guard in Listener.onMessage cannot be reproduced via the public surface (MockWebServer drops late frames at the wire when its own server-side WS is closed). Verified by mutating the guard to a no-op and watching this class still pass — see file-level KDoc. The guard's coverage relies on the E2E test's identify-time swap. Audit cycle: 2 rounds. First pass surfaced 2 HIGH (loose contains-based assertions; missing close-reason + advanced-timestamp pins) + 3 NIT (fragile whitespace strip; tests missing finally; no initialized assert post-close). All addressed. Second pass: ZERO findings. Unit tests: 66 -> 69 pass / 0 fail / 2 e2e-skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Closes H5 from the prior wider-scope audit: FBClientImpl's top-level integration surface had only happy-path offline coverage. None of the refactor's identify/close guarantees were exercised at the public API level. Adds 7 tests, all with documented mutations that would fail them: * `identify swaps synchronizer and evaluation reflects new user payload` — Polling mode + MockWebServer with 2 enqueued bodies (user-A's "alpha", user-B's "beta"). Proves the swap end-to-end: closeAndJoin of old sync, fresh sync starts, store updates, evaluation reflects the new user's data. * `close is idempotent` — second close < 50ms (microsecond-scale for an already-cancelled scope). Tighter than the initial 1_000ms threshold because audit pushed back that 999ms would still pass. * `close completes promptly when offline` — < 500ms, lower-bound pin. * `close is bounded when sync teardown is blocked on a non-responsive server` — kicks off a polling client against a MockWebServer with no enqueued responses (blocks in OkHttp socket.read), then measures close() wall-clock. Asserts < 2_500ms — pins SYNC_CLOSE_TIMEOUT_MS=2s per-phase budget, well under OkHttp's 8s readTimeout. Originally dismissed as "untestable without an injection seam"; audit pushed back with the exact recipe. * `close preserves store and bootstrap evaluations still work` — close doesn't wipe the store. * `offline identify returns true without network` — NullDataSync.start is vacuously successful. * `start returns false when polling sync cannot initialize within timeout` — withTimeoutOrNull semantics; also pins `assertFalse(client.initialized)` post-failed-start. Audit cycle: 3 rounds (1 HIGH + 5 NITs in round 1, 2 NITs in round 2, ZERO in round 3). The HIGH was the misleading "untestable" claim on the close-budget test — auditor designed the working test that's now in the file. Unit tests: 69 -> 77 pass / 0 fail / 2 e2e-skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…transient 5xx Closes M4 from the prior wider-scope audit. Polling previously had only 2 happy-path tests (first poll initializes; fatal 401 stops). The race-safety fix from prior Finding #2 (closeAndJoin awaits in-flight upserts) was entirely untested. Adds a BarrierStore helper (MemoryStore decorator that blocks the first upsert on a CompletableDeferred + 10s deadline) and 3 tests: * `closeAndJoin awaits in-flight upsert before returning` — sandwiches closeAndJoin between barrier-enter and barrier-release. Asserts closeAndJoin is still suspended while upsert is blocked, that the upsert completes before closeAndJoin returns, and — load-bearing — that start() resolves to `true` because closeAndJoin awaited the initializing upsert. Mutation-verified: replacing `cancelAndJoin()` with `cancel()` makes the test fail with "race window open". Also pins loop termination: after closeAndJoin, requestCount must not grow over a 200ms / 4-tick window at the test's 50ms interval. * `polling loop issues repeated requests across the polling interval` — 50ms interval, expects ≥3 requests within 180ms. Mutation: removing the `while (true)` loop produces a single request. * `transient 500 does not stop the loop and next 200 initializes` — enqueues 500 + 200, asserts start() returns true within 2s and the recovery payload landed. Mutation: treating 500 as fatal would stop the loop and start() would return false. Audit cycle: 2 rounds (3 HIGH + 2 NIT in round 1, ZERO in round 2). The HIGHs were a real bug in MY test: the post-closeAndJoin start() assertion was inverted (had `false`, should be `true`) — the audit caught it, and my own mutation test confirmed the corrected version catches the cancel-without-join regression. Unit tests: 77 -> 80 pass / 0 fail / 2 e2e-skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Closes H3 from the prior wider-scope audit. LifecycleController had only 4 happy-path tests; the race semantics auditor flagged as "what users actually hit on a phone" were untested. Adds 3 tests, each with a Wall-clock timing block and a documented mutation that would fail it: * `foreground flap within grace re-anchors pause to the latest off` — off → on → off within grace. Asserts pause does NOT fire at the original schedule's T=grace, then fires once at T>=2*grace anchored on the second off. Catches a regression that fails to cancel-and-null the pauseJob on the foreground-return path. * `network on while foreground off does not resume` — the phone-comes- back-online-while-still-in-background case. Both off → pause → network on alone → assert no resume → foreground on → assert resume. Mutation-verified: replacing `if (foreground && online)` with `if (online)` in production makes the test fail with "must NOT resume while foreground is still false". Restored. * `second inactive signal while pause is pending does not double- schedule` — foreground off → network off (while pause pending). The `pauseJob == null` guard must drop the second signal silently. Test advances to T=2*grace+1 to catch a delayed duplicate from a buggy double-schedule, which would fire at T=grace*1.25. Audit cycle: 2 rounds (3 NIT in round 1, ZERO in round 2). The NITs were all on doc-comment accuracy (timing math, mutation descriptions); test logic was correct from the start. Comments rewritten to match actual cumulative timings. Unit tests: 80 -> 83 pass / 0 fail / 2 e2e-skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Closes N2 from prior wider-scope audit. Store had only 4 single-thread tests; thread-safe-collection invariants and listener-snapshot semantics were untested. Adds 4 tests, each with documented mutation that would fail it: * `concurrent upserts are race-free and final state is consistent` — 16 threads × 250 upserts hammering the same flag. Pins no-exception + final value sanity + no garbage strings under contention. Honest scope: this does NOT catch lock removal (race produces wrong-but-plausible counts, not exceptions). Docstring documents the limitation rather than overclaiming. * `listener observes the just-written value (write happens-before notify)` — listener re-enters store.upsert mid-callback; reads the parent key and sees the just-written value. Honest scope: does NOT prove notify-outside-lock (JVM monitors are reentrant); proves happens-before for the items[] write only. * `add and remove listeners during dispatch do not corrupt iteration` — listener adds another listener mid-dispatch. Wraps upsert in try/catch ConcurrentModificationException with explicit fail message so a regression to plain ArrayList points the maintainer directly at the snapshot-iterator regression class. Asserts the late-added listener doesn't fire for the in-flight event but does for subsequent events. * `addChangeListener is idempotent — re-adding the same listener fires it only once` — pins the addIfAbsent semantic. Mutation-verified: replacing addIfAbsent with add fails "expected:<1> but was:<2>". Audit cycle: 2 rounds (1 HIGH + 1 MEDIUM + 1 LOW + 2 NIT in round 1, ZERO in round 2). Unit tests: 83 -> 87 pass / 0 fail / 2 e2e-skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Closes H1 from the prior wider-scope audit. Two distinct changes, bundled because the test asserts the fixed behavior: PRODUCTION FIX (data-loss bug): FlagTrackerImpl.dispatch used `subscribers.forEach { it.onChange(event) }`. CopyOnWriteArrayList.forEach halts on the first exception — so a single throwing subscriber silently broke delivery to every subscriber registered after it, AND propagated the throw back into store.upsert. New `safeNotify` helper wraps each callback in try/catch (Throwable) and logs to System.err (same fallback pattern as DefaultLogger.write when android.util.Log is unavailable). Iteration continues; the throw never escapes dispatch. TESTS (+5 covering H1): * `SharedFlow buffer caps at extraBufferCapacity under hung-collector burst` — hung-collector pattern pins replay=0+capacity=64 overflow drop. CountDownLatch-based subscription signal (not warmup-poll). * `slow subscriber does not prevent subsequent subscribers from firing` — synchronous-fan-out invariant; assertions fire IMMEDIATELY after upsert returns (no latch.await) so a launch()-based regression would fail the immediate check. * `subscriber exception does not starve other subscribers` — pins the production fix above. Mutation-verified: removing try/catch makes test fail with RuntimeException propagating out. * `keyed subscribers on different keys are isolated`. * `flagChanges Flow has no replay — late subscribers do not see prior events` — emits 5 pre-subscription events; catches replay=1, =3, =5 regressions. Audit cycle: 2 rounds (1 🔴 + 2 🟡 + 1 🔵 in round 1, ZERO in round 2). The 🔴 was the data-loss bug — audit pushed back on my "follow-up" framing, so the fix landed in this commit rather than being deferred. Unit tests: 87 -> 92 pass / 0 fail / 2 e2e-skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Closes M8 from wider-scope audit. TrackInsight had only 1 happy-path test. +6 tests, each with documented mutation: * `multi-element batch is posted as a single JSON array` — structurally decodes wire payload via kotlinx-serialization; pins keyId, featureFlagKey, timestamp, sendToExperiment per element. Catches serializer regressions on any of those fields. * `empty batch is a no-op — no HTTP request issued` — pins the `if (batch.isEmpty()) return` guard. * `network error is swallowed and does not propagate` — uses SocketPolicy.DISCONNECT_AT_START to force a real IOException through production's `catch (Exception)` (note: a 5xx response does NOT throw — it returns a normal Response object with status=500; only socket errors raise exceptions, so the test exercises the actual catch path). * `NoopTrackInsight run and close are no-ops` — pins the offline impl. * `close completes without throwing` — idempotent close. * `cancellation does not propagate as exception out of tracker run (current behavior)` — HONEST pin of observed behavior. Production's `catch (CancellationException) { throw ce }` is effectively unreachable for HTTP post because OkHttp's execute() is blocking; the underlying InterruptedIOException is swallowed via the generic catch. Docstring flags this as a follow-up: map InterruptedIOException → CancellationException in post() to honor structured concurrency. Test uses an explicit OkHttpClient with 1s readTimeout (test runs in 1s instead of OkHttp's default 8s). Audit cycle: 2 rounds (2 🔴 + 1 🟡 in round 1, ZERO in round 2). 🔴 #1 was my misunderstanding: I'd claimed "5xx swallowed" but OkHttp doesn't throw on 5xx — the test was passing for the wrong reason. Audit caught that. 🔴 #2 was missing per-field structural assertions. Unit tests: 92 -> 98 pass / 0 fail / 2 e2e-skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Layered architecture (domain/app/data/wire) within existing module. Public API stays byte-compatible at co.featbit.client.*. 7-commit implementation plan: one layer per commit, audited + tests green at each step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

7-task bite-sized plan per writing-plans skill format. One commit per layer move. Each task: git mv + package declaration update + caller imports + verification gates + audit + commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Pure Kotlin types (no Android / OkHttp / kotlinx-serialization deps) relocated into co.featbit.client.domain.* to make the dependency rule explicit. Public EvalDetail stays in evaluation/ (consumer-visible FQN). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@serializable

Task 1 audit surfaced domain→app reverse arrow (domain.Evaluator imports store.MemoryStore; Task 2 would have moved store/ into app/, creating the violation). Resolution per ports/adapters: - MemoryStore interface → domain/ (port consumed by Evaluator) - DefaultMemoryStore impl → app/ (adapter) - FlagValueChangedEvent + FlagChangeListener STAY in store/ — they are part of the public FlagTracker API surface (FlagTracker.subscribe takes FlagChangeListener; can't move without breaking consumers). Also documents the FeatureFlag @serializable carve-out as known architectural tension (FQN-stability vs domain purity), out of scope for this refactor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

MemoryStore (interface) -> domain/ as the port consumed by Evaluator. DefaultMemoryStore (impl) -> app/ as the adapter. FlagValueChangedEvent + FlagChangeListener stay in store/ -- they are public API via FlagTracker.subscribe(FlagChangeListener). LifecycleController + FlagTrackerImpl move into app/ as application services. Closes the domain->app reverse arrow surfaced by Task 1 audit (domain.Evaluator imported store.MemoryStore; would have become domain->app once store/ moved into app/). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Synchronizer adapters are concrete data-layer impls. Move from datasynchronizer/ into data/sync/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

FbApiClient, FBEndpoints, HttpConstants, GetUserFlags, ConnectionToken relocated from junk-drawer internal/ into data/http/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

TrackInsight + InsightDispatcher are the analytics surface. With this move, the internal/ junk drawer is dissolved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@serializable

EndUser and Insight (with VariationInsight, VariationData, CustomizedProperty) are wire-format DTOs decorated with kotlinx-serialization @serializable. Move them from model/ into wire/ so the model package contains only the public, user-facing types (FBUser, FeatureFlag). Behavior unchanged; all callers updated to import from wire/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@serializable

Wraps up the clean-architecture pass. Final layout: domain/, app/, data/{sync,http,insights}/, wire/ — plus the unchanged public-API packages (root, options/, model/, evaluation/, changetracker/, store/). Doc includes the layered tree, the dependency rule, and three known carve-outs (FeatureFlag @serializable, app.LifecycleController -> data.sync.DataSynchronizer, public MemoryStore + DefaultMemoryStore) each with the rationale for accepting the rule relaxation. Gates verified: * assembleDebug green across :featbit-client, :featbit-client-android, :example-app. * :featbit-client:testDebugUnitTest — 98 pass / 0 fail / 0 err. * FEATBIT_E2E=1 against real FeatBit stack on Colima — both FeatBitE2ETest + FeatBitStreamingE2ETest pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Three changes on the per-flag-check hot path that ran on every UI-thread evaluation, multiplied by app count × flag-check rate: 1. EvalDetail allocation on non-detail variation getters. Introduce evaluateValue() — the alloc-free sibling of evaluateCore that returns the typed value directly. boolVariation/intVariation/... now route through it; the *VariationDetail() callers keep going through evaluateCore so per-call reason strings still surface. 2. Insight construction in offline mode. With NoopTrackInsight installed, every Insight + VariationInsight + VariationData + EndUser allocation went to the consumer just to be discarded. Compute `insightsEnabled = tracker !is NoopTrackInsight` once at construction and short-circuit Insight.forEvaluation / Insight.forIdentify before any allocation. 3. FBUser.toEndUser() rebuilt EndUser (and its CustomizedProperty list) per evaluation. FBUser is immutable, so its wire form is too — cache it as a private val initialized once at construction. Also: allFlags() used `store.getAll().associateBy { it.id }` which walked the snapshot twice and allocated an intermediate List. Build the result LinkedHashMap directly from the snapshot in one pass. Tests: BuildersTest pins toEndUser identity stability across 1k calls; FBClientImplTest pins fast-path / detail-path value equivalence across every converter + the not-ready-without-bootstrap guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Polling and streaming both receive an N-flag snapshot per server response, then did `response.flags.forEach { store.upsert(it) }` — N monitor enters on the store's writeLock per response, plus N event-dispatch loops interleaved with writes. Add MemoryStore.upsertAll(flags) as a defaultable interface method; override on DefaultMemoryStore to take the lock once, compute every change event under it, and notify listeners outside the lock after all writes complete. A 200-flag polling snapshot now enters the writeLock exactly once instead of 200x. Listeners now observe a consistent post-batch snapshot — calling store.get(otherFlagInBatch) from a listener returns the post-batch value, not a half-written intermediate. The interface Kdoc documents that both the legacy "interleaved" ordering (default impl) and the batched ordering (DefaultMemoryStore override) are valid contracts, so consumers must not rely on which they see. PollingDataSynchronizer + StreamingDataSynchronizer now call upsertAll. Tests: empty-batch no-op; new-flag events fire one-per-flag in input order; mix changed/unchanged correctly skips no-op writes; oldValue is the pre-batch value (not post-batch); listeners see the consistent snapshot during dispatch; no-listeners path is safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@serializable

Three small wins on the wire and the variation type-conversion path: 1. GetUserFlags previously went body → JsonElement AST → navigate .jsonObject["data"].jsonArray["featureFlags"] → decodeFromJsonElement, allocating a full intermediate tree and walking it twice. Replace with a typed @serializable LatestAllEnvelope/LatestAllData carved out at internal scope, decoded in one pass via decodeFromString. Behavior is preserved: missing fields still default to emptyList(), shape mismatches still throw SerializationException (caught + logged by safePoll, not silently swallowed). 2. HttpTrackInsight previously rebuilt `ListSerializer(Insight.serializer())` on every send. Cache it as a companion-object val — initialized once at class load, thread-safe per kotlinx.serialization's docs. 3. ValueConverters.bool ran `value.trim().lowercase()` per call — `lowercase()` unconditionally allocates when the input has any uppercase letter. Replace with `equals("true", ignoreCase = true)` / `equals("false", ignoreCase = true)` which does case-insensitive compare without allocating. Test coverage expanded to pin mixed-case permutations (TRUE, True, tRuE, with leading/trailing whitespace) and near-match rejections (trues, yes, 1, 0, etc). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Pre-push adversarial audit (superpowers:code-reviewer) returned 6 findings against the perf-optimization commits 316d195/901b955/bc546fb. This commit addresses them; remaining items are documented push-backs. MEDIUM — GetUserFlags silently empty on `{"data": null}`: The single-pass typed envelope changed `data` to nullable with `= null` default. Under kotlinx-serialization 1.6.3 + the project's `explicitNulls = false` config, JSON null on a nullable field coerces to Kotlin null WITHOUT throwing — re-introducing the "indistinguishable from no flags, serve defaults forever" failure the old AST code guarded against. Empirically verified with a probe test against the pinned kotlinx version. Fix: make `data` non-nullable with a `LatestAllData()` default. Absent field still defaults to empty (matches old behavior); explicit JSON null now throws SerializationException, caught + logged by `safePoll`. Regression test added. MEDIUM — fast-path insight emission untested: The `value and detail getters agree` test ran against `offlineClientWith` where `insightsEnabled == false`, so a mutation dropping `insights.offer(...)` from `evaluateValue`'s Found branch slid silently. Added MockWebServer-based test that drives an online polling client, calls `boolVariation()`, and verifies an insight POST hits `/api/public/insight/`. Mutation-verified — removing the emit kills the test. MEDIUM — fast-path guard not exercised: Added paired negative test that confirms zero insight POSTs reach the server when the client is pre-init without bootstrap. MEDIUM — MemoryStore default-method binary compat: `public interface MemoryStore` gained `upsertAll` as a default method. Under Kotlin 1.9's default `-Xjvm-default=disable`, the body lands in `DefaultImpls` and the interface bytecode has the method as abstract — old Java binary implementers would `AbstractMethodError` on `store.upsertAll`. The SDK does not currently expose a `MemoryStore` injection point but the interface IS public per the clean-arch domain-port carve-out. Fix: enable `-Xjvm-default=all-compatibility` module-wide. Default-method bodies now land in interface bytecode (real Java 8 defaults) with a `DefaultImpls` fallback for legacy binaries. NIT — FBUser endUser defensive immutability: Wrap cached `customizedProperties` in `Collections.unmodifiableList` so SDK-internal code can't `as MutableList` and corrupt the per-FBUser-lifetime cache. NIT — corrected allocation-count comment "~3 → ~4" in FBClientImpl (forgot the `listOf(VariationInsight.of(...))` singleton wrapper). NIT — documented the directional improvement in `DefaultMemoryStore.upsertAll` listener-throw semantics: bulk path commits ALL writes under the lock before any listener fires, so a throwing listener leaves the store in its full post-batch state, not partially written. NIT — removed dead import `kotlinx.serialization.encodeToString` in GetUserFlags (call site uses StringFormat member fn, not the reified inline extension). NIT — MockWebServer race on the fast-path insight test: `takeRequest(1ms)` after `client.close()` raced the in-flight HTTP POST landing on the server. Replaced with a bounded poll loop (8 × 250ms = 2s budget) that breaks on first insight hit. Push-backs (documented in chat, not in code): * FBClientImpl.close insights-timeout nesting (pre-existing, not introduced by perf pass — out of scope). * DefaultMemoryStore `listeners.isEmpty()` short-circuit observability (author's existing Kdoc already concedes code-review -only coverage). Audit loop ended on a zero-finding pass per CLAUDE.md rule #3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

`explicitNulls = false` is marked experimental in kotlinx-serialization 1.6 and the compiler was warning on every build. The flag's decoding semantic is load-bearing for `GetUserFlags.LatestAllEnvelope.data` — the perf-pass fix relies on the experimental "JSON null on non-nullable field with default → throws" behavior to surface malformed payloads instead of silently returning empty. @OptIn at val-level (smallest precise scope) acknowledges the dependency. Kdoc explains why we knowingly accept the experimental contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Devild007 and others added 23 commits June 25, 2026 10:52

refactor: move synchronizers into data/sync/

d4cfc44

Synchronizer adapters are concrete data-layer impls. Move from datasynchronizer/ into data/sync/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

refactor: move HTTP plumbing into data/http/

0e62704

FbApiClient, FBEndpoints, HttpConstants, GetUserFlags, ConnectionToken relocated from junk-drawer internal/ into data/http/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

refactor: move insight pipeline into data/insights/

ea78755

TrackInsight + InsightDispatcher are the analytics surface. With this move, the internal/ junk drawer is dissolved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: clean-architecture + perf pass on evaluation hot path - #10

refactor: clean-architecture + perf pass on evaluation hot path#10
Devild007 wants to merge 23 commits into
mainfrom
refactor/clean-architecture

Devild007 commented Jun 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Devild007 commented Jun 25, 2026

Summary

Numbers

Test plan

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant